[FFmpeg-devel] [PATCH] split regression tests so make -jN can parallelise them
Mon Jan 28 02:01:03 CET 2008
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.
> 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.
mans at mansr.com
More information about the ffmpeg-devel