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