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

Diego Biurrun diego at biurrun.de
Fri May 18 16:12:06 CEST 2012


On Fri, May 18, 2012 at 02:57:47PM +0100, Måns Rullgård wrote:
> Diego Biurrun <diego at biurrun.de> writes:
> > On Fri, May 18, 2012 at 10:32:06AM +0100, Mans Rullgard wrote:
> >> --- 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.

Give in.  We have spaces around operators in the C code as well.

> >> --- 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.

Vertical alignment is overrated.  It's hard to achieve in shell scripts
anyway due to the synctactical rule of not having spaces around '='.

> >> +    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.

No problem:

    avconv -f $src_fmt $DEC_OPTS -i $target_srcfile $ENC_OPTS $enc_opt \
        $FLAGS -f $enc_fmt -y $target_encfile || return

> >> +    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.

OK - we can tackle this in the future if it is still an issue then.

Diego


More information about the libav-devel mailing list