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

[Andreas Rheinhardt]

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*.

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.

- Andreas

*: It actually always makes a deep copy of the side data, even if it is
already writable in the first place. It is not documented to do so and I
will send a patch for that.