Hi Alex,
Since two separate bugs are created for the suggestions given by Jim.
And you are okay with the fix provided for the bug
https://bugs.openjdk.java.net/browse/JDK-8147413 under webrev
http://cr.openjdk.java.net/~jdv/8147413/webrev.00/
Can we push the fix into JDK9 after one more approval?
Thanks,
Jay
-----Original Message-----
From: Alexander Scherbatiy
Sent: Friday, January 22, 2016 5:05 PM
To: Jim Graham; Phil Race; Jayathirth D V
Cc: 2d-dev@openjdk.java.net
Subject: Re: [OpenJDK 2D-Dev] Review request for JDK-8147413 :
api/java_awt/Image/MultiResolutionImage/index.html\#MultiResolutionRenderingHints[test_VALUE_RESOLUTION_VARIANT_BASE]
started to fail
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\#MultiResolutionR
>>> enderingHints[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
>>>
>>
