[11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

[11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Prahalad kumar Narayanan
Hello Everyone

Good day to you.

Request your time to review the fix for the bug:
JDK-8194489    Incorrect size computation at BandedSampleModel . createDataBuffer

Root Cause:
. The method BandedSampleModel . createDataBuffer does not consider number of banks and band offsets while computing the required memory size.
. As a result, ArrayIndexOutOfBounds exception is thrown when setting pixel values on banded sample models having -
      . Multiple bands of image data stored in multiple banks of DataBuffer with band offsets
      . Multiple bands of image data stored in single bank of DataBuffer

Solution:
. Appropriate logic has been added to createDataBuffer method.

Other Info:
. All Jtreg test-cases in java/awt/image were run with the build including the fix.
. No regressions were noticed.

Kindly review the changes at your convenience & share your feedback.
Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.00/

Thank you for your time in review
Have a good day

Prahalad N.
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Brian Burkhalter-2
Hello Prahalad,

On Jan 4, 2018, at 9:30 PM, Prahalad Kumar Narayanan <[hidden email]> wrote:

Request your time to review the fix for the bug:
JDK-8194489    Incorrect size computation at BandedSampleModel . createDataBuffer

https://bugs.openjdk.java.net/browse/JDK-8194489

Kindly review the changes at your convenience & share your feedback.
Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.00/

In general I think this looks good. A few comments:

BandedSampleModel
L193-196 and 203-207: In general I think it’s better for method-internal comments to use the “//“ form instead of the “/*…*/“ form.

BandedSampleModelSizeTest
Instead of defining the utility methods compareSamples(int[],int[]) and initSamples(int[],int) you might consider simply using j.u.Arrays.equals(int[],int[]) and j.u.Arrays.fill(int[],int), respectively.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Sergey Bylokhov
In reply to this post by Prahalad kumar Narayanan
Hi, Prahalad.
Not an expert here, but have a small question about implementation of
this method in the parent class(ComponentSampleModel).
Currently the parent class has a similar implementation of
createDataBuffer(), but instead of "scanlineStride * height" it uses
"getBufferSize()". And this "getBufferSize" has a number of additional
checks, should not we do something similar here as well(see JDK-7058602)?

On 04/01/2018 21:30, Prahalad Kumar Narayanan wrote:

> Request your time to review the fix for the bug:
> JDK-8194489    Incorrect size computation at BandedSampleModel . createDataBuffer
>
> Root Cause:
> . The method BandedSampleModel . createDataBuffer does not consider number of banks and band offsets while computing the required memory size.
> . As a result, ArrayIndexOutOfBounds exception is thrown when setting pixel values on banded sample models having -
>        . Multiple bands of image data stored in multiple banks of DataBuffer with band offsets
>        . Multiple bands of image data stored in single bank of DataBuffer
>
> Solution:
> . Appropriate logic has been added to createDataBuffer method.
>
> Other Info:
> . All Jtreg test-cases in java/awt/image were run with the build including the fix.
> . No regressions were noticed.
>
> Kindly review the changes at your convenience & share your feedback.
> Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.00/
>
> Thank you for your time in review
> Have a good day
>
> Prahalad N.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Prahalad kumar Narayanan
Hello Sergey

Thank you for your time in review.

Here are some findings that might help in our discussion:

. On Validating- scanlineStride, width, height with checks
      . When a user creates a BandedSampleModel object, the values of- scanlineStride, width and height are checked for non-negative values in the constructors.
      . The only additional check that is present in ComponentSampleModel 's getBufferSize method is the check for- (Integer.MAX_VALUE -1).
      . I don’t understand how this check helps. Does it avoid OutOfMemoryErrors during DataBuffer allocation? I'm not sure.
      . Hence, I didn't add additional validation to the fix proposed.

. On Specification & throws clause:
      . I looked into the specification of createDataBuffer method in- BandedSampleModel & ComponentSampleModel
      . Banded sample model mentions-
           * @throws IllegalArgumentException if {@code dataType} is not
           *                     one of the supported data types
           */
      . So if we wish to take up additional validation for our values- scanlineStride, width and height, we should update this spec as well. Kindly let me know your thoughts.
      . Surprisingly, ComponentSampleModel does not mention any throws clause in its spec for createDataBuffer method.
        But it throws quite a few IllegalArgumentExceptions in its getBufferSize invoked from createDataBuffer.

. On ComponentSampleModel's Buffer Size Calculation
      . In addition to above points, the logic to calculate buffer size within ComponentSampleModel's getBufferSize() is incorrect.
      . The documentation of the class says that it can be used to store images that are- Band interleaved, Pixel interleaved & Scanline interleaved.
      . For each of the above types- the values of scanline stride, pixel stride, number of banks would vary.
      . But ComponentSampleModel 's getBufferSize seems to have a default logic that doesn't seem to consider these scenarios.
      . There is one open bug- https://bugs.openjdk.java.net/browse/JDK-6604105.

Thank you for your review & this discussion
Have a good day

Prahalad N.


-----Original Message-----
From: Sergey Bylokhov
Sent: Saturday, January 06, 2018 2:13 PM
To: Prahalad Kumar Narayanan; 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Hi, Prahalad.
Not an expert here, but have a small question about implementation of this method in the parent class(ComponentSampleModel).
Currently the parent class has a similar implementation of createDataBuffer(), but instead of "scanlineStride * height" it uses "getBufferSize()". And this "getBufferSize" has a number of additional checks, should not we do something similar here as well(see JDK-7058602)?

On 04/01/2018 21:30, Prahalad Kumar Narayanan wrote:

> Request your time to review the fix for the bug:
> JDK-8194489    Incorrect size computation at BandedSampleModel . createDataBuffer
>
> Root Cause:
> . The method BandedSampleModel . createDataBuffer does not consider number of banks and band offsets while computing the required memory size.
> . As a result, ArrayIndexOutOfBounds exception is thrown when setting pixel values on banded sample models having -
>        . Multiple bands of image data stored in multiple banks of DataBuffer with band offsets
>        . Multiple bands of image data stored in single bank of
> DataBuffer
>
> Solution:
> . Appropriate logic has been added to createDataBuffer method.
>
> Other Info:
> . All Jtreg test-cases in java/awt/image were run with the build including the fix.
> . No regressions were noticed.
>
> Kindly review the changes at your convenience & share your feedback.
> Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.00/
>
> Thank you for your time in review
> Have a good day
>
> Prahalad N.
>


--
Best regards, Sergey.
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Prahalad kumar Narayanan
In reply to this post by Brian Burkhalter-2
Hello Brian

Thank you for your time in review & suggestions.

> BandedSampleModel
> L193-196 and 203-207: In general I think it's better for method-internal comments to use the "//" form instead of the "/*.*/" form.
   My views:
      . I find interspersed use of // and /* */ for multi-line comments in our client libs code.
      . For example: The D3DBlitLoops uses "//" while many parts of Image IO use /* ... */ for multi-line comments.
      . While implementing the fix, I try to maintain uniformity established by the file getting modified.
      . In general, I refer to this guideline- http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

> BandedSampleModelSizeTest
> Instead of defining the utility methods compareSamples(int[],int[]) and initSamples(int[],int)
> you might consider simply using j.u.Arrays.equals(int[],int[]) and j.u.Arrays.fill(int[],int), respectively.      
   My views:
      . Thank you for the suggestion. It helped.
      . I 've modified the test code to use Arrays.fill and Arrays.equals methods.

The modified code is now available for review under
Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.01/

Kindly review at your convenience and share your thoughts.

Thank you for your time in review
Have a good day

Prahalad N.


----- Original Message -----
From: Brian Burkhalter
Sent: Saturday, January 06, 2018 4:22 AM
To: Prahalad Kumar Narayanan
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Hello Prahalad,

On Jan 4, 2018, at 9:30 PM, Prahalad Kumar Narayanan <[hidden email]> wrote:


Request your time to review the fix for the bug:
JDK-8194489    Incorrect size computation at BandedSampleModel . createDataBuffer

https://bugs.openjdk.java.net/browse/JDK-8194489


Kindly review the changes at your convenience & share your feedback.
Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.00/

In general I think this looks good. A few comments:

BandedSampleModel
L193-196 and 203-207: In general I think it's better for method-internal comments to use the "//" form instead of the "/*.*/" form.

BandedSampleModelSizeTest
Instead of defining the utility methods compareSamples(int[],int[]) and initSamples(int[],int) you might consider simply using j.u.Arrays.equals(int[],int[]) and j.u.Arrays.fill(int[],int), respectively.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Brian Burkhalter-2
Hi Prahalad,

On Jan 9, 2018, at 3:42 AM, Prahalad Kumar Narayanan <[hidden email]> wrote:

BandedSampleModel
L193-196 and 203-207: In general I think it's better for method-internal comments to use the "//" form instead of the "/*.*/" form.
  My views:
     . I find interspersed use of // and /* */ for multi-line comments in our client libs code.
     . For example: The D3DBlitLoops uses "//" while many parts of Image IO use /* ... */ for multi-line comments.
     . While implementing the fix, I try to maintain uniformity established by the file getting modified.

I think that is a good approach.

     . In general, I refer to this guideline- http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

For the record I tend to prefer the “//“ for intra-method comments as I have found that not infrequently in debugging one needs to comment out a section and this is much easier to do if the comments in question do not use the “/*…*/“ style. I think you can leave this one as-is however.

BandedSampleModelSizeTest
Instead of defining the utility methods compareSamples(int[],int[]) and initSamples(int[],int) 
you might consider simply using j.u.Arrays.equals(int[],int[]) and j.u.Arrays.fill(int[],int), respectively.  
  My views:
     . Thank you for the suggestion. It helped.
     . I 've modified the test code to use Arrays.fill and Arrays.equals methods.

The modified code is now available for review under
Link: http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.01/

Kindly review at your convenience and share your thoughts.

I think this looks OK.

Thanks,

Brian
Reply | Threaded
Open this post in threaded view
|

Re: [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

Jayathirth D v

Hi Prahalad,

 

Change looks good to me.

 

Thanks,

Jay

 

From: Brian Burkhalter
Sent: Wednesday, January 10, 2018 6:52 AM
To: Prahalad Kumar Narayanan
Cc: 2d-dev
Subject: Re: [OpenJDK 2D-Dev] [11] RFR: [JDK-8194489] Incorrect size computation at BandedSampleModel.createDataBuffer

 

Hi Prahalad,

 

On Jan 9, 2018, at 3:42 AM, Prahalad Kumar Narayanan <[hidden email]> wrote:



BandedSampleModel
L193-196 and 203-207: In general I think it's better for method-internal comments to use the "//" form instead of the "/*.*/" form.

  My views:
     . I find interspersed use of // and /* */ for multi-line comments in our client libs code.
     . For example: The D3DBlitLoops uses "//" while many parts of Image IO use /* ... */ for multi-line comments.
     . While implementing the fix, I try to maintain uniformity established by the file getting modified.

 

I think that is a good approach.



     . In general, I refer to this guideline- http://cr.openjdk.java.net/~alundblad/styleguide/index-v6.html

 

For the record I tend to prefer the “//“ for intra-method comments as I have found that not infrequently in debugging one needs to comment out a section and this is much easier to do if the comments in question do not use the “/*…*/“ style. I think you can leave this one as-is however.



BandedSampleModelSizeTest
Instead of defining the utility methods compareSamples(int[],int[]) and initSamples(int[],int) 
you might consider simply using j.u.Arrays.equals(int[],int[]) and j.u.Arrays.fill(int[],int), respectively.  

  My views:
     . Thank you for the suggestion. It helped.
     . I 've modified the test code to use Arrays.fill and Arrays.equals methods.

The modified code is now available for review under
Link: 
http://cr.openjdk.java.net/~pnarayanan/8194489/webrev.01/

Kindly review at your convenience and share your thoughts.

 

I think this looks OK.

 

Thanks,

 

Brian