| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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 [...]
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". >
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 [...]
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 --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
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(-)