| Message ID | 20180323213822.10060-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 3/23/18, James Almer <jamrial@gmail.com> wrote: > Some bitstream filters may buffer said packet in their own contexts > for latter use. > The documentation for av_bsf_send_packet() doesn't forbid feeding > it non-reference counted packets, which depending on the way said > packets were internally buffered by the bsf it may result in the > data described in them to become invalid or unavailable at any time. > > This was the case with vp9_superframe after commit e1bc3f4396, which > was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the > case even today with vp9_reorder_raw. > > With this change the bitstream filters will not have to worry how to > store or consume the packets fed to them. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache > input packets using new references" for a local fix similar to what > vp9_superframe got with 37f4a093f7 and 7a02b364b6. > > A simple reproducer if you're curious is: > > ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null - > > Which segfauls with current git master. > > "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy > when possible" also works around this in most cases by doing what its > subject describes, but only affects the ffmpeg CLI only and not the > API itself, of course. > > libavcodec/bsf.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > index 38b423101c..25f7a20ad6 100644 > --- a/libavcodec/bsf.c > +++ b/libavcodec/bsf.c > @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket > *pkt) > ctx->internal->buffer_pkt->side_data_elems) > return AVERROR(EAGAIN); > > - av_packet_move_ref(ctx->internal->buffer_pkt, pkt); > + if (pkt->buf) Use { } here and below. > + av_packet_move_ref(ctx->internal->buffer_pkt, pkt); > + else { > + int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt); > + > + if (ret < 0) > + return ret; > + av_packet_unref(pkt); > + } > > return 0; > } > -- > 2.16.2 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, 23 Mar 2018 18:38:22 -0300 James Almer <jamrial@gmail.com> wrote: > Some bitstream filters may buffer said packet in their own contexts > for latter use. > The documentation for av_bsf_send_packet() doesn't forbid feeding > it non-reference counted packets, which depending on the way said > packets were internally buffered by the bsf it may result in the > data described in them to become invalid or unavailable at any time. > > This was the case with vp9_superframe after commit e1bc3f4396, which > was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the > case even today with vp9_reorder_raw. > > With this change the bitstream filters will not have to worry how to > store or consume the packets fed to them. > > Signed-off-by: James Almer <jamrial@gmail.com> > --- > Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache > input packets using new references" for a local fix similar to what > vp9_superframe got with 37f4a093f7 and 7a02b364b6. > > A simple reproducer if you're curious is: > > ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null - > > Which segfauls with current git master. > > "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy > when possible" also works around this in most cases by doing what its > subject describes, but only affects the ffmpeg CLI only and not the > API itself, of course. > > libavcodec/bsf.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c > index 38b423101c..25f7a20ad6 100644 > --- a/libavcodec/bsf.c > +++ b/libavcodec/bsf.c > @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt) > ctx->internal->buffer_pkt->side_data_elems) > return AVERROR(EAGAIN); > > - av_packet_move_ref(ctx->internal->buffer_pkt, pkt); > + if (pkt->buf) > + av_packet_move_ref(ctx->internal->buffer_pkt, pkt); > + else { > + int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt); > + > + if (ret < 0) > + return ret; > + av_packet_unref(pkt); > + } > > return 0; > } Fine, but we should probably really provide an API function that ensures that a packet is refcounted (and made refcounting if it isn't). av_dup_packet() does this, but it's deprecated and has a bad name.
On 3/23/2018 7:46 PM, wm4 wrote: > On Fri, 23 Mar 2018 18:38:22 -0300 > James Almer <jamrial@gmail.com> wrote: > >> Some bitstream filters may buffer said packet in their own contexts >> for latter use. >> The documentation for av_bsf_send_packet() doesn't forbid feeding >> it non-reference counted packets, which depending on the way said >> packets were internally buffered by the bsf it may result in the >> data described in them to become invalid or unavailable at any time. >> >> This was the case with vp9_superframe after commit e1bc3f4396, which >> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the >> case even today with vp9_reorder_raw. >> >> With this change the bitstream filters will not have to worry how to >> store or consume the packets fed to them. >> >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache >> input packets using new references" for a local fix similar to what >> vp9_superframe got with 37f4a093f7 and 7a02b364b6. >> >> A simple reproducer if you're curious is: >> >> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null - >> >> Which segfauls with current git master. >> >> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy >> when possible" also works around this in most cases by doing what its >> subject describes, but only affects the ffmpeg CLI only and not the >> API itself, of course. >> >> libavcodec/bsf.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c >> index 38b423101c..25f7a20ad6 100644 >> --- a/libavcodec/bsf.c >> +++ b/libavcodec/bsf.c >> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt) >> ctx->internal->buffer_pkt->side_data_elems) >> return AVERROR(EAGAIN); >> >> - av_packet_move_ref(ctx->internal->buffer_pkt, pkt); >> + if (pkt->buf) >> + av_packet_move_ref(ctx->internal->buffer_pkt, pkt); >> + else { >> + int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt); >> + >> + if (ret < 0) >> + return ret; >> + av_packet_unref(pkt); >> + } >> >> return 0; >> } > > Fine, but we should probably really provide an API function that > ensures that a packet is refcounted (and made refcounting if it isn't). > av_dup_packet() does this, but it's deprecated and has a bad name. av_packet_ref() ensures that, and so does av_packet_make_writable(), but as a side effect of their main intended use. The documentation even states to use av_packet_ref() for that purpose. If you want one exactly like av_dup_packet() but in a less hacky way that exclusively does "Make this packet's data ref counted if it's not, do nothing else", we could add an av_packet_make_refcounted() function or whatever. It should be trivial, so just tell me a name you'd like for it and I'll write it.
On 3/23/2018 8:15 PM, James Almer wrote: > On 3/23/2018 7:46 PM, wm4 wrote: >> On Fri, 23 Mar 2018 18:38:22 -0300 >> James Almer <jamrial@gmail.com> wrote: >> >>> Some bitstream filters may buffer said packet in their own contexts >>> for latter use. >>> The documentation for av_bsf_send_packet() doesn't forbid feeding >>> it non-reference counted packets, which depending on the way said >>> packets were internally buffered by the bsf it may result in the >>> data described in them to become invalid or unavailable at any time. >>> >>> This was the case with vp9_superframe after commit e1bc3f4396, which >>> was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the >>> case even today with vp9_reorder_raw. >>> >>> With this change the bitstream filters will not have to worry how to >>> store or consume the packets fed to them. >>> >>> Signed-off-by: James Almer <jamrial@gmail.com> >>> --- >>> Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache >>> input packets using new references" for a local fix similar to what >>> vp9_superframe got with 37f4a093f7 and 7a02b364b6. >>> >>> A simple reproducer if you're curious is: >>> >>> ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null - >>> >>> Which segfauls with current git master. >>> >>> "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy >>> when possible" also works around this in most cases by doing what its >>> subject describes, but only affects the ffmpeg CLI only and not the >>> API itself, of course. >>> >>> libavcodec/bsf.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c >>> index 38b423101c..25f7a20ad6 100644 >>> --- a/libavcodec/bsf.c >>> +++ b/libavcodec/bsf.c >>> @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt) >>> ctx->internal->buffer_pkt->side_data_elems) >>> return AVERROR(EAGAIN); >>> >>> - av_packet_move_ref(ctx->internal->buffer_pkt, pkt); >>> + if (pkt->buf) >>> + av_packet_move_ref(ctx->internal->buffer_pkt, pkt); >>> + else { >>> + int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt); >>> + >>> + if (ret < 0) >>> + return ret; >>> + av_packet_unref(pkt); >>> + } >>> >>> return 0; >>> } >> >> Fine, but we should probably really provide an API function that >> ensures that a packet is refcounted (and made refcounting if it isn't). >> av_dup_packet() does this, but it's deprecated and has a bad name. > > av_packet_ref() ensures that, and so does av_packet_make_writable(), but > as a side effect of their main intended use. The documentation even > states to use av_packet_ref() for that purpose. > > If you want one exactly like av_dup_packet() but in a less hacky way > that exclusively does "Make this packet's data ref counted if it's not, > do nothing else", we could add an av_packet_make_refcounted() function > or whatever. It should be trivial, so just tell me a name you'd like for > it and I'll write it. Patch pushed in the meantime.
diff --git a/libavcodec/bsf.c b/libavcodec/bsf.c index 38b423101c..25f7a20ad6 100644 --- a/libavcodec/bsf.c +++ b/libavcodec/bsf.c @@ -188,7 +188,15 @@ int av_bsf_send_packet(AVBSFContext *ctx, AVPacket *pkt) ctx->internal->buffer_pkt->side_data_elems) return AVERROR(EAGAIN); - av_packet_move_ref(ctx->internal->buffer_pkt, pkt); + if (pkt->buf) + av_packet_move_ref(ctx->internal->buffer_pkt, pkt); + else { + int ret = av_packet_ref(ctx->internal->buffer_pkt, pkt); + + if (ret < 0) + return ret; + av_packet_unref(pkt); + } return 0; }
Some bitstream filters may buffer said packet in their own contexts for latter use. The documentation for av_bsf_send_packet() doesn't forbid feeding it non-reference counted packets, which depending on the way said packets were internally buffered by the bsf it may result in the data described in them to become invalid or unavailable at any time. This was the case with vp9_superframe after commit e1bc3f4396, which was then promptly fixed in 37f4a093f7 and 7a02b364b6. It is still the case even today with vp9_reorder_raw. With this change the bitstream filters will not have to worry how to store or consume the packets fed to them. Signed-off-by: James Almer <jamrial@gmail.com> --- Regarding vp9_raw_reorder, see "[PATCH] avcodec/vp9_raw_reorder: cache input packets using new references" for a local fix similar to what vp9_superframe got with 37f4a093f7 and 7a02b364b6. A simple reproducer if you're curious is: ffmpeg -i INPUT -c:v copy -bsf:v vp9_raw_reorder -f null - Which segfauls with current git master. "[PATCH 2/2] ffmpeg: pass reference counted packets on codec copy when possible" also works around this in most cases by doing what its subject describes, but only affects the ffmpeg CLI only and not the API itself, of course. libavcodec/bsf.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)