[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: Wednesday, 29 September 2021 00:30
> 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 7: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 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?
> 
> Of course, but "It's done elsewhere" is not a good reason to refuse
> to
> do something if can be better.

I'm just trying to adapt to the rules here.

Considering the fact that I'm not even allowed to fix a typo in a log
message when submitting a patch, I don't think that a patch that is 
about ensuring that frames are writable should also be the first one
to introduce the use of ff_inlink_make_frame_writable() in a filter.

According to the project's logic, it would be rather required
to have another commit that applies that change to all filters,
in order not to combine two unrelated changes in a single commit.

That's what I meant. It goes without saying that reusing buffers 
from the frame pool makes a lot of sense in general.

(not to forget that in vf_vsplit it cannot be done anyway)

Kind regards,
softworkz