[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] Fix for JDK-8139192 : Custom ImageFilters return blank images in Java 8(.45) wh
From:       Jim Graham <james.graham () oracle ! com>
Date:       2016-06-02 21:17:45
Message-ID: da939651-41fc-1835-46c8-8dfb66bc7e96 () oracle ! com
[Download RAW message or body]

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
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > 
> > > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]