| Message ID | 20170913014557.19839-1-tfoucu@gmail.com |
|---|---|
| State | Accepted |
| Commit | eea64ef4cfb593cbe28465f45e6bd4c41a79cae1 |
| Headers | show |
On Tue, Sep 12, 2017 at 6:45 PM, Thierry Foucu <tfoucu@gmail.com> wrote: > Fix ticket #2674 > Tested with examples from ticket 2674. > --- > Sorry Michael, I forgot to configure using --enable-gpl. > Please find new patch. > > > ping? > libavfilter/vf_fps.c | 44 ++++++++++++++++++++++++++++++ > +++++----- > tests/ref/fate/filter-fps | 6 ++++++ > tests/ref/fate/filter-fps-r | 4 ++++ > tests/ref/fate/filter-mpdecimate | 1 - > tests/ref/fate/m4v-cfr | 1 - > 5 files changed, 49 insertions(+), 7 deletions(-) > > diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c > index 20ccd797d1..09fc66a73c 100644 > --- a/libavfilter/vf_fps.c > +++ b/libavfilter/vf_fps.c > @@ -34,6 +34,8 @@ > #include "libavutil/opt.h" > #include "libavutil/parseutils.h" > > +#define FF_INTERNAL_FIELDS 1 > +#include "framequeue.h" > #include "avfilter.h" > #include "internal.h" > #include "video.h" > @@ -137,13 +139,45 @@ static int request_frame(AVFilterLink *outlink) > AVFrame *buf; > > av_fifo_generic_read(s->fifo, &buf, sizeof(buf), NULL); > - buf->pts = av_rescale_q(s->first_pts, > ctx->inputs[0]->time_base, > - outlink->time_base) + s->frames_out; > + if (av_fifo_size(s->fifo)) { > + buf->pts = av_rescale_q(s->first_pts, > ctx->inputs[0]->time_base, > + outlink->time_base) + > s->frames_out; > > - if ((ret = ff_filter_frame(outlink, buf)) < 0) > - return ret; > + if ((ret = ff_filter_frame(outlink, buf)) < 0) > + return ret; > > - s->frames_out++; > + s->frames_out++; > + } else { > + /* This is the last frame, we may have to duplicate it to > match > + * the last frame duration */ > + int j; > + int delta = av_rescale_q_rnd(ctx->inputs[0]->current_pts > - s->first_pts, > + ctx->inputs[0]->time_base, > + outlink->time_base, > s->rounding) - s->frames_out ; > + /* if the delta is equal to 1, it means we just need to > output > + * the last frame. Greater than 1 means we will need > duplicate > + * delta-1 frames */ > + if (delta > 0 ) { > + for (j = 0; j < delta; j++) { > + AVFrame *dup = av_frame_clone(buf); > + > + av_log(ctx, AV_LOG_DEBUG, "Duplicating frame.\n"); > + dup->pts = av_rescale_q(s->first_pts, > ctx->inputs[0]->time_base, > + outlink->time_base) + > s->frames_out; > + > + if ((ret = ff_filter_frame(outlink, dup)) < 0) > + return ret; > + > + s->frames_out++; > + if (j > 0) s->dup++; > + } > + } else { > + /* for delta less or equal to 0, we should drop the > frame, > + * otherwise, we will have one or more extra frames */ > + av_frame_free(&buf); > + s->drop++; > + } > + } > } > return 0; > } > diff --git a/tests/ref/fate/filter-fps b/tests/ref/fate/filter-fps > index 55712cfb1c..242fb04e85 100644 > --- a/tests/ref/fate/filter-fps > +++ b/tests/ref/fate/filter-fps > @@ -85,3 +85,9 @@ > 0, 79, 79, 1, 30576, 0xa2fcd06f > 0, 80, 80, 1, 30576, 0xa2fcd06f > 0, 81, 81, 1, 30576, 0xd4150aad > +0, 82, 82, 1, 30576, 0xd4150aad > +0, 83, 83, 1, 30576, 0xd4150aad > +0, 84, 84, 1, 30576, 0xd4150aad > +0, 85, 85, 1, 30576, 0xd4150aad > +0, 86, 86, 1, 30576, 0xd4150aad > +0, 87, 87, 1, 30576, 0xd4150aad > diff --git a/tests/ref/fate/filter-fps-r b/tests/ref/fate/filter-fps-r > index 826b1ed6c6..c1bc7d1547 100644 > --- a/tests/ref/fate/filter-fps-r > +++ b/tests/ref/fate/filter-fps-r > @@ -72,3 +72,7 @@ > 0, 79, 79, 1, 30576, 0xa2fcd06f > 0, 80, 80, 1, 30576, 0xa2fcd06f > 0, 82, 82, 1, 30576, 0xd4150aad > +0, 83, 83, 1, 30576, 0xd4150aad > +0, 84, 84, 1, 30576, 0xd4150aad > +0, 85, 85, 1, 30576, 0xd4150aad > +0, 86, 86, 1, 30576, 0xd4150aad > diff --git a/tests/ref/fate/filter-mpdecimate b/tests/ref/fate/filter- > mpdecimate > index d438dacc2e..9c1dc36562 100644 > --- a/tests/ref/fate/filter-mpdecimate > +++ b/tests/ref/fate/filter-mpdecimate > @@ -22,4 +22,3 @@ > 0, 24, 24, 1, 115200, 0x056d40ca > 0, 26, 26, 1, 115200, 0xa4374737 > 0, 27, 27, 1, 115200, 0x3eaa3ae8 > -0, 29, 29, 1, 115200, 0x7551e9ee > diff --git a/tests/ref/fate/m4v-cfr b/tests/ref/fate/m4v-cfr > index 4eee84d01b..e2d02032fe 100644 > --- a/tests/ref/fate/m4v-cfr > +++ b/tests/ref/fate/m4v-cfr > @@ -44,4 +44,3 @@ > 0, 38, 38, 1, 115200, 0xf30825d5 > 0, 39, 39, 1, 115200, 0xe3c944a1 > 0, 40, 40, 1, 115200, 0x8fec4420 > -0, 41, 41, 1, 115200, 0x9381fdab > -- > 2.14.1.581.gf28d330327-goog > >
On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: > Fix ticket #2674 > Tested with examples from ticket 2674. > --- > Sorry Michael, I forgot to configure using --enable-gpl. > Please find new patch. > > > libavfilter/vf_fps.c | 44 +++++++++++++++++++++++++++++++++++----- > tests/ref/fate/filter-fps | 6 ++++++ > tests/ref/fate/filter-fps-r | 4 ++++ > tests/ref/fate/filter-mpdecimate | 1 - > tests/ref/fate/m4v-cfr | 1 - > 5 files changed, 49 insertions(+), 7 deletions(-) applied thanks [...]
On Fri, Sep 15, 2017 at 02:44:59AM +0200, Michael Niedermayer wrote: > On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: > > Fix ticket #2674 > > Tested with examples from ticket 2674. > > --- > > Sorry Michael, I forgot to configure using --enable-gpl. > > Please find new patch. > > > > > > libavfilter/vf_fps.c | 44 +++++++++++++++++++++++++++++++++++----- > > tests/ref/fate/filter-fps | 6 ++++++ > > tests/ref/fate/filter-fps-r | 4 ++++ > > tests/ref/fate/filter-mpdecimate | 1 - > > tests/ref/fate/m4v-cfr | 1 - > > 5 files changed, 49 insertions(+), 7 deletions(-) > > applied > asan doesn't like this commit (it stalls) and valgrind complains as well.
On 9/15/2017 10:15 AM, Clément Bœsch wrote: > On Fri, Sep 15, 2017 at 02:44:59AM +0200, Michael Niedermayer wrote: >> On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: >>> Fix ticket #2674 >>> Tested with examples from ticket 2674. >>> --- >>> Sorry Michael, I forgot to configure using --enable-gpl. >>> Please find new patch. >>> >>> >>> libavfilter/vf_fps.c | 44 +++++++++++++++++++++++++++++++++++----- >>> tests/ref/fate/filter-fps | 6 ++++++ >>> tests/ref/fate/filter-fps-r | 4 ++++ >>> tests/ref/fate/filter-mpdecimate | 1 - >>> tests/ref/fate/m4v-cfr | 1 - >>> 5 files changed, 49 insertions(+), 7 deletions(-) >> >> applied >> > > asan doesn't like this commit (it stalls) This is the second time it stalls with a leak. Sounds like gcc-asan 7.x is buggy. Wonder if we can somehow cook up a testcase for gcc's bugtracker that doesn't require "download and compile ffmpeg, and rsync 1gb of samples". Last time i tried the former (no rsync) it wasn't received with much enthusiasm either.
On Fri, Sep 15, 2017 at 11:15:14AM -0300, James Almer wrote: > On 9/15/2017 10:15 AM, Clément Bœsch wrote: > > On Fri, Sep 15, 2017 at 02:44:59AM +0200, Michael Niedermayer wrote: > >> On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: > >>> Fix ticket #2674 > >>> Tested with examples from ticket 2674. > >>> --- > >>> Sorry Michael, I forgot to configure using --enable-gpl. > >>> Please find new patch. > >>> > >>> > >>> libavfilter/vf_fps.c | 44 +++++++++++++++++++++++++++++++++++----- > >>> tests/ref/fate/filter-fps | 6 ++++++ > >>> tests/ref/fate/filter-fps-r | 4 ++++ > >>> tests/ref/fate/filter-mpdecimate | 1 - > >>> tests/ref/fate/m4v-cfr | 1 - > >>> 5 files changed, 49 insertions(+), 7 deletions(-) > >> > >> applied > >> > > > > asan doesn't like this commit (it stalls) > > This is the second time it stalls with a leak. Sounds like gcc-asan 7.x > is buggy. > it's a FATE bug, not an asan bug AFAICT
On Fri, Sep 15, 2017 at 04:53:45PM +0200, Clément Bœsch wrote: > On Fri, Sep 15, 2017 at 11:15:14AM -0300, James Almer wrote: > > On 9/15/2017 10:15 AM, Clément Bœsch wrote: > > > On Fri, Sep 15, 2017 at 02:44:59AM +0200, Michael Niedermayer wrote: > > >> On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: > > >>> Fix ticket #2674 > > >>> Tested with examples from ticket 2674. > > >>> --- > > >>> Sorry Michael, I forgot to configure using --enable-gpl. > > >>> Please find new patch. > > >>> > > >>> > > >>> libavfilter/vf_fps.c | 44 +++++++++++++++++++++++++++++++++++----- > > >>> tests/ref/fate/filter-fps | 6 ++++++ > > >>> tests/ref/fate/filter-fps-r | 4 ++++ > > >>> tests/ref/fate/filter-mpdecimate | 1 - > > >>> tests/ref/fate/m4v-cfr | 1 - > > >>> 5 files changed, 49 insertions(+), 7 deletions(-) > > >> > > >> applied > > >> > > > > > > asan doesn't like this commit (it stalls) > > > > This is the second time it stalls with a leak. Sounds like gcc-asan 7.x > > is buggy. > > > > it's a FATE bug, not an asan bug AFAICT > Too fast to reply again, my bad. It's indeed a GCC bug.
HI Clement On Fri, Sep 15, 2017 at 7:55 AM, Clément Bœsch <u@pkh.me> wrote: > On Fri, Sep 15, 2017 at 04:53:45PM +0200, Clément Bœsch wrote: > > On Fri, Sep 15, 2017 at 11:15:14AM -0300, James Almer wrote: > > > On 9/15/2017 10:15 AM, Clément Bœsch wrote: > > > > On Fri, Sep 15, 2017 at 02:44:59AM +0200, Michael Niedermayer wrote: > > > >> On Tue, Sep 12, 2017 at 06:45:57PM -0700, Thierry Foucu wrote: > > > >>> Fix ticket #2674 > > > >>> Tested with examples from ticket 2674. > > > >>> --- > > > >>> Sorry Michael, I forgot to configure using --enable-gpl. > > > >>> Please find new patch. > > > >>> > > > >>> > > > >>> libavfilter/vf_fps.c | 44 > +++++++++++++++++++++++++++++++++++----- > > > >>> tests/ref/fate/filter-fps | 6 ++++++ > > > >>> tests/ref/fate/filter-fps-r | 4 ++++ > > > >>> tests/ref/fate/filter-mpdecimate | 1 - > > > >>> tests/ref/fate/m4v-cfr | 1 - > > > >>> 5 files changed, 49 insertions(+), 7 deletions(-) > > > >> > > > >> applied > > > >> > > > > > > > > asan doesn't like this commit (it stalls) > > > > > > This is the second time it stalls with a leak. Sounds like gcc-asan 7.x > > > is buggy. > > > > > > > it's a FATE bug, not an asan bug AFAICT > > > > Too fast to reply again, my bad. It's indeed a GCC bug. > Sorry about it. I'm glad you found the problem. > > -- > Clément B. > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel > >
2017-09-15 17:13 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: >> Too fast to reply again, my bad. It's indeed a GCC bug. > > Sorry about it. I'm glad you found the problem. I suspect the comment above only applies to the asan hang, a valgrind issue was also mentioned in this thread. Carl Eugen
On Fri, Sep 15, 2017 at 8:24 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2017-09-15 17:13 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: > > >> Too fast to reply again, my bad. It's indeed a GCC bug. > > > > Sorry about it. I'm glad you found the problem. > > I suspect the comment above only applies to the asan hang, > a valgrind issue was also mentioned in this thread. > > valgrind seems to work for me here: valgrind ./ffmpeg -i fate-suite/qtrle/apple-animation-variable-fps-bug.mov -vf fps=30 -pix_fmt yuv420p -f null - VALGRIND_MEMORY_LIMIT_IN_MB=8192; VALGRIND_TIMEOUT=3600 ==134036== Memcheck, a memory error detector ==134036== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al. ==134036== Using Valgrind-3.9.0.SVN and LibVEX; rerun with -h for copyright info ==134036== Command: ./ffmpeg -i fate-suite/qtrle/apple-animation-variable-fps-bug.mov -vf fps=30 -pix_fmt yuv420p -f null - ==134036== ffmpeg version N-87233-ge59da0f7ff Copyright (c) 2000-2017 the FFmpeg developers built with gcc 4.8 (Ubuntu 4.8.4-2ubuntu1~14.04.3) configuration: WARNING: library configuration mismatch avutil configuration: --enable-nonfree --enable-gpl --enable-version3 avcodec configuration: --enable-nonfree --enable-gpl --enable-version3 avformat configuration: --enable-nonfree --enable-gpl --enable-version3 avdevice configuration: --enable-nonfree --enable-gpl --enable-version3 avfilter configuration: --enable-nonfree --enable-gpl --enable-version3 swscale configuration: --enable-nonfree --enable-gpl --enable-version3 swresample configuration: --enable-nonfree --enable-gpl --enable-version3 libavutil 55. 74.100 / 55. 74.100 libavcodec 57.105.100 / 57.105.100 libavformat 57. 81.100 / 57. 81.100 libavdevice 57. 8.100 / 57. 8.100 libavfilter 6.103.100 / 6.103.100 libswscale 4. 7.103 / 4. 7.103 libswresample 2. 8.100 / 2. 8.100 Input #0, mov,mp4,m4a,3gp,3g2,mj2, from 'fate-suite/qtrle/apple-animation-variable-fps-bug.mov': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt creation_time : 2012-07-27T07:09:56.000000Z Duration: 00:00:02.93, start: 0.000000, bitrate: 58 kb/s Stream #0:0(eng): Video: qtrle (rle / 0x20656C72), rgb24, 112x182, 55 kb/s, 1.69 fps, 30 tbr, 3k tbn, 3k tbc (default) Metadata: creation_time : 2012-07-27T07:09:56.000000Z handler_name : Apple Alias Data Handler encoder : Animation Stream mapping: Stream #0:0 -> #0:0 (qtrle (native) -> wrapped_avframe (native)) Press [q] to stop, [?] for help Output #0, null, to 'pipe:': Metadata: major_brand : qt minor_version : 537199360 compatible_brands: qt encoder : Lavf57.81.100 Stream #0:0(eng): Video: wrapped_avframe, yuv420p, 112x182, q=2-31, 200 kb/s, 30 fps, 30 tbn, 30 tbc (default) Metadata: creation_time : 2012-07-27T07:09:56.000000Z handler_name : Apple Alias Data Handler encoder : Lavc57.105.100 wrapped_avframe frame= 88 fps=0.0 q=-0.0 Lsize=N/A time=00:00:02.93 bitrate=N/A speed=3.35x video:45kB audio:0kB subtitle:0kB other streams:0kB global headers:0kB muxing overhead: unknown ==134036== ==134036== HEAP SUMMARY: ==134036== in use at exit: 140,670 bytes in 9 blocks ==134036== total heap usage: 4,339 allocs, 4,330 frees, 3,453,256 bytes allocated ==134036== ==134036== LEAK SUMMARY: ==134036== definitely lost: 528 bytes in 1 blocks ==134036== indirectly lost: 140,102 bytes in 7 blocks ==134036== possibly lost: 0 bytes in 0 blocks ==134036== still reachable: 40 bytes in 1 blocks ==134036== suppressed: 0 bytes in 0 blocks ==134036== Rerun with --leak-check=full to see details of leaked memory ==134036== ==134036== For counts of detected and suppressed errors, rerun with: -v ==134036== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 2 from 1) > Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, Sep 15, 2017 at 08:35:25AM -0700, Thierry Foucu wrote: > On Fri, Sep 15, 2017 at 8:24 AM, Carl Eugen Hoyos <ceffmpeg@gmail.com> > wrote: > > > 2017-09-15 17:13 GMT+02:00 Thierry Foucu <tfoucu@gmail.com>: > > > > >> Too fast to reply again, my bad. It's indeed a GCC bug. > > > > > > Sorry about it. I'm glad you found the problem. > > > > I suspect the comment above only applies to the asan hang, > > a valgrind issue was also mentioned in this thread. > > > > > valgrind seems to work for me here: you need --leak-check=full but i see you already posted a patc to fix a leak thx [...]
diff --git a/libavfilter/vf_fps.c b/libavfilter/vf_fps.c index 20ccd797d1..09fc66a73c 100644 --- a/libavfilter/vf_fps.c +++ b/libavfilter/vf_fps.c @@ -34,6 +34,8 @@ #include "libavutil/opt.h" #include "libavutil/parseutils.h" +#define FF_INTERNAL_FIELDS 1 +#include "framequeue.h" #include "avfilter.h" #include "internal.h" #include "video.h" @@ -137,13 +139,45 @@ static int request_frame(AVFilterLink *outlink) AVFrame *buf; av_fifo_generic_read(s->fifo, &buf, sizeof(buf), NULL); - buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base, - outlink->time_base) + s->frames_out; + if (av_fifo_size(s->fifo)) { + buf->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base, + outlink->time_base) + s->frames_out; - if ((ret = ff_filter_frame(outlink, buf)) < 0) - return ret; + if ((ret = ff_filter_frame(outlink, buf)) < 0) + return ret; - s->frames_out++; + s->frames_out++; + } else { + /* This is the last frame, we may have to duplicate it to match + * the last frame duration */ + int j; + int delta = av_rescale_q_rnd(ctx->inputs[0]->current_pts - s->first_pts, + ctx->inputs[0]->time_base, + outlink->time_base, s->rounding) - s->frames_out ; + /* if the delta is equal to 1, it means we just need to output + * the last frame. Greater than 1 means we will need duplicate + * delta-1 frames */ + if (delta > 0 ) { + for (j = 0; j < delta; j++) { + AVFrame *dup = av_frame_clone(buf); + + av_log(ctx, AV_LOG_DEBUG, "Duplicating frame.\n"); + dup->pts = av_rescale_q(s->first_pts, ctx->inputs[0]->time_base, + outlink->time_base) + s->frames_out; + + if ((ret = ff_filter_frame(outlink, dup)) < 0) + return ret; + + s->frames_out++; + if (j > 0) s->dup++; + } + } else { + /* for delta less or equal to 0, we should drop the frame, + * otherwise, we will have one or more extra frames */ + av_frame_free(&buf); + s->drop++; + } + } } return 0; } diff --git a/tests/ref/fate/filter-fps b/tests/ref/fate/filter-fps index 55712cfb1c..242fb04e85 100644 --- a/tests/ref/fate/filter-fps +++ b/tests/ref/fate/filter-fps @@ -85,3 +85,9 @@ 0, 79, 79, 1, 30576, 0xa2fcd06f 0, 80, 80, 1, 30576, 0xa2fcd06f 0, 81, 81, 1, 30576, 0xd4150aad +0, 82, 82, 1, 30576, 0xd4150aad +0, 83, 83, 1, 30576, 0xd4150aad +0, 84, 84, 1, 30576, 0xd4150aad +0, 85, 85, 1, 30576, 0xd4150aad +0, 86, 86, 1, 30576, 0xd4150aad +0, 87, 87, 1, 30576, 0xd4150aad diff --git a/tests/ref/fate/filter-fps-r b/tests/ref/fate/filter-fps-r index 826b1ed6c6..c1bc7d1547 100644 --- a/tests/ref/fate/filter-fps-r +++ b/tests/ref/fate/filter-fps-r @@ -72,3 +72,7 @@ 0, 79, 79, 1, 30576, 0xa2fcd06f 0, 80, 80, 1, 30576, 0xa2fcd06f 0, 82, 82, 1, 30576, 0xd4150aad +0, 83, 83, 1, 30576, 0xd4150aad +0, 84, 84, 1, 30576, 0xd4150aad +0, 85, 85, 1, 30576, 0xd4150aad +0, 86, 86, 1, 30576, 0xd4150aad diff --git a/tests/ref/fate/filter-mpdecimate b/tests/ref/fate/filter-mpdecimate index d438dacc2e..9c1dc36562 100644 --- a/tests/ref/fate/filter-mpdecimate +++ b/tests/ref/fate/filter-mpdecimate @@ -22,4 +22,3 @@ 0, 24, 24, 1, 115200, 0x056d40ca 0, 26, 26, 1, 115200, 0xa4374737 0, 27, 27, 1, 115200, 0x3eaa3ae8 -0, 29, 29, 1, 115200, 0x7551e9ee diff --git a/tests/ref/fate/m4v-cfr b/tests/ref/fate/m4v-cfr index 4eee84d01b..e2d02032fe 100644 --- a/tests/ref/fate/m4v-cfr +++ b/tests/ref/fate/m4v-cfr @@ -44,4 +44,3 @@ 0, 38, 38, 1, 115200, 0xf30825d5 0, 39, 39, 1, 115200, 0xe3c944a1 0, 40, 40, 1, 115200, 0x8fec4420 -0, 41, 41, 1, 115200, 0x9381fdab