Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values
Hi Phil, > Test case works properly before and after the code change. ? You mean it does not fail before the fix ? It definitely fails for me before the fix. - I mean as intended Phil. It fails before the fix and after the fix it passes. > Since we are touching getNumThumbnailsSupported() function and it is only > overridden in JPEGImageWriter we can add override annotation for the same. I would be more inclined to address that in a separate bug that adds the annotation for all the over-ridden JPEG reader+writer methods, rather than adding just one in the file. - Sure. We can take that in separate bug. Thanks, Jay -Original Message- From: Philip Race Sent: Tuesday, March 27, 2018 2:39 AM To: Jayathirth D V Cc: Prahalad Kumar Narayanan; 2d-dev Subject: Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values On 3/23/18, 2:21 AM, Jayathirth D V wrote: > Hi Prahalad, > > Change looks fine. > Test case works properly before and after the code change. ? You mean it does not fail before the fix ? It definitely fails for me before the fix. > > Since we are touching getNumThumbnailsSupported() function and it is only > overridden in JPEGImageWriter we can add override annotation for the same. I would be more inclined to address that in a separate bug that adds the annotation for all the over-ridden JPEG reader+writer methods, rather than adding just one in the file. -phil. > Thanks, > Jay > > -Original Message- > From: Prahalad Kumar Narayanan > Sent: Friday, March 23, 2018 1:40 PM > To: 2d-dev > Subject: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: > JPGWriter.getNumThumbnailsSupported does not return -1 when passing > null values > > Hello Everyone > > Good day to you. > > Request your time to review a simple fix for the bug > Bug: JDK-4954348 (https://bugs.openjdk.java.net/browse/JDK-4954348) > Title: JPGWriter.getNumThumbnailsSupported does not return -1 > when passing null values > > Root Cause: > . As per ImageWriter specification, the method- > getNumThumbnailsSupported, should return -1 when invoked with insufficient > data. > . However, the method's implementation in JpegImageWriter returns 0 > instead of -1 in this use-case. > > Information on the fix: > . The logic within concerned method of Jpeg image writer requires one of > the two inputs- ImageTypeSpecifer (or) IIOMetadata. > . If both the required inputs are 'Null' the method has insufficient > data& thus cannot validate. > . A simple check is now added in the method to detect this use-case and > return -1 as recommended in the specification. > > Kindly review the changes at your convenience > Link: http://cr.openjdk.java.net/~pnarayanan/4954348/webrev.00 > > Thank you > Have a good day > > Prahalad N.
Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values
On 3/23/18, 11:56 AM, Sergey Bylokhov wrote: Hi, Prahalad. A few small comments about the test: - Is it possible to test all installed ImageWriterSpi? It seems that the test itself is not a JPEG plugin specific? None of the other plugins support thumbnails so they will all return 0 regardless. So I think that aspect of the test is OK - You will need to dispose the jpgWriter even in case of exception. Probably should fix that. Everything else seems fine. -phil. On 23/03/2018 01:10, Prahalad Kumar Narayanan wrote: Kindly review the changes at your convenience Link: http://cr.openjdk.java.net/~pnarayanan/4954348/webrev.00 Thank you Have a good day Prahalad N.
Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values
On 3/23/18, 2:21 AM, Jayathirth D V wrote: Hi Prahalad, Change looks fine. Test case works properly before and after the code change. ? You mean it does not fail before the fix ? It definitely fails for me before the fix. Since we are touching getNumThumbnailsSupported() function and it is only overridden in JPEGImageWriter we can add override annotation for the same. I would be more inclined to address that in a separate bug that adds the annotation for all the over-ridden JPEG reader+writer methods, rather than adding just one in the file. -phil. Thanks, Jay -Original Message- From: Prahalad Kumar Narayanan Sent: Friday, March 23, 2018 1:40 PM To: 2d-dev Subject: [OpenJDK 2D-Dev] [11] RFR: [JDK-4954348]: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values Hello Everyone Good day to you. Request your time to review a simple fix for the bug Bug: JDK-4954348 (https://bugs.openjdk.java.net/browse/JDK-4954348) Title: JPGWriter.getNumThumbnailsSupported does not return -1 when passing null values Root Cause: . As per ImageWriter specification, the method- getNumThumbnailsSupported, should return -1 when invoked with insufficient data. . However, the method's implementation in JpegImageWriter returns 0 instead of -1 in this use-case. Information on the fix: . The logic within concerned method of Jpeg image writer requires one of the two inputs- ImageTypeSpecifer (or) IIOMetadata. . If both the required inputs are 'Null' the method has insufficient data& thus cannot validate. . A simple check is now added in the method to detect this use-case and return -1 as recommended in the specification. Kindly review the changes at your convenience Link: http://cr.openjdk.java.net/~pnarayanan/4954348/webrev.00 Thank you Have a good day Prahalad N.
Re: [OpenJDK 2D-Dev] [11] Upgrade to Marlin renderer 0.9.1
Hi Laurent,
It took me I at least 5 tries to get all the way through this.
I don't have any substantial comments, just a few clean up questions.
What is this in Curve.java + DCurve.java ?
+if (false) {
+dax = 0.0d;
+day = 0.0d;
+dbx = 0.0d;
+dby = 0.0d;
+}
In Renderer.java, should the commented line be removed ?
+&& ((Math.abs(ddx) + Math.abs(ddy) * _SCALE_DY)<= _INC_BND
+//&& (Math.abs(ddx + dddx) + Math.abs(ddy + dddy) * _SCALE_DY)<= _INC_BND
A similar pattern occurs a little later in the same file.
+//|| (Math.abs(ddx + dddx) + Math.abs(ddy + dddy) *
_SCALE_DY)>= _DEC_BND
+static final float LEN_TH = MarlinProperties.getSubdividerMinLength();
You really meant to name it LEN_TH and not LENGTH ?
There are a few TODO's I see but they seem to be more about later clean up or
optimisation
so are probably all OK.
So I am OK to push, and if you clean up any of the above first I don't need to
see a new webrev.
-phil.
On 3/23/18, 2:03 PM, Laurent Bourgès wrote:
Hi,
Sorry to insist but I would like to get feedback on this Marlin patch
soon before going forward on tile-size tuning in java2d accelerated
pipelines.
Laurent
2018-03-21 22:56 GMT+01:00 Laurent Bourgès >:
Hi,
Here is the updated webrev:
http://cr.openjdk.java.net/~lbourges/marlin/marlin-091.1/
>:
Sergey,
Le mer. 14 mars 2018 à 17:14, Sergey Bylokhov
> a écrit :
On 13/03/2018 17:04, Sergey Bylokhov wrote:
I have started to look to this review, will run some
closed tests and send a feedback soon.
No issues found so far, +1.
Thanks for your vote.
I need another approval I suppose ?
I will prepare another review asap reverting only tile size
changes as using large tiles has performance drop on d3d & ogl
that needs more work. It can be done later in follow-up issues.
Phil do you agree the proposed plan ?
Laurent
