[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
> Soft Works
> Sent: Wednesday, 29 September 2021 00:01
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure
> frames are writable when processing in-place
> 
> 
> 
> > -----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().

To be precise: In some cases it's using av_frame_make_writable()
and in some ff_inlink_make_frame_writable().

Considering vf_vflip specifically, we even must not call 
ff_inlink_make_frame_writable(), because .get_buffer.video is only
meant to be run in case of flip_bayer(), but not in the other case
that this patch is addressing.

(flip_bayer() is calling ff_get_video_buffer() which is in turn
calling .get_buffer.video)

softworkz