diff mbox series

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

Message ID 20210310171308.5091-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avutil/frame: ensure the frame is writable in av_frame_copy()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer March 10, 2021, 5:13 p.m. UTC
Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavutil/frame.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andreas Rheinhardt March 11, 2021, 1:56 p.m. UTC | #1
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
James Almer March 11, 2021, 2:18 p.m. UTC | #2
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.
James Almer March 12, 2021, 7:13 p.m. UTC | #3
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.
Anton Khirnov March 14, 2021, 10:47 a.m. UTC | #4
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.
James Almer March 14, 2021, 2:28 p.m. UTC | #5
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).
Anton Khirnov March 14, 2021, 2:37 p.m. UTC | #6
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.
diff mbox series

Patch

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);