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

> 
> softworkz
> 
> 
> It calls ff_get_video_buffer(), but I though it would call it s

?

- Andreas