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

[Christophe Gisquet]

Hello,

Le lun. 23 sept. 2024 à 16:19, Ramiro Polla <ramiro.polla at gmail.com> a écrit :
> For bit depths <= 14, amax is 16-bit and offset is 32-bit.
> For bit depths > 14, amax is 32-bit and offset is 64-bit.

[...]

> -static void chrRangeToJpeg_c(int16_t *dstU, int16_t *dstV, int width)
> +static void chrRangeToJpeg_c(int16_t *dstU, int16_t *dstV, int width,
> +                             int amax, int coeff, int64_t _offset)
>  {
> +    int offset = _offset;
>      int i;
>      for (i = 0; i < width; i++) {
> -        dstU[i] = (FFMIN(dstU[i], 30775) * 4663 - 9289992) >> 12; // -264
> -        dstV[i] = (FFMIN(dstV[i], 30775) * 4663 - 9289992) >> 12; // -264
> +        dstU[i] = (FFMIN(dstU[i], amax) * coeff + offset) >> 14;
> +        dstV[i] = (FFMIN(dstV[i], amax) * coeff + offset) >> 14;
>      }
>  }
>
> -static void chrRangeFromJpeg_c(int16_t *dstU, int16_t *dstV, int width)
> +static void chrRangeFromJpeg_c(int16_t *dstU, int16_t *dstV, int width,
> +                               int amax, int coeff, int64_t _offset)
>  {
> +    int offset = _offset;
>      int i;
>      for (i = 0; i < width; i++) {
> -        dstU[i] = (dstU[i] * 1799 + 4081085) >> 11; // 1469
> -        dstV[i] = (dstV[i] * 1799 + 4081085) >> 11; // 1469
> +        dstU[i] = (dstU[i] * coeff + offset) >> 14;
> +        dstV[i] = (dstV[i] * coeff + offset) >> 14;
>      }
>  }
>
> -static void lumRangeToJpeg_c(int16_t *dst, int width)
> +static void lumRangeToJpeg_c(int16_t *dst, int width,
> +                             int amax, int coeff, int64_t _offset)
>  {
> +    int offset = _offset;
>      int i;
>      for (i = 0; i < width; i++)
> -        dst[i] = (FFMIN(dst[i], 30189) * 19077 - 39057361) >> 14;
> +        dst[i] = (FFMIN(dst[i], amax) * coeff + offset) >> 14;
>  }

I'm a bit surprised by some of these formulas and the range you assert
above. Somehow you make it clear by casting the offset parameters to
narrower types, so all of the following is a non-issue.

So, maybe some cases are special and so on, and you are working within
some API constrain, but the code excerpts bother me a bit:
* If dst as input is 16 bits, I don't see why amax would really be an
int (except native integer etc)
* If dst[i] = anything >> 14, with dst[i] 16 bits, you want "anything"
to be at most of 30 bits of dynamics. offset doesn't sound like it
could be of a wider range, and not 64 bits.
* Any version that does treat things as 64 bits computations, will
essentially halve the likely throughput of the SIMD, besides
potentially using way slower computations and restricting these to
fewer archs.

I expect you know that, but I'd have expected an intermediate type of
functions with parameters eg '(int16_t *dst, int width, int16_t amax,
int16_t coeff, int32_t _offset)'
Maybe to avoid issues with the weird Win ABI where the MSBs can be
garbage, that can indeed be '(int16_t *dst, ptrdiff_t width, int amax,
int16_t coeff, int32_t _offset)'.

With regards,
-- 
Christophe