diff mbox series

[FFmpeg-devel] avcodec/snow: ensure current_picture is writable before modifying its data

Message ID 20200529172037.4595-1-jamrial@gmail.com
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/snow: ensure current_picture is writable before modifying its data
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer May 29, 2020, 5:20 p.m. UTC
current_picture was not writable here because a reference existed in
at least avctx->coded_frame, and potentially elsewhere if the caller
created new ones from it.

Signed-off-by: James Almer <jamrial@gmail.com>
---
For that matter, there are two filters that depend on whatever
s->mpvencdsp.draw_edges is doing on this buffer, accessing it through
avctx->coded_frame after passing their input to this encoder.
This is very hacky and the main blocker to remove coded_frame in the
next bump, so it must be changed.

If mpvencdsp.draw_edges needs to be accessed from lavfi, then it could
be shared, or its code duplicated. But the current snow usage from lavfi
is crazy and beyond the scope of coded_frame, which was meant to export
quality stats and not work as some sort of interface to access lavc image
processing features.

 libavcodec/snowenc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Michael Niedermayer May 29, 2020, 8:37 p.m. UTC | #1
On Fri, May 29, 2020 at 02:20:37PM -0300, James Almer wrote:
> current_picture was not writable here because a reference existed in
> at least avctx->coded_frame, and potentially elsewhere if the caller
> created new ones from it.

Please elaborate when and how the encoder internal buffer becomes
read only
i simply took your code and replaced

-        ret = av_frame_make_writable(s->current_picture);
-        if (ret < 0)
-            return ret;
+        ret = av_frame_is_writable(s->current_picture);
+        if (ret <= 0)
+            return -1;

and fate is fully happy which tests both the encoder and the filters
using it
also if this is for coded_frame then shouldnt it be under FF_API_CODED_FRAME?
iam clearly missing something here

thx

[...]
James Almer May 29, 2020, 10 p.m. UTC | #2
On 5/29/2020 5:37 PM, Michael Niedermayer wrote:
> On Fri, May 29, 2020 at 02:20:37PM -0300, James Almer wrote:
>> current_picture was not writable here because a reference existed in
>> at least avctx->coded_frame, and potentially elsewhere if the caller
>> created new ones from it.
> 
> Please elaborate when and how the encoder internal buffer becomes
> read only
> i simply took your code and replaced
> 
> -        ret = av_frame_make_writable(s->current_picture);
> -        if (ret < 0)
> -            return ret;
> +        ret = av_frame_is_writable(s->current_picture);
> +        if (ret <= 0)
> +            return -1;
> 
> and fate is fully happy which tests both the encoder and the filters
> using it
> also if this is for coded_frame then shouldnt it be under FF_API_CODED_FRAME?
> iam clearly missing something here

You need to also move the av_frame_unref(avctx->coded_frame) line back 
to where it was before my patch. I moved it before this check to ensure 
at least the reference stored there is freed before current_picture is 
written to, but there of course could still exist other references 
created by the user, and that's what this make_writable() call is for.

And since this specific chunk is not strictly coded_frame related, it 
doesn't need to be under that guard.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Michael Niedermayer May 29, 2020, 10:38 p.m. UTC | #3
On Fri, May 29, 2020 at 07:00:12PM -0300, James Almer wrote:
> On 5/29/2020 5:37 PM, Michael Niedermayer wrote:
> > On Fri, May 29, 2020 at 02:20:37PM -0300, James Almer wrote:
> > > current_picture was not writable here because a reference existed in
> > > at least avctx->coded_frame, and potentially elsewhere if the caller
> > > created new ones from it.
> > 
> > Please elaborate when and how the encoder internal buffer becomes
> > read only
> > i simply took your code and replaced
> > 
> > -        ret = av_frame_make_writable(s->current_picture);
> > -        if (ret < 0)
> > -            return ret;
> > +        ret = av_frame_is_writable(s->current_picture);
> > +        if (ret <= 0)
> > +            return -1;
> > 
> > and fate is fully happy which tests both the encoder and the filters
> > using it
> > also if this is for coded_frame then shouldnt it be under FF_API_CODED_FRAME?
> > iam clearly missing something here
> 
> You need to also move the av_frame_unref(avctx->coded_frame) line back to
> where it was before my patch. I moved it before this check to ensure at
> least the reference stored there is freed before current_picture is written
> to, 

i quite intentionally did not move that, my question was just about
"why av_frame_make_writable" after all the other changes


> but there of course could still exist other references created by the
> user, and that's what this make_writable() call is for.

ok, understand this. I guess my gut feeling was that creating references
to coded_frame was not allowed. but i guess there is nothing that forbids
it so teh API allowes it, and the av_frame_make_writable is ok
 
 
> 
> And since this specific chunk is not strictly coded_frame related, it
> doesn't need to be under that guard.

