[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:25
> 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 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?
> >
> 
> I wanted an explanation, not a riddle.

Sorry, what I meant to question is whether negative linesize values
would be properly handled at all places.

softworkz