[FFmpeg-devel] [PATCH 10/11] cbs_mpeg2: Fix parsing of picture headers
Mark Thompson
sw at jkqxz.net
Wed May 29 02:29:29 EEST 2019
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 at 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, ¤t->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, ¤t->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
More information about the ffmpeg-devel
mailing list