[FFmpeg-devel] [PATCH 1/3] avcodec/cbs_av1: add support for Padding OBUs

[Mark Thompson]

On 25/03/2019 14:22, James Almer wrote:
> Based on itut_t35 Matadata OBU parsing code.
> 
> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
>  libavcodec/cbs_av1.c                 | 20 +++++++++++++++++
>  libavcodec/cbs_av1.h                 |  7 ++++++
>  libavcodec/cbs_av1_syntax_template.c | 32 ++++++++++++++++++++++++++++
>  3 files changed, 59 insertions(+)
> 
> diff --git a/libavcodec/cbs_av1.c b/libavcodec/cbs_av1.c
> index 02f168b58d..22330eabf3 100644
> --- a/libavcodec/cbs_av1.c
> +++ b/libavcodec/cbs_av1.c
> @@ -857,6 +857,11 @@ static void cbs_av1_free_tile_data(AV1RawTileData *td)
>      av_buffer_unref(&td->data_ref);
>  }
>  
> +static void cbs_av1_free_padding(AV1RawPadding *pd)
> +{
> +    av_buffer_unref(&pd->payload_ref);
> +}
> +
>  static void cbs_av1_free_metadata(AV1RawMetadata *md)
>  {
>      switch (md->metadata_type) {
> @@ -883,6 +888,9 @@ static void cbs_av1_free_obu(void *unit, uint8_t *content)
>      case AV1_OBU_METADATA:
>          cbs_av1_free_metadata(&obu->obu.metadata);
>          break;
> +    case AV1_OBU_PADDING:
> +        cbs_av1_free_padding(&obu->obu.padding);
> +        break;
>      }
>  
>      av_freep(&obu);
> @@ -1057,6 +1065,12 @@ static int cbs_av1_read_unit(CodedBitstreamContext *ctx,
>          }
>          break;
>      case AV1_OBU_PADDING:
> +        {
> +            err = cbs_av1_read_padding(ctx, &gbc, &obu->obu.padding);
> +            if (err < 0)
> +                return err;
> +        }
> +        break;
>      default:
>          return AVERROR(ENOSYS);
>      }
> @@ -1182,6 +1196,12 @@ static int cbs_av1_write_obu(CodedBitstreamContext *ctx,
>          }
>          break;
>      case AV1_OBU_PADDING:
> +        {
> +            err = cbs_av1_write_padding(ctx, pbc, &obu->obu.padding);
> +            if (err < 0)
> +                return err;
> +        }
> +        break;
>      default:
>          return AVERROR(ENOSYS);
>      }
> diff --git a/libavcodec/cbs_av1.h b/libavcodec/cbs_av1.h
> index 71ceff9427..e799964b72 100644
> --- a/libavcodec/cbs_av1.h
> +++ b/libavcodec/cbs_av1.h
> @@ -364,6 +364,12 @@ typedef struct AV1RawMetadata {
>      } metadata;
>  } AV1RawMetadata;
>  
> +typedef struct AV1RawPadding {
> +    uint8_t     *payload;
> +    size_t       payload_size;
> +    AVBufferRef *payload_ref;
> +} AV1RawPadding;
> +
>  
>  typedef struct AV1RawOBU {
>      AV1RawOBUHeader header;
> @@ -377,6 +383,7 @@ typedef struct AV1RawOBU {
>          AV1RawTileGroup      tile_group;
>          AV1RawTileList       tile_list;
>          AV1RawMetadata       metadata;
> +        AV1RawPadding        padding;
>      } obu;
>  } AV1RawOBU;
>  
> diff --git a/libavcodec/cbs_av1_syntax_template.c b/libavcodec/cbs_av1_syntax_template.c
> index 76eb90b279..a6cafdd99f 100644
> --- a/libavcodec/cbs_av1_syntax_template.c
> +++ b/libavcodec/cbs_av1_syntax_template.c
> @@ -1763,3 +1763,35 @@ static int FUNC(metadata_obu)(CodedBitstreamContext *ctx, RWContext *rw,
>  
>      return 0;
>  }
> +
> +static int FUNC(padding)(CodedBitstreamContext *ctx, RWContext *rw,
> +                         AV1RawPadding *current)
> +{
> +    int i, err;
> +
> +    HEADER("Padding");
> +
> +#ifdef READ
> +    // The payload runs up to the start of the trailing bits, but there might
> +    // be arbitrarily many trailing zeroes so we need to read through twice.
> +    {
> +        GetBitContext tmp = *rw;
> +        current->payload_size = 0;
> +        for (i = 0; get_bits_left(rw) >= 8; i++) {
> +            if (get_bits(rw, 8))
> +                current->payload_size = i;
> +        }
> +        *rw = tmp;
> +
> +        current->payload_ref = av_buffer_alloc(current->payload_size);
> +        if (!current->payload_ref)
> +            return AVERROR(ENOMEM);
> +        current->payload = current->payload_ref->data;
> +    }
> +#endif

That looks factorisable.  Can we make a "measure length of payload and allocate buffer for it" function and use it in metadata_itu_t35 as well?

> +
> +    for (i = 0; i < current->payload_size; i++)
> +        xf(8, obu_padding_byte[i], current->payload[i], 0x00, 0xff, 1, i);
> +
> +    return 0;
> +}
> 

Code looks sensible, but could you explain a bit more about why this is helpful.  Is there a use-case for preserving the actual padding bytes?  If that's some sort of CBR application, is it actually going to need to preserve the trailing zeroes as well?

Thanks,

- Mark