[OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

[Prahalad Kumar Narayanan]

Hello Jay

I imported your changes & tested the same.
The change doesn't cause new regressions & attached test case passes. 

Looks good.

Thank you
Have a good day

Prahalad N.

----- Original Message -----
From: Jayathirth D V 
Sent: Tuesday, January 09, 2018 2:00 PM
To: Brian Burkhalter; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Hello Brian,

Thanks for reviewing the changes.

I am posting updated webrev since I need one more review and approval  from 2d-dev list.
Updated webrev for review : http://cr.openjdk.java.net/~jdv/8191073/webrev.02/ 

Thanks,
Jay

From: Brian Burkhalter 
Sent: Tuesday, January 09, 2018 2:30 AM
To: Jayathirth D V
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when trying to read image data from tables-only image

Hi Jay,

Sorry but I have a few picky comments.

 377         // If imagePositions list doesn't contain any of the image stream
 378         // starting position(i.e tables-only image) we should not try to access
 379         // imagePositions.size() as it done below, because it will lead to
 380         // IndexOutOfBoundsException with index -1.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then attempting to access the element at index
imagePositions.size() - 1 will cause an IndexOutOfBoundsException.

 381         if (seekForwardOnly && (!(imagePositions.isEmpty()))) {

I think this is more readable if some parentheses are eliminated:

 381         if (seekForwardOnly && !imagePositions.isEmpty()) {

 499         // We should not try to read image information from an input stream
 500         // which only contains tables-only(StreamMetadata) information.

You might consider this verbiage instead:

If the image positions list is empty as in the case of a tables-only stream, then no image data can be read.

No need to update the webrev.

Thanks,

Brian

On Jan 7, 2018, at 11:23 PM, Jayathirth D V <jayathirth.d.v at oracle.com> wrote:

I have removed the usage of tablesOnlyStream variable in code and updated the comment section.
Please find updated webrev for review:
http://cr.openjdk.java.net/~jdv/8191073/webrev.01/