[FFmpeg-devel,10/11] cbs_mpeg2: Fix parsing of picture headers

Submitted by Andreas Rheinhardt on May 22, 2019, 1:04 a.m.

Details

Message ID 20190522010441.44257-11-andreas.rheinhardt@gmail.com
State New
Headers show

Commit Message

Andreas Rheinhardt May 22, 2019, 1:04 a.m.
MPEG-2 picture and slice headers can contain optional extra information;
both use the same syntax for their extra information. And cbs_mpeg2's
implementations of both were buggy until recently; the one for the
picture headers still is and this is fixed in this commit.

The extra information in picture headers has simply been forgotten.
This meant that if this extra information was present, it was discarded
during reading; and unfortunately writing created invalid bitstreams in
this case (an extra_bit_picture - the last set bit of the whole unit -
indicated that there would be a further byte of data, although the output
didn't contain said data).

This has been fixed; both types of extra information are now
parsed via the same code and essentially passed through.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/cbs_mpeg2.c                 | 31 +++++++-----
 libavcodec/cbs_mpeg2.h                 | 12 +++--
 libavcodec/cbs_mpeg2_syntax_template.c | 66 +++++++++++++++-----------
 3 files changed, 66 insertions(+), 43 deletions(-)

Comments

Mark Thompson May 28, 2019, 11:29 p.m.
On 22/05/2019 02:04, Andreas Rheinhardt wrote:
> MPEG-2 picture and slice headers can contain optional extra information;
> both use the same syntax for their extra information. And cbs_mpeg2's
> implementations of both were buggy until recently; the one for the
> picture headers still is and this is fixed in this commit.
> 
> The extra information in picture headers has simply been forgotten.
> This meant that if this extra information was present, it was discarded
> during reading; and unfortunately writing created invalid bitstreams in
> this case (an extra_bit_picture - the last set bit of the whole unit -
> indicated that there would be a further byte of data, although the output
> didn't contain said data).
> 
> This has been fixed; both types of extra information are now
> parsed via the same code and essentially passed through.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
> ---
>  libavcodec/cbs_mpeg2.c                 | 31 +++++++-----
>  libavcodec/cbs_mpeg2.h                 | 12 +++--
>  libavcodec/cbs_mpeg2_syntax_template.c | 66 +++++++++++++++-----------
>  3 files changed, 66 insertions(+), 43 deletions(-)
> 
> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
> index 97425aa706..2354f665cd 100644
> --- a/libavcodec/cbs_mpeg2.c
> +++ b/libavcodec/cbs_mpeg2.c
> @@ -41,18 +41,18 @@
>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>  
>  #define ui(width, name) \
> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), 0)
>  #define uir(width, name) \
> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), 0)
>  #define uis(width, name, subs, ...) \
> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>  #define uirs(width, name, subs, ...) \
> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>  #define sis(width, name, subs, ...) \
> -        xsi(width, name, current->name, subs, __VA_ARGS__)
> +        xsi(width, #name, current->name, subs, __VA_ARGS__)
>  
>  #define marker_bit() \
> -        bit(marker_bit, 1)
> +        bit("marker_bit", 1)
>  #define bit(name, value) do { \
>          av_unused uint32_t bit = value; \
>          xui(1, name, bit, value, value, 0); \
> @@ -65,7 +65,7 @@
>  
>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
>          uint32_t value; \
> -        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
> +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, name, \
>                                     SUBSCRIPTS(subs, __VA_ARGS__), \
>                                     &value, range_min, range_max)); \
>          var = value; \
> @@ -73,7 +73,7 @@
>  
>  #define xsi(width, name, var, subs, ...) do { \
>          int32_t value; \
> -        CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \
> +        CHECK(ff_cbs_read_signed(ctx, rw, width, name, \
>                                   SUBSCRIPTS(subs, __VA_ARGS__), &value, \
>                                   MIN_INT_BITS(width), \
>                                   MAX_INT_BITS(width))); \
> @@ -104,13 +104,13 @@
>  #define RWContext PutBitContext
>  
>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
> -        CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
> +        CHECK(ff_cbs_write_unsigned(ctx, rw, width, name, \
>                                      SUBSCRIPTS(subs, __VA_ARGS__), \
>                                      var, range_min, range_max)); \
>      } while (0)
>  
>  #define xsi(width, name, var, subs, ...) do { \
> -        CHECK(ff_cbs_write_signed(ctx, rw, width, #name, \
> +        CHECK(ff_cbs_write_signed(ctx, rw, width, name, \
>                                    SUBSCRIPTS(subs, __VA_ARGS__), var, \
>                                    MIN_INT_BITS(width), \
>                                    MAX_INT_BITS(width))); \

Calling the inner functions directly in extra_information feels like it would be cleaner?  This part makes the intermediate macros for mpeg2 act in a way which is subtly different to all the other codecs.

> @@ -138,6 +138,13 @@
>  #undef infer
>  
>  
> +static void cbs_mpeg2_free_picture_header(void *unit, uint8_t *content)
> +{
> +    MPEG2RawPictureHeader *picture = (MPEG2RawPictureHeader*)content;
> +    av_buffer_unref(&picture->extra_information_picture.extra_information_ref);
> +    av_free(content);
> +}
> +
>  static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
>  {
>      MPEG2RawUserData *user = (MPEG2RawUserData*)content;
> @@ -148,7 +155,7 @@ static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
>  static void cbs_mpeg2_free_slice(void *unit, uint8_t *content)
>  {
>      MPEG2RawSlice *slice = (MPEG2RawSlice*)content;
> -    av_buffer_unref(&slice->header.extra_information_ref);
> +    av_buffer_unref(&slice->header.extra_information_slice.extra_information_ref);
>      av_buffer_unref(&slice->data_ref);
>      av_free(content);
>  }
> @@ -251,7 +258,7 @@ static int cbs_mpeg2_read_unit(CodedBitstreamContext *ctx,
>              } \
>              break;
>              START(MPEG2_START_PICTURE,   MPEG2RawPictureHeader,
> -                                                        picture_header,  NULL);
> +                               picture_header, &cbs_mpeg2_free_picture_header);
>              START(MPEG2_START_USER_DATA, MPEG2RawUserData,
>                                           user_data, &cbs_mpeg2_free_user_data);
>              START(MPEG2_START_SEQUENCE_HEADER, MPEG2RawSequenceHeader,
> diff --git a/libavcodec/cbs_mpeg2.h b/libavcodec/cbs_mpeg2.h
> index db5ebc56ea..2befaab275 100644
> --- a/libavcodec/cbs_mpeg2.h
> +++ b/libavcodec/cbs_mpeg2.h
> @@ -114,6 +114,12 @@ typedef struct MPEG2RawGroupOfPicturesHeader {
>      uint8_t broken_link;
>  } MPEG2RawGroupOfPicturesHeader;
>  
> +typedef struct MPEG2RawExtraInformation {
> +    uint8_t     *extra_information;
> +    AVBufferRef *extra_information_ref;
> +    size_t       extra_information_length;
> +} MPEG2RawExtraInformation;
> +
>  typedef struct MPEG2RawPictureHeader {
>      uint8_t picture_start_code;
>  
> @@ -126,7 +132,7 @@ typedef struct MPEG2RawPictureHeader {
>      uint8_t full_pel_backward_vector;
>      uint8_t backward_f_code;
>  
> -    uint8_t extra_bit_picture;
> +    MPEG2RawExtraInformation extra_information_picture;
>  } MPEG2RawPictureHeader;
>  
>  typedef struct MPEG2RawPictureCodingExtension {
> @@ -194,9 +200,7 @@ typedef struct MPEG2RawSliceHeader {
>      uint8_t slice_picture_id_enable;
>      uint8_t slice_picture_id;
>  
> -    size_t extra_information_length;
> -    uint8_t *extra_information;
> -    AVBufferRef *extra_information_ref;
> +    MPEG2RawExtraInformation extra_information_slice;
>  } MPEG2RawSliceHeader;
>  
>  typedef struct MPEG2RawSlice {
> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
> index 1f2d2497c3..325a48676e 100644
> --- a/libavcodec/cbs_mpeg2_syntax_template.c
> +++ b/libavcodec/cbs_mpeg2_syntax_template.c
> @@ -173,6 +173,40 @@ static int FUNC(group_of_pictures_header)(CodedBitstreamContext *ctx, RWContext
>      return 0;
>  }
>  
> +static int FUNC(extra_information)(CodedBitstreamContext *ctx, RWContext *rw,
> +                                   MPEG2RawExtraInformation *current,
> +                                   const char *element_name, const char *marker_name)
> +{
> +    int err;
> +    size_t k;
> +#ifdef READ
> +    GetBitContext start = *rw;
> +    uint8_t bit;
> +
> +    for (k = 0; nextbits(1, 1, bit); k++)
> +        skip_bits(rw, 1 + 8);
> +    current->extra_information_length = k;
> +    if (k > 0) {
> +        *rw = start;
> +        current->extra_information_ref =
> +            av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
> +        if (!current->extra_information_ref)
> +            return AVERROR(ENOMEM);
> +        current->extra_information = current->extra_information_ref->data;
> +    }
> +#endif
> +
> +    for (k = 0; k < current->extra_information_length; k++) {
> +        bit(marker_name, 1);
> +        xui(8, element_name,
> +            current->extra_information[k], 0, 255, 1, k);
> +    }
> +
> +    bit(marker_name, 0);
> +
> +    return 0;
> +}
> +
>  static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
>                                  MPEG2RawPictureHeader *current)
>  {
> @@ -197,7 +231,8 @@ static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
>          ui(3, backward_f_code);
>      }
>  
> -    ui(1, extra_bit_picture);
> +    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_picture,
> +                                  "extra_information_picture[k]", "extra_bit_picture"));
>  
>      return 0;
>  }
> @@ -369,33 +404,10 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>          ui(1, intra_slice);
>          ui(1, slice_picture_id_enable);
>          ui(6, slice_picture_id);
> -
> -        {
> -            size_t k;
> -#ifdef READ
> -            GetBitContext start;
> -            uint8_t bit;
> -            start = *rw;
> -            for (k = 0; nextbits(1, 1, bit); k++)
> -                skip_bits(rw, 1 + 8);
> -            current->extra_information_length = k;
> -            if (k > 0) {
> -                *rw = start;
> -                current->extra_information_ref =
> -                    av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
> -                if (!current->extra_information_ref)
> -                    return AVERROR(ENOMEM);
> -                current->extra_information = current->extra_information_ref->data;
> -            }
> -#endif
> -            for (k = 0; k < current->extra_information_length; k++) {
> -                bit(extra_bit_slice, 1);
> -                xui(8, extra_information_slice[k],
> -                    current->extra_information[k], 0, 255, 1, k);
> -            }
> -        }
>      }
> -    bit(extra_bit_slice, 0);
> +
> +    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_slice,
> +                                  "extra_information_slice[k]", "extra_bit_slice"));
>  
>      return 0;
>  }
> 

