[FFmpeg-devel] [PATCH] split regression tests so make -jN can parallelise them

[Måns Rullgård]

Diego Biurrun <diego at biurrun.de> writes:

> Just a cursory review, I'm getting tired, still there should be some
> useful comments in there...
>
> On Sun, Jan 27, 2008 at 08:46:12PM +0000, M?ns Rullg?rd wrote:
>> The attached patch splits the regression tests allowing make to run
>> most of them in parallel.  On my machine, this brings the time for
>> codectest down to 17 seconds from previously being just over a minute.
>
> That's a multicore machine right?  Do you expect a speedup on a single
> core CPU?  I cannot test now, multiple compiles are running and will
> take some time to finish :)

This is on a quad-core machine.  Since the tests are largely
CPU-bound, I wouldn't expect any speedup on a single-core machine.

>> The various lavf tests could be separated as well, but that would
>> require more work, and those tests are pretty quick anyway.
>
> Quick on your machine you mean?

Quick compared to the codec tests on any machine.

>> --- a/Makefile
>> +++ b/Makefile
>> @@ -258,12 +258,69 @@ TAGS:
>>  
>>  fulltest test: codectest libavtest seektest
>>  
>> +REGFILES = $(addprefix tests/data/,$(addsuffix .$(1),$(2)))
>> +
>> +VSYNTH_REG   = tests/data/vsynth.regression
>> +ROTOZOOM_REG = tests/data/rotozoom.regression
>> +LAVF_REG     = tests/data/lavf.regression
>
> I like alphabetical order even in places like this (same below) :)

I don't mind.  Will change.

>>  FFMPEG_REFFILE   = $(SRC_PATH)/tests/ffmpeg.regression.ref
>>  FFSERVER_REFFILE = $(SRC_PATH)/tests/ffserver.regression.ref
>>  LIBAV_REFFILE    = $(SRC_PATH)/tests/libav.regression.ref
>>  ROTOZOOM_REFFILE = $(SRC_PATH)/tests/rotozoom.regression.ref
>>  SEEK_REFFILE     = $(SRC_PATH)/tests/seek.regression.ref
>>  
>> +CODEC_TESTS =                                   \
>> +        mpeg                                    \
>> +        mpeg2                                   \
>> +        mpeg2thread                             \
>> +        msmpeg4v2                               \
>> +        msmpeg4                                 \
>> +        wmv1                                    \
>> +        wmv2                                    \
>> +        h261                                    \
>> +        h263                                    \
>> +        h263p                                   \
>> +        mpeg4                                   \
>> +        huffyuv                                 \
>> +        rc                                      \
>> +        mpeg4adv                                \
>> +        mpeg4thread                             \
>> +        mp4psp                                  \
>> +        error                                   \
>> +        mpeg4nr                                 \
>> +        mpeg1b                                  \
>> +        mjpeg                                   \
>> +        ljpeg                                   \
>> +        jpegls                                  \
>> +        rv10                                    \
>> +        rv20                                    \
>> +        asv1                                    \
>> +        asv2                                    \
>> +        flv                                     \
>> +        ffv1                                    \
>> +        snow                                    \
>> +        snowll                                  \
>> +        dv                                      \
>> +        dv50                                    \
>> +        svq1                                    \
>> +        flashsv                                 \
>> +        mp2                                     \
>> +        ac3                                     \
>> +        g726                                    \
>> +        adpcm_ima_wav                           \
>> +        adpcm_ms                                \
>> +        adpcm_yam                               \
>> +        adpcm_swf                               \
>> +        flac                                    \
>> +        wma
>
> Add a backslash after the last test.

Will do.

> This could be put into alphabetical order at some point.  I'm aware
> that this requires reordering the reference files as well.
>
>> +CODEC_VSYNTH   = $(call REGFILES,vsynth.regression,$(CODEC_TESTS))
>> +CODEC_ROTOZOOM = $(call REGFILES,rotozoom.regression,$(CODEC_TESTS))
>
> The definition of REGFILES is about a page of code away, it would be
> more readable if you moved it just above these lines.

I realised the same myself after sending the patch.

>> +LAVF_TESTS = lavf
>> +LAVF_REGFILES = $(call REGFILES,lavf.regression,$(LAVF_TESTS))
>
> The LAVF_TESTS variable looks like useless (and confusing) indirection
> to me.

It is useless for now, although it serves as a place-holder for a
future split of the lavf tests.

>> @@ -271,18 +328,38 @@ test-server: ffserver$(EXESUF) tests/vsynth1/00.pgm tests/asynth1.sw
>>  
>> -libavtest: ffmpeg$(EXESUF) tests/vsynth1/00.pgm tests/asynth1.sw
>> -	$(SRC_PATH)/tests/regression.sh $@ $(LIBAV_REFFILE) tests/vsynth1
>> +libavtest: $(LAVF_REG)
>> +	diff -u -w $(LIBAV_REFFILE) $(LAVF_REG)
>
> Why diff -w here?

Try without, and you'll see.  Don't ask me how that came about though.

>> -seektest: tests/seek_test$(EXESUF)
>> +seektest: codectest libavtest tests/seek_test$(EXESUF)
>
> Are these dependencies new?  Otherwise this is a separate issue.

The seek test uses output files from the other tests, so the
dependency is not new.

>> @@ -311,7 +388,7 @@ tests/seek_test$(EXESUF): tests/seek_test.c .libs
>>  .PHONY: codectest libavtest seektest test-server fulltest test
>> -.PHONY: mpeg4 mpeg ac3 snow snowll swscale-error
>> +.PHONY: $(CODEC_TESTS) swscale-error
>
> I think lavf and ref are phony as well.

True.  Well spotted.

>> --- a/tests/regression.sh
>> +++ b/tests/regression.sh

[...]

>> @@ -502,7 +432,7 @@ fi
>>  ###################################
>>  if [ -n "$do_dv50" ] ; then
>>  # dv50
>> -do_video_encoding dv.dv "-dct int" pgmyuv "-s pal -pix_fmt yuv422p -an"
>> +do_video_encoding dv50.dv "-dct int" pgmyuv "-s pal -pix_fmt yuv422p -an"
>
> Looks like a separate issue, commit right away.

Well, it only becomes an issue when running tests in parallel.

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