[FFmpeg-devel] [PATCH v2 8/9] av1_parser: Use CBS parser interface

[James Almer]

On 4/1/2019 8:39 PM, Mark Thompson wrote:
> This simplifies the parser and improves performance by reducing the number
> of allocations and eliminating redundant copies.
> ---
>  libavcodec/av1_parser.c | 63 +++++++++--------------------------------
>  1 file changed, 13 insertions(+), 50 deletions(-)
> 
> diff --git a/libavcodec/av1_parser.c b/libavcodec/av1_parser.c
> index b916608d65..4a743d92d4 100644
> --- a/libavcodec/av1_parser.c
> +++ b/libavcodec/av1_parser.c
> @@ -27,7 +27,7 @@
>  
>  typedef struct AV1ParseContext {
>      CodedBitstreamContext *cbc;
> -    CodedBitstreamFragment temporal_unit;
> +    AV1RawFrameHeader frame_header;
>      int parsed_extradata;
>  } AV1ParseContext;
>  
> @@ -50,8 +50,10 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>                              const uint8_t *data, int size)
>  {
>      AV1ParseContext *s = ctx->priv_data;
> -    CodedBitstreamFragment *td = &s->temporal_unit;
>      CodedBitstreamAV1Context *av1 = s->cbc->priv_data;
> +    AV1RawSequenceHeader *seq;
> +    AV1RawColorConfig *color;
> +    AV1RawFrameHeader *frame;
>      int ret;
>  
>      *out_data = data;
> @@ -66,67 +68,35 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>      if (avctx->extradata_size && !s->parsed_extradata) {
>          s->parsed_extradata = 1;
>  
> -        ret = ff_cbs_read(s->cbc, td, avctx->extradata, avctx->extradata_size);
> -        if (ret < 0) {
> +        ret = ff_cbs_parse_headers(s->cbc, NULL,
> +                                   avctx->extradata, avctx->extradata_size);
> +        if (ret < 0)
>              av_log(avctx, AV_LOG_WARNING, "Failed to parse extradata.\n");
> -        }
> -
> -        ff_cbs_fragment_reset(s->cbc, td);
>      }
>  
> -    ret = ff_cbs_read(s->cbc, td, data, size);
> +    ret = ff_cbs_parse_headers(s->cbc, &s->frame_header, data, size);
>      if (ret < 0) {
>          av_log(avctx, AV_LOG_ERROR, "Failed to parse temporal unit.\n");
>          goto end;
>      }
> +    frame = &s->frame_header;
>  
>      if (!av1->sequence_header) {
>          av_log(avctx, AV_LOG_ERROR, "No sequence header available\n");
>          goto end;
>      }
> +    seq = av1->sequence_header;
> +    color = &seq->color_config;
>  
> -    for (int i = 0; i < td->nb_units; i++) {
> -        CodedBitstreamUnit *unit = &td->units[i];
> -        AV1RawOBU *obu = unit->content;
> -        AV1RawSequenceHeader *seq = av1->sequence_header;
> -        AV1RawColorConfig *color = &seq->color_config;
> -        AV1RawFrameHeader *frame;
> -        int frame_type;
> -
> -        if (unit->type == AV1_OBU_FRAME)
> -            frame = &obu->obu.frame.header;
> -        else if (unit->type == AV1_OBU_FRAME_HEADER)
> -            frame = &obu->obu.frame_header;
> -        else
> -            continue;
> -
> -        if (frame->show_existing_frame) {
> -            AV1ReferenceFrameState *ref = &av1->ref[frame->frame_to_show_map_idx];
> -
> -            if (!ref->valid) {
> -                av_log(avctx, AV_LOG_ERROR, "Invalid reference frame\n");
> -                goto end;
> -            }
> -
> -            ctx->width  = ref->frame_width;
> -            ctx->height = ref->frame_height;
> -            frame_type  = ref->frame_type;
> -
> -            ctx->key_frame = 0;
> -        } else if (!frame->show_frame) {
> -            continue;
> -        } else {
>              ctx->width  = av1->frame_width;
>              ctx->height = av1->frame_height;

These should take the value from frame instead. As i said in another
reply, i don't think anything in the spec enforces the last frame in a
TU to be the visible one, only that there must be exactly one.

These two lines are in the parser in its current form, so either it's
fixed before this patch, or as part of it (The former would make it
easier to backport to release branches).

> -            frame_type  = frame->frame_type;
>  
> -            ctx->key_frame = frame_type == AV1_FRAME_KEY;
> -        }
> +            ctx->key_frame = frame->frame_type == AV1_FRAME_KEY;
>  
>          avctx->profile = seq->seq_profile;
>          avctx->level   = seq->seq_level_idx[0];
>  
> -        switch (frame_type) {
> +        switch (frame->frame_type) {
>          case AV1_FRAME_KEY:
>          case AV1_FRAME_INTRA_ONLY:
>              ctx->pict_type = AV_PICTURE_TYPE_I;
> @@ -155,11 +125,8 @@ static int av1_parser_parse(AVCodecParserContext *ctx,
>              break;
>          }
>          av_assert2(ctx->format != AV_PIX_FMT_NONE);
> -    }
>  
>  end:
> -    ff_cbs_fragment_reset(s->cbc, td);
> -
>      s->cbc->log_ctx = NULL;
>  
>      return size;
> @@ -182,9 +149,6 @@ static av_cold int av1_parser_init(AVCodecParserContext *ctx)
>      if (ret < 0)
>          return ret;
>  
> -    s->cbc->decompose_unit_types    = (CodedBitstreamUnitType *)decompose_unit_types;
> -    s->cbc->nb_decompose_unit_types = FF_ARRAY_ELEMS(decompose_unit_types);

You should also delete the decompose_unit_types array.

> -
>      return 0;
>  }
>  
> @@ -192,7 +156,6 @@ static void av1_parser_close(AVCodecParserContext *ctx)
>  {
>      AV1ParseContext *s = ctx->priv_data;
>  
> -    ff_cbs_fragment_free(s->cbc, &s->temporal_unit);
>      ff_cbs_close(&s->cbc);
>  }
>  
>