Not sure if this would be better, but maybe consider reordering to put adding the new extra_information structure before 9/11 so you can just drop in the correct function call there?

- Mark
Andreas Rheinhardt May 29, 2019, 4:54 a.m.
Mark Thompson:
> On 22/05/2019 02:04, Andreas Rheinhardt wrote:
>> MPEG-2 picture and slice headers can contain optional extra information;
>> both use the same syntax for their extra information. And cbs_mpeg2's
>> implementations of both were buggy until recently; the one for the
>> picture headers still is and this is fixed in this commit.
>>
>> The extra information in picture headers has simply been forgotten.
>> This meant that if this extra information was present, it was discarded
>> during reading; and unfortunately writing created invalid bitstreams in
>> this case (an extra_bit_picture - the last set bit of the whole unit -
>> indicated that there would be a further byte of data, although the output
>> didn't contain said data).
>>
>> This has been fixed; both types of extra information are now
>> parsed via the same code and essentially passed through.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>> ---
>>  libavcodec/cbs_mpeg2.c                 | 31 +++++++-----
>>  libavcodec/cbs_mpeg2.h                 | 12 +++--
>>  libavcodec/cbs_mpeg2_syntax_template.c | 66 +++++++++++++++-----------
>>  3 files changed, 66 insertions(+), 43 deletions(-)
>>
>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>> index 97425aa706..2354f665cd 100644
>> --- a/libavcodec/cbs_mpeg2.c
>> +++ b/libavcodec/cbs_mpeg2.c
>> @@ -41,18 +41,18 @@
>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>  
>>  #define ui(width, name) \
>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), 0)
>>  #define uir(width, name) \
>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
>> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), 0)
>>  #define uis(width, name, subs, ...) \
>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>  #define uirs(width, name, subs, ...) \
>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>  #define sis(width, name, subs, ...) \
>> -        xsi(width, name, current->name, subs, __VA_ARGS__)
>> +        xsi(width, #name, current->name, subs, __VA_ARGS__)
>>  
>>  #define marker_bit() \
>> -        bit(marker_bit, 1)
>> +        bit("marker_bit", 1)
>>  #define bit(name, value) do { \
>>          av_unused uint32_t bit = value; \
>>          xui(1, name, bit, value, value, 0); \
>> @@ -65,7 +65,7 @@
>>  
>>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
>>          uint32_t value; \
>> -        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
>> +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, name, \
>>                                     SUBSCRIPTS(subs, __VA_ARGS__), \
>>                                     &value, range_min, range_max)); \
>>          var = value; \
>> @@ -73,7 +73,7 @@
>>  
>>  #define xsi(width, name, var, subs, ...) do { \
>>          int32_t value; \
>> -        CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \
>> +        CHECK(ff_cbs_read_signed(ctx, rw, width, name, \
>>                                   SUBSCRIPTS(subs, __VA_ARGS__), &value, \
>>                                   MIN_INT_BITS(width), \
>>                                   MAX_INT_BITS(width))); \
>> @@ -104,13 +104,13 @@
>>  #define RWContext PutBitContext
>>  
>>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
>> -        CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
>> +        CHECK(ff_cbs_write_unsigned(ctx, rw, width, name, \
>>                                      SUBSCRIPTS(subs, __VA_ARGS__), \
>>                                      var, range_min, range_max)); \
>>      } while (0)
>>  
>>  #define xsi(width, name, var, subs, ...) do { \
>> -        CHECK(ff_cbs_write_signed(ctx, rw, width, #name, \
>> +        CHECK(ff_cbs_write_signed(ctx, rw, width, name, \
>>                                    SUBSCRIPTS(subs, __VA_ARGS__), var, \
>>                                    MIN_INT_BITS(width), \
>>                                    MAX_INT_BITS(width))); \
> 
> Calling the inner functions directly in extra_information feels like it would be cleaner?  This part makes the intermediate macros for mpeg2 act in a way which is subtly different to all the other codecs.
> 
Agreed. The rationale I did it the way I did is of course that there
turned out to be exactly one call to xui in the mpeg2-syntax-template.
Or maybe one should add a new macro that is the macro actually calling
the inner functions directly and gets used by xui? If we hadn't used
the 's' for subscripts, xuis would be the obvious choice for this.
>> @@ -138,6 +138,13 @@
>>  #undef infer
>>  
>>  
>> +static void cbs_mpeg2_free_picture_header(void *unit, uint8_t *content)
>> +{
>> +    MPEG2RawPictureHeader *picture = (MPEG2RawPictureHeader*)content;
>> +    av_buffer_unref(&picture->extra_information_picture.extra_information_ref);
>> +    av_free(content);
>> +}
>> +
>>  static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
>>  {
>>      MPEG2RawUserData *user = (MPEG2RawUserData*)content;
>> @@ -148,7 +155,7 @@ static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
>>  static void cbs_mpeg2_free_slice(void *unit, uint8_t *content)
>>  {
>>      MPEG2RawSlice *slice = (MPEG2RawSlice*)content;
>> -    av_buffer_unref(&slice->header.extra_information_ref);
>> +    av_buffer_unref(&slice->header.extra_information_slice.extra_information_ref);
>>      av_buffer_unref(&slice->data_ref);
>>      av_free(content);
>>  }
>> @@ -251,7 +258,7 @@ static int cbs_mpeg2_read_unit(CodedBitstreamContext *ctx,
>>              } \
>>              break;
>>              START(MPEG2_START_PICTURE,   MPEG2RawPictureHeader,
>> -                                                        picture_header,  NULL);
>> +                               picture_header, &cbs_mpeg2_free_picture_header);
>>              START(MPEG2_START_USER_DATA, MPEG2RawUserData,
>>                                           user_data, &cbs_mpeg2_free_user_data);
>>              START(MPEG2_START_SEQUENCE_HEADER, MPEG2RawSequenceHeader,
>> diff --git a/libavcodec/cbs_mpeg2.h b/libavcodec/cbs_mpeg2.h
>> index db5ebc56ea..2befaab275 100644
>> --- a/libavcodec/cbs_mpeg2.h
>> +++ b/libavcodec/cbs_mpeg2.h
>> @@ -114,6 +114,12 @@ typedef struct MPEG2RawGroupOfPicturesHeader {
>>      uint8_t broken_link;
>>  } MPEG2RawGroupOfPicturesHeader;
>>  
>> +typedef struct MPEG2RawExtraInformation {
>> +    uint8_t     *extra_information;
>> +    AVBufferRef *extra_information_ref;
>> +    size_t       extra_information_length;
>> +} MPEG2RawExtraInformation;
>> +
>>  typedef struct MPEG2RawPictureHeader {
>>      uint8_t picture_start_code;
>>  
>> @@ -126,7 +132,7 @@ typedef struct MPEG2RawPictureHeader {
>>      uint8_t full_pel_backward_vector;
>>      uint8_t backward_f_code;
>>  
>> -    uint8_t extra_bit_picture;
>> +    MPEG2RawExtraInformation extra_information_picture;
>>  } MPEG2RawPictureHeader;
>>  
>>  typedef struct MPEG2RawPictureCodingExtension {
>> @@ -194,9 +200,7 @@ typedef struct MPEG2RawSliceHeader {
>>      uint8_t slice_picture_id_enable;
>>      uint8_t slice_picture_id;
>>  
>> -    size_t extra_information_length;
>> -    uint8_t *extra_information;
>> -    AVBufferRef *extra_information_ref;
>> +    MPEG2RawExtraInformation extra_information_slice;
>>  } MPEG2RawSliceHeader;
>>  
>>  typedef struct MPEG2RawSlice {
>> diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
>> index 1f2d2497c3..325a48676e 100644
>> --- a/libavcodec/cbs_mpeg2_syntax_template.c
>> +++ b/libavcodec/cbs_mpeg2_syntax_template.c
>> @@ -173,6 +173,40 @@ static int FUNC(group_of_pictures_header)(CodedBitstreamContext *ctx, RWContext
>>      return 0;
>>  }
>>  
>> +static int FUNC(extra_information)(CodedBitstreamContext *ctx, RWContext *rw,
>> +                                   MPEG2RawExtraInformation *current,
>> +                                   const char *element_name, const char *marker_name)
>> +{
>> +    int err;
>> +    size_t k;
>> +#ifdef READ
>> +    GetBitContext start = *rw;
>> +    uint8_t bit;
>> +
>> +    for (k = 0; nextbits(1, 1, bit); k++)
>> +        skip_bits(rw, 1 + 8);
>> +    current->extra_information_length = k;
>> +    if (k > 0) {
>> +        *rw = start;
>> +        current->extra_information_ref =
>> +            av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
>> +        if (!current->extra_information_ref)
>> +            return AVERROR(ENOMEM);
>> +        current->extra_information = current->extra_information_ref->data;
>> +    }
>> +#endif
>> +
>> +    for (k = 0; k < current->extra_information_length; k++) {
>> +        bit(marker_name, 1);
>> +        xui(8, element_name,
>> +            current->extra_information[k], 0, 255, 1, k);
>> +    }
>> +
>> +    bit(marker_name, 0);
>> +
>> +    return 0;
>> +}
>> +
>>  static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>                                  MPEG2RawPictureHeader *current)
>>  {
>> @@ -197,7 +231,8 @@ static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>          ui(3, backward_f_code);
>>      }
>>  
>> -    ui(1, extra_bit_picture);
>> +    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_picture,
>> +                                  "extra_information_picture[k]", "extra_bit_picture"));
>>  
>>      return 0;
>>  }
>> @@ -369,33 +404,10 @@ static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
>>          ui(1, intra_slice);
>>          ui(1, slice_picture_id_enable);
>>          ui(6, slice_picture_id);
>> -
>> -        {
>> -            size_t k;
>> -#ifdef READ
>> -            GetBitContext start;
>> -            uint8_t bit;
>> -            start = *rw;
>> -            for (k = 0; nextbits(1, 1, bit); k++)
>> -                skip_bits(rw, 1 + 8);
>> -            current->extra_information_length = k;
>> -            if (k > 0) {
>> -                *rw = start;
>> -                current->extra_information_ref =
>> -                    av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
>> -                if (!current->extra_information_ref)
>> -                    return AVERROR(ENOMEM);
>> -                current->extra_information = current->extra_information_ref->data;
>> -            }
>> -#endif
>> -            for (k = 0; k < current->extra_information_length; k++) {
>> -                bit(extra_bit_slice, 1);
>> -                xui(8, extra_information_slice[k],
>> -                    current->extra_information[k], 0, 255, 1, k);
>> -            }
>> -        }
>>      }
>> -    bit(extra_bit_slice, 0);
>> +
>> +    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_slice,
>> +                                  "extra_information_slice[k]", "extra_bit_slice"));
>>  
>>      return 0;
>>  }
>>
> 
> Not sure if this would be better, but maybe consider reordering to put adding the new extra_information structure before 9/11 so you can just drop in the correct function call there?
> 
I don't mind. There is no need to do this in the slice headers, so I
didn't. But your proposal leads to a smaller diff overall, so I'll do it.
> - Mark
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>
Mark Thompson June 2, 2019, 5:13 p.m.
On 29/05/2019 05:54, Andreas Rheinhardt wrote:
> Mark Thompson:
>> On 22/05/2019 02:04, Andreas Rheinhardt wrote:
>>> MPEG-2 picture and slice headers can contain optional extra information;
>>> both use the same syntax for their extra information. And cbs_mpeg2's
>>> implementations of both were buggy until recently; the one for the
>>> picture headers still is and this is fixed in this commit.
>>>
>>> The extra information in picture headers has simply been forgotten.
>>> This meant that if this extra information was present, it was discarded
>>> during reading; and unfortunately writing created invalid bitstreams in
>>> this case (an extra_bit_picture - the last set bit of the whole unit -
>>> indicated that there would be a further byte of data, although the output
>>> didn't contain said data).
>>>
>>> This has been fixed; both types of extra information are now
>>> parsed via the same code and essentially passed through.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
>>> ---
>>>  libavcodec/cbs_mpeg2.c                 | 31 +++++++-----
>>>  libavcodec/cbs_mpeg2.h                 | 12 +++--
>>>  libavcodec/cbs_mpeg2_syntax_template.c | 66 +++++++++++++++-----------
>>>  3 files changed, 66 insertions(+), 43 deletions(-)
>>>
>>> diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
>>> index 97425aa706..2354f665cd 100644
>>> --- a/libavcodec/cbs_mpeg2.c
>>> +++ b/libavcodec/cbs_mpeg2.c
>>> @@ -41,18 +41,18 @@
>>>  #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
>>>  
>>>  #define ui(width, name) \
>>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
>>> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), 0)
>>>  #define uir(width, name) \
>>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
>>> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), 0)
>>>  #define uis(width, name, subs, ...) \
>>> -        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>> +        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>>  #define uirs(width, name, subs, ...) \
>>> -        xui(width, name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>> +        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
>>>  #define sis(width, name, subs, ...) \
>>> -        xsi(width, name, current->name, subs, __VA_ARGS__)
>>> +        xsi(width, #name, current->name, subs, __VA_ARGS__)
>>>  
>>>  #define marker_bit() \
>>> -        bit(marker_bit, 1)
>>> +        bit("marker_bit", 1)
>>>  #define bit(name, value) do { \
>>>          av_unused uint32_t bit = value; \
>>>          xui(1, name, bit, value, value, 0); \
>>> @@ -65,7 +65,7 @@
>>>  
>>>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
>>>          uint32_t value; \
>>> -        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
>>> +        CHECK(ff_cbs_read_unsigned(ctx, rw, width, name, \
>>>                                     SUBSCRIPTS(subs, __VA_ARGS__), \
>>>                                     &value, range_min, range_max)); \
>>>          var = value; \
>>> @@ -73,7 +73,7 @@
>>>  
>>>  #define xsi(width, name, var, subs, ...) do { \
>>>          int32_t value; \
>>> -        CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \
>>> +        CHECK(ff_cbs_read_signed(ctx, rw, width, name, \
>>>                                   SUBSCRIPTS(subs, __VA_ARGS__), &value, \
>>>                                   MIN_INT_BITS(width), \
>>>                                   MAX_INT_BITS(width))); \
>>> @@ -104,13 +104,13 @@
>>>  #define RWContext PutBitContext
>>>  
>>>  #define xui(width, name, var, range_min, range_max, subs, ...) do { \
>>> -        CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
>>> +        CHECK(ff_cbs_write_unsigned(ctx, rw, width, name, \
>>>                                      SUBSCRIPTS(subs, __VA_ARGS__), \
>>>                                      var, range_min, range_max)); \
>>>      } while (0)
>>>  
>>>  #define xsi(width, name, var, subs, ...) do { \
>>> -        CHECK(ff_cbs_write_signed(ctx, rw, width, #name, \
>>> +        CHECK(ff_cbs_write_signed(ctx, rw, width, name, \
>>>                                    SUBSCRIPTS(subs, __VA_ARGS__), var, \
>>>                                    MIN_INT_BITS(width), \
>>>                                    MAX_INT_BITS(width))); \
>>
>> Calling the inner functions directly in extra_information feels like it would be cleaner?  This part makes the intermediate macros for mpeg2 act in a way which is subtly different to all the other codecs.
>>
> Agreed. The rationale I did it the way I did is of course that there
> turned out to be exactly one call to xui in the mpeg2-syntax-template.
> Or maybe one should add a new macro that is the macro actually calling
> the inner functions directly and gets used by xui? If we hadn't used
> the 's' for subscripts, xuis would be the obvious choice for this.

This last suggestion seems nicest, though I'm not sure how to actually implement it without duplication (the subscript varargs are hard to pass through without using something like the ,## extension).

If you have a clean way to do it then that sounds good to me.

Thanks,

- Mark

Patch hide | download patch | download mbox

diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c
index 97425aa706..2354f665cd 100644
--- a/libavcodec/cbs_mpeg2.c
+++ b/libavcodec/cbs_mpeg2.c
@@ -41,18 +41,18 @@ 
 #define SUBSCRIPTS(subs, ...) (subs > 0 ? ((int[subs + 1]){ subs, __VA_ARGS__ }) : NULL)
 
 #define ui(width, name) \
-        xui(width, name, current->name, 0, MAX_UINT_BITS(width), 0)
+        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), 0)
 #define uir(width, name) \
-        xui(width, name, current->name, 1, MAX_UINT_BITS(width), 0)
+        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), 0)
 #define uis(width, name, subs, ...) \
