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

[James Almer]

On 9/28/2021 7:00 PM, Soft Works wrote:
> 
> 
>> -----Original Message-----
>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>> James Almer
>> Sent: Tuesday, 28 September 2021 23:50
>> 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 6:22 PM, Soft Works wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
>>>> James Almer
>>>> Sent: Tuesday, 28 September 2021 23:19
>>>> 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 6:00 PM, Soft Works wrote:
>>>>>
>>>>>
>>>>>> -----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.
>>>>
>>>> Ok, but i insist you should use ff_inlink_make_frame_writable().
>> If
>>>> it
>>>> wont use the vflip custom get_buffer.video function, it will
>> instead
>>>> use
>>>> the default allocator, which makes use of a lavfi frame pool.
>>>
>>> I've gone through very single video filter's filter_frame function
>>> and I've seen not a single one making use of it.
>>
>> That'd be because it's called automatically for filters that set
>> AVFILTERPAD_FLAG_NEEDS_WRITABLE. Since these don't, you should do it
>> manually.
>>
>>>
>>> That might be a task for a different day, I suppose...?
>>
>> Why? You're adding av_frame_make_writable() calls. I'm asking you to
>> instead make them ff_inlink_make_frame_writable() calls.
> 
> 25 filters are calling av_frame_make_writable() and another 4 filters
> are using it implicitly via  ff_framesync_dualinput_get_writable().
> 
> 0 filters are using ff_inlink_make_frame_writable().
> 
> That's why I meant that changing filters from using av_frame_make_writable()
> to ff_inlink_make_frame_writable() would be a separate and unrelated
> change.
> 
> Doesn't that make sense?

Of course, but "It's done elsewhere" is not a good reason to refuse to 
do something if can be better.

Either way, I'm not going to insist. It can be changed later.

> 
> Kind regards,
> softworkz
> 
> 
> 
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>