[FFmpeg-devel] [PATCH 2/2] libswcale/input: fix incorrect rgbf32 yuv conversions
Michael Niedermayer
michael at niedermayer.cc
Mon Sep 28 20:33:40 EEST 2020
On Sat, Sep 26, 2020 at 10:01:30PM -0700, Mark Reid wrote:
> On Mon, Sep 14, 2020 at 6:31 PM Mark Reid <mindmark at gmail.com> wrote:
>
> >
> >
> > On Mon, Sep 14, 2020 at 2:44 PM Michael Niedermayer <michael at niedermayer.cc>
> > wrote:
> >
> >> On Sun, Sep 13, 2020 at 04:04:42PM -0700, Mark Reid wrote:
> >> > On Sun, Sep 13, 2020 at 8:55 AM Michael Niedermayer
> >> <michael at niedermayer.cc>
> >> > wrote:
> >> >
> >> > > On Sat, Sep 12, 2020 at 02:07:14AM -0700, mindmark at gmail.com wrote:
> >> > > > From: Mark Reid <mindmark at gmail.com>
> >> > > >
> >> > > > ---
> >> > > > libswscale/input.c | 12 +++++-------
> >> > > > tests/ref/fate/filter-pixfmts-scale | 8 ++++----
> >> > > > 2 files changed, 9 insertions(+), 11 deletions(-)
> >> > >
> >> > > Can you provide some tests that show that this is better ?
> >> > > Iam asking that because some of the numbers in some of the code
> >> > > (i dont remember which) where tuned to give more accurate overall
> >> results
> >> > >
> >> > > an example for tests would be converting from A->B->A then compare to
> >> the
> >> > > orig
> >> > >
> >> > > thx
> >> > >
> >> > >
> >> > Hopefully i can explain this clearly!
> >> >
> >> > It's easier to see the error if you run a black image through the old
> >> > conversion.
> >> > zero values don't get mapped to zero. (attached sample image)
> >> >
> >> > ffmpeg -i 4x4_zero.exr -pix_fmt rgb48le -f rawvideo 4x4_zero.rawvideo
> >> > The image should be rgb 0, 0, 0 everywhere but instead it's 353, 0, 407
> >> >
> >> >
> >> > I think this is a error in fixed point rounding, the issue is basically
> >> > boils down to
> >> >
> >> > 128 << 8 != 257 << 7
> >> > and
> >> > 16 << 8 != 33 << 7
> >> >
> >> > https://en.wikipedia.org/wiki/YUV#Studio_swing_for_BT.601
> >> > the 8 bit rgb to yuv formula is
> >> >
> >> > Y = ry*r + gy*g + by*b + 16
> >> > U = ru*r + gu*g + bu*b + 128
> >> > V = rv*r + gv*g + bv*b + 128
> >> >
> >> > I think the studio swing offsets at the end are calculated wrong in the
> >> old
> >> > code.
> >> > (257 << (RGB2YUV_SHIFT + bpc - 9)))
> >> > 257 is correct for 8 bit rounding but not for 16-bit.
> >> >
> >> > the 257 i believe is from (128 << 1) + 1
> >> > the +1 is for rounding
> >> >
> >> > for rounding 16-bit (128 << 9) + 1 = 0x10001
> >> >
> >> > therefore I think the correct rounding any bit depth with the old
> >> formula
> >> > would be (untested)
> >> > (((128 << (bpc - 7)) + 1) << (RGB2YUV_SHIFT-1) )
> >> >
> >> > I just simplified it to
> >> > (0x10001 << (RGB2YUV_SHIFT - 1))
> >> >
> >> > The rgb48ToUV and rgb48ToY funcs in input.c use the formula I'm using.
> >>
> >> You quite possibly are correct, can you test that this actually works
> >> out. The test sample only covers 1 color (black)
> >> a testsample covering a wide range of the color cube would be more
> >> convincing that this change is needed and sufficient to fix this.
> >>
> >> thx
> >>
> >
> > I wrote a small python script to compare the raw gbrpf32le images if that
> > works? I attached it and also a more colorful test pattern.
> >
> > it runs these two commands and compares the 2 raw float images
> > ffmpeg -y -i test_pattern.exr -f rawvideo original.gbrpf32le
> > ffmpeg -y -i test_pattern.exr -vf
> > format=pix_fmts=rgb48le,format=pix_fmts=gbrpf32le -f rawvideo
> > converted.gbrpf32le
> >
> > python gbrpf32le_diff.py test_pattern.exr
> >
> > without patch:
> > avg error: 237.445495855
> > min error: 0.0
> > max error: 468.399102688
> >
> > with patch:
> > avg error: 15.9312244829
> > min error: 0.0
> > max error: 69.467689991
> >
> >
> > These are floating points scaled to 16-bit values.
> > Even with my patch, I'm kinda disturbed how much error there is.
> >
>
> ping
> I re-wrote the python script as a c swscale test, if that helps
> replicate my results. attached is patch for that.
> it generates an image of random float values and does the
> conversion/comparison .
>
> before patch:
> gbrpf32le -> yuva444p16le -> gbrpf32le
> avg diff: 0.003852
> min diff: 0.000000
> max diff: 0.006638
>
> after patch:
> gbrpf32le -> yuva444p16le -> gbrpf32le
> avg diff: 0.000125
> min diff: 0.000000
> max diff: 0.000501
>
>
> >
> >
> >>
> >> [...]
> >>
> >> --
> >> Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >>
> >> There will always be a question for which you do not know the correct
> >> answer.
> >> _______________________________________________
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel at ffmpeg.org
> >> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >> To unsubscribe, visit link above, or email
> >> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> >
> >
> Makefile | 1
> tests/.gitignore | 1
> tests/floatimg_cmp.c | 267 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 269 insertions(+)
> a04783297b0a5d9be6186a150606724457e7b0c5 0001-libswscale-tests-add-floatimg_cmp-test.patch
> From 9c4bc86201037aec454a98b60301d9250fecc7ca Mon Sep 17 00:00:00 2001
> From: Mark Reid <mindmark at gmail.com>
> Date: Sun, 20 Sep 2020 20:45:08 -0700
> Subject: [PATCH] libswscale/tests: add floatimg_cmp test
>
> ---
> libswscale/Makefile | 1 +
> libswscale/tests/.gitignore | 1 +
> libswscale/tests/floatimg_cmp.c | 267 ++++++++++++++++++++++++++++++++
> 3 files changed, 269 insertions(+)
> create mode 100644 libswscale/tests/floatimg_cmp.c
>
> diff --git a/libswscale/Makefile b/libswscale/Makefile
> index 5e03e6fa0a..4b8f9de425 100644
> --- a/libswscale/Makefile
> +++ b/libswscale/Makefile
> @@ -25,5 +25,6 @@ OBJS-$(CONFIG_SHARED) += log2_tab.o
> SLIBOBJS-$(HAVE_GNU_WINDRES) += swscaleres.o
>
> TESTPROGS = colorspace \
> + floatimg_cmp \
> pixdesc_query \
> swscale \
> diff --git a/libswscale/tests/.gitignore b/libswscale/tests/.gitignore
> index 1a26f038c4..c56abf0ee7 100644
> --- a/libswscale/tests/.gitignore
> +++ b/libswscale/tests/.gitignore
> @@ -1,3 +1,4 @@
> /colorspace
> +/floatimg_cmp
> /pixdesc_query
> /swscale
> diff --git a/libswscale/tests/floatimg_cmp.c b/libswscale/tests/floatimg_cmp.c
> new file mode 100644
> index 0000000000..affb6fa492
> --- /dev/null
> +++ b/libswscale/tests/floatimg_cmp.c
This seems to produce some compiler warnings
also turning this into a fate test would be a good idea i think
CC libswscale/tests/floatimg_cmp.o
libswscale/tests/floatimg_cmp.c: In function ‘main’:
libswscale/tests/floatimg_cmp.c:118:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
const AVPixFmtDescriptor *desc = av_pix_fmt_desc_get(inFormat);
^~~~~
libswscale/tests/floatimg_cmp.c:162:13: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
uint8_t *ptr = rgbIn[p];
^~~~~~~
libswscale/tests/floatimg_cmp.c:197:26: warning: passing argument 2 of ‘sws_scale’ from incompatible pointer type [-Wincompatible-pointer-types]
res = sws_scale(sws, rgbIn, rgbStride, 0, h, (uint8_t * const *) dst, dstStride);
^~~~~
In file included from libswscale/tests/floatimg_cmp.c:35:0:
./libswscale/swscale.h:217:5: note: expected ‘const uint8_t * const* {aka const unsigned char * const*}’ but argument is of type ‘uint8_t ** {aka unsigned char **}’
int sws_scale(struct SwsContext *c, const uint8_t *const srcSlice[],
^~~~~~~~~
libswscale/tests/floatimg_cmp.c:212:26: warning: passing argument 2 of ‘sws_scale’ from incompatible pointer type [-Wincompatible-pointer-types]
res = sws_scale(sws, dst, dstStride, 0, h, (uint8_t * const *) rgbOut, rgbStride);
^~~
In file included from libswscale/tests/floatimg_cmp.c:35:0:
./libswscale/swscale.h:217:5: note: expected ‘const uint8_t * const* {aka const unsigned char * const*}’ but argument is of type ‘uint8_t ** {aka unsigned char **}’
int sws_scale(struct SwsContext *c, const uint8_t *const srcSlice[],
^~~~~~~~~
libswscale/tests/floatimg_cmp.c:219:5: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
double sum = 0;
^~~~~~
libswscale/tests/floatimg_cmp.c:240:17: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
float diff = fabsf(v0.f - v1.f);
^~~~~
LD libswscale/tests/floatimg_cmp
thx
[...]
--
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
Its not that you shouldnt use gotos but rather that you should write
readable code and code with gotos often but not always is less readable
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20200928/0aae6c6a/attachment.sig>
More information about the ffmpeg-devel
mailing list