[FFmpeg-devel] [PATCH 01/18] cbs: Allow non-blank packets in ff_cbs_write_packet

[James Almer]

On 6/17/2019 12:42 AM, Andreas Rheinhardt wrote:
> Up until now, ff_cbs_write_packet always initialized the packet
> structure it received without documenting this behaviour; furthermore,
> the packet's buffer would (on success) be overwritten with the new
> buffer without unreferencing the old. This meant that the input packet
> had to be either clean (otherwise there would be memleaks) in which case
> the initialization is redundant or uninitialized. ff_cbs_write_packet
> was never used with uninitialized packets, so the initialization was
> redundant. Worse yet, it forced callers to use more than one packet and
> made it difficult to add side-data to a packet designated for output,
> because said side-data could only be attached after the call to
> ff_cbs_write_packet.
> 
> This has been changed. It is now allowed to use a non-blank packet.
> The currently existing buffer will be unreferenced and replaced by
> the new one, as will be the accompanying fields (i.e. data and size).
> The rest isn't touched at all.
> 
> This change will enable us to use only one packet in the bitstream
> filters that rely on CBS.
> 
> This commit also updates the documentation of ff_cbs_write_extradata
> and ff_cbs_write_packet (to better describe existing behaviour and in
> the latter case to also describe the new behaviour).
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> ---
> I could also have made it unref the packet's buffer at the beginning;
> this would have the advantage that the packet's buffer would be freed
> after the units have been rewritten (if they are rewritten) and after
> the fragment's buffer has been unreferenced, so that maximum memory
> consumption would decrease. It would also be in line with all current
> callers of ff_cbs_write_packet, but maybe it wouldn't be what a future
> caller wants. What do you think? 
>  libavcodec/cbs.c |  3 ++-
>  libavcodec/cbs.h | 10 +++++++++-
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index 0260ba6f67..47679eca1b 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -357,7 +357,8 @@ int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>      if (!buf)
>          return AVERROR(ENOMEM);
>  
> -    av_init_packet(pkt);
> +    av_buffer_unref(&pkt->buf);

You should probably unref the packet, not just the AVBufferRef.

> +
>      pkt->buf  = buf;
>      pkt->data = frag->data;
>      pkt->size = frag->data_size;
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 967dcd1468..5260a39c63 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -297,7 +297,8 @@ int ff_cbs_write_fragment_data(CodedBitstreamContext *ctx,
>  /**
>   * Write the bitstream of a fragment to the extradata in codec parameters.
>   *
> - * This replaces any existing extradata in the structure.
> + * Modifies context and fragment as ff_cbs_write_fragment_data does and
> + * replaces any existing extradata in the structure.
>   */
>  int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>                             AVCodecParameters *par,
> @@ -305,6 +306,13 @@ int ff_cbs_write_extradata(CodedBitstreamContext *ctx,
>  
>  /**
>   * Write the bitstream of a fragment to a packet.
> + *
> + * Modifies context and fragment as ff_cbs_write_fragment_data does.
> + *
> + * On success, the packet's buf is unreferenced and its buf, data and
> + * size fields are set to the corresponding values from the newly updated
> + * fragment; other fields are not touched.  On failure, the packet is not
> + * touched at all.
>   */
>  int ff_cbs_write_packet(CodedBitstreamContext *ctx,
>                          AVPacket *pkt,
>