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

Tue Feb 2 01:46:02 UTC 2016

I'm OK with the additional issues being addressed in follow-on fixes if 
that is what you are asking.  The original fix in webrev.00 adds no 
additional harm and fixes a common case that is already breaking some 
code...

			...jim

On 2/1/16 1:04 AM, Jayathirth D V wrote:
> 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 at 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
>>>>
>>>
>