RFR: 8352407: PixelInterleavedSampleModel with unused components throws RasterFormatException: Incorrect pixel stride [v2]

[Sergey Bylokhov]

On Wed, 2 Apr 2025 11:25:09 GMT, Sergey Bylokhov <serb at openjdk.org> wrote:

>> As I understand, an important thing here is that this calculation needs to be correct for any kind of pixel/scanline/band interleaving.
>> What if our channels are stored separately (complete image of only R followed by complete image of G and so on) - your formula would only work if we change "**pointer to the last pixel**" -> "**pointer to the last band of the last pixel**". But then it would be wrong for the pixel-interleaving case (RGBA for each pixel stored together), because adding `pixelStride=4` to the "**last band of the last pixel**" would actually be 3 bytes more than needed.
>> 
>> The idea of the current approach, as I understand it is that we find the last band of the first pixel, be it 3 for pixel-interleaved RGBA, or 30000 for band-interleaved image, and then add 1 to it, meaning the size sufficient to fit that last band:
>> 
>> int size = maxBandOff + 1;
>> 
>> At this point we can already fit the first pixel, so what's left is to fit the remaining first scanline (hence `width - 1`):
>> 
>> int val = pixelStride * (width - 1);
>> size += val;
>> 
>> And the remaining rows (hence `height - 1`):
>> 
>> val = scanlineStride * (height - 1);
>> size += val;
>> 
>> 
>> ### This gets us to our subject:
>> 
>> As current calculation only considers used bands by finding `maxBandOff`, it doesn't really care when we have pixel interleaving and `pixelStride > numBands`, it will happily cut any unused space at the end of the last pixel, thus causing various problems when we just want to, say, make an RGBx (`pixelStride=4`, `bandOffsets = {0, 1, 2}`) raster.
>> 
>> In my approach I just fixed that space which was cut by the previous calculation by aligning the size up to `pixelStride`. Not that I find this particularly elegant, but it seems to work and not cause regressions in non-pixel interleaved variants.
>
>>The idea of the current approach, as I understand it is that we find the last band of the first pixel, be it 3 for pixel-interleaved RGBA, or 30000 for band-interleaved image, and then add 1 to it, meaning the size sufficient to fit that last band:
>>int size = maxBandOff + 1;
>>At this point we can already fit the first pixel, so what's left is to fit the remaining first scanline (hence width - 1):
> 
> This looks suspicious. Let's try calculating the size or pointer to the last pixel using the formula:
> "starting offset + pointer to the last pixel + pixelStride" but in reverse order.
>    
>     // This moves the pointer to the start of the last row.
>     size = scanlineStride * (height - 1); 
>     // This moves the pointer to the start of the last pixel in the last row.
>     size += pixelStride * (width - 1);
> 
> **Note**: At this point, the size is not necessarily aligned to pixelStride because scanlineStride may include a gap larger than pixelStride at the end.
> 
> Now, we need to account for the last pixel in the last row. In a theoretical case where the offset is 0, simply adding pixelStride one more time would be sufficient.
> 
> However, in our case, we add maxBandOff + 1, which seems odd since maxBandOff represents the offset of the largest component, placing it somewhere within the pixelStride.
> 
> To fix this, we should actually add the second part of the pixelStride only if maxBandOff is smaller than pixelStride.
> 
> btw, I'm not sure why we can't use the first component instead of the last one:
> 
> 
>  int minBandOff = bandOffsets[0];
>  for (int i = 1; i < bandOffsets.length; i++) {
>      minBandOff = Math.min(minBandOff, bandOffsets[i]);
>  }
> 
>  long size = 0;
> 
>  if (minBandOff >= 0)
>      size += minBandOff;
>  if (pixelStride > 0)
>      size += pixelStride * width;
>  if (scanlineStride > 0)
>      size += scanlineStride * (height - 1);         
>  return (int) size;
> 
> 
> ...Still investigating this.

Or maybe we should modify the code that relies on the current size?

Why does our code work fine when skipping the end of the scanline in cases where there is a gap, but it cannot skip the pixelStride when there is a gap as well?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2024716514