[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 02:56:22 EEST 2021



> -----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?


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

softworkz


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


More information about the ffmpeg-devel mailing list