From patchwork Sun Jun 2 22:37:29 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 13384 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 353014485A8 for ; Mon, 3 Jun 2019 01:39:05 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 1DDCE689A43; Mon, 3 Jun 2019 01:39:05 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f66.google.com (mail-wr1-f66.google.com [209.85.221.66]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id BF938689A9C for ; Mon, 3 Jun 2019 01:38:58 +0300 (EEST) Received: by mail-wr1-f66.google.com with SMTP id r18so1046213wrm.10 for ; Sun, 02 Jun 2019 15:38:58 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=MOizYGkgz7EDkMGysuU6jaBehgl0DPg+0MRn90fa2zc=; b=WoYZhgMTTTa0d8DqQTv2LpbQDo1LXumZzWr5d6QcFhbWFvYTrtnfR8iPdjQ2QNDgvQ DFesrO0mcu9UFx3zqQYgGEb7rouf8zdCwzG8b8MW1LpViIlZPUglqnZ8P4DO3ZBR/4RQ 2jGabaDiCqbbD0J2a9hzxNh3tT6ZBo9PDsKVKjvqITNwrGRQbe/LjrD0gfVIp+Z1708q FS4W/UcC9zxmyThXxl2HxbJU6JLRKxBeRsdKzJ7z3DTA72tXGQr79lheRNlJ1oogZEzK vRjvMrA9J89bzmHxMEZzOzX8zXmQlIM6ukx1m7XQK+/OO+4KOiVEsZtkbMiV3h3qTdNs BlCA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=MOizYGkgz7EDkMGysuU6jaBehgl0DPg+0MRn90fa2zc=; b=gZLDhXTT2omsjKrBdW38amkcsmGrH7V9j/mfGhkZkLmas6syCjqstwusPXiCB+Wepg qcmd02JFQeHvHPC2Qj36/aTukgy24uDcj5il3tG2cEPBwT52C0HKQivIXytD+c1XKX/W XWfwufsegFszZ6i6u45f2kDExMW6kKoxo5DwFyO8JWgmU/gxbTdrr+T+sLJNibbx3F2W kBFToTtTrop8PYHx+gGvRYZCG24m9wQYbNNG0eCcO5o2sFyBsAH0Jiz69ZwUjG2ejlYK 3QXKZmUH1W2yYC+qHhav9LihMTnMjxLdZmxMfOT7lumRXL2CXq7V6D3BisaWGfs2t26/ vr8w== X-Gm-Message-State: APjAAAXURxkGAJ9cpjI/SM4q0KV4jf9oTMJK0E9mqmEeEcTatWpsceg3 MV+taztklKqBlfX9FrvT3PWbwjPogV8= X-Google-Smtp-Source: APXvYqxxj+Sms8Weedu8o1JEdrJ2RtOIBP57cNKZkco2fTBn0ZasHp3jS51akdPBwN1Xdb8JUiRlPg== X-Received: by 2002:adf:f003:: with SMTP id j3mr154482wro.250.1559515137919; Sun, 02 Jun 2019 15:38:57 -0700 (PDT) Received: from localhost.localdomain (ipbcc063db.dynamic.kabel-deutschland.de. [188.192.99.219]) by smtp.gmail.com with ESMTPSA id o1sm23913501wre.76.2019.06.02.15.38.57 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Sun, 02 Jun 2019 15:38:57 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Mon, 3 Jun 2019 00:37:29 +0200 Message-Id: <20190602223730.10992-3-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.21.0 In-Reply-To: <20190602223730.10992-1-andreas.rheinhardt@gmail.com> References: <20190602223730.10992-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 3/4] cbs_mpeg2: Fix parsing of picture and slice headers X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 1. The extra information in slice headers was parsed incorrectly: In the first reading pass to derive the length of the extra information, one should look at bits n, n + 9, n + 18, ... and check whether they equal one (further extra information) or zero (end of extra information), but instead bits n, n + 8, n + 16, ... were inspected. The second pass of reading (where the length is already known and the bytes between the length-determining bits are copied into a buffer) did not record what was in bits n, n + 9, n + 18, ..., presuming they equal one. And during writing, the bytes in the buffer are interleaved with set bits and written. This means that if the detected length of the extra information was greater than the real length, the output was corrupted. Fortunately no sample is known that made use of this mechanism: The extra information in slices is still marked as reserved in the specifications. cbs_mpeg2 is now ready in case this changes. 2. Furthermore, the buffer is now padded and slightly different, but very similar code for reading resp. writing has been replaced by code used for both. This was made possible by a new macro, the equivalent to cbs_h2645's fixed(). 3. These changes also made it possible to remove the extra_bit_slice element from the MPEG2RawSliceHeader structure. Said element was always zero except when the detected length of the extra information was less than the real length. 4. The extra information in picture headers (which uses essentially the same syntax as the extra information in slice 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 --- 1. I squashed the two commits because just dropping in a function call seems to be too little for a commit of its own. 2. The __VA_ARGS__ problem you mentioned is not a problem here and it is nothing new: a) In both the new and the old code the SUBSCRIPTS macro gets expanded to "(0 > 0 ? ((int[0 + 1]){ 0, }) : NULL)" in case someone uses no subscripts. So there is nothing new. b) And more importantly, this is completely legal C (89 as well as any later standard), as a trailing comma is allowed in initializers. 3. The 'a' in the macro stands for "advanced". I couldn't come up with a better name: 's' (for string) would clash with subscripts. 4. Given that you merged the version without the range parameters for the xsi macro (which James objected to), I have not added a xsia macro just for the purpose of consistency (because there is no need for it). libavcodec/cbs_mpeg2.c | 43 ++++++++------- libavcodec/cbs_mpeg2.h | 14 ++--- libavcodec/cbs_mpeg2_syntax_template.c | 72 ++++++++++++++------------ 3 files changed, 71 insertions(+), 58 deletions(-) diff --git a/libavcodec/cbs_mpeg2.c b/libavcodec/cbs_mpeg2.c index ede6e806e1..3aa003286e 100644 --- a/libavcodec/cbs_mpeg2.c +++ b/libavcodec/cbs_mpeg2.c @@ -48,17 +48,26 @@ 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__) +#define xui(width, name, var, range_min, range_max, subs, ...) \ + xuia(width, #name, var, range_min, range_max, subs, __VA_ARGS__) #define sis(width, name, subs, ...) \ xsi(width, name, current->name, subs, __VA_ARGS__) +#define marker_bit() \ + bit("marker_bit", 1) +#define bit(string, value) do { \ + av_unused uint32_t bit = value; \ + xuia(1, string, bit, value, value, 0); \ + } while (0) + #define READ #define READWRITE read #define RWContext GetBitContext -#define xui(width, name, var, range_min, range_max, subs, ...) do { \ +#define xuia(width, string, 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, string, \ SUBSCRIPTS(subs, __VA_ARGS__), \ &value, range_min, range_max)); \ var = value; \ @@ -73,11 +82,6 @@ var = value; \ } while (0) -#define marker_bit() do { \ - av_unused uint32_t one; \ - CHECK(ff_cbs_read_unsigned(ctx, rw, 1, "marker_bit", NULL, &one, 1, 1)); \ - } while (0) - #define nextbits(width, compare, var) \ (get_bits_left(rw) >= width && \ (var = show_bits(rw, width)) == (compare)) @@ -91,9 +95,8 @@ #undef READ #undef READWRITE #undef RWContext -#undef xui +#undef xuia #undef xsi -#undef marker_bit #undef nextbits #undef infer @@ -102,8 +105,8 @@ #define READWRITE write #define RWContext PutBitContext -#define xui(width, name, var, range_min, range_max, subs, ...) do { \ - CHECK(ff_cbs_write_unsigned(ctx, rw, width, #name, \ +#define xuia(width, string, var, range_min, range_max, subs, ...) do { \ + CHECK(ff_cbs_write_unsigned(ctx, rw, width, string, \ SUBSCRIPTS(subs, __VA_ARGS__), \ var, range_min, range_max)); \ } while (0) @@ -115,10 +118,6 @@ MAX_INT_BITS(width))); \ } while (0) -#define marker_bit() do { \ - CHECK(ff_cbs_write_unsigned(ctx, rw, 1, "marker_bit", NULL, 1, 1, 1)); \ - } while (0) - #define nextbits(width, compare, var) (var) #define infer(name, value) do { \ @@ -135,13 +134,19 @@ #undef READ #undef READWRITE #undef RWContext -#undef xui +#undef xuia #undef xsi -#undef marker_bit #undef nextbits #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_freep(&content); +} + static void cbs_mpeg2_free_user_data(void *unit, uint8_t *content) { MPEG2RawUserData *user = (MPEG2RawUserData*)content; @@ -152,7 +157,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_freep(&content); } @@ -255,7 +260,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 11f93b9df8..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,11 +200,7 @@ typedef struct MPEG2RawSliceHeader { uint8_t slice_picture_id_enable; uint8_t slice_picture_id; - uint8_t extra_bit_slice; - - 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 d9ef480f39..e7332abe6e 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); + xuia(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,39 +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, 8); - current->extra_information_length = k; - if (k > 0) { - *rw = start; - current->extra_information_ref = - av_buffer_alloc(current->extra_information_length); - if (!current->extra_information_ref) - return AVERROR(ENOMEM); - current->extra_information = current->extra_information_ref->data; - for (k = 0; k < current->extra_information_length; k++) { - xui(1, extra_bit_slice, bit, 1, 1, 0); - xui(8, extra_information_slice[k], - current->extra_information[k], 0, 255, 1, k); - } - } -#else - for (k = 0; k < current->extra_information_length; k++) { - xui(1, extra_bit_slice, 1, 1, 1, 0); - xui(8, extra_information_slice[k], - current->extra_information[k], 0, 255, 1, k); - } -#endif - } } - ui(1, extra_bit_slice); + + CHECK(FUNC(extra_information)(ctx, rw, ¤t->extra_information_slice, + "extra_information_slice[k]", "extra_bit_slice")); return 0; }