[OpenJDK 2D-Dev] Review request for JDK-8147413 : api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE] started to fail

[Jim Graham]

BASE essentially just means to draw the 1:1 image.  The problem is that 
returning null is not the way to achieve that.  Returning null means 
"MRI doesn't apply here, just use the original image", but in the case 
of BASE we have to make sure it isn't an MRI first otherwise the caller 
will get an exception.

The change does achieve that since BASE handling is built in to the 
helper getResolutionVariant() method in SG2D and so it should ask for 
the 1:1 image, but if the getRV() helper method returns null then we 
still return null from the drawHiDPIImage() method and that causes the 
caller to use the original MRI in a normal-image pipeline.  There should 
perhaps be an else clause for the "if (rV != img && rV != null) { } else 
{ ???; }".  What that "???" is depends on why the helper method returned 
the original image and/or a null, but it should not be "return null;". 
(Also, there is no check if it returned another different MRI.  That 
should probably not be allowed, but we never state that in the interface 
description...)

The remaining question is why the helper method would return either the 
original image or null.

I don't think the helper method ever returns the original MRI knowingly, 
but the MRI itself might do that.  That would then fall under the "what 
to do if the MRI returns any sort of non-standard image" case, and 
perhaps the exception is appropriate there?  We should probably detect 
that, though, and throw a more obvious exception (or recurse?). 
Recursion can get ugly, so perhaps a very explicit error with a 
reasonable description would be better than letting the DrawImage 
pipeline fail in various ways.

The helper function can return null if the operation is a NOP (sxy1 == 
sxy2 - i.e. an empty subimage rectangle), but that case should be 
handled here and end up with returning ?true? which matches what many of 
the drawImage() variants return when they detect a trivial NOP case.

The helper function can also return null if the srcWidth/Height are 
negative which would imply an uninitialized image.  I'm not sure what to 
do there because normally an uninitialized image is tracked by the 
DrawImage pipe and the observer is notified as the image data is loaded, 
but we have no mechanism to do that with an MRI.  This case may require 
more work, or something explicit in the MRI spec (what happens when 
someone lazily loads an image with an @x2 variant now?).

It also returns null if the return value from the MRI is a ToolkitImage 
with an error - but I think that is wrong, it should just return the 
ToolkitImage and let the caller send it to the DrawImage pipeline to 
detect the error and respond in the appropriate manner.  Or perhaps it 
should cause the drawHiDPIImage() method to return whatever boolean is 
appropriate for an error to save time, but there is no way to 
orchestrate that from the helper method since it can only return a 
(non-MRI) image.  It's probably best to just return the errored TKImage 
in that case...?

The last way it can return null is if the MRI returned null from its 
getRV() method, but the interface doesn't discuss what that means.  I 
suppose it might mean that all of the resolution variants are being 
loaded asynchronously and none of them are available yet, but we are 
then missing a mechanism to inform the observer of when the variants are 
loaded.  (Is there a bugid for that?  I seem to recall having a 
discussion about that missing mechanism in the past...?)

So, I think the change made here is along the right track, but we have a 
couple more holes to plug before we can consider this fixed...

			...jim

On 1/20/2016 12:57 PM, Phil Race wrote:
> It seems like the expectation was that BASE could be drawn by the old
> imaging path.
> which would be lower overhead.
> I think we should ask Alexandr what the intention was here and whether the
> code that handles the base image needs to be taught how to extract data from
> a MultiResolutionImage.
>
> -phil.
>
> On 01/20/2016 08:20 AM, Jayathirth D V wrote:
>>
>> Hi,
>>
>> __
>>
>> _Please review the following fix in JDK9:_
>>
>> Bug : https://bugs.openjdk.java.net/browse/JDK-8147413
>>
>> Webrev : http://cr.openjdk.java.net/~jdv/8147413/webrev.00/
>> <http://cr.openjdk.java.net/%7Ejdv/8147413/webrev.00/>
>>
>> Issue : JCK testcase
>> api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE]
>> is failing from b96 JDK9 build.
>>
>> Root cause : In getManager API of SurfaceManager.java we are trying to
>> typecast BaseMultiResolutionImage to BufferedImage and it is causing
>> ClassCastException and in turn IllegalArgumentException seen in result
>> of test case. It is happening because of change made in JDK-8073320
>> <https://bugs.openjdk.java.net/browse/JDK-8073320> in
>> SunGraphics2D.java. In case of VALUE_RESOLUTION_VARIANT_BASE type we
>> are not trying to convert MultiResolutionImage to BufferedImage.
>>
>> Solution : Modify the condition present in drawHiDPIImage API to
>> convert all MultiResolutionImage to BufferedImage irrespective of
>> KEY_RESOLUTION_VARIANT type.
>>
>> Thanks,
>>
>> Jay
>>
>