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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with unique IDAT chunk positi
From:       Prasanta Sadhukhan <prasanta.sadhukhan () oracle ! com>
Date:       2017-11-21 10:57:36
Message-ID: cd59df65-c678-ed70-04fa-27a652cbf657 () oracle ! com
[Download RAW message or body]

+1

Regards
Prasanta
On 11/21/2017 9:06 AM, Prahalad Kumar Narayanan wrote:
> Hello Jay
> 
> The change looks good.
> 
> Thanks
> Have a good day
> 
> Prahalad
> 
> -----Original Message-----
> From: Jayathirth D V
> Sent: Tuesday, November 21, 2017 9:01 AM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images \
> with unique IDAT chunk positions will cause IIOException 
> Hi Prahalad,
> 
> Thanks for the review.
> I merged the two functions into one instead of moving BufferedImage code into the \
> two functions as it reduces the LOC by good amount. 
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191431/webrev.02/
> 
> Thanks,
> Jay
> 
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Tuesday, November 21, 2017 8:13 AM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images \
> with unique IDAT chunk positions will cause IIOException 
> Hello Jay
> 
> Good day to you. I looked into the modified test code.
> 
> The test file has two methods ( writeAndReadImageWithPalette & \
>                 writeAndReadImageWithoutPalette ) with exactly the same \
>                 implementation.
> . Can we merge them into one method ?
> . Another option would be to move the logic that prepares required buffered image \
> within these methods. 
> 86     private static void writeAndReadImageWithoutPalette(BufferedImage image)
> 87             throws IOException {
> 88         File output = File.createTempFile("output", ".png");
> 89         ImageInputStream stream = null;
> 90         try {
> 91             ImageIO.write(image, "png", output);
> 92
> 93             stream = ImageIO.createImageInputStream(output);
> 94             ImageReadParam param = PNG_READER.getDefaultReadParam();
> 95             PNG_READER.setInput(stream, true, true);
> 96             PNG_READER.read(0, param);
> 97         } finally {
> 98             if (stream != null) {
> 99                 stream.close();
> 100             }
> 101             Files.delete(output.toPath());
> 102         }
> 103     }
> 
> 105     private static void writeAndReadImageWithPalette(BufferedImage image)
> 106             throws IOException {
> 107         File output = File.createTempFile("output", ".png");
> 108         ImageInputStream stream = null;
> 109         try {
> 110             ImageIO.write(image, "png", output);
> 111
> 112             stream = ImageIO.createImageInputStream(output);
> 113             ImageReadParam param = PNG_READER.getDefaultReadParam();
> 114             PNG_READER.setInput(stream, true, true);
> 115             PNG_READER.read(0, param);
> 116         } finally {
> 117             if (stream != null) {
> 118                 stream.close();
> 119             }
> 120             Files.delete(output.toPath());
> 121         }
> 122     }
> 123 }
> 
> Thank you
> Have a good day
> 
> Prahalad
> 
> -----Original Message-----
> From: Jayathirth D V
> Sent: Monday, November 20, 2017 6:18 PM
> To: Prahalad Kumar Narayanan; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images \
> with unique IDAT chunk positions will cause IIOException 
> Hi Prahalad,
> 
> Thanks for the review.
> I have updated the test case.
> 
> Please find updated webrev for review:
> http://cr.openjdk.java.net/~jdv/8191431/webrev.01/
> 
> Thanks,
> Jay
> 
> -----Original Message-----
> From: Prahalad Kumar Narayanan
> Sent: Monday, November 20, 2017 5:25 PM
> To: Jayathirth D V; 2d-dev
> Subject: RE: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images \
> with unique IDAT chunk positions will cause IIOException 
> Hello Jay
> 
> The code change at PNGImageReader is correct.
> But the test wouldn't delete temporary files when exception occurs.
> 
> Thank you
> Have a good day
> 
> Prahalad
> 
> ----- Original Message -----
> From: Jayathirth D V
> Sent: Monday, November 20, 2017 4:34 PM
> To: 2d-dev
> Subject: [OpenJDK 2D-Dev] [10] RFR JDK-8191431: Reading multiple PNG images with \
> unique IDAT chunk positions will cause IIOException 
> Hello All,
> 
> Please review the following fix in JDK10 :
> 
> Bug : https://bugs.openjdk.java.net/browse/JDK-8191431
> Webrev : http://cr.openjdk.java.net/~jdv/8191431/webrev.00/
> 
> Issue : When we try to read multiple PNG images with different IDAT chunk positions \
> using the same PNGImageReader instance we get "IIOException: Error reading PNG \
> image data". 
> Root cause : Issue is happening because of changes present in JDK-8164971.
> Under JDK-8164971 we have made changes such that imageStartPosition for IDAT chunk \
> will be updated only once for a given PNGImageReader instance while reading \
> metadata. case IDAT_TYPE:
> // If chunk type is 'IDAT', we've reached the image data.
> if (imageStartPosition == -1L) {
> /*
> * PNGs may contain multiple IDAT chunks containing
> * a portion of image data. We store the position of
> * the first IDAT chunk and continue with iteration
> * of other chunks that follow image data.
> */
> imageStartPosition = stream.getStreamPosition() - 8;
> }
> 
> When we use same PNGImageReader instance to read another PNG image and if IDAT \
> chunk position and length differs we will get IIOException. 
> Solution : We already have resetStreamSettings() method which we call when user \
> tries to decode an image under ImageReader.setInput(). In resetStreamSettings() \
> function if we initialize imageStartPosition to '-1L' it will resolve this issue. 
> Thanks,
> Jay


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