[FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure frames are writable when processing in-place
Soft Works
softworkz at hotmail.com
Wed Sep 29 06:01:28 EEST 2021
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Soft Works
> Sent: Wednesday, 29 September 2021 02:48
> To: FFmpeg development discussions and patches <ffmpeg-
> devel at ffmpeg.org>
> Subject: Re: [FFmpeg-devel] [PATCH v2 1/1] avfilter/frames: Ensure
> frames are writable when processing in-place
>
>
>
> > -----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.
Even when it would be handled properly everywhere - the horrible part
is still that it's coming as a surprise to me.
The inline code documentation of AVFrame doesn't state that linesize
values can be negative or that data[] pointers could point to the end
of the data, requiring backward iteration via negative line sizes.
At many places, memory is allocated by calculating linesize * h,
which confirms the assumption that it would be safe to do so; the
fact that it is done differently in certain cases (where it might
be negative) cannot be recognized easily.
If there would exist a consistent practice all over the project
regarding the use of signed vs. unsigned integers, it would have
made me wonder at least, when seeing linesize being signed.
I'm saying very clearly that I wasn't aware of the fact, that
linesizes can be negative, even though I know which conclusions
some devs might draw from it. Those might even be right,
when considering expertise as knowing about a list of undocumented
details, but what I think is that it shouldn't be necessary to
look at a filter like vf_vflip to become aware of the possibility
of negative line sizes.
Kind regards,
softworkz
More information about the ffmpeg-devel
mailing list