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

[Alexander Scherbatiy]

I have created two separated issues on it:
   8148045 Handle null resolution variant in SunGraphics2D
     https://bugs.openjdk.java.net/browse/JDK-8148045

   8148046 Investigate the case where MRI.getResolutionVariant() returns 
MultiResolutionImage
     https://bugs.openjdk.java.net/browse/JDK-8148046

Thanks,
Alexandr.

On 22/01/16 00:34, Jim Graham wrote:
> 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
>>>
>>