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

- Andreas