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 ESMTPS id 2968C4CFC2 for ; Fri, 14 Feb 2025 05:04:25 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id CB12768C154; Fri, 14 Feb 2025 07:04:20 +0200 (EET) Received: from EUR05-VI1-obe.outbound.protection.outlook.com (mail-vi1eur05olkn2042.outbound.protection.outlook.com [40.92.90.42]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6836068BE4A for ; Fri, 14 Feb 2025 07:04:14 +0200 (EET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=p0tKNYT24ke8EGI2CpHkAjH5njMB4ohPZHjsqpmnfZZzwPiC3ZblgRzKT/XW1m+/X2nE6rxKUro7k4hpnArmE+se4GZ1VDt5Ria7UdOPpxSY5Zee/7Jlu+lkpasA4Hm4csnWQv9Hqjj4FLU+MwuJPXn/JHRP4HqukX+1lAf/UkEOiSxMVDUKMw0TaX+aW7iQA2Zje6ytpLv85rejEHpTXTt9z66AWsM7RTnAKGn5Out+zT0xnQHO4Fog8TyXGIt37HTqyloy0T+/H51V+zsCKx1CrJXVuqisArpewHD6JhrTt1oc8UOo7+mFuEFEX9KSSfesiKv4poMg94wHF1z0oQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; 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=mN0DrOJIHEaf/yeTzI5wzuIZ3GBjEde4HETnaMwIMs8=; b=rdv6XPeWL6FVh3LsYWIXd5SZkIhm4d1YLzQsaVQFVg+fb9lYBiBwwduDusUCO4g9SpyBMKpKM4bXXSPcK1VEGtkAiWQ0ogSLlbOwxLJW80wHz1LETPFPzmuOM36YLRbdRDmqwKKOjFo7yA23cYtTeTHixfZfG/F8DesoWKGK/HT7xWdWvwd2t4wT7fSrXZMMISa6Nwys0alRHNqKt39muLxxE+eL7bmBrN9pRlWIKHvma8tI23wfs/FE3V+vex9mEoFvPao39vdztIm88U8oTwp7rlRxHwA+S7izuPZ7ISu7HtMGkdqg8bov4SfrQx8cQ+ZomGUe2jWY0tUOG/+C2Q== 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=mN0DrOJIHEaf/yeTzI5wzuIZ3GBjEde4HETnaMwIMs8=; b=FO6o+2s5kF5amQivOeimJM7eg+G3XLSrb5YsbBq7+5lDFYM+o2hu4Dsl2WdyQHQlv2mqCeqBH3txdqGAjJZxDz6CVzX+3O5w5/vXVIxx+Cs9bnHRMHxEI/XMw3xr5+v5PF4c+eX8cJE/dJAeqXIzdekulX1sIq4GlWXsGp5SU0h8PmmZTq/96Hu01GRmPA6DP5CDezgIWzazTQW3mR8wdj0i0/hWpzIvcyBtCSNKU/35Boqa3a8YNL6DLP6+c72wb5BUCVyc38ygYybX6rdUjc3KA0bhbV2odKBSN7DQ8sPHzYDNaKdjnEVHXYoe2kctr8Hiov0SxuoqDRPQ24GITw== Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) by PR3P250MB0370.EURP250.PROD.OUTLOOK.COM (2603:10a6:102:17d::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8445.17; Fri, 14 Feb 2025 05:04:12 +0000 Received: from AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::384d:40d4:ecb7:1c9]) by AS8P250MB0744.EURP250.PROD.OUTLOOK.COM ([fe80::384d:40d4:ecb7:1c9%5]) with mapi id 15.20.8445.013; Fri, 14 Feb 2025 05:04:12 +0000 Message-ID: Date: Fri, 14 Feb 2025 06:04:11 +0100 User-Agent: Mozilla Thunderbird To: ffmpeg-devel@ffmpeg.org References: <20250213212208.29414-1-pkoshevoy@gmail.com> Content-Language: en-US From: Andreas Rheinhardt In-Reply-To: <20250213212208.29414-1-pkoshevoy@gmail.com> X-ClientProxiedBy: FR0P281CA0120.DEUP281.PROD.OUTLOOK.COM (2603:10a6:d10:a8::10) To AS8P250MB0744.EURP250.PROD.OUTLOOK.COM (2603:10a6:20b:541::14) X-Microsoft-Original-Message-ID: <2a973b68-62cf-4f35-a9c4-a144bb4f0bbe@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: AS8P250MB0744:EE_|PR3P250MB0370:EE_ X-MS-Office365-Filtering-Correlation-Id: 7e0cc438-cc92-4e60-67ca-08dd4cb5058d X-Microsoft-Antispam: BCL:0; ARA:14566002|6090799003|12121999004|5072599009|8060799006|19110799003|15080799006|461199028|7092599003|3412199025|440099028|10035399004|41001999003; X-Microsoft-Antispam-Message-Info: =?utf-8?B?bzdFQnNZTDNsNDJwNHJRUU5ialBvWHBpUVExZU1TUHZrL04vWFhFY09ZSllj?= =?utf-8?B?aDgzZW9DTUp0TzFxbWVLWjZ1SEpMZWVQdXl2OEx3b2F6cFZrQ0p5cDZPMGQr?= =?utf-8?B?dlFNdnBsLzFJVFczMzNJVnZJeU51VHM0ejlpS1VkRDZUVkxhaVhHZW00QUZJ?= =?utf-8?B?TU1SVVpxZS9ZRXlhbyt2YnpXdGROOGx3b0dOK0RneDdHQXI2NVd1YTRObG1o?= =?utf-8?B?bVo5dHJRVEVDVUowNFhMSStheDdMdGxmQXQxb2ZBTUZ4WHhmZTgzS2dQU1Fq?= =?utf-8?B?bnF1TkFJOVl4dTYvSzB1ZStJTmxVN0l5d25xRjZEWGZZZmdxc1BlQkZKREJC?= =?utf-8?B?L0tMNXhzbnRRZ1QwUzdMZ3hLbVMvd1dKR1RWSHFScnZrRVQ2Uno4QjlraU8r?= =?utf-8?B?N05HekpHWTYzZXVGdEVDeG9LdkFLVzBTTjVueUlmVzFxT2JqUHA1bG1xSGRz?= =?utf-8?B?Wm1tMDFJem0rUWRtNHdIMVNPS0xXTG4vU1BNRThmc0ZxR296Y2VKMjFGTmMz?= =?utf-8?B?UTBsREhmM1krL0pxSW1WT1VwWUFGbWp4aTlSWmlLS3FTOHRTRU9NeU9laml0?= =?utf-8?B?a1JxYUpoUzI1TWY0NFhyU0JUQUIzNGZieVplU2FOMCthK0NqeW5FeGlDYlhY?= =?utf-8?B?eGVMb1l6STFDUW9tMGtnc2hNTUpQOUt4RHp0MElzczBxdk1icVNjenY4TjJ2?= =?utf-8?B?dDhpQ3lkeWcySjk4ZE0yckFEeGx4VDFqZmNnbVdXemQ2L1M2TjcrSExXaUZD?= =?utf-8?B?WlE4OGxscDNyT2tFd3AwYzVlNXhTMkszMTM0cjVib2RWVHJKSzgwcmhsWGZs?= =?utf-8?B?OEZmSWRpaHl4eWNxc0pmcVd2c3dDN0RiVjNpMlczL0tqdDhXR1NKYUF0Nnk1?= =?utf-8?B?TDU1WXhYd3poRTVnQXZ2T3J4SXUzSHV1WUpTdkVEZFdmWVBaeit1MkNSNGtD?= =?utf-8?B?U3NqdzRrU0tQcVhyUURuQmNKT3JhRWVvSUppOXpIOFZuUGNaR25QSDBXYThz?= =?utf-8?B?R013WTVYY0k2NC9LRE1BQ0RmNGhvOTF6dGtBQ1pJWlhocXNXU1ZUZFYwVkZk?= =?utf-8?B?Zi9kaklPdHdsQzRjUFhtditYQ2UvRVExbno2SU0yUlRmdk0rbElHSll2VmlL?= =?utf-8?B?TkdCUkJpL3hRZ3ZmcUNXTThOdGJoOGZaY0pWUkdNejl3c3Z0Q3Z4QjR6Y3NG?= =?utf-8?B?bTVnMWFpdy9LcWNGVDVxa2JxUEFqNUIxUXhnMEMrcTdHNEFzZkZ6cVJlcFBY?= =?utf-8?B?NXJiTkJRRERpbnhCNndSMlNud0VkVDNPZ0hDUEs1dzdISUMrdTBQdGROSEtM?= =?utf-8?B?czdzdVdJVnpIYmxBWVF6WEp4ckFKbVBPU2YwUkVUMXNnYXRXK2tLRWxURGJm?= =?utf-8?B?N1N2ZzYwR3hOTi9Xano0YTVYN0huTW9Ed240eVdrQjRiT0Zmajg4SENWTGVr?= =?utf-8?B?bUFQNkZlYjA5dnRZOGJINmRSQ2o2Z0p1L01hUzJRcUN1U2ZuZThvWnJrOEFR?= =?utf-8?B?MTd4dEZDbmRMQ2V2UzI2UGFBbmF0NzNERHJCS2M4ZXhBbkdDVlFEc0o4anUv?= =?utf-8?B?U2dsQ0xBSmtuWDF6aU41K1pPR0NScDg5OGwxb2lFb2VDQW11UDEvZUczdVlF?= =?utf-8?B?bk0vWEVLQnVRSGQrQ1ErT3RNclovY2c9PQ==?= X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?azBaU0tINkZVcWptcmFINXVxSVBTbHNjZnp5eFhZRXQyQXJVUEh6MGU5eWdY?= =?utf-8?B?WmRHN3l5WEMxMkwwUUNWNEQrWUhnMUxyeEVibGc3aFhzcXNKQWlPRUJvMGtO?= =?utf-8?B?T1hCYU1aQkpQMkJxZVpFNUtJcEdTMEh5ZERRUkhyTE02cERMVGxKU2VUdFNR?= =?utf-8?B?K2RxMHd1cG8zdzBvWStlWi9DRXV2c2lKZFJsMUV4ekcwZWJlbFVHcEpKVFVl?= =?utf-8?B?eHQyNWhEWngyaFFpTVZVbWNhRFdxZG9FU3orQlg0QWNVSGszLzNTMkF4RFM2?= =?utf-8?B?Nm92ZGR1VjAzQUVJUmNTcDVwNDBjMithdjRiYzN1WERRZFM4Q05WQkp1NVE2?= =?utf-8?B?aHlEUnRXaE1Qd0RPNlhFUEdxK1psVVAvSXozRFp4cGZFZjFXMXliMDZFNEZi?= =?utf-8?B?VEJ5RDIydHBWaUtkVmtzcWNOS0F4VVJvaXRCWE5lR042YkdBdTZNa2I2VjBZ?= =?utf-8?B?MWFHV2xUVDNxbEMxVU90emNxT3hhejVrcEp6dGlpVDRlbHlwTnJHd0Fzdlgv?= =?utf-8?B?U3VoVnp6RklONU5uelo2NURqZGM2TXZJbEhHT3Y4ZnBKVUt6OG9jK3I4bnhU?= =?utf-8?B?R2JneWxya0hDdmVFeFVBcDloelorcGVhNU4zV1RjZ3V2ZnJFbWVGbUVXRnVO?= =?utf-8?B?RitYYS9yTjVUVU1DbDhtNUw4bkxrYTRFcFhKUE0zMHFJaVh4VUROZTVSTjBh?= =?utf-8?B?aGhHOUxpakxhV2RGSi93N1VzUlRnM3VqbzBwT1AxNHhYVFpTRXpDOURFT1RH?= =?utf-8?B?MktLYklYdllwWnRTQ2hocGFLQUlVLzJIMmFvL1RIakdLbFNqK2FlZGVPT0RD?= =?utf-8?B?bVNvT3FRZ1pJV0lRZlVGR2VuQVdYSGFHc0dqZzNVY3J6OXZPb21XV2dwZGl5?= =?utf-8?B?dkd1Ymd2UEVzQy9EQWtQNGlaVFNtMXdaVENvY0labU1rNG1tdVJuS0EvTVNN?= =?utf-8?B?NmtENmtMeHhQOXVrZWdvczBkMmQ2NU9KYWJFWlA5MTFUMVhOdzZxdzNGNTQ0?= =?utf-8?B?Szd5L3N6eVp3VGRYQlZ3Kzl0RFI4TnJZdmI0QmkxSEVMZkt1TEVHNXZOdXNI?= =?utf-8?B?akkzZTV4OHBNVTRhSzFHNCtYVWJLMmVnZTVvUm1JZENwcXVXSDRoT1R1Ly92?= =?utf-8?B?elBXSEN0ZHRXSU05OEk5WHp1TzgrRXNvN1RiM0JFZnRVaHF0N0pGQjFlV0RS?= =?utf-8?B?b25DWWFRZkxDVGZLbHZ2ZldiZU4wSGIrazRqVVNEWXlNenorRTNkVzFETHhF?= =?utf-8?B?bUtWc284OTdrbU1ZQThkUDlpUFZiMnpzaFNJdWRnSVFHN29yTyt0NnROOXVi?= =?utf-8?B?b09sMm5wL0c0MWI0UXNJWWNIYUlWNVRwNFBaTFdDNEV5OTVlS0N4NEU3cmNH?= =?utf-8?B?Q0lTTXhzR2VKaHQvQmRiYVVqYi93N2RzbTFzK1ZxTjRBYi9OZS9GUkNwTDI0?= =?utf-8?B?RkhjVDhKV3pHRzRSbDRSOHBjWkVneGxIVzkwZW10SVhaTm0yL0xSakFJLzBj?= =?utf-8?B?NU9lYXJYUytYRHNsc20rY1FoREFoY3J4VWVEZ3Nlam1oSzBONy8rUHNRV1VZ?= =?utf-8?B?UnRhaVF5ZWpOcU9DeWZzb25TS0RqWTU4RG5QMVRSWkJxQzhSZm5qYVBtOTVk?= =?utf-8?B?M20rbDYyVFgvQ2JkSldXNDBiMGtEKzhaUTcxL1NoM25PWmhzN2hYbEJ5TFNy?= =?utf-8?B?VTdGYVh2VTdiSXhKS2tCTWVheTJyNUUwMFBIa0NPRE82NkgyUlpBWkF3PT0=?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 7e0cc438-cc92-4e60-67ca-08dd4cb5058d X-MS-Exchange-CrossTenant-AuthSource: AS8P250MB0744.EURP250.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Feb 2025 05:04:12.6031 (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: PR3P250MB0370 Subject: Re: [FFmpeg-devel] [PATCH] avformat/mov: (v4) fix get_eia608_packet 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: Pavel Koshevoy: > The problem is reproducible with "Test for Quicktime 608 CC file.mov" > from https://samples.ffmpeg.org/MPEG2/subcc/ > > ffmpeg -i "Test for Quicktime 608 CC file.mov" -map 0 -c copy -y remuxed.mov > > Prior to the fix QuickTime Player playback of remuxed.mov would > render garbage text for "English CC" subtitles. Is remuxing necessary for there being garbage? > --- > libavformat/mov.c | 70 +++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 59 insertions(+), 11 deletions(-) > > diff --git a/libavformat/mov.c b/libavformat/mov.c > index 85aef33b19..5a91ef5b8c 100644 > --- a/libavformat/mov.c > +++ b/libavformat/mov.c > @@ -10788,25 +10788,73 @@ static int mov_change_extradata(AVStream *st, AVPacket *pkt) > return 0; > } > > -static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int size) > +static int get_eia608_packet(AVIOContext *pb, AVPacket *pkt, int src_size) > { > - int new_size, ret; > + /* We can't make assumptions about the structure of the payload, > + because it may include multiple cdat and cdt2 samples. */ > + const uint32_t cdat = AV_RB32("cdat"); > + const uint32_t cdt2 = AV_RB32("cdt2"); I don't think that using (non-variable) variables for these improves clarity (e.g. it means that the definition of the actual values used for the comparisons below is now further away from its use). Why not simply use MKBETAG('c','d','a','t') below? > + int ret, out_size = 0; > > - if (size <= 8) > + /* a valid payload must have size, 4cc, and at least 1 byte pair: */ > + if (src_size < 10) > return AVERROR_INVALIDDATA; > - new_size = ((size - 8) / 2) * 3; > - ret = av_new_packet(pkt, new_size); > + > + /* avoid an int overflow: */ > + if ((src_size - 8) / 2 >= INT_MAX / 3) > + return AVERROR_INVALIDDATA; > + > + ret = av_new_packet(pkt, ((src_size - 8) / 2) * 3); > if (ret < 0) > return ret; > > - avio_skip(pb, 8); > - for (int j = 0; j < new_size; j += 3) { > - pkt->data[j] = 0xFC; > - pkt->data[j+1] = avio_r8(pb); > - pkt->data[j+2] = avio_r8(pb); > + /* parse and re-format the c608 payload in one pass. */ > + while (src_size >= 10) { > + const uint32_t atom_size = avio_rb32(pb); > + const uint32_t atom_type = avio_rb32(pb); > + const uint32_t data_size = atom_size - 8; This may wrap around (if atom_size is < 8). If int is 32 bits, then the data_size > src_size check will catch this, but in case of 64 bit ints it may not. Relying on (unsigned, defined) integer wraparound should be avoided unless it is advantageous to use it; in this case, this is just not true: Just compare atom_size to 10 below. > + const uint8_t cc_field = > + atom_type == cdat ? 1 : > + atom_type == cdt2 ? 2 : > + 0; > + > + /* account for bytes consumed for atom size and type. */ > + src_size -= 8; > + > + /* make sure the data size stays within the buffer boundaries. */ > + if (data_size < 2 || data_size > src_size) { > + ret = AVERROR_INVALIDDATA; > + break; > + } > + > + /* make sure the data size is consistent with N byte pairs. */ > + if (data_size % 2 != 0) { We typically try to avoid redundant "!= 0". > + ret = AVERROR_INVALIDDATA; > + break; > + } > + > + if (!cc_field) { > + /* neither cdat or cdt2 ... skip it */ > + avio_skip(pb, data_size); > + src_size -= data_size; > + continue; > + } > + > + for (int32_t i = 0; i < data_size; i += 2) { int32_t? Why signed? (And why use a separate loop counter at all? Simply decrement data_size by 2 in each iteration. > + pkt->data[out_size] = (0x1F << 3) | (1 << 2) | (cc_field - 1); > + pkt->data[out_size + 1] = avio_r8(pb); > + pkt->data[out_size + 2] = avio_r8(pb); > + out_size += 3; > + src_size -= 2; > + } > } > > - return 0; > + if (src_size > 0) > + /* skip any remaining unread portion of the input payload */ > + avio_skip(pb, src_size); > + > + av_shrink_packet(pkt, out_size); > + return ret; > } > > static int mov_finalize_packet(AVFormatContext *s, AVStream *st, AVIndexEntry *sample, Generally, I believe that reading the input into pkt->data[size / 2] would be advantageous: It would make it simple to check for EOF and I/O errors (notice that the avio_r* reads above are unchecked) and would read the data in one go, avoiding all the avio_skip(). - Andreas _______________________________________________ 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".