[FFmpeg-devel] [PATCH] avutil/frame: ensure the frame is writable in av_frame_copy()

[Anton Khirnov]

Quoting James Almer (2021-03-14 15:28:31)
> On 3/14/2021 7:47 AM, Anton Khirnov wrote:
> > Quoting James Almer (2021-03-11 15:18:38)
> >> On 3/11/2021 10:56 AM, Andreas Rheinhardt wrote:
> >>> James Almer:
> >>>> Signed-off-by: James Almer <jamrial at gmail.com>
> >>>> ---
> >>>>    libavutil/frame.c | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/libavutil/frame.c b/libavutil/frame.c
> >>>> index eab51b6a32..ec79d053e1 100644
> >>>> --- a/libavutil/frame.c
> >>>> +++ b/libavutil/frame.c
> >>>> @@ -800,6 +800,8 @@ int av_frame_copy(AVFrame *dst, const AVFrame *src)
> >>>>    {
> >>>>        if (dst->format != src->format || dst->format < 0)
> >>>>            return AVERROR(EINVAL);
> >>>> +    if (!av_frame_is_writable(dst))
> >>>> +        return AVERROR(EINVAL);
> >>>>    
> >>>>        if (dst->width > 0 && dst->height > 0)
> >>>>            return frame_copy_video(dst, src);
> >>>>
> >>> This will break scenarios where one owns all the references to a frame's
> >>> data without the frame being writable?
> >>
> >> It would, but that's by design. We define a frame as writable when its
> >> buffers are both reference counted and there's at most a single
> >> reference to them. Who owns any of the references does not play a part
> >> in that.
> > 
> > I disagree that this is a hard definition. It's merely a convention. If
> > all the references are under control of a single bit of code, it can
> > have its own convention. E.g. frame threading in libavcodec does exactly
> > that.
> 
> Shouldn't this function then warn about it? A simple warning that 
> ownership *and* writability of all buffer references within dst should 
> be ensured before calling it would maybe suffice.
> And i put emphasis on both characteristics because the user, despite 
> having the only reference/s, may have not created the frame and have no 
> knowledge of how it was created. A decoder could have handcrafted the 
> frame by explicitly doing something like frame->buf[0] = 
> av_buffer_create(..., AV_BUFFER_FLAG_READONLY), because they point at 
> something that's effectively not writable (Internal buffers from an 
> external library used by the lavc decoder).

Yes, extending the documentation would be good. Maybe also link to the
AVBuffer API doxy, which discusses writability.

-- 
Anton Khirnov