[libav-devel] [PATCH 3/3] fate: convert codec-regression.sh to makefile rules

Måns Rullgård mans at mansr.com
Fri May 18 15:57:47 CEST 2012


Diego Biurrun <diego at biurrun.de> writes:

> On Fri, May 18, 2012 at 10:32:06AM +0100, Mans Rullgard wrote:
>> ---
>> There are probably still a few corner-case problems lurking here.
>> ---
>
> Possible problems nonwithstanding, I like the general idea very much.
>
>> --- a/tests/Makefile
>> +++ b/tests/Makefile
>> @@ -22,7 +19,16 @@ tests/data/asynth1.sw: tests/audiogen$(HOSTEXESUF) | tests/data
>>  
>> -tests/data/asynth% tests/vsynth%/00.pgm: TAG = GEN
>> +tests/data/vsynth1.ref.yuv: tests/videogen$(HOSTEXESUF) | tests/data
>> +	$(M)$< >$@
>> +
>> +tests/data/vsynth2.ref.yuv: tests/rotozoom$(HOSTEXESUF) | tests/data
>> +	$(M)$< $(SRC_PATH)/tests/lena.pnm >$@
>
> Please leave a space after '>', it's easy to confuse it with automatic
> variables otherwise.

I prefer it that way.

>> --- a/tests/fate-run.sh
>> +++ b/tests/fate-run.sh
>> @@ -111,6 +111,34 @@ enc_dec_pcm(){
>>  
>> +enc_dec(){
>> +    src_fmt=$1
>> +    srcfile=$2
>
> src_file would be more consistent here.

But then it wouldn't be nicely aligned, and I thought that was important
to you.

>> +    enc_fmt=$3
>> +    enc_opt=$4
>> +    dec_fmt=$5
>> +    dec_opt=$6
>> +    encfile="${outdir}/${test}.${enc_fmt}"
>> +    decfile="${outdir}/${test}.out.${dec_fmt}"
>> +    cleanfiles="$cleanfiles $decfile"
>> +    test "$7" = -keep || cleanfiles="$cleanfiles $encfile"
>> +    tsrcfile=$(target_path $srcfile)
>> +    tencfile=$(target_path $encfile)
>> +    tdecfile=$(target_path $decfile)
>
> Prefixing the target variables with 't' is a bit terse for my taste

Keeping already long lines somewhat manageable is more important.

>> +    avconv -f $src_fmt $DEC_OPTS -i $tsrcfile $ENC_OPTS $enc_opt $FLAGS \
>> +        -f $enc_fmt -y $tencfile || return
>> +    do_md5sum $encfile
>> +    echo $(wc -c $encfile)
>
> Perhaps it's time to wrap our wc workaround in a separate function.

When I'm done, this will probably be the only instance of it.

>> rename from tests/ref/seek/alac_m4a
>> rename to tests/ref/seek/alac_mov
>> index 51e23e1..a281d2e 100644
>> --- a/tests/ref/seek/alac_m4a
>> +++ b/tests/ref/seek/alac_mov
>> @@ -1,53 +1,53 @@
>> -ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:     40 size:  3236
>> +ret: 0         st: 0 flags:1 dts: 0.000000 pts: 0.000000 pos:     36 size:  3236
>
> Why does this change?

The old tests supply a -f argument only in some cases.  Since format
selection based only on filename is not always possible, this generic
system has to always use -f.  A *.m4a filename corresponds to -f ipod,
which is not valid as an input format name.  Using mov instead works
just as well, but the header is 4 bytes smaller, hence the shift in
returned file offsets.

-- 
Måns Rullgård
mans at mansr.com


More information about the libav-devel mailing list