[OpenJDK 2D-Dev] [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.