[FFmpeg-devel] [PATCH 1/5] tests/fate-run: only support le format result on pixfmts

Michael Niedermayer michael at niedermayer.cc
Mon Jan 28 02:22:26 EET 2019


On Mon, Jan 28, 2019 at 05:29:41AM +0700, Muhammad Faiz wrote:
> On Mon, Jan 28, 2019 at 3:07 AM Michael Niedermayer
> <michael at niedermayer.cc> wrote:
> >
> > On Sun, Jan 27, 2019 at 11:24:46PM +0700, Muhammad Faiz wrote:
> > > On Sun, Jan 27, 2019 at 10:53 PM Michael Niedermayer
> > > <michael at niedermayer.cc> wrote:
> > > >
> > > > On Sun, Jan 27, 2019 at 04:36:15PM +0700, Muhammad Faiz wrote:
> > > > > regardless of the actual supported formats.
> > > > > This allows filters to support only native-endian formats,
> > > > > and also allows consistency checks between little-endian
> > > > > and big-endian implementation.
> > > > >
> > > > > This also reveals bugs on gbrap10, p010, p016 format, and
> > > > > super2xsai filter (mismatched checksums between little-endian
> > > > > and big-endian).
> > > > >
> > > > > Suggested-by: Hendrik Leppkes <h.leppkes at gmail.com>
> > > > > Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
> > > > > ---
> > > > > Old thread is here: https://ffmpeg.org/pipermail/ffmpeg-devel/2016-June/195930.html
> > > > >
> > > > >  tests/fate-run.sh                        |   6 +-
> > > > >  tests/ref/fate/filter-pixfmts-copy       | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-crop       | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-field      | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-fieldorder |  88 +++++++++---------
> > > > >  tests/ref/fate/filter-pixfmts-hflip      | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-il         | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-null       | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-scale      | 112 +++++++++++------------
> > > > >  tests/ref/fate/filter-pixfmts-super2xsai |   8 +-
> > > > >  tests/ref/fate/filter-pixfmts-swapuv     |  56 ++++++------
> > > > >  tests/ref/fate/filter-pixfmts-transpose  |  90 +++++++++---------
> > > > >  tests/ref/fate/filter-pixfmts-vflip      | 112 +++++++++++------------
> > > > >  13 files changed, 573 insertions(+), 571 deletions(-)
> > > > >
> > > > > diff --git a/tests/fate-run.sh b/tests/fate-run.sh
> > > > > index aece90a01d..faa4285e71 100755
> > > > > --- a/tests/fate-run.sh
> > > > > +++ b/tests/fate-run.sh
> > > > > @@ -297,8 +297,10 @@ pixfmts(){
> > > > >
> > > > >      outertest=$test
> > > > >      for pix_fmt in $pix_fmts; do
> > > > > -        test=$pix_fmt
> > > > > -        video_filter "${prefilter_chain}format=$pix_fmt,$filter=$filter_args" -pix_fmt $pix_fmt -frames:v $nframes
> > > > > +        # force little endian format on result
> > > > > +        pix_fmt_le=`echo $pix_fmt | sed 's/be$/le/'`
> > > > > +        test=$pix_fmt_le
> > > > > +        video_filter "${prefilter_chain}format=$pix_fmt,$filter=$filter_args" -pix_fmt $pix_fmt_le -frames:v $nframes
> > > > >      done
> > > >
> > > > if the input to a filter is a big endian format and the output is little
> > > > endian. Then there really isnt a gurantee that the filter will work with
> > > > big endian data. The libavfilter core could convert before or after the filter
> > > > as it preferrs. At least thats how i remember it from the last time i looked
> > > > at the code.
> > > > This also makes sense, as there are very good reasons to convert before,
> > > > for example when doing so results in better quality or higher speed
> > > > or fewer converts overall in multi input or multi output filters...
> > >
> > > Of course, this can be easily fixed by adding format=$pix_fmt after
> > > $filter=$filter_args.
> > >
> >
> > > >
> > > > So if the output is always forced to LE then this may unintentionally
> > > > remove testing a range of cases.
> > > > also this removes testing the codepath for big endian formats after the
> > > > convert. Or do we have remaining cases that test these ?
> > >
> > > Do you suggest to duplicate test to output BE and LE simultaneously?
> >
> > I dont really have a specific suggestion, just dont want to have some
> > codepathes be lost from testing
> > a solution that doesnt duplicate all the tests would be better i guess
> > as it would be quicker
> 
> Do you feel that because BE disappears in the output, it means that BE
> is untested?

no, rather that it implies that the code path after the final convert to LE
dont see BE anymore in the affected tests. I did not check if other cases
remain that test BE there

the disappearance might confuse people though, 
for example a developer who runs the tests after a code change and sees
a checksum change. Its important that he understands what the change means
so he can quickly fix his code.
Seeing a LE test fail causes one to probably not think about a BE specific bug.

The case of developers seeing their or someone elses code cause a fate failure
is possibly the main area where understanding the meaning matters.
I dont know how much this is an issue but with these checksums
seeing a difference its not immedeatly obvious if its teh LE or BE path even
if one knows that it can be the BE path
Iam not saying iam against this because of this, just wanted to bring it up
as i realize this could affect people and as you mentioned the subject ...

thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

No human being will ever know the Truth, for even if they happen to say it
by chance, they would not even known they had done so. -- Xenophanes
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190128/fefdb2a0/attachment.sig>


More information about the ffmpeg-devel mailing list