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

[Diego Biurrun]

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 :)

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

> --- 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) :)

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

> +LAVF_TESTS = lavf
> +LAVF_REGFILES = $(call REGFILES,lavf.regression,$(LAVF_TESTS))

The LAVF_TESTS variable looks like useless (and confusing) indirection
to me.

> @@ -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?

> -seektest: tests/seek_test$(EXESUF)
> +seektest: codectest libavtest tests/seek_test$(EXESUF)

Are these dependencies new?  Otherwise this is a separate issue.

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

> --- a/tests/regression.sh
> +++ b/tests/regression.sh
> @@ -4,107 +4,34 @@
>  #
>  #set -x
> -# Even in the 21st century some diffs do not support -u.
> -diff -u "$0" "$0" > /dev/null 2>&1
> -if [ $? -eq 0 ]; then
> -  diff_cmd="diff -u"
> -else
> -  diff_cmd="diff"
> -fi
> -
> -diff -w "$0" "$0" > /dev/null 2>&1
> -if [ $? -eq 0 ]; then
> -  diff_cmd="$diff_cmd -w"
> -fi

Ugliness be gone :)

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

Diego