| Message ID | 1500457259-23469-1-git-send-email-t.rapp@noa-archive.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Tobias,
On Wed, Jul 19, 2017 at 5:40 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote:
> +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519)
I'm assuming this is because of floating point rounding differences. Is it
possible to use a test similar to the floating-point codec tests that
allows slight offs? The differences are usually miniscule, right? Ideally,
the x86 SIMD would be tested also.
Ronald
On 7/19/17, Ronald S. Bultje <rsbultje@gmail.com> wrote: > Hi Tobias, > > On Wed, Jul 19, 2017 at 5:40 AM, Tobias Rapp <t.rapp@noa-archive.com> wrote: > >> +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) > This is unacceptable hack. > > I'm assuming this is because of floating point rounding differences. Is it > possible to use a test similar to the floating-point codec tests that > allows slight offs? The differences are usually miniscule, right? Ideally, > the x86 SIMD would be tested also. No, filter's asm code, overreads buffer, causing crash.
Hi, On Wed, Jul 19, 2017 at 6:38 AM, Paul B Mahol <onemda@gmail.com> wrote: > On 7/19/17, Ronald S. Bultje <rsbultje@gmail.com> wrote: > > Hi Tobias, > > > > On Wed, Jul 19, 2017 at 5:40 AM, Tobias Rapp <t.rapp@noa-archive.com> > wrote: > > > >> +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) > > > > This is unacceptable hack. > > > > > I'm assuming this is because of floating point rounding differences. Is > it > > possible to use a test similar to the floating-point codec tests that > > allows slight offs? The differences are usually miniscule, right? > Ideally, > > the x86 SIMD would be tested also. > > No, filter's asm code, overreads buffer, causing crash. Oh... That's bad, I'll try to fix that... Ronald
On 19.07.2017 12:45, Ronald S. Bultje wrote: > Hi, > > On Wed, Jul 19, 2017 at 6:38 AM, Paul B Mahol <onemda@gmail.com> wrote: > >> On 7/19/17, Ronald S. Bultje <rsbultje@gmail.com> wrote: >>> Hi Tobias, >>> >>> On Wed, Jul 19, 2017 at 5:40 AM, Tobias Rapp <t.rapp@noa-archive.com> >> wrote: >>> >>>> +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) >>> >> >> This is unacceptable hack. >> >>> >>> I'm assuming this is because of floating point rounding differences. Is >> it >>> possible to use a test similar to the floating-point codec tests that >>> allows slight offs? The differences are usually miniscule, right? >> Ideally, >>> the x86 SIMD would be tested also. >> >> No, filter's asm code, overreads buffer, causing crash. > > > Oh... That's bad, I'll try to fix that... That would be great as my assembler knowledge isn't that good. There are some Valgrind logs and my other findings available at https://trac.ffmpeg.org/ticket/6519 . Regards, Tobias
Hi. Thanks for the patch. Here are comments unrelated to the issues of asm failure. Le primidi 1er thermidor, an CCXXV, Tobias Rapp a écrit : > Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> > --- > tests/fate/filter-video.mak | 17 +++++++++++++ > tests/ref/fate/filter-refcmp-psnr-rgb | 45 +++++++++++++++++++++++++++++++++++ > tests/ref/fate/filter-refcmp-psnr-yuv | 45 +++++++++++++++++++++++++++++++++++ > tests/ref/fate/filter-refcmp-ssim-rgb | 30 +++++++++++++++++++++++ > tests/ref/fate/filter-refcmp-ssim-yuv | 30 +++++++++++++++++++++++ > 5 files changed, 167 insertions(+) > create mode 100644 tests/ref/fate/filter-refcmp-psnr-rgb > create mode 100644 tests/ref/fate/filter-refcmp-psnr-yuv > create mode 100644 tests/ref/fate/filter-refcmp-ssim-rgb > create mode 100644 tests/ref/fate/filter-refcmp-ssim-yuv > > diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak > index 53fc7a6..581297f 100644 > --- a/tests/fate/filter-video.mak > +++ b/tests/fate/filter-video.mak > @@ -698,6 +698,23 @@ FATE_FILTER_SAMPLES-$(call ALLYES, MOV_DEMUXER H264_DECODER AAC_FIXED_DECODER PC > fate-filter-meta-4560-rotate0: tests/data/file4560-override2rotate0.mov > fate-filter-meta-4560-rotate0: CMD = framecrc -flags +bitexact -c:a aac_fixed -i $(TARGET_PATH)/tests/data/file4560-override2rotate0.mov > > +fate-filter-refcmp%: CMD = ffmpeg -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ > + -f lavfi -i "testsrc=size=300x200:rate=1:duration=5" \ > + -filter:v "format=$(PIXFMT),split[ref][tmp]\;[tmp]avgblur=4[enc]\;[enc][ref]$(REFCMP),metadata=print:file=-" \ -lavfi "testsrc=...,format=,...", with "-lavfi" instead of "-f lavfi -i" and a single graph. Also, better use testsrc2, it is faster and with testrc, the yuv422p test will invoke scale. Last, I think the backslashes before the semicolons are suspicious. > + -f null /dev/null > + > +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER PSNR_FILTER METADATA_FILTER) += fate-filter-refcmp-psnr-rgb fate-filter-refcmp-psnr-yuv > +fate-filter-refcmp-psnr%: REFCMP = psnr > +fate-filter-refcmp-psnr-rgb: PIXFMT = rgb24 > +fate-filter-refcmp-psnr-yuv: PIXFMT = yuv422p > + > +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER SSIM_FILTER METADATA_FILTER) += fate-filter-refcmp-ssim-rgb fate-filter-refcmp-ssim-yuv > +fate-filter-refcmp-ssim%: REFCMP = ssim > +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) > +fate-filter-refcmp-ssim%: CPUFLAGS = 0 > +fate-filter-refcmp-ssim-rgb: PIXFMT = rgb24 > +fate-filter-refcmp-ssim-yuv: PIXFMT = yuv422p > + > FATE_SAMPLES_FFPROBE += $(FATE_METADATA_FILTER-yes) > FATE_SAMPLES_FFMPEG += $(FATE_FILTER_SAMPLES-yes) > FATE_FFMPEG += $(FATE_FILTER-yes) Regards,
On 19.07.2017 14:24, Nicolas George wrote: > Hi. Thanks for the patch. Here are comments unrelated to the issues of > asm failure. > > Le primidi 1er thermidor, an CCXXV, Tobias Rapp a écrit : >> Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> >> --- >> tests/fate/filter-video.mak | 17 +++++++++++++ >> tests/ref/fate/filter-refcmp-psnr-rgb | 45 +++++++++++++++++++++++++++++++++++ >> tests/ref/fate/filter-refcmp-psnr-yuv | 45 +++++++++++++++++++++++++++++++++++ >> tests/ref/fate/filter-refcmp-ssim-rgb | 30 +++++++++++++++++++++++ >> tests/ref/fate/filter-refcmp-ssim-yuv | 30 +++++++++++++++++++++++ >> 5 files changed, 167 insertions(+) >> create mode 100644 tests/ref/fate/filter-refcmp-psnr-rgb >> create mode 100644 tests/ref/fate/filter-refcmp-psnr-yuv >> create mode 100644 tests/ref/fate/filter-refcmp-ssim-rgb >> create mode 100644 tests/ref/fate/filter-refcmp-ssim-yuv >> >> diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak >> index 53fc7a6..581297f 100644 >> --- a/tests/fate/filter-video.mak >> +++ b/tests/fate/filter-video.mak >> @@ -698,6 +698,23 @@ FATE_FILTER_SAMPLES-$(call ALLYES, MOV_DEMUXER H264_DECODER AAC_FIXED_DECODER PC >> fate-filter-meta-4560-rotate0: tests/data/file4560-override2rotate0.mov >> fate-filter-meta-4560-rotate0: CMD = framecrc -flags +bitexact -c:a aac_fixed -i $(TARGET_PATH)/tests/data/file4560-override2rotate0.mov >> >> +fate-filter-refcmp%: CMD = ffmpeg -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ > >> + -f lavfi -i "testsrc=size=300x200:rate=1:duration=5" \ >> + -filter:v "format=$(PIXFMT),split[ref][tmp]\;[tmp]avgblur=4[enc]\;[enc][ref]$(REFCMP),metadata=print:file=-" \ > > -lavfi "testsrc=...,format=,...", with "-lavfi" instead of "-f lavfi -i" > and a single graph. > > Also, better use testsrc2, it is faster and with testrc, the yuv422p > test will invoke scale. Changed locally. > Last, I think the backslashes before the semicolons are suspicious. Indeed they are remainders from previous edits. It seems that stripping any whitespace within the filter string is enough to ensure that it is passed as one argument token to ffmpeg. So fixed locally. >> + -f null /dev/null BTW: Is it OK to redirect output to "/dev/null" here or does this introduce an unwanted platform dependency (i.e. blocks FATE from running on MSYS/Windows)? >> + >> +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER PSNR_FILTER METADATA_FILTER) += fate-filter-refcmp-psnr-rgb fate-filter-refcmp-psnr-yuv >> +fate-filter-refcmp-psnr%: REFCMP = psnr >> +fate-filter-refcmp-psnr-rgb: PIXFMT = rgb24 >> +fate-filter-refcmp-psnr-yuv: PIXFMT = yuv422p >> + >> +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER SSIM_FILTER METADATA_FILTER) += fate-filter-refcmp-ssim-rgb fate-filter-refcmp-ssim-yuv >> +fate-filter-refcmp-ssim%: REFCMP = ssim >> +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) >> +fate-filter-refcmp-ssim%: CPUFLAGS = 0 >> +fate-filter-refcmp-ssim-rgb: PIXFMT = rgb24 >> +fate-filter-refcmp-ssim-yuv: PIXFMT = yuv422p >> + >> FATE_SAMPLES_FFPROBE += $(FATE_METADATA_FILTER-yes) >> FATE_SAMPLES_FFMPEG += $(FATE_FILTER_SAMPLES-yes) >> FATE_FFMPEG += $(FATE_FILTER-yes) > Thanks for review, Tobias
Le primidi 1er thermidor, an CCXXV, Tobias Rapp a écrit : > Indeed they are remainders from previous edits. It seems that stripping any > whitespace within the filter string is enough to ensure that it is passed as > one argument token to ffmpeg. So fixed locally. I suppose you left the quotes, otherwise the semicolon separates commands and the brackets are globbing patterns. > >>+ -f null /dev/null > BTW: Is it OK to redirect output to "/dev/null" here or does this introduce > an unwanted platform dependency (i.e. blocks FATE from running on > MSYS/Windows)? With -f null, you can put anything you want there. I usually put -, but that is just a convenience to be able to change it to "-f fmt - | cmd". Regards,
diff --git a/tests/fate/filter-video.mak b/tests/fate/filter-video.mak index 53fc7a6..581297f 100644 --- a/tests/fate/filter-video.mak +++ b/tests/fate/filter-video.mak @@ -698,6 +698,23 @@ FATE_FILTER_SAMPLES-$(call ALLYES, MOV_DEMUXER H264_DECODER AAC_FIXED_DECODER PC fate-filter-meta-4560-rotate0: tests/data/file4560-override2rotate0.mov fate-filter-meta-4560-rotate0: CMD = framecrc -flags +bitexact -c:a aac_fixed -i $(TARGET_PATH)/tests/data/file4560-override2rotate0.mov +fate-filter-refcmp%: CMD = ffmpeg -flags +bitexact -sws_flags +accurate_rnd+bitexact -fflags +bitexact \ + -f lavfi -i "testsrc=size=300x200:rate=1:duration=5" \ + -filter:v "format=$(PIXFMT),split[ref][tmp]\;[tmp]avgblur=4[enc]\;[enc][ref]$(REFCMP),metadata=print:file=-" \ + -f null /dev/null + +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER PSNR_FILTER METADATA_FILTER) += fate-filter-refcmp-psnr-rgb fate-filter-refcmp-psnr-yuv +fate-filter-refcmp-psnr%: REFCMP = psnr +fate-filter-refcmp-psnr-rgb: PIXFMT = rgb24 +fate-filter-refcmp-psnr-yuv: PIXFMT = yuv422p + +FATE_FILTER_SAMPLES-$(call ALLYES, FFMPEG LAVFI_INDEV AVGBLUR_FILTER SSIM_FILTER METADATA_FILTER) += fate-filter-refcmp-ssim-rgb fate-filter-refcmp-ssim-yuv +fate-filter-refcmp-ssim%: REFCMP = ssim +# FIXME: override CPUFLAGS to avoid failure on x86 (issue #6519) +fate-filter-refcmp-ssim%: CPUFLAGS = 0 +fate-filter-refcmp-ssim-rgb: PIXFMT = rgb24 +fate-filter-refcmp-ssim-yuv: PIXFMT = yuv422p + FATE_SAMPLES_FFPROBE += $(FATE_METADATA_FILTER-yes) FATE_SAMPLES_FFMPEG += $(FATE_FILTER_SAMPLES-yes) FATE_FFMPEG += $(FATE_FILTER-yes) diff --git a/tests/ref/fate/filter-refcmp-psnr-rgb b/tests/ref/fate/filter-refcmp-psnr-rgb new file mode 100644 index 0000000..367226c --- /dev/null +++ b/tests/ref/fate/filter-refcmp-psnr-rgb @@ -0,0 +1,45 @@ +frame:0 pts:0 pts_time:0 +lavfi.psnr.mse.r=1678.35 +lavfi.psnr.psnr.r=15.88 +lavfi.psnr.mse.g=1118.72 +lavfi.psnr.psnr.g=17.64 +lavfi.psnr.mse.b=946.38 +lavfi.psnr.psnr.b=18.37 +lavfi.psnr.mse_avg=1247.82 +lavfi.psnr.psnr_avg=17.17 +frame:1 pts:1 pts_time:1 +lavfi.psnr.mse.r=1589.83 +lavfi.psnr.psnr.r=16.12 +lavfi.psnr.mse.g=1076.14 +lavfi.psnr.psnr.g=17.81 +lavfi.psnr.mse.b=733.06 +lavfi.psnr.psnr.b=19.48 +lavfi.psnr.mse_avg=1133.01 +lavfi.psnr.psnr_avg=17.59 +frame:2 pts:2 pts_time:2 +lavfi.psnr.mse.r=1603.11 +lavfi.psnr.psnr.r=16.08 +lavfi.psnr.mse.g=1064.91 +lavfi.psnr.psnr.g=17.86 +lavfi.psnr.mse.b=719.50 +lavfi.psnr.psnr.b=19.56 +lavfi.psnr.mse_avg=1129.17 +lavfi.psnr.psnr_avg=17.60 +frame:3 pts:3 pts_time:3 +lavfi.psnr.mse.r=1640.67 +lavfi.psnr.psnr.r=15.98 +lavfi.psnr.mse.g=1040.98 +lavfi.psnr.psnr.g=17.96 +lavfi.psnr.mse.b=717.82 +lavfi.psnr.psnr.b=19.57 +lavfi.psnr.mse_avg=1133.16 +lavfi.psnr.psnr_avg=17.59 +frame:4 pts:4 pts_time:4 +lavfi.psnr.mse.r=1691.15 +lavfi.psnr.psnr.r=15.85 +lavfi.psnr.mse.g=962.75 +lavfi.psnr.psnr.g=18.30 +lavfi.psnr.mse.b=775.03 +lavfi.psnr.psnr.b=19.24 +lavfi.psnr.mse_avg=1142.98 +lavfi.psnr.psnr_avg=17.55 diff --git a/tests/ref/fate/filter-refcmp-psnr-yuv b/tests/ref/fate/filter-refcmp-psnr-yuv new file mode 100644 index 0000000..916580f --- /dev/null +++ b/tests/ref/fate/filter-refcmp-psnr-yuv @@ -0,0 +1,45 @@ +frame:0 pts:0 pts_time:0 +lavfi.psnr.mse.y=434.61 +lavfi.psnr.psnr.y=21.75 +lavfi.psnr.mse.u=392.27 +lavfi.psnr.psnr.u=22.19 +lavfi.psnr.mse.v=679.26 +lavfi.psnr.psnr.v=19.81 +lavfi.psnr.mse_avg=485.19 +lavfi.psnr.psnr_avg=21.27 +frame:1 pts:1 pts_time:1 +lavfi.psnr.mse.y=386.12 +lavfi.psnr.psnr.y=22.26 +lavfi.psnr.mse.u=355.19 +lavfi.psnr.psnr.u=22.63 +lavfi.psnr.mse.v=683.13 +lavfi.psnr.psnr.v=19.79 +lavfi.psnr.mse_avg=452.64 +lavfi.psnr.psnr_avg=21.57 +frame:2 pts:2 pts_time:2 +lavfi.psnr.mse.y=407.59 +lavfi.psnr.psnr.y=22.03 +lavfi.psnr.mse.u=307.40 +lavfi.psnr.psnr.u=23.25 +lavfi.psnr.mse.v=680.56 +lavfi.psnr.psnr.v=19.80 +lavfi.psnr.mse_avg=450.78 +lavfi.psnr.psnr_avg=21.59 +frame:3 pts:3 pts_time:3 +lavfi.psnr.mse.y=420.90 +lavfi.psnr.psnr.y=21.89 +lavfi.psnr.mse.u=295.22 +lavfi.psnr.psnr.u=23.43 +lavfi.psnr.mse.v=676.06 +lavfi.psnr.psnr.v=19.83 +lavfi.psnr.mse_avg=453.27 +lavfi.psnr.psnr_avg=21.57 +frame:4 pts:4 pts_time:4 +lavfi.psnr.mse.y=393.71 +lavfi.psnr.psnr.y=22.18 +lavfi.psnr.mse.u=328.99 +lavfi.psnr.psnr.u=22.96 +lavfi.psnr.mse.v=675.83 +lavfi.psnr.psnr.v=19.83 +lavfi.psnr.mse_avg=448.06 +lavfi.psnr.psnr_avg=21.62 diff --git a/tests/ref/fate/filter-refcmp-ssim-rgb b/tests/ref/fate/filter-refcmp-ssim-rgb new file mode 100644 index 0000000..f47215b --- /dev/null +++ b/tests/ref/fate/filter-refcmp-ssim-rgb @@ -0,0 +1,30 @@ +frame:0 pts:0 pts_time:0 +lavfi.ssim.R=0.68 +lavfi.ssim.G=0.77 +lavfi.ssim.B=0.80 +lavfi.ssim.All=0.75 +lavfi.ssim.dB=6.03 +frame:1 pts:1 pts_time:1 +lavfi.ssim.R=0.69 +lavfi.ssim.G=0.77 +lavfi.ssim.B=0.83 +lavfi.ssim.All=0.76 +lavfi.ssim.dB=6.27 +frame:2 pts:2 pts_time:2 +lavfi.ssim.R=0.69 +lavfi.ssim.G=0.78 +lavfi.ssim.B=0.83 +lavfi.ssim.All=0.77 +lavfi.ssim.dB=6.39 +frame:3 pts:3 pts_time:3 +lavfi.ssim.R=0.69 +lavfi.ssim.G=0.79 +lavfi.ssim.B=0.84 +lavfi.ssim.All=0.77 +lavfi.ssim.dB=6.44 +frame:4 pts:4 pts_time:4 +lavfi.ssim.R=0.68 +lavfi.ssim.G=0.80 +lavfi.ssim.B=0.82 +lavfi.ssim.All=0.77 +lavfi.ssim.dB=6.32 diff --git a/tests/ref/fate/filter-refcmp-ssim-yuv b/tests/ref/fate/filter-refcmp-ssim-yuv new file mode 100644 index 0000000..15b4dc2 --- /dev/null +++ b/tests/ref/fate/filter-refcmp-ssim-yuv @@ -0,0 +1,30 @@ +frame:0 pts:0 pts_time:0 +lavfi.ssim.Y=0.77 +lavfi.ssim.U=0.76 +lavfi.ssim.V=0.67 +lavfi.ssim.All=0.75 +lavfi.ssim.dB=5.94 +frame:1 pts:1 pts_time:1 +lavfi.ssim.Y=0.78 +lavfi.ssim.U=0.77 +lavfi.ssim.V=0.68 +lavfi.ssim.All=0.75 +lavfi.ssim.dB=6.10 +frame:2 pts:2 pts_time:2 +lavfi.ssim.Y=0.78 +lavfi.ssim.U=0.78 +lavfi.ssim.V=0.68 +lavfi.ssim.All=0.76 +lavfi.ssim.dB=6.11 +frame:3 pts:3 pts_time:3 +lavfi.ssim.Y=0.78 +lavfi.ssim.U=0.78 +lavfi.ssim.V=0.67 +lavfi.ssim.All=0.75 +lavfi.ssim.dB=6.09 +frame:4 pts:4 pts_time:4 +lavfi.ssim.Y=0.79 +lavfi.ssim.U=0.78 +lavfi.ssim.V=0.68 +lavfi.ssim.All=0.76 +lavfi.ssim.dB=6.20
Signed-off-by: Tobias Rapp <t.rapp@noa-archive.com> --- tests/fate/filter-video.mak | 17 +++++++++++++ tests/ref/fate/filter-refcmp-psnr-rgb | 45 +++++++++++++++++++++++++++++++++++ tests/ref/fate/filter-refcmp-psnr-yuv | 45 +++++++++++++++++++++++++++++++++++ tests/ref/fate/filter-refcmp-ssim-rgb | 30 +++++++++++++++++++++++ tests/ref/fate/filter-refcmp-ssim-yuv | 30 +++++++++++++++++++++++ 5 files changed, 167 insertions(+) create mode 100644 tests/ref/fate/filter-refcmp-psnr-rgb create mode 100644 tests/ref/fate/filter-refcmp-psnr-yuv create mode 100644 tests/ref/fate/filter-refcmp-ssim-rgb create mode 100644 tests/ref/fate/filter-refcmp-ssim-yuv