[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
> Andreas Rheinhardt
> Sent: Wednesday, 29 September 2021 02:04
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [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
> >> Andreas Rheinhardt
> >> Sent: Wednesday, 29 September 2021 01:42
> >> To: ffmpeg-devel at ffmpeg.org
> >> Subject: Re: [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 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.
> >>>
> >>
> >> You are completely misunderstanding the semantics of ownership for
> >> AVFrames: The owner of an AVFrame owns the AVFrame and can modify
> it
> >> ad
> >> libitum: He owns the AVFrame itself, the references to the data
> >> buffers,
> >> the metadata dictionary as well as the side data array and the
> >> references contained therein. He may change any of this (unless
> >> different semantics apply like for the frame-threaded decoding
> API);
> >> but
> >> in case of reference-counted objects owning a reference does not
> mean
> >> that one owns the underlying object. Ownership of these is shared
> >> with
> >> all the other owners of said object and you only own it if you own
> >> all
> >> the references to it.
> >> Given that the AVFrame itself is always writable (for the owner of
> >> said
> >> frame, not for any non-owner which typically only get a pointer to
> a
> >> const AVFrame), av_frame_make_writable() does not need to make the
> >> AVFrame itself writable and instead makes the data buffers
> writable*.
> >
> > How did you come to the idea that I would think otherwise?
> >
> 
> Because of your patch for vflip. Obviously.
> 
> >
> >> In the non-bayer codepath the vflip filter does not modify the
> >> underlying data (potentially shared with others), it only modifies
> >> its
> >> own AVFrame structure. Therefore there is no need for
> >> av_frame_make_writable() in this codepath at all.
> >
> > I see it now. It is changing the data pointers and applying a
> negative
> > line size.
> >
> > Horrible IMO, but yes, it's not changing the data in the buffer.
> 
> Very efficient. A copy would be horrible.

Efficient - sure. But compatible?

softworkz