On Mon, May 21, 2012 at 12:55:45PM +0100, Måns Rullgård wrote:
> Diego Biurrun <[email protected]> writes:
> > On Sun, May 20, 2012 at 11:01:18PM +0100, Mans Rullgard wrote:
> >> --- a/tests/Makefile
> >> +++ b/tests/Makefile
> >> @@ -1,9 +1,5 @@
> >> -AREF = fate-acodec-aref
> >> -VREF = fate-vsynth1-vref fate-vsynth2-vref
> >> -REFS = $(AREF) $(VREF)
> >> -
> >> -$(VREF): avconv$(EXESUF) tests/vsynth1/00.pgm tests/vsynth2/00.pgm
> >> -$(AREF): avconv$(EXESUF) tests/data/asynth1.sw
> >> +VREF = tests/vsynth1/00.pgm
> >> +AREF = tests/data/asynth1.sw
> >
> > Dropping "tests/vsynth2/00.pgm" looks suspicous. Suddenly nothing
> > depends on that file anymore, but the rule to generate it remains
> > in place. Either the rule needs to go as well or something does
> > still depend on the file.
>
> The file is indeed not needed, or generated, any more.
Drop the rule to generate it then.
> >> @@ -22,7 +18,16 @@ tests/data/asynth1.sw: tests/audiogen$(HOSTEXESUF) |
> >> tests/data
> >>
> >> -tests/data/asynth% tests/vsynth%/00.pgm: TAG = GEN
> >> +tests/data/asynth% tests/data/vsynth%.yuv tests/vsynth%/00.pgm: TAG = GEN
> >
> > Will the following simpler but more broad pattern work as well:
> >
> > tests/data/% tests/vsynth%/00.pgm: TAG = GEN
> >
> > ?
>
> That's a bit too generic and might match something it's not supposed to.
OK
> >> @@ -93,23 +92,14 @@ FATE += $(FATE_LIBAVUTIL)
> >>
> >> -$(FATE_LAVFI): $(REFS) tools/lavfi-showfiltfmts$(EXESUF)
> >> -$(FATE_SEEK): fate-codec fate-lavf libavformat/seek-test$(EXESUF)
> >> +$(FATE_LAVFI): $(VREF) tools/lavfi-showfiltfmts$(EXESUF)
> >> +$(FATE_SEEK): fate-acodec fate-vsynth2 fate-lavf
> >> libavformat/seek-test$(EXESUF)
> >
> > The previous dependency of FATE_LAVFI on AREF was incorrect?
>
> Yes.
>
> > Same for the previously missing dependency of FATE_SEEK on fate-vsynth2.
>
> It wasn't missing. FATE_SEEK incorrectly depended on both vsynth1 and
> vsynth2 via fate-vcodec.
>
> > If so, this could be split out into a separate patch.
>
> Not easily.
OK, it's not a big deal, keep it in this patch then.
> >> --- a/tests/fate-run.sh
> >> +++ b/tests/fate-run.sh
> >> @@ -148,7 +172,7 @@ seektest(){
> >>
> >> exec 3>&2
> >> -$command > "$outfile" 2>$errfile
> >> +eval $command >"$outfile" 2>$errfile
> >
> > You went to the trouble of removing the space after '>' here
> > compared to the previous incarnation of the patch? tsk, tsk :)
>
> That inconsistency has been bothering me for a while. Now I fixed it.
Haha :)
> >> --- /dev/null
> >> +++ b/tests/fate/vcodec.mak
> >> --- a/tests/regression-funcs.sh
> >> +++ b/tests/regression-funcs.sh
> >> @@ -20,17 +20,13 @@ outfile="$datadir/$test_ref/"
> >>
> >> # various files
> >> avconv="$target_exec ${target_path}/avconv"
> >> -tiny_psnr="tests/tiny_psnr"
> >> raw_src="${target_path}/$raw_src_dir/%02d.pgm"
> >> raw_dst="$datadir/$this.out.yuv"
> >> -raw_ref="$datadir/$test_ref.ref.yuv"
> >> pcm_src="$target_datadir/asynth1.sw"
> >> -pcm_dst="$datadir/$this.out.wav"
> >> -pcm_ref="$datadir/$test_ref.ref.wav"
> >> crcfile="$datadir/$this.crc"
> >> target_crcfile="$target_datadir/$this.crc"
> >
> > Some of these variables are now unused in regression-funcs.sh, but only
> > used in lavf-regression.sh (maybe among others, I did not check each one).
> > They should be moved there.
>
> All of those files will be removed soon enough. Cleaning them up now
> would be wasted effort.
I always worry about interim solutions becoming permanent. If you plan
to eliminate them soon, that is good enough for me.
Diego
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
