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 F000843EAD for ; Tue, 16 Aug 2022 20:39:07 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 059DC68B8E7; Tue, 16 Aug 2022 23:39:05 +0300 (EEST) Received: from EUR04-HE1-obe.outbound.protection.outlook.com (mail-oln040092073094.outbound.protection.outlook.com [40.92.73.94]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 22AB868B72B for ; Tue, 16 Aug 2022 23:38:58 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q83sGhTFIIL2Mgggm9fWYwzxVGHqFBEqzX+xXJBYj7WSwHTx3FIP8c3WuMg3s3Qw//FuV2Oi3kRO9FR2pD5i9N4hreaQOvrhgOBMy/F/qpzrkicC2Li0CpYKDkOYEMVoLMBJDqvRAmKwrO2qZHp5JJgsOQtyaLldb7t3BqME/89221nmRqEdXpeX768S8d95XbnqPbp+2vNTzp9TiSNZOzSsYxOnDasPY05KC9tEEcjTpA5yoxyeuHtgVeviAS01pNtVDmpejzX7JemajG74/YBEhnrXyEGIF3PfxDvYfINFXdBAIqTEQra+FcZLAXzk70tUX/8xxJUzprWtREBg3A== 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=owAebSXO8noGKa2xzuviXZ2uYP9Zn9+BZ7NQVeHeu4s=; b=VuJZiH9qEjhHXaN45k3Fp3X2zrP1M9xrDDE+yNbCFySvfb6bY20x7tUwlOl2VIoE5TdepZrlTXln65BB60wdc1Xo8Db110hcDqqPw828YsGE8Bsbo3Uez+PS0OIUgS15/PmMRV7oJEL2NiapfoxkDc+BtkQ4NoJAXlrOitx/W7r//mCFuahC/sUfBAdnptKp6kBFZjbGUCr7C0U2uzYTzjiW3F7G4O46CL/0AQYQOg55ugGaxsOx6X80vxd8tlhsInJyUt/9M7TL3ontmfXiCJ7/hHWRJycPQZDN0jFMBaRQw4vkwZCTMCFMfitD5O5fUH6buV0cSPW4M74O6ksD6A== 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=owAebSXO8noGKa2xzuviXZ2uYP9Zn9+BZ7NQVeHeu4s=; b=ZCacmj30T3o7LCfQCM9votsCvy+k1jdinsSAWQjbtsMmfCGZnXVV/Bmn1zVZRcHQNNaVEhk61keOlBtJqPK/EookNvtySm0+qssn1Xlzne1D/Xhg8vG1GPINu3bQ9JcfpCkepJeIVDkbDRG/EplqteXOo1ut7LC5gXcHckBcER1Zp1+nkPsGki91I2sqguOKGBV4RgcwA4f+hfV33AU7xniiitUwOnhxzfgPzyowHLnFphbxBIIcrP10sztDQnMA0J/N/t4aqK1R8eqZBD5YebK2hVvRB8a4DzlG87vjRNd8WC2x8R7nJm+QGpJeeBy85SEcbbCW10zIQFMCiY0iWg== Received: from DB6PR0101MB2214.eurprd01.prod.exchangelabs.com (2603:10a6:4:42::27) by AM0PR01MB3715.eurprd01.prod.exchangelabs.com (2603:10a6:208:b7::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5504.24; Tue, 16 Aug 2022 20:38:56 +0000 Received: from DB6PR0101MB2214.eurprd01.prod.exchangelabs.com ([fe80::210e:b627:bcc9:8c46]) by DB6PR0101MB2214.eurprd01.prod.exchangelabs.com ([fe80::210e:b627:bcc9:8c46%11]) with mapi id 15.20.5504.028; Tue, 16 Aug 2022 20:38:56 +0000 Message-ID: Date: Tue, 16 Aug 2022 22:38:55 +0200 Content-Language: en-US To: ffmpeg-devel@ffmpeg.org References: <20220816194837.GA2088045@pb2> From: Andreas Rheinhardt In-Reply-To: <20220816194837.GA2088045@pb2> X-TMN: [4Yrqdd+K4c+lTwHIVarYEHTGW1eQKAfW] X-ClientProxiedBy: AS9PR06CA0653.eurprd06.prod.outlook.com (2603:10a6:20b:46f::33) To DB6PR0101MB2214.eurprd01.prod.exchangelabs.com (2603:10a6:4:42::27) X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 1ca23661-b7a4-49cf-0f06-08da7fc756bf X-MS-TrafficTypeDiagnostic: AM0PR01MB3715:EE_ X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: Mgdyi1TaxkfEUZPDWDVwbtCK5l47pCzlALxsQV7sGBDHmKeBDM/IJJqE81DUfrHN91uB10TrH6FNxpdTywHEnvLY7zOejWDv33ujz3ycu+vgMMR36Mpth6da0A27bchMa/aFpskSppRtXyWMsw7P3rxwwhEhIqjyPMsxa12YMcjHoz4g/NjmJJ2Q3yh5OCDrid4QPER+udO2//BtMmsTKH1QPKbWrjkNU6azkkDQV+Tg8qQsev6MI3RzKRsABox9aJIMX28ccnRMY8shsHfpmIMI2pqlrM4WZ933h1+ihvQl032VvhGkQ6ed/XqA0l2jj+SICwa9VQGfFcyXqCWO/0DpIFdSSYdDp3cObKe13gNtt20rP040G5VHd5EKr7NAhLOr4WQ4/krgpH6Dbsuy7BadbNgePir9CMW731Z50LDwxWy0Zy8wCbtPqRgl1/VWHCgN6KGQUzjSHGWTKr7uSKI0egwRf1F19b11b21de35bawamzJJufNBHaRUgZyodDzD5CUakNYC4jbaYP3BZBoDFKJBk79gz4gEqeJZ8wZj8ffsL+k7uBCHaAM9N+z+RBi75mLJW1gvaeoQSqQ4DlJUguqFm282uxoW5JCSQgDe9QDHv9SGzNG6fA3DRHrBggl2V2XQkzbRFZvZCFo2c5MXHOhrUlQCFpWdgCqUkrfGCgJVr01vTMp1OpO568CPs X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?SzlqSVZOWjFMZS83SEJQczdJd2k5bXpZbm0vQmlRVWhvN0tESHdQanFDMEtw?= =?utf-8?B?MlpWL1BMTFN0aUIyUHpneW5iRjlYZVVGdXRxMS9jTU1WTDhwQWxjYk9hUGlk?= =?utf-8?B?TVBDSy9PR1NqVWl6eEtHcm4vOTdZSEtTM2RXNmxsNkUyY1lVcjFqYk81djBw?= =?utf-8?B?OVNNaDc2blRWYksvc2czUlhueFp1UFkrbXVOenBEbWlPNXEwbjRSWDMrTHc2?= =?utf-8?B?Slo0YlVkb292YWtLWXhSQVExV1Z5aXBDckdTUENqMFVzOG1keE0yMzNvK2JM?= =?utf-8?B?VURuQTdIK3hMNmxMY1JFb1dzQTFyYjArMlNEZUEvaWZlbUVOaGd4TDVJT2hU?= =?utf-8?B?QURMTTUyeFhHVDRKSE9xcTN4M09LZzQrTVN4SVVFK0VCaWVHM2grSFFOd25E?= =?utf-8?B?TjZHRzJkZk43bnZwOUpMYld3SzFHYnJ5NjF3eUo4T0FNdzNKN2Z0ckZCY0dK?= =?utf-8?B?VVh1eFAzWmwzNlAxQVFKa1p2cTFBb3BnWUxmRUE0YUUvWUgrclhYT3h3ZGNR?= =?utf-8?B?MDV3R09yR2Y0aDFUNzZWcmI0b2llMElySys0YzN5VFBQSlhaM3E5cTJqSlZy?= =?utf-8?B?cmErZ1laMnV5NnBOTVdxTkdFK0NKZkZLcFhXejdVNXRhekVKUDVTMkJVbjJK?= =?utf-8?B?anA1d2JTYWhIQzBGM2Faa0tKQ1RBenc5ZTJBTG1ZbmVNVDZneEJ2QTNCRG1F?= =?utf-8?B?N0NFR2RPWnAxZHp0STZVMHVuKzBpUHRsUWJFSDk4ajdQL1ZqV0VoMHRBUzBh?= =?utf-8?B?ajZZVFZpeE5lVGNtU2d4aXBJazd3cGpxODlLMTZ0c2k2S3c0L2J5Q3ZwcWgw?= =?utf-8?B?NlRGKzhaWFU2RklQcjlqdDlnZ0pGTU5lZ213V25ObmpRR3NXdW1mTXA5eFE4?= =?utf-8?B?b0NDbmdpb0FsMHlETnB5NDM5TFJ4WmMvZUlTMDc5ak9JV1Jud0o4dysrZmtG?= =?utf-8?B?cU1IWXJhRWQ4c3llaEN6bnE2aHdVc01NT01wb2d1TzlNaHloOGRkakwvUmt1?= =?utf-8?B?R2c4cHBGbkhMaDNMN0RGZEVDVi9CUXRJQjJzWjltUzhNSmI1dWp5WFE2WDFk?= =?utf-8?B?UFFpZkFSS3dpUzQ3TE8rR1RPMkswTGdxWWdyNk5COFlWL3VGbTZ0QmYxTk9R?= =?utf-8?B?aUxwQlQvQ0pOby9jUmFzdVFrSkpSWk5GeGpDRHVQM0kxNHcvOHR0NWZYWTFK?= =?utf-8?B?WDFaMndVQU53eTJGTjRUSWpkeDBSdjdmQnlPcHNLcDJMTXlpcm53aHBiWDIy?= =?utf-8?B?VStiNkxZK0NUVVZhdklGS1FOMmloSE55R1A3S25mTm8zemJjd0Q2dG1yUzF4?= =?utf-8?B?cWlIQjBRVDNSeW1RYS9BNVNiR2w3eFZWYS9hYlI2RmNpcWVJbmVKUWVsYmY3?= =?utf-8?B?K3dKL0NYZEorSnJ1YkdUeWovNkpMRnI2ZHRPT1RIaUliRzNQa3dveFRiM09x?= =?utf-8?B?YkFrVnoxRGdUZFV2ZmhscVFRUndSMWs0ZXVmYzh4OHA0cmtuSXJzam45VEl1?= =?utf-8?B?T01WTk44Yno1bkR1R2JualZvREh5ZmdxRk1Nc3VKUmdrOEREK0d4Ylh3dEo1?= =?utf-8?B?UW5QSnlSckpjUGNQQmpaWHBNOG15eGdhMjNLdTVoNVJqNUxqTkpjSC82M0RG?= =?utf-8?B?dCtGSUhGU3ZNMzE3Um9lTklDcXk0Y1UvSmNYS3h0UTVmSGNQd216c1YydFBp?= =?utf-8?B?V29ScEI2bG5EOTJqcDVKaTZCU3M3dEVSZnVVQk9WY2ZYNVBRb0NpcjFNUDVY?= =?utf-8?Q?vqarfVfKetLX6RjLN4=3D?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1ca23661-b7a4-49cf-0f06-08da7fc756bf X-MS-Exchange-CrossTenant-AuthSource: DB6PR0101MB2214.eurprd01.prod.exchangelabs.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Aug 2022 20:38:56.2083 (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: AM0PR01MB3715 Subject: Re: [FFmpeg-devel] [PATCH 4/6] avcodec/mpegpicture: Don't copy unnecessarily, fix race 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: Michael Niedermayer: > On Sat, Aug 13, 2022 at 05:03:04PM +0200, Andreas Rheinhardt wrote: >> mpegvideo uses an array of Pictures and when it is done with using >> them, it only unreferences them incompletely: Some buffers are kept >> so that they can be reused lateron if the same slot in the Picture >> array is reused, making this a sort of a bufferpool. >> (Basically, a Picture is considered used if the AVFrame's buf is set.) >> Yet given that other pieces of the decoder may have a reference to >> these buffers, they need not be writable and are made writable using >> av_buffer_make_writable() when preparing a new Picture. This involves >> reading the buffer's data, although the old content of the buffer >> need not be retained. >> >> Worse, this read can be racy, because the buffer can be used by another >> thread at the same time. This happens for Real Video 3 and 4. >> >> This commit fixes this race by no longer copying the data; >> instead the old buffer is replaced by a new, zero-allocated buffer. >> >> (Here are the details of what happens with three or more decoding threads >> when decoding rv30.rm from the FATE-suite as happens in the rv30 test: >> The first decoding thread uses the first slot of its picture array >> to store its current pic; update_thread_context copies this for the >> second thread that decodes a P-frame. It uses the second slot in its >> Picture array to store its P-frame. This arrangement is then copied >> to the third decode thread, which decodes a B-frame. It uses the third >> slot in its Picture array for its current frame. >> update_thread_context copies this to the next thread. It unreferences >> the third slot containing the other B-frame and then it reuses this >> slot for its current frame. Because the pic array slots are only >> incompletely unreferenced, the buffers of the previous B-frame are >> still in there and they are not writable; in fact the previous >> thread is concurrently writing to them, causing races when making >> the buffer writable.) >> >> Signed-off-by: Andreas Rheinhardt >> --- >> libavcodec/mpegpicture.c | 16 +++++++++++++++- >> 1 file changed, 15 insertions(+), 1 deletion(-) > > This causes a difference in some frames of: (i have not investigates why > just reporting as i noticed) > quite possibly thats just showing your bugfix in action > > ./ffmpeg -y -bitexact -i fate/svq3/Vertical400kbit.sorenson3.mov -ps 50 -bf 1 -bitexact -an -qscale 5 -ss 40 -error_rate 4 -threads 1 /tmp/out4.avi > ./ffmpeg -y -bitexact -v -1 -loglevel 0 -i /tmp/out4.avi -bitexact -vsync drop -f framecrc - > > @@ -141,7 +141,7 @@ > 0, 22, 22, 1, 115200, 0x4cc933e9 > 0, 23, 23, 1, 115200, 0x693a40a9 > 0, 24, 24, 1, 115200, 0x956e3b15 > -0, 25, 25, 1, 115200, 0x61763ff4 > +0, 25, 25, 1, 115200, 0xc9e53d1c > 0, 26, 26, 1, 115200, 0x5c5c3dfc > 0, 27, 27, 1, 115200, 0x7de641ea > 0, 28, 28, 1, 115200, 0xe6cc4136 > @@ -187,7 +187,7 @@ > 0, 68, 68, 1, 115200, 0x49dcbf4e > 0, 69, 69, 1, 115200, 0x1ea1c7d1 > 0, 70, 70, 1, 115200, 0xdf77c67b > -0, 71, 71, 1, 115200, 0x7f6bd16d > +0, 71, 71, 1, 115200, 0x33d9d206 > 0, 72, 72, 1, 115200, 0x5e37cb3a > 0, 73, 73, 1, 115200, 0x15abcda3 > 0, 74, 74, 1, 115200, 0xbf4dcbd4 > @@ -199,7 +199,7 @@ > 0, 80, 80, 1, 115200, 0x17d1d667 > 0, 81, 81, 1, 115200, 0x0c1fdf9c > 0, 82, 82, 1, 115200, 0x7eabde6b > -0, 83, 83, 1, 115200, 0xe623e7af > +0, 83, 83, 1, 115200, 0x3bf6e873 > 0, 84, 84, 1, 115200, 0xf480dc82 > 0, 85, 85, 1, 115200, 0x5fd6e098 > 0, 86, 86, 1, 115200, 0xf520de95 > @@ -211,7 +211,7 @@ > 0, 92, 92, 1, 115200, 0x34cfe1c2 > 0, 93, 93, 1, 115200, 0x1d94e1c3 > 0, 94, 94, 1, 115200, 0x6d32e147 > -0, 95, 95, 1, 115200, 0x7e40ee91 > +0, 95, 95, 1, 115200, 0x09fbefd0 > 0, 96, 96, 1, 115200, 0xa5f5eb43 > 0, 97, 97, 1, 115200, 0x39b9ec3d > 0, 98, 98, 1, 115200, 0x3256ec18 > > [...] > Decoding this sample uses earlier values of mbskip_table. If I zero mbskip_table_buf in every ff_alloc_picture(), nothing changes due to this patch and (most importantly) the output of decoding does not depend upon the number of threads used (which it currently does -- with and without this patch). I don't know exactly where the bug is or whether there is a cheaper way to mitigate it than just to zero the buffer every time. I guess there are more such bugs affecting damaged samples than we are currently aware of. - 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".