From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTP id A6F3647462 for ; Thu, 7 Sep 2023 22:39:23 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id D711068C873; Fri, 8 Sep 2023 01:39:20 +0300 (EEST) Received: from EUR05-DB8-obe.outbound.protection.outlook.com (mail-db8eur05olkn2051.outbound.protection.outlook.com [40.92.89.51]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 51F5C68C81A for ; Fri, 8 Sep 2023 01:39:14 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=EGdz4JPOKpsxayL801Dt/kdwCp1QJ8DeGEbq6pfQUEadoSelzxYQmAjs1ZN/YjEr29h5lUr3YfDIOOUTum0V6EEnjm2oVR0WuTDXQFpJYYTIoj9+KHRDjeZnPIUZtwfYSSCsCRpECEToGJaCTEwkbqDpkNA+WwDEp8PDVWpoITGWH6vEf3P0vqGA2dK0lqeGaHcxwr/YKbp4/YL7dfAbeCkHuFmOn/itMs7znpOlZZqWMnFdcIJUUstEba66tSfeRppze5n7XkRWnXvoc8Xz1icaAeN17Eg8Hr8TYn1FnWFFOTco8YjpnJ6S7HuEzhs9s5UfS4fECQ/kg0y1qBp+pw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=z9ABJV43B08XT53Ee6+uVtzjGkP3veGjlkl7zkraUfo=; b=VgB+C/XtH9wpQ7YGX1dHbOYfeXoZxawfk4ekheDAoiVIy2Y9U/VhklzP5D4V/9Kc/rC0iwyZhMgIi0HhnNu1Jj5EbOEf7aPlXAAfiYbg/zWIyHTKb1F+m2qtE7QYuujUIVc8EEMT4b+7sSIOGJzrn2p6gTwy8EAlGlB30umXdVz+sbLVHF9aOZejRsoGs84NJIvBjReGeKZv7pJKny01QZSJ787I47hb2VB8ndsqYA7JMPRvzu4PItotxtivlHbOWQgC13vA2SekJaIAoAlegNobe2SuCcnbp+WwumdcGexNvMwdz6KOpQA7aN6GP3+oAh7lRmMS1hXyigqQTN9vlg== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=z9ABJV43B08XT53Ee6+uVtzjGkP3veGjlkl7zkraUfo=; b=h29ugDRcpKKBCwjNPWZ2u/najAgRZRGEphzCoWDmOmY2WJLIe1mYIDMiXZVBu/FrDZHNjq7DPIGYvJ0WZFww5LyEtLPd8bq/BqFHpLVzXjH/zriedeu81ljME8Nj61y/+4IQ90mLbtWSPTIxBqGvm4JjoMNsJBNfFXOWbmvVR9W+1ac1jVYkdFyYepzTUnhuSE9SLVeLqVo470IX+J0WepZZyT/cMGh2U3gzHvn+e4Qc/ijkNiNB6n4HDiSnncT3XWDh4lYs70xCrryim4BSyAtKXEh5EdbLnKujFSuhaIE5sxkZP4YemOQ1qQwTzLiKIXnhLq3bHDi+F0lTTi7/vw== Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) by PR3P250MB0225.EURP250.PROD.OUTLOOK.COM (2603:10a6:102:178::13) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6768.30; Thu, 7 Sep 2023 22:39:12 +0000 Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::5e01:aea5:d3a8:cafa]) by AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::5e01:aea5:d3a8:cafa%3]) with mapi id 15.20.6745.034; Thu, 7 Sep 2023 22:39:12 +0000 Message-ID: Date: Fri, 8 Sep 2023 00:40:30 +0200 To: ffmpeg-devel@ffmpeg.org References: <20230907192721.50459-1-christophe.gisquet@gmail.com> <20230907192721.50459-2-christophe.gisquet@gmail.com> Content-Language: en-US From: Andreas Rheinhardt In-Reply-To: <20230907192721.50459-2-christophe.gisquet@gmail.com> X-TMN: [3vRYPO+pFmuOJOxbjvaEty1I/OjWZXWB] X-ClientProxiedBy: FR0P281CA0244.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:af::13) To AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) X-Microsoft-Original-Message-ID: <2489bfdb-0325-5d67-cc6e-83605da24bb1@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: AS8P250MB0744:EE_|PR3P250MB0225:EE_ X-MS-Office365-Filtering-Correlation-Id: 4fec49fe-0578-4529-264c-08dbaff341f3 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: mBK3y9Iq8ZhistG8hXVCc+Wj8bgD6obzuinxWGfAiqhgRAgPAZrm7x9zS8bzO6xQPXg6vLnmX9/bWERy1UwIiRj2luVkrzQNsO1LLm8cuPskNaEePrTvvMVWFgd5l7pZbPK+AuDQIyaXVoXVaaVxhJeBH8qQTPJW4GUdLmPEAECN5TcfsleOIulU58pGXhGytH8CIaIeZh6pk7g8uR8XATWLZ9PsBfD4RihYl6V0GoFVQWSfght7I0EktUPzFGpUwHjjX+YEaPyu98f8t0sVwnKujkBwKMbv0s8ba+TuqwmvPvG9Z6kyyf3I1Lb9B0B59lUOptraHE/1HxmUIEjzYFnQUfkOaQApIJrbkYaW/72fWYQMeBfzEN2TseyP5hw9N3lewCkT18vv7iG/bvBAzhvFxUsRDOI6t83eo1fOgR6dEpaB67ebebLlqJu7q/5zlUDomZ4xBdAta+VEjPHYWh3ciVbbGTKoViVfWuiq6Fh2kNmRRMP5MnNGqfk50HYgaMQAv0pvj69vhJ7O6beTlZ2Wu7c4Liq2wdSUEN0sLE/j4470EmYlk8Av2ZAUzR045YxdZxJNgfmXNDr2Tj2b7A== X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aitadFpMTUxNR2IxUUpqbEQrbmFNYnpaQmJDeEQ5bXZobWdURzNkcTBKL2Yv?= =?utf-8?B?Ujc0Vmg2d0pmOVhKUzhKV2QzdTRxc0dwK2MwTmROd1FBL3BuWkRKNXRUR2NM?= =?utf-8?B?d3h6a2hZMm1SZ1RkajF1RDVtaDlZaTNuVTlMTGNaaTBmUUZVYzRxWmM3RW5C?= =?utf-8?B?RlRJSndSbjdmVHg3UXhkSitCakRCM2s3Q3g4b1EyWjZFaGI1QnlBVnJkcFNN?= =?utf-8?B?cDJ3dzkvQ0VwZUVNSVdXRUdlSURsVmlmaFdSeTJtTnZQcHpGNGtXd3dvUDZ6?= =?utf-8?B?VWZXckIxNHBPVWZxZTlyZldrSDdvSzlvYjZWcDVOckpMNG9HN3BYUlZPZEEx?= =?utf-8?B?dDlhbW9WR3hZaHN5bG1SSnBJZnRzTTZoUWJsRHAxcU1BR3pzOHk0LzFBTk1J?= =?utf-8?B?a1hHMDdEc0o1QTVFTGdid3FvY0t5UmQyRGlCRjMvZTkwNitVcDVaTTM1SE9D?= =?utf-8?B?ek9xeVdxY3lHSTRvYjBMZGJ0MCtHUzh1NVJTSDV6bkp3dHVzUC9xUzZGZWxw?= =?utf-8?B?ZjVTT09kUkJ0UmRIbXA0VEZPMlJQK0l3MXpHeVpCbTIxQTJJVWZQb1Ezc1FY?= =?utf-8?B?NkNSS2pkQkpZVGRoYzBpRkNsRlltekhSTkhDeG8yNVNHdUY5ajVJV2VUNWs4?= =?utf-8?B?b1JONDVOWnpKMWRxZEFQM3o0eGFtWWVyMEpKWlhDM2FmOHVrbUd5WTZhNGN0?= =?utf-8?B?UmdkeE01anZ2S2MyQnFJeXFyeGZ1TTR1ZGFoY1d2Z2UvRlNTL2UyZjJXcytl?= =?utf-8?B?TjBlbjg3SVFuQzJhRVA2OUl6UGp4dFowOUNPYiszVVEzYjVYVWNkMndJanJi?= =?utf-8?B?cGpMN25IcjFHQURyL1Z6U2VHOW1rL1VaZTh3TFJEQTdZTzk5d3NoRDY0K21W?= =?utf-8?B?VENTMytyWEJuczJRaTJYNFpVNWdNWG4xNHA0NmlEMG5rTExuOFZpQVlqNmJG?= =?utf-8?B?Z0dWekgyUndXWlpPSjNsUjVHT2dUeHpJK0tZNXh4MVFiSE8zWVJsUFUzaURN?= =?utf-8?B?endvUDdVWml4Q09HU2NDcUNwcXVwK2VWTXowTHRBNFVGS1NreTJVZG1ESnV0?= =?utf-8?B?ZmtIUUlScGd3elFMeEJQeUM1ajV4Uk9udmo3eWpNVm51TFh6UWxJak1PR09X?= =?utf-8?B?TllCb0hTVitieC9FVXpqRk9FTjNYb3lPK1NCSFFWZ0VMLzNncnRuRmNKMWta?= =?utf-8?B?eEFGKzQrOWplUE0rYnVjWTFJclRRbGZ0cllqR09GcnI1UFMvc1dXQ0RqWnUy?= =?utf-8?B?WGVWZUlUWkJVeWsxQzZsRVpOajBzd1lSTkJ4dm1zeXFEbEdlM2t2MnE4TjJF?= =?utf-8?B?a0JtTGJNaG5wT2tJYm1ueHFOTDQyb0dscWIvU0xwdGcrZzVIb1p5Z29YWWJI?= =?utf-8?B?N2srT0NrK1c3bWhuVFlEY1lCbG1Va2FQaVhjK1BBaXEvM3FYeFNmRGZwWllT?= =?utf-8?B?RDJSaHV5WlVNaG1IY1NMT2lFanZTVVRBTkRTY3NPWGoybFZKWS9adzVsdVFh?= =?utf-8?B?UUpmb2hjTW8zUDNSYTlwSUlObFhTdnVNKytjTEFRemZCaWRucktKTms1YVN3?= =?utf-8?B?cXVHU01wQjRSOG4wcHBPM0dTb1JIS1RUMGRzM3YzNlJxbTJKMmhYY04yZVN1?= =?utf-8?B?T0NHeWZ4cVMwMTR5WitZR0NGbHVtZUtYSmVFdFBWQ00vMjhyY0Vnd2RrUWhS?= =?utf-8?Q?Ph+xLSikk5djjRGwFtr4?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 4fec49fe-0578-4529-264c-08dbaff341f3 X-MS-Exchange-CrossTenant-AuthSource: AS8P250MB0744.EURP250.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Sep 2023 22:39:12.3752 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: PR3P250MB0225 Subject: Re: [FFmpeg-devel] [PATCH 1/2] Expose and start using skip_remaining X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.29 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 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" Archived-At: List-Archive: List-Post: Christophe Gisquet: > Bitstream readers sometimes have already checked there are enough > bits, and the check is redundant. This patch aims to do two things; and these should be in separate patches so that one can see immediately where you just change the name and where you change the actual code. > --- > libavcodec/bitstream.h | 8 +++++--- > libavcodec/bitstream_template.h | 22 +++++++++++----------- > libavcodec/get_bits.h | 1 + > 3 files changed, 17 insertions(+), 14 deletions(-) > > diff --git a/libavcodec/bitstream.h b/libavcodec/bitstream.h > index 35b7873b9c..dd043fb349 100644 > --- a/libavcodec/bitstream.h > +++ b/libavcodec/bitstream.h > @@ -95,6 +95,7 @@ > # define bits_peek_signed bits_peek_signed_le > # define bits_peek_signed_nz bits_peek_signed_nz_le > # define bits_skip bits_skip_le > +# define bits_skip_remaining bits_skip_remaining_le > # define bits_seek bits_seek_le > # define bits_align bits_align_le > # define bits_read_xbits bits_read_xbits_le > @@ -124,6 +125,7 @@ > # define bits_peek_signed bits_peek_signed_be > # define bits_peek_signed_nz bits_peek_signed_nz_be > # define bits_skip bits_skip_be > +# define bits_skip_remaining bits_skip_remaining_be > # define bits_seek bits_seek_be > # define bits_align bits_align_be > # define bits_read_xbits bits_read_xbits_be > @@ -146,7 +148,7 @@ > n = table[index].len; \ > \ > if (max_depth > 1 && n < 0) { \ > - bits_skip(bc, bits); \ > + skip_remaining(bc, bits); \ This is problematic, because you seem to think that bits_peek(bc, bits) ensures that there are at least `bits` available in the cache; yet this is not so, because it can happen that one reached the end of input in which case no refilling happens. See https://github.com/mkver/FFmpeg/commit/fba57506a9cf6be2f4aa5eeee7b10d54729fd92a for a way that fixes this. Now that I have written this, I have to admit that the current code here is also very problematic: bits_skip() is also suffering from the fallacy that priv_refill_64() always works. Even worse, the code simply increments the buffer ptr without checking the bounds (the whole branch for n >= 64 is of course nonsense for BITS_RL_VLC. In fact, I think that we will end up with the exact same state in case the reloading in bits_peek() failed with bits_skip() and skip_remaining(). Luckily BITS_RL_VLC is absolutely unused. Needless to say, a proper fix involves something along the lines of my patch above. But this patch is based around the assumption that the combined amount of bits consumed in any get_vlc2/GET_RL_VLC/BITS_RL_VLC call can't exceed 32. Is this assumption actually still true now that we have multi-vlc stuff? https://github.com/mkver/FFmpeg/commit/9b5a977957968c0718dea55a5b15f060ef6201dc and https://github.com/mkver/FFmpeg/commits/aligned32_le_bitstream_reader are probably also of interest to you. > \ > nb_bits = -n; \ > \ > @@ -154,7 +156,7 @@ > level = table[index].level; \ > n = table[index].len; \ > if (max_depth > 2 && n < 0) { \ > - bits_skip(bc, nb_bits); \ > + skip_remaining(bc, nb_bits); \ > nb_bits = -n; \ > \ > index = bits_peek(bc, nb_bits) + level; \ > @@ -163,7 +165,7 @@ > } \ > } \ > run = table[index].run; \ > - bits_skip(bc, n); \ > + skip_remaining(bc, n); \ > } while (0) > > #endif /* AVCODEC_BITSTREAM_H */ > diff --git a/libavcodec/bitstream_template.h b/libavcodec/bitstream_template.h > index 0308e3a924..3f90fc6a07 100644 > --- a/libavcodec/bitstream_template.h > +++ b/libavcodec/bitstream_template.h > @@ -175,7 +175,7 @@ static inline uint64_t BS_FUNC(priv_val_show)(BSCTX *bc, unsigned int n) > #endif > } > > -static inline void BS_FUNC(priv_skip_remaining)(BSCTX *bc, unsigned int n) > +static inline void BS_FUNC(skip_remaining)(BSCTX *bc, unsigned int n) > { > #ifdef BITSTREAM_TEMPLATE_LE > bc->bits >>= n; > @@ -192,7 +192,7 @@ static inline uint64_t BS_FUNC(priv_val_get)(BSCTX *bc, unsigned int n) > av_assert2(n > 0 && n < 64); > > ret = BS_FUNC(priv_val_show)(bc, n); > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > > return ret; > } > @@ -375,7 +375,7 @@ static inline int BS_FUNC(peek_signed)(BSCTX *bc, unsigned int n) > static inline void BS_FUNC(skip)(BSCTX *bc, unsigned int n) > { > if (n < bc->bits_valid) > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > else { > n -= bc->bits_valid; > bc->bits = 0; > @@ -389,7 +389,7 @@ static inline void BS_FUNC(skip)(BSCTX *bc, unsigned int n) > } > BS_FUNC(priv_refill_64)(bc); > if (n) > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > } > } > > @@ -425,7 +425,7 @@ static inline int BS_FUNC(read_xbits)(BSCTX *bc, unsigned int n) > { > int32_t cache = BS_FUNC(peek)(bc, 32); > int sign = ~cache >> 31; > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > > return ((((uint32_t)(sign ^ cache)) >> (32 - n)) ^ sign) - sign; > } > @@ -508,14 +508,14 @@ static inline int BS_FUNC(read_vlc)(BSCTX *bc, const VLCElem *table, > int n = table[idx].len; > > if (max_depth > 1 && n < 0) { > - BS_FUNC(priv_skip_remaining)(bc, bits); > + BS_FUNC(skip_remaining)(bc, bits); > code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table); > if (max_depth > 2 && n < 0) { > - BS_FUNC(priv_skip_remaining)(bc, nb_bits); > + BS_FUNC(skip_remaining)(bc, nb_bits); > code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table); > } > } > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > > return code; > } > @@ -534,17 +534,17 @@ static inline int BS_FUNC(read_vlc_multi)(BSCTX *bc, uint8_t *dst, > code = table[idx].sym; > n = table[idx].len; > if (max_depth > 1 && n < 0) { > - BS_FUNC(priv_skip_remaining)(bc, bits); > + BS_FUNC(skip_remaining)(bc, bits); > code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table); > if (max_depth > 2 && n < 0) { > - BS_FUNC(priv_skip_remaining)(bc, nb_bits); > + BS_FUNC(skip_remaining)(bc, nb_bits); > code = BS_FUNC(priv_set_idx)(bc, code, &n, &nb_bits, table); > } > } > AV_WN16(dst, code); > ret = n > 0; > } > - BS_FUNC(priv_skip_remaining)(bc, n); > + BS_FUNC(skip_remaining)(bc, n); > > return ret; > } > diff --git a/libavcodec/get_bits.h b/libavcodec/get_bits.h > index 0594e104bb..2e24632cb5 100644 > --- a/libavcodec/get_bits.h > +++ b/libavcodec/get_bits.h > @@ -79,6 +79,7 @@ typedef BitstreamContext GetBitContext; > #define get_bits_left bits_left > #define skip_bits_long bits_skip > #define skip_bits bits_skip > +#define skip_remaining bits_skip_remaining > #define get_bits bits_read_nz > #define get_bitsz bits_read > #define get_bits_long bits_read _______________________________________________ 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".