[FFmpeg-devel] [PATCH 10/14] swscale/range_convert: fix mpeg ranges in yuv range conversion for non-8-bit pixel formats

[Christophe Gisquet]

Hi,

Le sam. 28 sept. 2024 à 18:16, Ramiro Polla <ramiro.polla at gmail.com> a écrit :
> I think you missed that the internal representation (the contents of
> dst) is 15 bits for bit_depth <= 14, and 19 bits for bit_depth > 14.
> It is confusing, but when I say bit depths > 14, I mean input bit
> depths > 14, which have an internal representation of 19 bits (and
> therefore the contents of dst are int32_t). Look at chrRangeToJpeg16_c
> for example.

Yes I did miss these, even I suspected it was there: I mentioned I
suspect the discussion would end up about the amount of casting and
function prototypes.

> For bit depth > 14, I had to raise the precision of the calculation in
> order to respect the mpeg range. In this case, the shift is 18 bits
> instead of 14, and the offset becomes greater than 32 bits.

I don't know what the dst buffers are for (or their dynamics), but I
have no problem accepting this is the way.

> lum from 16 coeff 224260 (0x036c04) offset   8589631520 ( 0x01fffb6020)
> chr from 16 coeff 229380 (0x038004) offset   8589672480 ( 0x01fffc0020)
> lum to   16 coeff 306429 (0x04acfd) offset -10041292800 (-0x025681f800)
> chr to   16 coeff 299589 (0x049245) offset  -9817128960 (-0x0249258000)

Yes as you said, only > 14 requires int32_t dst buffers, int32_t coeff
and int64_t offset.
I can understand you didn't want to bother with this rare case beyond
having offset this large.

> The API is indeed a bit confusing because it is shared for all bit
> depths. It should be:
> (int16_t *dst, int width, uint16_t coeff, int32_t offset) for bit_depth <= 14
> and
> (int32_t *dst, int width, uint32_t coeff, int64_t offset) for bit_depth > 14

My point, but cf. just above. Except maybe, use ptrdiff_t for width,
to avoid the Win64 ABI woes.

In any case, I didn't follow well enough the complete changes, and I
didn't look for the corresponding SIMD changes I was also concerned
with. But given the above, it must be fine.