[FFmpeg-devel] [PATCH 2/3] h2645_parse: Make ff_h2645_packet_split reference-compatible

[Mark Thompson]

On 21/11/18 18:34, Andreas Rheinhardt wrote:
> This is in preparation for a patch for cbs_h2645. Now the packet's
> rbsp_buffer can be owned by an AVBuffer.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at googlemail.com>
> ---
>  libavcodec/cbs_h2645.c             | 12 ++++-----
>  libavcodec/extract_extradata_bsf.c |  4 +--
>  libavcodec/h2645_parse.c           | 43 +++++++++++++++++++++++++++---
>  libavcodec/h2645_parse.h           | 10 +++++--
>  libavcodec/h264_parse.c            |  4 +--
>  libavcodec/h264dec.c               |  6 ++---
>  libavcodec/hevc_parse.c            |  5 ++--
>  libavcodec/hevc_parser.c           |  4 +--
>  libavcodec/hevcdec.c               |  4 +--
>  9 files changed, 67 insertions(+), 25 deletions(-)
> 
> ...
> diff --git a/libavcodec/h2645_parse.c b/libavcodec/h2645_parse.c
> index aaa4b8f443..ad5d04f0a4 100644
> --- a/libavcodec/h2645_parse.c
> +++ b/libavcodec/h2645_parse.c
> @@ -343,9 +343,37 @@ static int find_next_start_code(const uint8_t *buf, const uint8_t *next_avc)
>      return i + 3;
>  }
>  
> +static void handle_ref_buffer(H2645RBSP *rbsp, AVBufferRef **ref, int size)
> +{
> +    if (size + AV_INPUT_BUFFER_PADDING_SIZE <= 0)
> +        goto fail;
> +
> +    size += AV_INPUT_BUFFER_PADDING_SIZE;
> +
> +    if (*ref && (*ref)->size >= size && av_buffer_is_writable(*ref))
> +        return;
> +
> +    av_buffer_unref(ref);
> +
> +    size = FFMAX(size + (size >> 4) + 32, size);
> +
> +    *ref = av_buffer_alloc(size);
> +    if (*ref) {
> +        rbsp->rbsp_buffer            = (*ref)->data;
> +        rbsp->rbsp_buffer_alloc_size = size;
> +        return;
> +    }
> +
> +fail:
> +    av_buffer_unref(ref);
> +    rbsp->rbsp_buffer            = NULL;
> +    rbsp->rbsp_buffer_alloc_size = 0;
> +    return;
> +}
> +
>  int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>                            void *logctx, int is_nalff, int nal_length_size,
> -                          enum AVCodecID codec_id, int small_padding)
> +                          enum AVCodecID codec_id, int small_padding, AVBufferRef **ref)
>  {
>      GetByteContext bc;
>      int consumed, ret = 0;
> @@ -353,7 +381,11 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>      int64_t padding = small_padding ? 0 : MAX_MBPAIR_SIZE;
>  
>      bytestream2_init(&bc, buf, length);
> -    av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
> +    if (!ref)
> +        av_fast_padded_malloc(&pkt->rbsp.rbsp_buffer, &pkt->rbsp.rbsp_buffer_alloc_size, length + padding);
> +    else
> +        handle_ref_buffer(&pkt->rbsp, ref, length + padding);
> +
>      if (!pkt->rbsp.rbsp_buffer)
>          return AVERROR(ENOMEM);
>  
> @@ -466,7 +498,7 @@ int ff_h2645_packet_split(H2645Packet *pkt, const uint8_t *buf, int length,
>      return 0;
>  }
>  
> -void ff_h2645_packet_uninit(H2645Packet *pkt)
> +void ff_h2645_packet_uninit(H2645Packet *pkt, AVBufferRef *ref)
>  {
>      int i;
>      for (i = 0; i < pkt->nals_allocated; i++) {
> @@ -474,6 +506,9 @@ void ff_h2645_packet_uninit(H2645Packet *pkt)
>      }
>      av_freep(&pkt->nals);
>      pkt->nals_allocated = 0;
> -    av_freep(&pkt->rbsp.rbsp_buffer);
> +    if (!ref)
> +        av_freep(&pkt->rbsp.rbsp_buffer);
> +    else
> +        pkt->rbsp.rbsp_buffer = NULL;
>      pkt->rbsp.rbsp_buffer_alloc_size = pkt->rbsp.rbsp_buffer_size = 0;
>  }

I don't think I like how you've ended up with this dummy argument to uninit saying whether the content should be freed or not.

Can we instead move the AVBufferRef into the H2645Packet structure itself, so the user doesn't have to separately keep track of it?  Since the same packet structure is reused in cbs_h2645.c without an uninit, I think that avoids having to pass an extra argument to either function.

Thanks,

- Mark