-        xui(width, name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
+        xui(width, #name, current->name, 0, MAX_UINT_BITS(width), subs, __VA_ARGS__)
 #define uirs(width, name, subs, ...) \
-        xui(width, name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
+        xui(width, #name, current->name, 1, MAX_UINT_BITS(width), subs, __VA_ARGS__)
 #define sis(width, name, subs, ...) \
-        xsi(width, name, current->name, subs, __VA_ARGS__)
+        xsi(width, #name, current->name, subs, __VA_ARGS__)
 
 #define marker_bit() \
-        bit(marker_bit, 1)
+        bit("marker_bit", 1)
 #define bit(name, value) do { \
         av_unused uint32_t bit = value; \
         xui(1, name, bit, value, value, 0); \
@@ -65,7 +65,7 @@ 
 
 #define xui(width, name, var, range_min, range_max, subs, ...) do { \
         uint32_t value; \
-        CHECK(ff_cbs_read_unsigned(ctx, rw, width, #name, \
+        CHECK(ff_cbs_read_unsigned(ctx, rw, width, name, \
                                    SUBSCRIPTS(subs, __VA_ARGS__), \
                                    &value, range_min, range_max)); \
         var = value; \
@@ -73,7 +73,7 @@ 
 
 #define xsi(width, name, var, subs, ...) do { \
         int32_t value; \
-        CHECK(ff_cbs_read_signed(ctx, rw, width, #name, \
+        CHECK(ff_cbs_read_signed(ctx, rw, width, name, \
                                  SUBSCRIPTS(subs, __VA_ARGS__), &value, \
                                  MIN_INT_BITS(width), \
                                  MAX_INT_BITS(width))); \
@@ -104,13 +104,13 @@ 
 #define RWContext PutBitContext
 
 #define xui(width, name, var, range_min, range_max, subs, ...) do { \
-        CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \
+        CHECK(ff_cbs_write_unsigned(ctx, rw, width, name, \
                                     SUBSCRIPTS(subs, __VA_ARGS__), \
                                     var, range_min, range_max)); \
     } while (0)
 
 #define xsi(width, name, var, subs, ...) do { \
-        CHECK(ff_cbs_write_signed(ctx, rw, width, #name, \
+        CHECK(ff_cbs_write_signed(ctx, rw, width, name, \
                                   SUBSCRIPTS(subs, __VA_ARGS__), var, \
                                   MIN_INT_BITS(width), \
                                   MAX_INT_BITS(width))); \
@@ -138,6 +138,13 @@ 
 #undef infer
 
 
+static void cbs_mpeg2_free_picture_header(void *unit, uint8_t *content)
+{
+    MPEG2RawPictureHeader *picture = (MPEG2RawPictureHeader*)content;
+    av_buffer_unref(&picture->extra_information_picture.extra_information_ref);
+    av_free(content);
+}
+
 static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
 {
     MPEG2RawUserData *user = (MPEG2RawUserData*)content;
@@ -148,7 +155,7 @@  static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content)
 static void cbs_mpeg2_free_slice(void *unit, uint8_t *content)
 {
     MPEG2RawSlice *slice = (MPEG2RawSlice*)content;
-    av_buffer_unref(&slice->header.extra_information_ref);
+    av_buffer_unref(&slice->header.extra_information_slice.extra_information_ref);
     av_buffer_unref(&slice->data_ref);
     av_free(content);
 }
@@ -251,7 +258,7 @@  static int cbs_mpeg2_read_unit(CodedBitstreamContext *ctx,
             } \
             break;
             START(MPEG2_START_PICTURE,   MPEG2RawPictureHeader,
-                                                        picture_header,  NULL);
+                               picture_header, &cbs_mpeg2_free_picture_header);
             START(MPEG2_START_USER_DATA, MPEG2RawUserData,
                                          user_data, &cbs_mpeg2_free_user_data);
             START(MPEG2_START_SEQUENCE_HEADER, MPEG2RawSequenceHeader,
diff --git a/libavcodec/cbs_mpeg2.h b/libavcodec/cbs_mpeg2.h
index db5ebc56ea..2befaab275 100644
--- a/libavcodec/cbs_mpeg2.h
+++ b/libavcodec/cbs_mpeg2.h
@@ -114,6 +114,12 @@  typedef struct MPEG2RawGroupOfPicturesHeader {
     uint8_t broken_link;
 } MPEG2RawGroupOfPicturesHeader;
 
+typedef struct MPEG2RawExtraInformation {
+    uint8_t     *extra_information;
+    AVBufferRef *extra_information_ref;
+    size_t       extra_information_length;
+} MPEG2RawExtraInformation;
+
 typedef struct MPEG2RawPictureHeader {
     uint8_t picture_start_code;
 
@@ -126,7 +132,7 @@  typedef struct MPEG2RawPictureHeader {
     uint8_t full_pel_backward_vector;
     uint8_t backward_f_code;
 
-    uint8_t extra_bit_picture;
+    MPEG2RawExtraInformation extra_information_picture;
 } MPEG2RawPictureHeader;
 
 typedef struct MPEG2RawPictureCodingExtension {
@@ -194,9 +200,7 @@  typedef struct MPEG2RawSliceHeader {
     uint8_t slice_picture_id_enable;
     uint8_t slice_picture_id;
 
-    size_t extra_information_length;
-    uint8_t *extra_information;
-    AVBufferRef *extra_information_ref;
+    MPEG2RawExtraInformation extra_information_slice;
 } MPEG2RawSliceHeader;
 
 typedef struct MPEG2RawSlice {
diff --git a/libavcodec/cbs_mpeg2_syntax_template.c b/libavcodec/cbs_mpeg2_syntax_template.c
index 1f2d2497c3..325a48676e 100644
--- a/libavcodec/cbs_mpeg2_syntax_template.c
+++ b/libavcodec/cbs_mpeg2_syntax_template.c
@@ -173,6 +173,40 @@  static int FUNC(group_of_pictures_header)(CodedBitstreamContext *ctx, RWContext
     return 0;
 }
 
+static int FUNC(extra_information)(CodedBitstreamContext *ctx, RWContext *rw,
+                                   MPEG2RawExtraInformation *current,
+                                   const char *element_name, const char *marker_name)
+{
+    int err;
+    size_t k;
+#ifdef READ
+    GetBitContext start = *rw;
+    uint8_t bit;
+
+    for (k = 0; nextbits(1, 1, bit); k++)
+        skip_bits(rw, 1 + 8);
+    current->extra_information_length = k;
+    if (k > 0) {
+        *rw = start;
+        current->extra_information_ref =
+            av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
+        if (!current->extra_information_ref)
+            return AVERROR(ENOMEM);
+        current->extra_information = current->extra_information_ref->data;
+    }
+#endif
+
+    for (k = 0; k < current->extra_information_length; k++) {
+        bit(marker_name, 1);
+        xui(8, element_name,
+            current->extra_information[k], 0, 255, 1, k);
+    }
+
+    bit(marker_name, 0);
+
+    return 0;
+}
+
 static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
                                 MPEG2RawPictureHeader *current)
 {
@@ -197,7 +231,8 @@  static int FUNC(picture_header)(CodedBitstreamContext *ctx, RWContext *rw,
         ui(3, backward_f_code);
     }
 
-    ui(1, extra_bit_picture);
+    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_picture,
+                                  "extra_information_picture[k]", "extra_bit_picture"));
 
     return 0;
 }
@@ -369,33 +404,10 @@  static int FUNC(slice_header)(CodedBitstreamContext *ctx, RWContext *rw,
         ui(1, intra_slice);
         ui(1, slice_picture_id_enable);
         ui(6, slice_picture_id);
-
-        {
-            size_t k;
-#ifdef READ
-            GetBitContext start;
-            uint8_t bit;
-            start = *rw;
-            for (k = 0; nextbits(1, 1, bit); k++)
-                skip_bits(rw, 1 + 8);
-            current->extra_information_length = k;
-            if (k > 0) {
-                *rw = start;
-                current->extra_information_ref =
-                    av_buffer_allocz(k + AV_INPUT_BUFFER_PADDING_SIZE);
-                if (!current->extra_information_ref)
-                    return AVERROR(ENOMEM);
-                current->extra_information = current->extra_information_ref->data;
-            }
-#endif
-            for (k = 0; k < current->extra_information_length; k++) {
-                bit(extra_bit_slice, 1);
-                xui(8, extra_information_slice[k],
-                    current->extra_information[k], 0, 255, 1, k);
-            }
-        }
     }
-    bit(extra_bit_slice, 0);
+
+    CHECK(FUNC(extra_information)(ctx, rw, &current->extra_information_slice,
+                                  "extra_information_slice[k]", "extra_bit_slice"));
 
     return 0;
 }