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

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 2 ++
 1 file changed, 2 insertions(+)

James Almer:
> Signed-off-by: James Almer <jamrial@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?

- Andreas

On 3/11/2021 10:56 AM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial@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.
There's av_image_copy() if you really want to just force copying and 
take the risk, but to me an AVFrame helper function like this must 
properly follow the struct's requirements.

FATE doesn't break, so that scenario is apparently currently not 
happening within the libraries. And if it was, it would require the 
target frame to be made writable.

That being said, if someone wants to give introducing a method to 
signal/identify ownership of references a try, that would be cool indeed.

On 3/10/2021 2:13 PM, James Almer wrote:
> Signed-off-by: James Almer <jamrial@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);

Will apply.

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

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

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