[FFmpeg-devel] [PATCH v2 2/5] swscale/swscale_unscaled: add missing gbrap10 on ff_get_unscaled_swscale

[Muhammad Faiz]

On Tue, Jan 29, 2019 at 5:14 AM Michael Niedermayer
<michael at niedermayer.cc> wrote:
>
> On Mon, Jan 28, 2019 at 05:07:38PM +0700, Muhammad Faiz wrote:
> > Fix inconsistent checksums between gbrap10be and gbrap10le
> > on fate-filter-pixfmts.
> >
> > Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
> > ---
> >  libswscale/swscale_unscaled.c            | 3 +++
> >  tests/ref/fate/filter-pixfmts-copy       | 2 +-
> >  tests/ref/fate/filter-pixfmts-crop       | 2 +-
> >  tests/ref/fate/filter-pixfmts-field      | 2 +-
> >  tests/ref/fate/filter-pixfmts-fieldorder | 2 +-
> >  tests/ref/fate/filter-pixfmts-hflip      | 2 +-
> >  tests/ref/fate/filter-pixfmts-il         | 2 +-
> >  tests/ref/fate/filter-pixfmts-null       | 2 +-
> >  tests/ref/fate/filter-pixfmts-scale      | 2 +-
> >  tests/ref/fate/filter-pixfmts-transpose  | 2 +-
> >  tests/ref/fate/filter-pixfmts-vflip      | 2 +-
> >  11 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/libswscale/swscale_unscaled.c b/libswscale/swscale_unscaled.c
> > index 058f2b94db..734a527e68 100644
> > --- a/libswscale/swscale_unscaled.c
> > +++ b/libswscale/swscale_unscaled.c
> > @@ -1942,6 +1942,7 @@ void ff_get_unscaled_swscale(SwsContext *c)
> >           dstFormat == AV_PIX_FMT_GBRP12LE || dstFormat == AV_PIX_FMT_GBRP12BE ||
> >           dstFormat == AV_PIX_FMT_GBRP14LE || dstFormat == AV_PIX_FMT_GBRP14BE ||
> >           dstFormat == AV_PIX_FMT_GBRP16LE || dstFormat == AV_PIX_FMT_GBRP16BE ||
> > +         dstFormat == AV_PIX_FMT_GBRAP10LE || dstFormat == AV_PIX_FMT_GBRAP10BE ||
> >           dstFormat == AV_PIX_FMT_GBRAP12LE || dstFormat == AV_PIX_FMT_GBRAP12BE ||
> >           dstFormat == AV_PIX_FMT_GBRAP16LE || dstFormat == AV_PIX_FMT_GBRAP16BE ))
> >          c->swscale = Rgb16ToPlanarRgb16Wrapper;
> > @@ -1951,6 +1952,7 @@ void ff_get_unscaled_swscale(SwsContext *c)
> >           srcFormat == AV_PIX_FMT_GBRP10LE || srcFormat == AV_PIX_FMT_GBRP10BE ||
> >           srcFormat == AV_PIX_FMT_GBRP12LE || srcFormat == AV_PIX_FMT_GBRP12BE ||
> >           srcFormat == AV_PIX_FMT_GBRP14LE || srcFormat == AV_PIX_FMT_GBRP14BE ||
> > +         srcFormat == AV_PIX_FMT_GBRAP10LE || srcFormat == AV_PIX_FMT_GBRAP10BE ||
> >           srcFormat == AV_PIX_FMT_GBRAP12LE || srcFormat == AV_PIX_FMT_GBRAP12BE ||
> >           srcFormat == AV_PIX_FMT_GBRAP16LE || srcFormat == AV_PIX_FMT_GBRAP16BE) &&
> >          (dstFormat == AV_PIX_FMT_RGB48LE  || dstFormat == AV_PIX_FMT_RGB48BE  ||
> > @@ -1997,6 +1999,7 @@ void ff_get_unscaled_swscale(SwsContext *c)
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRP12) ||
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRP14) ||
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRP16) ||
> > +        IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRAP10) ||
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRAP12) ||
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_GBRAP16) ||
> >          IS_DIFFERENT_ENDIANESS(srcFormat, dstFormat, AV_PIX_FMT_RGB444) ||
>
> have you tested these ?
> iam asking as the previous iteration had a bug which was missed in testing

fate-filter-pixfmts only covers gbrap10le <-> gbrap10be conversion.
Actually the first two changes fix gbrap10 <-> rgb(a)(64/48) conversions.

This is a simple test:
FFMPEG=./ffmpeg

$FFMPEG -loglevel quiet -filter_complex "allyuv=d=0.01" -pix_fmt
gbrap10le -f rawvideo -y in.rgb

for fmt in rgba64be bgra64be rgba64le bgra64be gbrap10be; do
    $FFMPEG -loglevel quiet -s 4096x4096 -pix_fmt gbrap10le -f
rawvideo -i in.rgb -vf format=$fmt -pix_fmt gbrap10le -f rawvideo -y
out-$fmt.rgb
    if cmp in.rgb out-$fmt.rgb; then
        echo "in.rgb out-$fmt.rgb are equal"
    fi
done

> also adding a test or tests to fate which would cover that codepath that was
> missed could be a good idea

As I suggested in 1/5, adding a test that covers all lossless
conversions (not only gbrap10 case) is a good idea.
But that is beyond of this patch.

(Also note that p010 and p016 problems haven't yet been fixed.)

>
> the change itself looks fine i think but i have not tested it beyond seeing
> that it changes the output for some files
>
> Thanks