[FFmpeg-devel] [PATCH] avcodec/bsf: make sure the AVBSFInternal stored packet is reference counted

[wm4]

On Fri, 23 Mar 2018 18:38:22 -0300
James Almer <jamrial at 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 at 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.