[FFmpeg-devel] [PATCH] avfilter: do not write to unwritable frame

[Muhammad Faiz]

On 1/27/17, wm4 <nfxjfg at googlemail.com> wrote:
> On Fri, 27 Jan 2017 22:04:50 +0700
> Muhammad Faiz <mfcc64 at gmail.com> wrote:
>
>> On 1/27/17, wm4 <nfxjfg at googlemail.com> wrote:
>> > On Fri, 27 Jan 2017 18:42:03 +0700
>> > Muhammad Faiz <mfcc64 at 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 at 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