[FFmpeg-devel] [PATCH 2/3] libavcodec/cbs: Don't zero twice

[Mark Thompson]

On 05/02/2019 20:08, Andreas Rheinhardt wrote:
> Up until now, a fragment that got reused was zeroed twice: Once during
> uninit and once during reading the next packet/extradata/buffer. The
> second zeroing has now been made optional.
> 
> This is also in preparation of actually reusing a fragment's units array.
> Otherwise it would be lost during reading.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
>  libavcodec/av1_metadata_bsf.c       |  4 ++--
>  libavcodec/av1_parser.c             |  4 ++--
>  libavcodec/cbs.c                    | 17 +++++++++++------
>  libavcodec/cbs.h                    | 20 +++++++++++++++++---
>  libavcodec/filter_units_bsf.c       |  4 ++--
>  libavcodec/h264_metadata_bsf.c      |  4 ++--
>  libavcodec/h264_redundant_pps_bsf.c |  4 ++--
>  libavcodec/h265_metadata_bsf.c      |  4 ++--
>  libavcodec/mpeg2_metadata_bsf.c     |  4 ++--
>  libavcodec/trace_headers_bsf.c      |  4 ++--
>  libavcodec/vp9_metadata_bsf.c       |  2 +-
>  11 files changed, 45 insertions(+), 26 deletions(-)
> 
> ...
> diff --git a/libavcodec/cbs.c b/libavcodec/cbs.c
> index b61dedb1eb..71f9fcbe32 100644
> --- a/libavcodec/cbs.c
> +++ b/libavcodec/cbs.c
> @@ -217,11 +217,13 @@ static int cbs_fill_fragment_data(CodedBitstreamContext *ctx,
>  
>  int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>                            CodedBitstreamFragment *frag,
> -                          const AVCodecParameters *par)
> +                          const AVCodecParameters *par,
> +                          int reuse)
>  {
>      int err;
>  
> -    memset(frag, 0, sizeof(*frag));
> +    if (!reuse)
> +        memset(frag, 0, sizeof(*frag));
>  
>      err = cbs_fill_fragment_data(ctx, frag, par->extradata,
>                                   par->extradata_size);
> @@ -237,11 +239,12 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>  
>  int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>                         CodedBitstreamFragment *frag,
> -                       const AVPacket *pkt)
> +                       const AVPacket *pkt, int reuse)
>  {
>      int err;
>  
> -    memset(frag, 0, sizeof(*frag));
> +    if (!reuse)
> +        memset(frag, 0, sizeof(*frag));
>  
>      if (pkt->buf) {
>          frag->data_ref = av_buffer_ref(pkt->buf);
> @@ -266,11 +269,13 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>  
>  int ff_cbs_read(CodedBitstreamContext *ctx,
>                  CodedBitstreamFragment *frag,
> -                const uint8_t *data, size_t size)
> +                const uint8_t *data, size_t size,
> +                int reuse)
>  {
>      int err;
>  
> -    memset(frag, 0, sizeof(*frag));
> +    if (!reuse)
> +        memset(frag, 0, sizeof(*frag));
>  
>      err = cbs_fill_fragment_data(ctx, frag, data, size);
>      if (err < 0)
> diff --git a/libavcodec/cbs.h b/libavcodec/cbs.h
> index 229cb129aa..2265b5d5bd 100644
> --- a/libavcodec/cbs.h
> +++ b/libavcodec/cbs.h
> @@ -240,10 +240,15 @@ void ff_cbs_close(CodedBitstreamContext **ctx);
>   * This also updates the internal state, so will need to be called for
>   * codecs with extradata to read parameter sets necessary for further
>   * parsing even if the fragment itself is not desired.
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and units members must
> + * be valid and all the other members have to be zero/NULL.
>   */
>  int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>                            CodedBitstreamFragment *frag,
> -                          const AVCodecParameters *par);
> +                          const AVCodecParameters *par,
> +                          int reuse);
>  
>  /**
>   * Read the data bitstream from a packet into a fragment, then
> @@ -252,10 +257,14 @@ int ff_cbs_read_extradata(CodedBitstreamContext *ctx,
>   * This also updates the internal state of the coded bitstream context
>   * with any persistent data from the fragment which may be required to
>   * read following fragments (e.g. parameter sets).
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and units members must
> + * be valid and all the other members have to be zero/NULL.
>   */
>  int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>                         CodedBitstreamFragment *frag,
> -                       const AVPacket *pkt);
> +                       const AVPacket *pkt, int reuse);
>  
>  /**
>   * Read a bitstream from a memory region into a fragment, then
> @@ -264,10 +273,15 @@ int ff_cbs_read_packet(CodedBitstreamContext *ctx,
>   * This also updates the internal state of the coded bitstream context
>   * with any persistent data from the fragment which may be required to
>   * read following fragments (e.g. parameter sets).
> + *
> + * If reuse is not set, the fragment will be zeroed before usage;
> + * otherwise, the fragment's units_allocated and units members must
> + * be valid and all the other members have to be zero/NULL.
>   */
>  int ff_cbs_read(CodedBitstreamContext *ctx,
>                  CodedBitstreamFragment *frag,
> -                const uint8_t *data, size_t size);
> +                const uint8_t *data, size_t size,
> +                int reuse);
>  
>  
>  /**

I don't think this patch should be needed.  Can we just document that your fragment must either be zeroed (so, allocated by av_*allocz() or memset() to zero) or you've called the ff_cbs_fragment_reset() (whatever the name is) function before any read call?  It can even check that the user hasn't messed up by asserting that data, data_size and nb_units are all zero.

Thanks,

- Mark