[FFmpeg-devel,2/2] fate: add decoder test for ATRAC9

Submitted by Rostislav Pehlivanov on July 1, 2018, 11:46 a.m.

Details

Message ID 20180701114600.5442-1-atomnuker@gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov July 1, 2018, 11:46 a.m.
Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 tests/fate/atrac.mak | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer July 1, 2018, 12:45 p.m.
On Sun, Jul 01, 2018 at 12:46:00PM +0100, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  tests/fate/atrac.mak | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
> index acf79a539c..38a5f33150 100644
> --- a/tests/fate/atrac.mak
> +++ b/tests/fate/atrac.mak
> @@ -33,7 +33,17 @@ fate-atrac3p-2: REF = $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm

fails on mips:
stddev:  200.81 PSNR: 50.27 MAXDIFF: 2448 bytes:   937984/   937984
MAXDIFF: |2448 - 0| >= 1
Test atrac9-2 failed. Look at tests/data/fate/atrac9-2.err for details.
make: *** [fate-atrac9-2] Error 1
make: *** Waiting for unfinished jobs....



[...]
James Almer July 1, 2018, 12:51 p.m.
On 7/1/2018 8:46 AM, Rostislav Pehlivanov wrote:
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  tests/fate/atrac.mak | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
> index acf79a539c..38a5f33150 100644
> --- a/tests/fate/atrac.mak
> +++ b/tests/fate/atrac.mak
> @@ -33,7 +33,17 @@ fate-atrac3p-2: REF = $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm
>  
>  FATE_ATRAC3P-$(call DEMDEC, OMA, ATRAC3P) += $(FATE_ATRAC3P)
>  
> -FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes) $(FATE_ATRAC3P-yes)
> +FATE_ATRAC9 += fate-atrac9-1
> +fate-atrac9-1: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/sy_title01.at9
> +fate-atrac9-1: REF = $(SAMPLES)/atrac9/sy_title01.at9.pcm
> +
> +FATE_ATRAC9 += fate-atrac9-2
> +fate-atrac9-2: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9
> +fate-atrac9-2: REF = $(SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9.pcm

Are these pcm files from a test suit (if any), or at least created by an
official binary decoder?
I'd rather avoid adding tests using reference files created by this same
decoder, seeing you for example resent the patch three times to fix a
wrong calculation each time. You never know if you'll find others later,
which could result in different output that may just be different enough
to fail on some hardware (usually mips/arm/x86_32).

But if there is no other way, make sure to create the reference files
using cpuflags 0 using an x86_32 build (for x87 float math), and not
with asm enabled (imdct and such), or with an x86_64 build.
If you can't run a x86_32 build, I or someone else could do it instead.

> +
> +FATE_ATRAC9-$(call DEMDEC, OMA, ATRAC9) += $(FATE_ATRAC9)
> +
> +FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes) $(FATE_ATRAC3P-yes) $(FATE_ATRAC9-yes)
>  
>  $(FATE_ATRAC_ALL): CMP = oneoff
>  
> @@ -42,3 +52,4 @@ FATE_SAMPLES_AVCONV += $(FATE_ATRAC_ALL)
>  fate-atrac:   $(FATE_ATRAC_ALL)
>  fate-atrac3:  $(FATE_ATRAC3-yes)
>  fate-atrac3p: $(FATE_ATRAC3P-yes)
> +fate-atrac9:  $(FATE_ATRAC9-yes)
>
Carl Eugen Hoyos July 3, 2018, 7:35 p.m.
2018-07-01 14:51 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 7/1/2018 8:46 AM, Rostislav Pehlivanov wrote:
>> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
>> ---
>>  tests/fate/atrac.mak | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
>> index acf79a539c..38a5f33150 100644
>> --- a/tests/fate/atrac.mak
>> +++ b/tests/fate/atrac.mak
>> @@ -33,7 +33,17 @@ fate-atrac3p-2: REF =
>> $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm
>>
>>  FATE_ATRAC3P-$(call DEMDEC, OMA, ATRAC3P) += $(FATE_ATRAC3P)
>>
>> -FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes)
>> $(FATE_ATRAC3P-yes)
>> +FATE_ATRAC9 += fate-atrac9-1
>> +fate-atrac9-1: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/sy_title01.at9
>> +fate-atrac9-1: REF = $(SAMPLES)/atrac9/sy_title01.at9.pcm
>> +
>> +FATE_ATRAC9 += fate-atrac9-2
>> +fate-atrac9-2: CMD = pcm -i
>> $(TARGET_SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9
>> +fate-atrac9-2: REF =
>> $(SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9.pcm
>
> Are these pcm files from a test suit (if any), or at least created
> by an official binary decoder?

I believe we added support for codecs when we were not able to
produce "official" binary output.

> I'd rather avoid adding tests using reference files created by
> this same decoder

Why not?
I agree that in an ideal world, we would only have known correct
reference files but this is not the case now (we have always been
knowingly testing "wrong" output") and, more important, I don't
think fate is a test for correctness but much more a regression
test to find unintended changes in FFmpeg's behaviour, don't you
agree? Finding such unintended changes even makes sense for
decoders that are known not to produce correct output.

Good to know that this decoder is bit-exact at least on some
platform=-)

Carl Eugen
Michael Niedermayer July 3, 2018, 11:09 p.m.
On Tue, Jul 03, 2018 at 09:35:53PM +0200, Carl Eugen Hoyos wrote:
> 2018-07-01 14:51 GMT+02:00, James Almer <jamrial@gmail.com>:
> > On 7/1/2018 8:46 AM, Rostislav Pehlivanov wrote:
> >> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> >> ---
> >>  tests/fate/atrac.mak | 13 ++++++++++++-
> >>  1 file changed, 12 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
> >> index acf79a539c..38a5f33150 100644
> >> --- a/tests/fate/atrac.mak
> >> +++ b/tests/fate/atrac.mak
> >> @@ -33,7 +33,17 @@ fate-atrac3p-2: REF =
> >> $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm
> >>
> >>  FATE_ATRAC3P-$(call DEMDEC, OMA, ATRAC3P) += $(FATE_ATRAC3P)
> >>
> >> -FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes)
> >> $(FATE_ATRAC3P-yes)
> >> +FATE_ATRAC9 += fate-atrac9-1
> >> +fate-atrac9-1: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/sy_title01.at9
> >> +fate-atrac9-1: REF = $(SAMPLES)/atrac9/sy_title01.at9.pcm
> >> +
> >> +FATE_ATRAC9 += fate-atrac9-2
> >> +fate-atrac9-2: CMD = pcm -i
> >> $(TARGET_SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9
> >> +fate-atrac9-2: REF =
> >> $(SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9.pcm
> >
> > Are these pcm files from a test suit (if any), or at least created
> > by an official binary decoder?
> 
> I believe we added support for codecs when we were not able to
> produce "official" binary output.
> 
> > I'd rather avoid adding tests using reference files created by
> > this same decoder
> 
> Why not?
> I agree that in an ideal world, we would only have known correct
> reference files but this is not the case now (we have always been
> knowingly testing "wrong" output") and, more important, I don't
> think fate is a test for correctness but much more a regression
> test to find unintended changes in FFmpeg's behaviour, don't you
> agree? Finding such unintended changes even makes sense for
> decoders that are known not to produce correct output.

The problem with raw (pcm,rgb.yuv) reference files coming from
our decoder and not teh official decoder is that they can change
(that is for lossy codecs mainly, its harder to miss a relevant bug in a
 lossless decoder)
Each time a bug is fixed in the decoder the reference can change
and the pcm/yuv/rgb file in fate samples may then need to be updated.
And we cannot change files in fate just add more files. So this can
lead to "dead" files in the samples ...


[...]
Carl Eugen Hoyos July 3, 2018, 11:16 p.m.
2018-07-04 1:09 GMT+02:00, Michael Niedermayer <michael@niedermayer.cc>:
> On Tue, Jul 03, 2018 at 09:35:53PM +0200, Carl Eugen Hoyos wrote:
>> 2018-07-01 14:51 GMT+02:00, James Almer <jamrial@gmail.com>:
>> > On 7/1/2018 8:46 AM, Rostislav Pehlivanov wrote:
>> >> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
>> >> ---
>> >>  tests/fate/atrac.mak | 13 ++++++++++++-
>> >>  1 file changed, 12 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
>> >> index acf79a539c..38a5f33150 100644
>> >> --- a/tests/fate/atrac.mak
>> >> +++ b/tests/fate/atrac.mak
>> >> @@ -33,7 +33,17 @@ fate-atrac3p-2: REF =
>> >> $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm
>> >>
>> >>  FATE_ATRAC3P-$(call DEMDEC, OMA, ATRAC3P) += $(FATE_ATRAC3P)
>> >>
>> >> -FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes)
>> >> $(FATE_ATRAC3P-yes)
>> >> +FATE_ATRAC9 += fate-atrac9-1
>> >> +fate-atrac9-1: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/sy_title01.at9
>> >> +fate-atrac9-1: REF = $(SAMPLES)/atrac9/sy_title01.at9.pcm
>> >> +
>> >> +FATE_ATRAC9 += fate-atrac9-2
>> >> +fate-atrac9-2: CMD = pcm -i
>> >> $(TARGET_SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9
>> >> +fate-atrac9-2: REF =
>> >> $(SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9.pcm
>> >
>> > Are these pcm files from a test suit (if any), or at least created
>> > by an official binary decoder?
>>
>> I believe we added support for codecs when we were not able to
>> produce "official" binary output.
>>
>> > I'd rather avoid adding tests using reference files created by
>> > this same decoder
>>
>> Why not?
>> I agree that in an ideal world, we would only have known correct
>> reference files but this is not the case now (we have always been
>> knowingly testing "wrong" output") and, more important, I don't
>> think fate is a test for correctness but much more a regression
>> test to find unintended changes in FFmpeg's behaviour, don't you
>> agree? Finding such unintended changes even makes sense for
>> decoders that are known not to produce correct output.
>
> The problem with raw (pcm,rgb.yuv) reference files coming from
> our decoder and not teh official decoder is that they can change
> (that is for lossy codecs mainly, its harder to miss a relevant bug in a
>  lossless decoder)
> Each time a bug is fixed in the decoder the reference can change
> and the pcm/yuv/rgb file in fate samples may then need to be updated.
> And we cannot change files in fate just add more files. So this can
> lead to "dead" files in the samples ...

Thank you for reminding me, I misunderstood.

Carl Eugen

Patch hide | download patch | download mbox

diff --git a/tests/fate/atrac.mak b/tests/fate/atrac.mak
index acf79a539c..38a5f33150 100644
--- a/tests/fate/atrac.mak
+++ b/tests/fate/atrac.mak
@@ -33,7 +33,17 @@  fate-atrac3p-2: REF = $(SAMPLES)/atrac3p/sonateno14op27-2-cut.pcm
 
 FATE_ATRAC3P-$(call DEMDEC, OMA, ATRAC3P) += $(FATE_ATRAC3P)
 
-FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes) $(FATE_ATRAC3P-yes)
+FATE_ATRAC9 += fate-atrac9-1
+fate-atrac9-1: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/sy_title01.at9
+fate-atrac9-1: REF = $(SAMPLES)/atrac9/sy_title01.at9.pcm
+
+FATE_ATRAC9 += fate-atrac9-2
+fate-atrac9-2: CMD = pcm -i $(TARGET_SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9
+fate-atrac9-2: REF = $(SAMPLES)/atrac9/3_br144_nb10_ib10_g04_bex1_sf1_d1.at9.pcm
+
+FATE_ATRAC9-$(call DEMDEC, OMA, ATRAC9) += $(FATE_ATRAC9)
+
+FATE_ATRAC_ALL = $(FATE_ATRAC1-yes) $(FATE_ATRAC3-yes) $(FATE_ATRAC3P-yes) $(FATE_ATRAC9-yes)
 
 $(FATE_ATRAC_ALL): CMP = oneoff
 
@@ -42,3 +52,4 @@  FATE_SAMPLES_AVCONV += $(FATE_ATRAC_ALL)
 fate-atrac:   $(FATE_ATRAC_ALL)
 fate-atrac3:  $(FATE_ATRAC3-yes)
 fate-atrac3p: $(FATE_ATRAC3P-yes)
+fate-atrac9:  $(FATE_ATRAC9-yes)