| Message ID | 1485517323-19039-1-git-send-email-mfcc64@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Fri, 27 Jan 2017 18:42:03 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > affect filters that set partial_buf_size > test-case > ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; > [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > [a1][b1] vstack' > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavfilter/avfilter.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c > index c12d491..7e7e83c 100644 > --- a/libavfilter/avfilter.c > +++ b/libavfilter/avfilter.c > @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, > frame = ff_framequeue_peek(&link->fifo, 0); > av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n, > link->channels, link->format); > + > + if (!av_frame_is_writable(frame)) { > + AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples); > + if (!new) > + return AVERROR(ENOMEM); > + ret = av_frame_copy_props(new, frame); > + if (ret < 0) > + return ret; > + av_samples_copy(new->extended_data, frame->extended_data, > + 0, 0, frame->nb_samples, > + av_frame_get_channels(frame), > + frame->format); > + av_frame_unref(frame); > + av_frame_move_ref(frame, new); > + av_frame_free(&new); > + } > + > frame->nb_samples -= n; > av_samples_copy(frame->extended_data, frame->extended_data, 0, n, > frame->nb_samples, link->channels, link->format); There's a function that does this automatically: av_frame_make_writable() Might not be fully equivalent though due to ff_get_audio_buffer()
On 1/27/17, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 27 Jan 2017 18:42:03 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> affect filters that set partial_buf_size >> test-case >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit >> [a][b]; >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >> [a1][b1] vstack' >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> --- >> libavfilter/avfilter.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c >> index c12d491..7e7e83c 100644 >> --- a/libavfilter/avfilter.c >> +++ b/libavfilter/avfilter.c >> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, >> unsigned min, unsigned max, >> frame = ff_framequeue_peek(&link->fifo, 0); >> av_samples_copy(buf->extended_data, frame->extended_data, p, 0, >> n, >> link->channels, link->format); >> + >> + if (!av_frame_is_writable(frame)) { >> + AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples); >> + if (!new) >> + return AVERROR(ENOMEM); >> + ret = av_frame_copy_props(new, frame); >> + if (ret < 0) >> + return ret; >> + av_samples_copy(new->extended_data, frame->extended_data, >> + 0, 0, frame->nb_samples, >> + av_frame_get_channels(frame), >> + frame->format); >> + av_frame_unref(frame); >> + av_frame_move_ref(frame, new); >> + av_frame_free(&new); >> + } >> + >> frame->nb_samples -= n; >> av_samples_copy(frame->extended_data, frame->extended_data, 0, n, >> frame->nb_samples, link->channels, link->format); > > There's a function that does this automatically: > av_frame_make_writable() OK, I have known this. > > Might not be fully equivalent though due to ff_get_audio_buffer() If it is permitted to not use ff_get_audio_buffer(), I will use av_frame_make_writable(). Thank's.
On Fri, 27 Jan 2017 22:04:50 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > On 1/27/17, wm4 <nfxjfg@googlemail.com> wrote: > > On Fri, 27 Jan 2017 18:42:03 +0700 > > Muhammad Faiz <mfcc64@gmail.com> wrote: > > > >> affect filters that set partial_buf_size > >> test-case > >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit > >> [a][b]; > >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > >> [a1][b1] vstack' > >> > >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > >> --- > >> libavfilter/avfilter.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c > >> index c12d491..7e7e83c 100644 > >> --- a/libavfilter/avfilter.c > >> +++ b/libavfilter/avfilter.c > >> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, > >> unsigned min, unsigned max, > >> frame = ff_framequeue_peek(&link->fifo, 0); > >> av_samples_copy(buf->extended_data, frame->extended_data, p, 0, > >> n, > >> link->channels, link->format); > >> + > >> + if (!av_frame_is_writable(frame)) { > >> + AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples); > >> + if (!new) > >> + return AVERROR(ENOMEM); > >> + ret = av_frame_copy_props(new, frame); > >> + if (ret < 0) > >> + return ret; > >> + av_samples_copy(new->extended_data, frame->extended_data, > >> + 0, 0, frame->nb_samples, > >> + av_frame_get_channels(frame), > >> + frame->format); > >> + av_frame_unref(frame); > >> + av_frame_move_ref(frame, new); > >> + av_frame_free(&new); > >> + } > >> + > >> frame->nb_samples -= n; > >> av_samples_copy(frame->extended_data, frame->extended_data, 0, n, > >> frame->nb_samples, link->channels, link->format); > > > > There's a function that does this automatically: > > av_frame_make_writable() > > OK, I have known this. > > > > > Might not be fully equivalent though due to ff_get_audio_buffer() > > If it is permitted to not use ff_get_audio_buffer(), I will use > av_frame_make_writable(). I don't know about the details. Actually it looks like the ff_ function does something non-trivial (like using a buffer pool or allocating a buffer from the next filter). So it looks like your code is actually slightly more correct than a solution using av_frame_make_writable(). Although it's sad that most of the av_frame_make_writable() function has to be sort-of duplicated. So, patch is ok by me. Sorry for the additional confusion. (Maybe av_frame_copy() could be used instead of av_samples_copy().)
On 1/27/17, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 27 Jan 2017 22:04:50 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> On 1/27/17, wm4 <nfxjfg@googlemail.com> wrote: >> > On Fri, 27 Jan 2017 18:42:03 +0700 >> > Muhammad Faiz <mfcc64@gmail.com> wrote: >> > >> >> affect filters that set partial_buf_size >> >> test-case >> >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, >> >> asplit >> >> [a][b]; >> >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >> >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >> >> [a1][b1] vstack' >> >> >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> >> --- >> >> libavfilter/avfilter.c | 17 +++++++++++++++++ >> >> 1 file changed, 17 insertions(+) >> >> >> >> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c >> >> index c12d491..7e7e83c 100644 >> >> --- a/libavfilter/avfilter.c >> >> +++ b/libavfilter/avfilter.c >> >> @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, >> >> unsigned min, unsigned max, >> >> frame = ff_framequeue_peek(&link->fifo, 0); >> >> av_samples_copy(buf->extended_data, frame->extended_data, p, >> >> 0, >> >> n, >> >> link->channels, link->format); >> >> + >> >> + if (!av_frame_is_writable(frame)) { >> >> + AVFrame *new = ff_get_audio_buffer(link, >> >> frame->nb_samples); >> >> + if (!new) >> >> + return AVERROR(ENOMEM); >> >> + ret = av_frame_copy_props(new, frame); >> >> + if (ret < 0) >> >> + return ret; >> >> + av_samples_copy(new->extended_data, frame->extended_data, >> >> + 0, 0, frame->nb_samples, >> >> + av_frame_get_channels(frame), >> >> + frame->format); >> >> + av_frame_unref(frame); >> >> + av_frame_move_ref(frame, new); >> >> + av_frame_free(&new); >> >> + } >> >> + >> >> frame->nb_samples -= n; >> >> av_samples_copy(frame->extended_data, frame->extended_data, 0, >> >> n, >> >> frame->nb_samples, link->channels, >> >> link->format); >> > >> > There's a function that does this automatically: >> > av_frame_make_writable() >> >> OK, I have known this. >> >> > >> > Might not be fully equivalent though due to ff_get_audio_buffer() >> >> If it is permitted to not use ff_get_audio_buffer(), I will use >> av_frame_make_writable(). > > I don't know about the details. Actually it looks like the ff_ function > does something non-trivial (like using a buffer pool or allocating a > buffer from the next filter). > > So it looks like your code is actually slightly more correct than a > solution using av_frame_make_writable(). Although it's sad that most of > the av_frame_make_writable() function has to be sort-of duplicated. > > So, patch is ok by me. Sorry for the additional confusion. > > (Maybe av_frame_copy() could be used instead of av_samples_copy().) Changed to use av_frame_copy, also fix memleak
L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : > affect filters that set partial_buf_size > test-case > ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; > [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > [a1][b1] vstack' > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavfilter/avfilter.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) Maybe this can be of use: https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/28c62df672865890cbb13e5f0e94bde29c8fbacd Regards,
On 1/28/17, Nicolas George <george@nsup.org> wrote: > L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : >> affect filters that set partial_buf_size >> test-case >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit >> [a][b]; >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >> [a1][b1] vstack' >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> --- >> libavfilter/avfilter.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) > > Maybe this can be of use: > > https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/28c62df672865890cbb13e5f0e94bde29c8fbacd Unfortunately, this modify pointer to AVFrame, i think it is incompatible with framequeue framework. Note that av_frame_make_writable() takes AVFrame*, while ff_inlink_make_frame_writable() takes AVFrame**. Thank's
On Sat, 28 Jan 2017 15:59:11 +0700 Muhammad Faiz <mfcc64@gmail.com> wrote: > On 1/28/17, Nicolas George <george@nsup.org> wrote: > > L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : > >> affect filters that set partial_buf_size > >> test-case > >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit > >> [a][b]; > >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > >> [a1][b1] vstack' > >> > >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > >> --- > >> libavfilter/avfilter.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > > > > Maybe this can be of use: > > > > https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/28c62df672865890cbb13e5f0e94bde29c8fbacd > > Unfortunately, this modify pointer to AVFrame, i think it is incompatible > with framequeue framework. > > Note that av_frame_make_writable() takes AVFrame*, > while ff_inlink_make_frame_writable() takes AVFrame**. There doesn't seem to be any reason that it takes AVFrame**. AVFrame refs can be moved, so even if a frame is newly allocated it can be put into the AVFrame passed to the function without replacing the pointer. Also the code for copying the data in the function could have be reduced to 1 line by using av_frame_copy(). Maybe someone should have reviewed the commit that added this. Anyway, maybe you can fix it and use it. That would be the ideal outcome.
On 1/28/17, wm4 <nfxjfg@googlemail.com> wrote: > On Sat, 28 Jan 2017 15:59:11 +0700 > Muhammad Faiz <mfcc64@gmail.com> wrote: > >> On 1/28/17, Nicolas George <george@nsup.org> wrote: >> > L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : >> >> affect filters that set partial_buf_size >> >> test-case >> >> ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, >> >> asplit >> >> [a][b]; >> >> [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; >> >> [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; >> >> [a1][b1] vstack' >> >> >> >> Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> >> >> --- >> >> libavfilter/avfilter.c | 17 +++++++++++++++++ >> >> 1 file changed, 17 insertions(+) >> > >> > Maybe this can be of use: >> > >> > https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/28c62df672865890cbb13e5f0e94bde29c8fbacd >> > >> >> Unfortunately, this modify pointer to AVFrame, i think it is incompatible >> with framequeue framework. >> >> Note that av_frame_make_writable() takes AVFrame*, >> while ff_inlink_make_frame_writable() takes AVFrame**. > > There doesn't seem to be any reason that it takes AVFrame**. AVFrame > refs can be moved, so even if a frame is newly allocated it can be put > into the AVFrame passed to the function without replacing the pointer. > > Also the code for copying the data in the function could have be > reduced to 1 line by using av_frame_copy(). > > Maybe someone should have reviewed the commit that added this. > > Anyway, maybe you can fix it and use it. That would be the ideal > outcome. Good idea. I've posted it. Thank's
L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : > affect filters that set partial_buf_size > test-case > ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; > [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > [a1][b1] vstack' > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavfilter/avfilter.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) I think there is a much cleaner solution. Please let me have a closer look at it. Regards,
L'octidi 8 pluviôse, an CCXXV, Muhammad Faiz a écrit : > affect filters that set partial_buf_size > test-case > ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; > [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; > [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; > [a1][b1] vstack' > > Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> > --- > libavfilter/avfilter.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) Please have look at this patch: https://ffmpeg.org/pipermail/ffmpeg-devel/2017-January/206333.html https://patchwork.ffmpeg.org/patch/2355/ Regards,
diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c index c12d491..7e7e83c 100644 --- a/libavfilter/avfilter.c +++ b/libavfilter/avfilter.c @@ -1235,6 +1235,23 @@ static int take_samples(AVFilterLink *link, unsigned min, unsigned max, frame = ff_framequeue_peek(&link->fifo, 0); av_samples_copy(buf->extended_data, frame->extended_data, p, 0, n, link->channels, link->format); + + if (!av_frame_is_writable(frame)) { + AVFrame *new = ff_get_audio_buffer(link, frame->nb_samples); + if (!new) + return AVERROR(ENOMEM); + ret = av_frame_copy_props(new, frame); + if (ret < 0) + return ret; + av_samples_copy(new->extended_data, frame->extended_data, + 0, 0, frame->nb_samples, + av_frame_get_channels(frame), + frame->format); + av_frame_unref(frame); + av_frame_move_ref(frame, new); + av_frame_free(&new); + } + frame->nb_samples -= n; av_samples_copy(frame->extended_data, frame->extended_data, 0, n, frame->nb_samples, link->channels, link->format);
affect filters that set partial_buf_size test-case ffplay -i lavfi 'aevalsrc=sin(1000*t*t), aformat=sample_fmts=fltp, asplit [a][b]; [a] firequalizer=fixed=on, showcqt=s=1280x360 [a1]; [b] firequalizer=fixed=on, showcqt=s=1280x360 [b1]; [a1][b1] vstack' Signed-off-by: Muhammad Faiz <mfcc64@gmail.com> --- libavfilter/avfilter.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)