Re: [FFmpeg-devel] [PATCH 1/8] fate/filter-refcmp-*: make refcmp_metadata fail on empty input

From: Tobias Rapp <t.rapp@noa-archive.com>
To: ffmpeg-devel@ffmpeg.org
Subject: Re: [FFmpeg-devel] [PATCH 1/8] fate/filter-refcmp-*: make refcmp_metadata fail on empty input
Date: Thu, 31 Mar 2022 09:38:15 +0200
Message-ID: <45f5a877-bc0c-51b4-075f-264f29451938@noa-archive.com> (raw)
In-Reply-To: <20220330203205.25937-1-cus@passwd.hu>

On 30/03/2022 22:31, Marton Balint wrote:
> On empty input the awk script was always successful which caused the
> filter-refcmp tests to always succeed.
> 
> Also fix the command lines for refcmp_metadata compare function because it
> needs auto conversion filters, and update reference of test
> filter-refcmp-psnr-rgb because it was missed in
> a7fc78c1a638a32c3695c06f727774c740d675c2 but was never noticed due to the
> original issue...
> 
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>   tests/fate-run.sh                     |  2 +-
>   tests/ref/fate/filter-refcmp-psnr-rgb | 80 +++++++++++++--------------
>   tests/refcmp-metadata.awk             |  3 +
>   3 files changed, 44 insertions(+), 41 deletions(-)
> 
> diff --git a/tests/fate-run.sh b/tests/fate-run.sh
> index fbfc0a925d..5e8d607d88 100755
> --- a/tests/fate-run.sh
> +++ b/tests/fate-run.sh
> @@ -377,7 +377,7 @@ refcmp_metadata(){
>       refcmp=$1
>       pixfmt=$2
>       fuzz=${3:-0.001}
> -    ffmpeg $FLAGS $ENC_OPTS \
> +    ffmpeg -auto_conversion_filters $FLAGS $ENC_OPTS \
>           -lavfi "testsrc2=size=300x200:rate=1:duration=5,format=${pixfmt},split[ref][tmp];[tmp]avgblur=4[enc];[enc][ref]${refcmp},metadata=print:file=-" \
>           -f null /dev/null | awk -v ref=${ref} -v fuzz=${fuzz} -f ${base}/refcmp-metadata.awk -
>   }
> diff --git a/tests/ref/fate/filter-refcmp-psnr-rgb b/tests/ref/fate/filter-refcmp-psnr-rgb
> index f06db575ac..20abd3dc5a 100644
> --- a/tests/ref/fate/filter-refcmp-psnr-rgb
> +++ b/tests/ref/fate/filter-refcmp-psnr-rgb
> @@ -1,45 +1,45 @@
>   frame:0    pts:0       pts_time:0
> -lavfi.psnr.mse.r=1381.80
> -lavfi.psnr.psnr.r=16.73
> -lavfi.psnr.mse.g=896.00
> -lavfi.psnr.psnr.g=18.61
> -lavfi.psnr.mse.b=277.38
> -lavfi.psnr.psnr.b=23.70
> -lavfi.psnr.mse_avg=851.73
> -lavfi.psnr.psnr_avg=18.83
> +lavfi.psnr.mse.r=1367.642090
> +lavfi.psnr.psnr.r=16.771078
> +lavfi.psnr.mse.g=885.804382
> +lavfi.psnr.psnr.g=18.657425
> +lavfi.psnr.mse.b=274.825073
> +lavfi.psnr.psnr.b=23.740240
> +lavfi.psnr.mse_avg=842.757202
> +lavfi.psnr.psnr_avg=18.873779
>   frame:1    pts:1       pts_time:1
> -lavfi.psnr.mse.r=1380.37
> -lavfi.psnr.psnr.r=16.73
> -lavfi.psnr.mse.g=975.91
> -lavfi.psnr.psnr.g=18.24
> -lavfi.psnr.mse.b=435.72
> -lavfi.psnr.psnr.b=21.74
> -lavfi.psnr.mse_avg=930.67
> -lavfi.psnr.psnr_avg=18.44
> +lavfi.psnr.mse.r=1356.681152
> +lavfi.psnr.psnr.r=16.806026
> +lavfi.psnr.mse.g=958.161560
> +lavfi.psnr.psnr.g=18.316416
> +lavfi.psnr.mse.b=428.238312
> +lavfi.psnr.psnr.b=21.813948
> +lavfi.psnr.mse_avg=914.360352
> +lavfi.psnr.psnr_avg=18.519630
>   frame:2    pts:2       pts_time:2
> -lavfi.psnr.mse.r=1403.20
> -lavfi.psnr.psnr.r=16.66
> -lavfi.psnr.mse.g=954.05
> -lavfi.psnr.psnr.g=18.34
> -lavfi.psnr.mse.b=494.22
> -lavfi.psnr.psnr.b=21.19
> -lavfi.psnr.mse_avg=950.49
> -lavfi.psnr.psnr_avg=18.35
> +lavfi.psnr.mse.r=1387.254883
> +lavfi.psnr.psnr.r=16.709242
> +lavfi.psnr.mse.g=939.230957
> +lavfi.psnr.psnr.g=18.403080
> +lavfi.psnr.mse.b=493.913757
> +lavfi.psnr.psnr.b=21.194292
> +lavfi.psnr.mse_avg=940.133179
> +lavfi.psnr.psnr_avg=18.398911
>   frame:3    pts:3       pts_time:3
> -lavfi.psnr.mse.r=1452.80
> -lavfi.psnr.psnr.r=16.51
> -lavfi.psnr.mse.g=1001.02
> -lavfi.psnr.psnr.g=18.13
> -lavfi.psnr.mse.b=557.39
> -lavfi.psnr.psnr.b=20.67
> -lavfi.psnr.mse_avg=1003.74
> -lavfi.psnr.psnr_avg=18.11
> +lavfi.psnr.mse.r=1433.291260
> +lavfi.psnr.psnr.r=16.567459
> +lavfi.psnr.mse.g=990.005859
> +lavfi.psnr.psnr.g=18.174425
> +lavfi.psnr.mse.b=550.512329
> +lavfi.psnr.psnr.b=20.723133
> +lavfi.psnr.mse_avg=991.269836
> +lavfi.psnr.psnr_avg=18.168884
>   frame:4    pts:4       pts_time:4
> -lavfi.psnr.mse.r=1401.25
> -lavfi.psnr.psnr.r=16.67
> -lavfi.psnr.mse.g=1009.80
> -lavfi.psnr.psnr.g=18.09
> -lavfi.psnr.mse.b=602.42
> -lavfi.psnr.psnr.b=20.33
> -lavfi.psnr.mse_avg=1004.49
> -lavfi.psnr.psnr_avg=18.11
> +lavfi.psnr.mse.r=1385.949341
> +lavfi.psnr.psnr.r=16.713329
> +lavfi.psnr.mse.g=997.065796
> +lavfi.psnr.psnr.g=18.143566
> +lavfi.psnr.mse.b=601.962952
> +lavfi.psnr.psnr.b=20.335106
> +lavfi.psnr.mse_avg=994.992676
> +lavfi.psnr.psnr_avg=18.152605
> diff --git a/tests/refcmp-metadata.awk b/tests/refcmp-metadata.awk
> index fa21aad0e0..e7ed5ae809 100644
> --- a/tests/refcmp-metadata.awk
> +++ b/tests/refcmp-metadata.awk
> @@ -50,12 +50,15 @@ BEGIN {
>   }
>   
>   END {
> +    result = result && (NR != 0);

Checking for "NR == ref_nr" would additionally catch truncated input.

>       if (result) {
>           for (i = 1; i <= ref_nr; i++)
>               print ref_lines[i];
>       } else {
>           for (i = 1; i <= NR; i++)
>               print cmp_lines[i];
> +        if (NR == 0)
> +            print "[refcmp] no input";

Output should go to stderr here.

>           if (NR != ref_nr)
>               print "[refcmp] lines: " NR " != " ref_nr > "/dev/stderr";

Maybe add an "else" before the "if" to avoid that both lines are printed 
for empty input.

>           if (delta_max >= fuzz)

Otherwise looks good to me. Thanks for catching the issue!

Regards,
Tobias

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".