[FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure frames are writable when processing in-place

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> James Almer
> Sent: Tuesday, 28 September 2021 23:50
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure
> frames are writable when processing in-place
> 
> On 9/28/2021 6:22 PM, Soft Works wrote:
> >
> >
> >> -----Original Message-----
> >> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> >> James Almer
> >> Sent: Tuesday, 28 September 2021 23:19
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure
> >> frames are writable when processing in-place
> >>
> >> On 9/28/2021 6:00 PM, Soft Works wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf
> Of
> >>>> James Almer
> >>>> Sent: Tuesday, 28 September 2021 22:08
> >>>> To: ffmpeg-devel at ffmpeg.org
> >>>> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/frames:
> Ensure
> >>>> frames are writable when processing in-place
> >>>>
> >>>> On 9/28/2021 4:54 PM, Soft Works wrote:
> >>>>> Signed-off-by: softworkz <softworkz at hotmail.com>
> >>>>> ---
> >>>>> v2: Reduced to cases without AVFILTERPAD_FLAG_NEEDS_WRITABLE
> >>>>
> >>>> Can't this flag used in these filters?
> >>>
> >>> Not in vf_vflip, because in case of flip_bayer, it's using a
> >>> custom allocation (.get_buffer.video)
> >>>
> >>>>>     libavfilter/vf_cover_rect.c | 7 +++++--
> >>>>>     libavfilter/vf_floodfill.c  | 5 +++++
> >>>>>     libavfilter/vf_vflip.c      | 7 ++++++-
> >>>>>     3 files changed, 16 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/libavfilter/vf_cover_rect.c
> >>>> b/libavfilter/vf_cover_rect.c
> >>>>> index 0a8c10e06d..2367afb4b3 100644
> >>>>> --- a/libavfilter/vf_cover_rect.c
> >>>>> +++ b/libavfilter/vf_cover_rect.c
> >>>>> @@ -136,7 +136,7 @@ static int filter_frame(AVFilterLink
> *inlink,
> >>>> AVFrame *in)
> >>>>>         AVFilterContext *ctx = inlink->dst;
> >>>>>         CoverContext *cover = ctx->priv;
> >>>>>         AVDictionaryEntry *ex, *ey, *ew, *eh;
> >>>>> -    int x = -1, y = -1, w = -1, h = -1;
> >>>>> +    int x = -1, y = -1, w = -1, h = -1, ret;
> >>>>>         char *xendptr = NULL, *yendptr = NULL, *wendptr = NULL,
> >>>> *hendptr = NULL;
> >>>>>
> >>>>>         ex = av_dict_get(in->metadata, "lavfi.rect.x", NULL,
> >>>> AV_DICT_MATCH_CASE);
> >>>>> @@ -181,7 +181,10 @@ static int filter_frame(AVFilterLink
> >> *inlink,
> >>>> AVFrame *in)
> >>>>>         x = av_clip(x, 0, in->width  - w);
> >>>>>         y = av_clip(y, 0, in->height - h);
> >>>>>
> >>>>> -    av_frame_make_writable(in);
> >>>>> +    if ((ret = av_frame_make_writable(in)) < 0) {
> >>>>> +        av_frame_free(&in);
> >>>>> +        return ret;
> >>>>> +    }
> >>>>>
> >>>>>         if (cover->mode == MODE_BLUR) {
> >>>>>             blur (cover, in, x, y);
> >>>>> diff --git a/libavfilter/vf_floodfill.c
> >>>> b/libavfilter/vf_floodfill.c
> >>>>> index 21741cdb4f..292b27505e 100644
> >>>>> --- a/libavfilter/vf_floodfill.c
> >>>>> +++ b/libavfilter/vf_floodfill.c
> >>>>> @@ -294,6 +294,11 @@ static int filter_frame(AVFilterLink
> *link,
> >>>> AVFrame *frame)
> >>>>>         const int h = frame->height;
> >>>>>         int i, ret;
> >>>>>
> >>>>> +    if ((ret = av_frame_make_writable(frame)) < 0) {
> >>>>> +        av_frame_free(&frame);
> >>>>> +        return ret;
> >>>>> +    }
> >>>>> +
> >>>>>         if (is_inside(s->x, s->y, w, h)) {
> >>>>>             s->pick_pixel(frame, s->x, s->y, &s0, &s1, &s2,
> &s3);
> >>>>>
> >>>>> diff --git a/libavfilter/vf_vflip.c b/libavfilter/vf_vflip.c
> >>>>> index 0d624512f9..622bd46db3 100644
> >>>>> --- a/libavfilter/vf_vflip.c
> >>>>> +++ b/libavfilter/vf_vflip.c
> >>>>> @@ -108,11 +108,16 @@ static int flip_bayer(AVFilterLink *link,
> >>>> AVFrame *in)
> >>>>>     static int filter_frame(AVFilterLink *link, AVFrame *frame)
> >>>>>     {
> >>>>>         FlipContext *flip = link->dst->priv;
> >>>>> -    int i;
> >>>>> +    int i, ret;
> >>>>>
> >>>>>         if (flip->bayer)
> >>>>>             return flip_bayer(link, frame);
> >>>>>
> >>>>> +    if ((ret = av_frame_make_writable(frame)) < 0) {
> >>>>
> >>>> vf_vflip defines a custom get_buffer.video() function, so you
> >> can't
> >>>> use
> >>>> av_frame_make_writable() as the buffer it will return in case it
> >>>> needs
> >>>> to allocate a writable one may not be suitable. You need to use
> >>>> ff_inlink_make_frame_writable() instead, and in all three
> filters
> >>>> while
> >>>> at it.
> >>>
> >>> .get_buffer.video is used for the case of flip_bayer, but not
> >> otherwise.
> >>> Otherwise it was currently writing to the incoming frame
> directly,
> >>> and that's what this patch fixes.
> >>
> >> Ok, but i insist you should use ff_inlink_make_frame_writable().
> If
> >> it
> >> wont use the vflip custom get_buffer.video function, it will
> instead
> >> use
> >> the default allocator, which makes use of a lavfi frame pool.
> >
> > I've gone through very single video filter's filter_frame function
> > and I've seen not a single one making use of it.
> 
> That'd be because it's called automatically for filters that set
> AVFILTERPAD_FLAG_NEEDS_WRITABLE. Since these don't, you should do it
> manually.
> 
> >
> > That might be a task for a different day, I suppose...?
> 
> Why? You're adding av_frame_make_writable() calls. I'm asking you to
> instead make them ff_inlink_make_frame_writable() calls.

25 filters are calling av_frame_make_writable() and another 4 filters 
are using it implicitly via  ff_framesync_dualinput_get_writable().

0 filters are using ff_inlink_make_frame_writable().

That's why I meant that changing filters from using av_frame_make_writable()
to ff_inlink_make_frame_writable() would be a separate and unrelated 
change. 

Doesn't that make sense?

Kind regards,
softworkz