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

[James Almer]

On 4/1/2019 7:47 PM, Mark Thompson wrote:
> 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?

Will do.

> 
>> +
>> +    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?  

The padding could be anything, and samples could exploit it to insert
arbitrary data for whatever reason into the bitstream without damaging
it (ASCII art signature? :P).
I figured it was better to let the CBS caller decide if they want to
keep them, replace them with something else, or even inserting a brand
new one. Hardcoding it to 0xff or whatever didn't seem proper.

> If that's some sort of CBR application, is it actually going to need to preserve the trailing zeroes as well?

I don't think we're keeping them for any OBU type whatsoever when for
example when doing an av1_metadata packet passthrough. In
cbs_av1_write_obu() the obu_size is calculated based on actual defined
bits in the OBU syntax, plus tile data if any. The trailing bits it
writes are essentially just byte boundary alignment at the end of the OBU.

In any case, I personally don't think it's worth keeping extra trailing
zeroes, since they are fixed and can't have arbitrary data. For that
kind of extra padding, you could just add them to the obu_padding_byte
array.

> 
> Thanks,
> 
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>