but coded_frame is the only current way by which a user can create a
reference, unless iam missing something
Am i guessing correctly that you intend to add another way to export
the frame or is there something iam missing ?
because without some other method this make_writable doesnt make sense
without coded_frame and should be removed in case coded_frame is removed

thx

[...]
James Almer May 29, 2020, 11:06 p.m. UTC | #4
On 5/29/2020 7:38 PM, Michael Niedermayer wrote:
> On Fri, May 29, 2020 at 07:00:12PM -0300, James Almer wrote:
>> On 5/29/2020 5:37 PM, Michael Niedermayer wrote:
>>> On Fri, May 29, 2020 at 02:20:37PM -0300, James Almer wrote:
>>>> current_picture was not writable here because a reference existed in
>>>> at least avctx->coded_frame, and potentially elsewhere if the caller
>>>> created new ones from it.
>>>
>>> Please elaborate when and how the encoder internal buffer becomes
>>> read only
>>> i simply took your code and replaced
>>>
>>> -        ret = av_frame_make_writable(s->current_picture);
>>> -        if (ret < 0)
>>> -            return ret;
>>> +        ret = av_frame_is_writable(s->current_picture);
>>> +        if (ret <= 0)
>>> +            return -1;
>>>
>>> and fate is fully happy which tests both the encoder and the filters
>>> using it
>>> also if this is for coded_frame then shouldnt it be under FF_API_CODED_FRAME?
>>> iam clearly missing something here
>>
>> You need to also move the av_frame_unref(avctx->coded_frame) line back to
>> where it was before my patch. I moved it before this check to ensure at
>> least the reference stored there is freed before current_picture is written
>> to, 
> 
> i quite intentionally did not move that, my question was just about
> "why av_frame_make_writable" after all the other changes
> 
> 
>> but there of course could still exist other references created by the
>> user, and that's what this make_writable() call is for.
> 
> ok, understand this. I guess my gut feeling was that creating references
> to coded_frame was not allowed. but i guess there is nothing that forbids
> it so teh API allowes it, and the av_frame_make_writable is ok
>  
>  
>>
>> And since this specific chunk is not strictly coded_frame related, it
>> doesn't need to be under that guard.
> 
> but coded_frame is the only current way by which a user can create a
> reference, unless iam missing something
> Am i guessing correctly that you intend to add another way to export
> the frame or is there something iam missing ?

No, i expected you'd change this and find a way to get this
functionality in lavfi, since it's your code and the only non standard
coded_frame usage that's blocking its removal. I mentioned you as much
last major bump when we postponed the removal of coded_frame.
As i mentioned before, an encoder should not work as some sort of
interface to access lavc image processing features. Lavfi should either
use some lavc API, or feature its own implementation of this
functionality that's currently done here.

This patch is only to remove the current wrong behavior or writing on
potentially non writable frames, for the purpose of backporting to
existing branches (and so it's also present in 4.3). It should not
matter after the major bump.

> because without some other method this make_writable doesnt make sense
> without coded_frame and should be removed in case coded_frame is removed

Alright, i'll wrap it with the FF_API_ check before pushing.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
diff mbox series

Patch

diff --git a/libavcodec/snowenc.c b/libavcodec/snowenc.c
index 3f2a75a670..3543574ec5 100644
--- a/libavcodec/snowenc.c
+++ b/libavcodec/snowenc.c
@@ -1624,10 +1624,20 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
         s->lambda = 0;
     }//else keep previous frame's qlog until after motion estimation
 
+#if FF_API_CODED_FRAME
+FF_DISABLE_DEPRECATION_WARNINGS
+    av_frame_unref(avctx->coded_frame);
+FF_ENABLE_DEPRECATION_WARNINGS
+#endif
+
     if (s->current_picture->data[0]) {
         int w = s->avctx->width;
         int h = s->avctx->height;
 
+        ret = av_frame_make_writable(s->current_picture);
+        if (ret < 0)
+            return ret;
+
         s->mpvencdsp.draw_edges(s->current_picture->data[0],
                                 s->current_picture->linesize[0], w   , h   ,
                                 EDGE_WIDTH  , EDGE_WIDTH  , EDGE_TOP | EDGE_BOTTOM);
@@ -1645,7 +1655,6 @@  static int encode_frame(AVCodecContext *avctx, AVPacket *pkt,
     ff_snow_frame_start(s);
 #if FF_API_CODED_FRAME
 FF_DISABLE_DEPRECATION_WARNINGS
-    av_frame_unref(avctx->coded_frame);
     ret = av_frame_ref(avctx->coded_frame, s->current_picture);
 FF_ENABLE_DEPRECATION_WARNINGS
 #endif