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

[Martin Storsjö]

On Sun, 29 Sep 2024, Ramiro Polla wrote:

> On Sat, Sep 28, 2024 at 11:41 PM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
>> On Fri, Sep 27, 2024 at 02:52:37PM +0200, Ramiro Polla wrote:
>> > There is an issue with the constants used in YUV to YUV range conversion,
>> > where the upper bound is not respected when converting to mpeg range.
>> >
>> > With this commit, the constants are calculated at runtime, depending on
>> > the bit depth. This approach also allows us to more easily understand how
>> > the constants are derived.
>> >
>> > For bit depths <= 14, the number of fixed point bits has been set to 14
>> > for all conversions, to simplify the code.
>> > For bit depths > 14, the number of fixed points bits has been raised and
>> > set to 18, to allow for the conversion to be accurate enough for the mpeg
>> > range to be respected.
>> >
>> > The convert functions now take the conversion constants (coeff and offset)
>> > as function arguments.
>> > For bit depths <= 14, offset is 32-bit.
>> > For bit depths > 14, offset is 64-bit.
>> >
>> > x86_64:
>> > chrRangeFromJpeg8_1920_c:    5804.5  5845.2 ( 0.99x)
>> > chrRangeFromJpeg16_1920_c:   5792.8  5809.1 ( 1.00x)
>> > chrRangeToJpeg8_1920_c:      9388.6  9462.2 ( 0.99x)
>> > chrRangeToJpeg16_1920_c:     5796.5  9261.5 ( 0.63x)
>> > lumRangeFromJpeg8_1920_c:    4147.9  4191.4 ( 0.99x)
>> > lumRangeFromJpeg16_1920_c:   4529.0  4143.4 ( 1.09x)
>> > lumRangeToJpeg8_1920_c:      5694.1  5720.5 ( 1.00x)
>> > lumRangeToJpeg16_1920_c:     5334.2  5139.5 ( 1.04x)
>> >
>> > aarch64 A55:
>> > chrRangeFromJpeg8_1920_c:   28833.8 28834.8 ( 1.00x)
>> > chrRangeFromJpeg16_1920_c:  28842.8 28840.6 ( 1.00x)
>> > chrRangeToJpeg8_1920_c:     23070.6 23072.5 ( 1.00x)
>> > chrRangeToJpeg16_1920_c:    17313.8 23075.1 ( 0.75x)
>> > lumRangeFromJpeg8_1920_c:   15388.1 15386.7 ( 1.00x)
>> > lumRangeFromJpeg16_1920_c:  15388.0 15383.8 ( 1.00x)
>> > lumRangeToJpeg8_1920_c:     19226.2 19223.6 ( 1.00x)
>> > lumRangeToJpeg16_1920_c:    19225.5 19225.5 ( 1.00x)
>> >
>> > aarch64 A76:
>> > chrRangeFromJpeg8_1920_c:    6317.8  6318.5 ( 1.00x)
>> > chrRangeFromJpeg16_1920_c:   6322.9  6323.5 ( 1.00x)
>> > chrRangeToJpeg8_1920_c:      9287.1  9170.0 ( 1.01x)
>> > chrRangeToJpeg16_1920_c:     6104.9  9195.6 ( 0.66x)
>> > lumRangeFromJpeg8_1920_c:    4359.1  4425.5 ( 0.98x)
>> > lumRangeFromJpeg16_1920_c:   4358.8  4436.8 ( 0.98x)
>> > lumRangeToJpeg8_1920_c:      5957.2  6017.2 ( 0.99x)
>> > lumRangeToJpeg16_1920_c:     6072.5  6017.2 ( 1.01x)
>> >
>> > NOTE: all simd optimizations for range_convert have been disabled.
>> >       they will be re-enabled when they are fixed for each architecture.
>> >
>> > NOTE2: the same issue still exists in rgb2yuv conversions, which is not
>> >        addressed in this commit.
>> > ---
>>
>> seems to break fate:
>>
>> make -j32 fate-filter-owdenoise-sample
>> TEST    filter-owdenoise-sample
>> stddev:12247.77 PSNR: 14.57 MAXDIFF:65280 bytes:   576000/   576000
>> MAXDIFF: |65280 - 1| >= 3539
>> Test filter-owdenoise-sample failed. Look at tests/data/fate/filter-owdenoise-sample.err for details.
>> make: *** [tests/Makefile:311: fate-filter-owdenoise-sample] Error 1
>
> Ok, I got it now. The reference is in the fate suite samples. That's
> why it doesn't show up on git diff after running fate with GEN=1.
>
> It seems to me there is an unnecessary "-color range mpeg" in that
> test. But fixing this requires changing both the reference in the fate
> suite and the test in tests/fate/filter-video.mak. How can we fix this
> without breaking fate for people that haven't run make fate-rsync?
> Perhaps disable the test, update the reference, wait a couple of days,
> and enable the test again without -color_range? Or just keeping a
> resulting checksum in git, so that it can more easily be updated when
> needed?

People may want to run fate on old checkouts or old releases too, so we 
can't really change the reference file. If we need to, we'd have to update 
a new reference file with a new name, and update the tests to compare 
against that.

// Martin