[OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7
Jim Graham
james.graham at oracle.com
Thu Jun 2 21:17:45 UTC 2016
Looks good!
...jim
On 6/2/16 2:51 AM, Ajit Ghaisas wrote:
> Thanks Jim and Phil for reviewing this.
>
> Adding a comment is a good suggestion. I have added it and requested a push of following webrev-
> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.04/
> (Sent here for a reference)
>
> Regards,
> Ajit
>
>
>
> -----Original Message-----
> From: Jim Graham
> Sent: Thursday, June 02, 2016 2:54 AM
> To: Ajit Ghaisas; Philip Race; Sergey Bylokhov; 2d-dev
> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) while working in 7
>
> +1.
>
> If you wanted to add a comment on the new catch, you could say:
>
> } catch (RuntimeException e) {
> // We did not previously call this method here and some filters
> // were not prepared for it to be called at this time so we
> // allow them to have runtime issues for this one call only
> // without triggering the ERROR condition below.
> ... (printstack)
> }
>
> Your call, I don't need to approve a webrev for that comment addition...
>
> ...jim
>
> On 6/1/16 3:28 AM, Ajit Ghaisas wrote:
>> Thanks Jim and Phil for valuable feedback and suggestions.
>>
>> The detailed discussion helped me to understand why catching RuntimeException is a better option for a call to imageComplete(STATICIMAGEDONE).
>> I have modified the code on line 192 accordingly.
>>
>> I have not modified the code catching NullPointerException (on line 196) as doing so would mask a wider variety of legitimate exceptions. So, line 196 remains unchanged.
>>
>> I have also modified the test to throw NullPointerException instead of calling intValue() on a null.
>>
>> Please review updated webrev :
>> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.03/
>>
>> Regards,
>> Ajit
>>
>> -----Original Message-----
>> From: Phil Race
>> Sent: Wednesday, June 01, 2016 1:41 AM
>> To: Jim Graham; Ajit Ghaisas
>> Cc: Sergey Bylokhov; 2d-dev
>> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
>> ImageFilters return blank images in Java 8(.45) while working in 7
>>
>> I agree with the suggestion of RuntimeException as likely sufficient but not too much ...
>>
>> -phil.
>>
>>
>> On 5/31/2016 12:29 PM, Jim Graham wrote:
>>> I agree with this. It would be nice if we didn't spit out print
>>> statements by default, but I'm not sure what a good flag would be to
>>> use to diagnose this that balances how likely developers will think
>>> to use it.
>>>
>>> The point about catching a wider variety of exceptions is not that it
>>> is good practice in general, but we've added a new call to a
>>> long-standing algorithm which has decades (really? plural? pretty
>>> much, I think) of past code that isn't expecting the new call, and
>>> that call is basically "informative" not something that they really
>>> need to successfully handle, so just as someone might accidentally
>>> process a null pointer in a case like that, someone else might run
>>> off the end of an array (AIOOB) or any other "I wasn't expecting that"
>>> sort of issue.
>>>
>>> Throwable might be a bit much, though. Phil? I was thinking
>>> Exception because those tend to focus on "your code made a mistake"
>>> whereas Error is something like "you are mix/matching incompatible
>>> code that was compiled to contain conflicting information" (kind of
>>> like calling a method that doesn't exist in this release, etc.).
>>> It's hard to say what the proper level of forgiveness should be here.
>>> Another thought is "RuntimeException", since any other exception
>>> would need to be declared to be thrown and we don't declare it for
>>> that method, so their implementation shouldn't be allowed to declare
>>> it either...
>>>
>>> ...jim
>>>
>>> On 05/31/2016 10:34 AM, Phil Race wrote:
>>>> Based on what Ajit wrote in the bug report, he at least found a jar
>>>> file that contains this code, but I have so far failed to locate the
>>>> source code as all the versions I find on the net use the Java 2D
>>>> JDK 1.2 BufferedImageOp APIs so I suspect this bug is in a very old
>>>> version and it may not be open source.
>>>> Therefore I am not sure how much e.printStackTrace() will help them
>>>> if they don't own that source except to encourage them to upgrade.
>>>> I'd be inclined to stick it behind a debugging property if we have
>>>> one that we could re-use except that in such a case they probably
>>>> won't know enough to set the property.
>>>> So on balance it is probably best to do as it has been presented
>>>> here except that catch (Throwable t) probably makes sense as we
>>>> don't want to revisit this for a different exception.
>>>>
>>>> Minor nit about the test: instead of calling intValue() on a null
>>>> Integer, why not just "throw new NullPointerException("reason ..") ?
>>>>
>>>>
>>>> -phil.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On 05/30/2016 01:22 AM, Ajit Ghaisas wrote:
>>>>> I did contemplate catching 'Exception' instead of
>>>>> 'NullPointerException' while raising the webrev, but decided not to
>>>>> do it as- 1. Complaint in current image filter was due to NPE only
>>>>> 2. Catching & consuming generic exception does not seem quite
>>>>> correct here as we are just printing stack trace and not taking any
>>>>> concrete action.
>>>>> 3. There is no reported issue of any other type of Exception out of
>>>>> this method.
>>>>>
>>>>> Regards,
>>>>> Ajit
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Jim Graham
>>>>> Sent: Saturday, May 28, 2016 4:41 AM
>>>>> To: Ajit Ghaisas; Sergey Bylokhov; 2d-dev; Philip Race
>>>>> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
>>>>> ImageFilters return blank images in Java 8(.45) while working in 7
>>>>>
>>>>> That looks good, but I wonder if it should catch all exceptions
>>>>> instead of just NPE. Historically we were only catching NPE before
>>>>> we added the call to STATICIMAGEDONE, and the current filter in
>>>>> question turns out to throw an NPE, but if a filter that was
>>>>> commonly used with offscreen images was not expecting the
>>>>> STATICIMAGEDONE as was this WaterFilter, then they could
>>>>> potentially throw a wider variety of exceptions.
>>>>>
>>>>> I'm good with NPE since that is the only case we've seen, but I'd
>>>>> also be good with changing line 192 to catch all exceptions. Phil?
>>>>>
>>>>> ...jim
>>>>>
>>>>> On 5/27/16 1:32 AM, Ajit Ghaisas wrote:
>>>>>> Hi Jim,
>>>>>>
>>>>>> Thanks for in-depth analysis and detailed review.
>>>>>> I appreciate your concern for filtered image being blank in
>>>>>> Java 8, while it used to work in Java 7.
>>>>>>
>>>>>> Yes. I totally agree with your suggestion to do both -
>>>>>> 1. Not send IMAGEERROR if there is exception from ImageFilter
>>>>>> while processing STATICIMAGE done
>>>>>> 2. At the same time, as we are consuming the exception, show
>>>>>> the exception stacktrace.
>>>>>>
>>>>>> With this fix the functionality of
>>>>>> com.jhlabs.image.WaterFilter is restored. It does not return blank images anymore.
>>>>>> Also, as additional information, exception details are shown
>>>>>> as a stacktrace.
>>>>>>
>>>>>> Please review the updated webrev :
>>>>>> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.02/
>>>>>>
>>>>>> Regards,
>>>>>> Ajit
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jim Graham
>>>>>> Sent: Thursday, May 26, 2016 4:57 AM
>>>>>> To: Sergey Bylokhov; Ajit Ghaisas; 2d-dev; Philip Race
>>>>>> Subject: Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom
>>>>>> ImageFilters return blank images in Java 8(.45) while working in 7
>>>>>>
>>>>>> Their complaint is that the resulting image is empty - most likely
>>>>>> because the error gets passed through and clears the image buffer.
>>>>>> It appears that the sequence of events is:
>>>>>>
>>>>>> We send SINGLEFRAMEDONE.
>>>>>> - the image is displayable
>>>>>> We send STATICIMAGEDONE.
>>>>>> - filter throws NPE
>>>>>> - image is probably still displayable We send ERROR
>>>>>> - if it gets passed through to the toolkit image consumer then
>>>>>> we'll clear the image buffer
>>>>>> - image is no longer displayable
>>>>>>
>>>>>> It's hard to say for sure without having an instance of their
>>>>>> filter in-house for testing, but previously we weren't sending the
>>>>>> STATICDONE notice and the program was working just fine. Since the
>>>>>> NPE comes from their code when we send it, it shouldn't have
>>>>>> affected any down-stream consumers, so we should be OK with just
>>>>>> continuing on and the image should still be displayable just as if
>>>>>> we hadn't sent the STATICDONE notice in the first place.
>>>>>>
>>>>>> Now, *during debugging*, they were thwarted by the fact that we
>>>>>> ate the exception, true, but the original issue is that the image
>>>>>> wasn't displayable at all. We might want to do both:
>>>>>>
>>>>>> - not send ERROR for STATICDONE when we used to not send
>>>>>> STATICDONE at all
>>>>>> - show the exception so that someone realizes that there is a
>>>>>> problem, even though we've made it not hurt their functionality.
>>>>>>
>>>>>> Does that make sense?
>>>>>>
>>>>>> ...jim
>>>>>>
>>>>>> On 5/25/2016 1:36 PM, Sergey Bylokhov wrote:
>>>>>>> On 25.05.16 23:33, Jim Graham wrote:
>>>>>>>> How about option 3 -
>>>>>>>>
>>>>>>>> NPE before imageComplete sends an ERROR as it does now.
>>>>>>>>
>>>>>>>> NPE during imageComplete is ignored, both for backwards
>>>>>>>> compatibility and because it is more of a "hint" than an
>>>>>>>> operation that requires action from the consumer.
>>>>>>> I guess the case is that when we ignore this NPE(w/o any
>>>>>>> notification) the users complains that the bug is in jdk.
>>>>>>>
>>>>>>>> ...jim
>>>>>>>>
>>>>>>>> On 5/25/2016 1:31 AM, Ajit Ghaisas wrote:
>>>>>>>>> Hi
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Bug :
>>>>>>>>>
>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8139192
>>>>>>>>>
>>>>>>>>> Custom ImageFilters return blank images in Java 8(.45) while
>>>>>>>>> working in 7
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Root cause :
>>>>>>>>>
>>>>>>>>> private method produce() in OffScreenImageSource.java consumes
>>>>>>>>> a NullPointerException that originates from a custom
>>>>>>>>> ImageConsumer (a 3^rd party image library class -
>>>>>>>>> com.jhlabs.image.WaterFilter)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Analysis:
>>>>>>>>>
>>>>>>>>> 1. How the behavior changed between JDK7 and JK8 :
>>>>>>>>>
>>>>>>>>> A call to imageComplete(ImageConsumer.SINGLEFRAMEDONE) was
>>>>>>>>> added in addition to imageComplete(ImageConsumer.
>>>>>>>>> STATICIMAGEDONE) as a fix for JDK-7143612.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 2. What debugging revealed:
>>>>>>>>>
>>>>>>>>> A NullPointerException is being thrown from the library during
>>>>>>>>> the call to imageComplete(ImageConsumer.STATICIMAGEDONE)
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 3. It looks like the fix of JDK-7143612 is valid and successive
>>>>>>>>> calls to
>>>>>>>>> imageComplete() are allowed.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 4. The 3rd party library behavior appears incorrect (if it can
>>>>>>>>> not handle subsequent calls to imageComplete(), it should
>>>>>>>>> de-register itself).
>>>>>>>>>
>>>>>>>>> The 3rd-party library code should be fixed.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Possible modifications in JDK :
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Currently, OffScreenImageSource::produce() consumes the
>>>>>>>>> NullPointerException - this is incorrect and results in silent
>>>>>>>>> failure of this particular image filter.
>>>>>>>>>
>>>>>>>>> We need to let the image filter library know that exception has
>>>>>>>>> occurred in its code and not in JDK. We have two options -
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Option 1 : Rethrow the NullPointerException --- It is the
>>>>>>>>> clearest way
>>>>>>>>> to let 3^rd party library know that their code is erroneous
>>>>>>>>> with latest JDK. This will change the 3^rd party image filter
>>>>>>>>> behavior that generates blank image.
>>>>>>>>>
>>>>>>>>> Option 2 : Add a stack trace where the exception is being
>>>>>>>>> consumed
>>>>>>>>> --- Adding stack trace provides more information regarding
>>>>>>>>> failure of 3^rd party image filter with retaining the current
>>>>>>>>> behavior that generates blank image.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I have implemented both the options:
>>>>>>>>>
>>>>>>>>> Option 1:
>>>>>>>>> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.00/
>>>>>>>>>
>>>>>>>>> Option 2:
>>>>>>>>> http://cr.openjdk.java.net/~aghaisas/8139192/webrev.01/
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Request you to review both the webrevs and provide your preference.
>>>>>>>>>
>>>>>>>>> I will add a test to the selected webrev.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>>
>>>>>>>>> Ajit
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>
>>
More information about the 2d-dev
mailing list