On Wed, 2 Apr 2025 12:24:54 GMT, Sergey Bylokhov <s...@openjdk.org> wrote:
>>>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?
> why we can't use the first component instead of the last one
We definitely cannot do that because of the band-interleaved case: imagine if
there are like 10000 bytes of red samples followed by another 10000 bytes of
green samples - with `pixelStride=1` and `scanlineStride=100` if we take the
first component offset (0), our calculated size would only fit the red channel.
In your calculations using "starting offset + pointer to the last pixel +
pixelStride", you assume only pixel-interleaved variant, but this calculation
would break with other interleaving modes. Actually, for pixel-interleaved
layout there is a simple commonly-used formula, like: `size = startingOffset +
scanlineStride * height`. But the problem is that we don't really know the
`startingOffset`, we only know offsets of the individual components, which
leads to the other issue...
> the size is not necessarily aligned to pixelStride because scanlineStride may
> include a gap larger than pixelStride at the end.
Good point, and because of the way `pixelStride` and `scanlineStride` are
formulated (offsets between same-band samples of the adjacent
pixels/scanlines), they don't tell us anything about the alignment. And
moreover, in the case of unused components, where our `bandOffsets` don't fully
fill the `pixelStride`, there is an ambiguity:
Consider `bandOffsets = {1, 2, 3}` and `pixelStride=4`, is that a `xRGB` layout
with `startingOffset=0`, or `RGBx` with `startingOffset=1`? The answer is - we
don't have enough info as far as I'm concerned...
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24111#discussion_r2024717944
