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

List:       openjdk-2d-dev
Subject:    Re: [OpenJDK 2D-Dev] [11] RFR JDK-8191073: JpegImageReader throws IndexOutOfBoundsException when try
From:       Prahalad Kumar Narayanan <prahalad.kumar.narayanan () oracle ! com>
Date:       2018-01-10 6:04:38
Message-ID: f9114aa1-c507-425b-899b-92de286f7f94 () default
[Download RAW message or body]

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


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