On Wed, 2 Apr 2025 11:25:09 GMT, Sergey Bylokhov <s...@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
