From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from ffbox0-bg.ffmpeg.org (ffbox0-bg.ffmpeg.org [79.124.17.100]) by master.gitmailbox.com (Postfix) with ESMTPS id CAFA94D771 for ; Sun, 1 Jun 2025 23:14:33 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTP id 0348568DE1D; Mon, 2 Jun 2025 02:14:29 +0300 (EEST) Received: from NAM04-MW2-obe.outbound.protection.outlook.com (mail-mw2nam04olkn2013.outbound.protection.outlook.com [40.92.46.13]) by ffbox0-bg.ffmpeg.org (Postfix) with ESMTPS id 3818868DC72 for ; Mon, 2 Jun 2025 02:14:22 +0300 (EEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=kfQoZ8gppOiprjgGY/XtOYD41pQ1QqH+U6Bt+FN+J318lDimcLrJD9rlj0XnKbl4UgCxWEUpf+leRJgPAn3o75NFmFVPXSbSrn7brQ+giCS3tSktwe2xwI+v3xKnNfcpC/5kRpfRs3Se4Isnh0xcnoYUjyVjKOdWmz1pCWt6H6NenOoDf/uqO2vdmq/Vt+z9KsutVnx7GUpyb5GzW+MPX6jXktWGVyocVHaa9/qQ2Finxkeg6s81diCpNjj5pM0aLaENvtlL4IUVXW5HQRPpM1/Y7mYDzjVGKx6yKmuKQXZKV4KhGreLZlymfTkAVHiK/ytpBjHuqh1TyApaub6q3Q== 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=FR0TmDRsa1eUjz1YM5N6vFGkKe0zcBHLwLb5oBDTk4I=; b=zGMb9VxAwJkEB+4LXEb5M0g1JuKTtZFneAjytSM2kDjV/9ai+XX8iCJDscYPZ+UfuGdE5PfencwUGN6f7U9k/zyMTcXQlm0ruX+mB0JKYvLBYj3CMKzKySymtuKEXrUmDq1UO6fYTMPFLO63N50oBZdrFO3qVrnp3SW6aUfQ8Ds0MBiebkkE3fvlEXHEUSDwFkTzIlhrxqi1N1GbWAw3y6zO3IiGdSBajO0Pkap98Yo6Do9YWifTTBmv3KZ84Y2R3YJBKCzlKd3UPffDBw0qIkNxqaMe0+Bh6mxh2HydzpBZcaf1fBCOx0tvOOPHiFD2ClTiz4948IvH5b5kgl9JFg== 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=hotmail.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=FR0TmDRsa1eUjz1YM5N6vFGkKe0zcBHLwLb5oBDTk4I=; b=lkoB0FNtxp+yUb81M8Tjr1LkW/xYe0h9fuqlqQk6xICjY5bl3o1AjOhWNllXpREXeswwuU1bUDTcL53pEtZrmOYFCUZiNSRzJ62iRFdt8WV3ZaTFVvMb7hzEokjgo5066baO0P/LnNKXxEi+MX8R1uTrXkGmN1dfvAtkomhEFZP0np6EYXkrIgo2Th+km9ux8n+RiuvnjcayPN/Lg3bK5Cw9ePFpx/OgZ1gbsqNVdgrb8+3AKhtpCyceflUPWv1PBFh3WvfLplPiZ2ayfCgmNNiy6j0IoCZEHu4z2TwfWh+Kao3o7HWaSvVUKSphr6UwQvcyDbrq65NJWi2j4iRo/Q== Received: from DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM (2603:10b6:8:b::20) by PH3PPF2DAC04883.NAMP223.PROD.OUTLOOK.COM (2603:10b6:518:1::518) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.8769.37; Sun, 1 Jun 2025 23:14:18 +0000 Received: from DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM ([fe80::bf09:8e9:b07f:98a7]) by DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM ([fe80::bf09:8e9:b07f:98a7%7]) with mapi id 15.20.8769.031; Sun, 1 Jun 2025 23:14:18 +0000 From: "softworkz ." To: FFmpeg development discussions and patches Thread-Topic: [FFmpeg-devel] [PATCH 01/11] fftools/graph/graphprint: Fix races when initializing graphprint Thread-Index: AQHb0qUjQJixtXGMNUqVWA513ZB097PtrunwgABuyACAAJCM0IAAL6kAgAACaHA= Date: Sun, 1 Jun 2025 23:14:18 +0000 Message-ID: References: In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-traffictypediagnostic: DM8P223MB0365:EE_|PH3PPF2DAC04883:EE_ x-ms-office365-filtering-correlation-id: ea18053d-7ba6-406d-3c6e-08dda16208c2 x-ms-exchange-slblob-mailprops: Cq7lScuPrnoSu68Y5FdRDlCPOBk43FEtC66ibW2S+2K/daHgP61pVoIEkEqLuSMwllSFBjN0CNBLw8nVDWCDfm9GpGy096ymIVNv2uKDbqHdAJ4oH4pY9r8ZZnwcpGmySJOKjz/h43KWrq7XQCiy45+mS0aCQQCVJ0fmvtv0g3iFc/GAfH+SqDTwrHUBO7gpdPheM1O4YtXdtcLcmUxRHxOavn3yO0Rn0asfe6iBQxHvZmev8xi/ufe2VrOebnb5PNw5kZGicqyHNIFfBoFA16DAyrl7fx1bl2mVHnlnJ49Bddwal3hvP00EQvEKCE72BBdEyeY04XaAqMiQdixZnCOPaOgbzulSKcqdEfq+qS1Qzfk3mgJdugYjmyzC/YJU/UMR2LO0BvdHJshwpwWuEi3ZLrfymjIDQPWZMSEwgT7eLD75DeePEEFSIhzLOkz7PX/uGLdgbrO/MVWeIvjsa3/LHdBxoqT+GUA27r/QLXoQgtur0cmnmZIeb2dIA6wAYLlxYJ+cG2O7h3LznYU7R9K6l3qVZcAAUTo6nc5qN8tpozqVG/3TefYVBCZyav1G8x0P+M7BrUp0p8Fu9/3VE8uDhbev73/X9fM6KgVe39EFQxR1YCgUTqnVMGIe+Th2YyX4/l2upIv7feZSHI71ynqF1vcH8fj6njBCF5O6DzqJ1RNRzoMobuUIOug88NmUWJsVAmb/+YEW+LOaHYwL/3O3T9wq+jzK02nd5/yNSkAe+E3Pz9jLNbmfDWp67CT8enCLtkq7Rh0= x-microsoft-antispam: BCL:0; ARA:14566002|8062599006|8060799009|461199028|7092599006|19110799006|15080799009|3412199025|440099028|102099032|56899033; x-microsoft-antispam-message-info: =?us-ascii?Q?nqOhn5mSQorzKYDkbL3QvWWyV8NESAG/Hnmoi/C7febpHZV3of1g5OmGleo7?= =?us-ascii?Q?Bidgq71N8RAy6ruRvDfniS8GKe6rdB72HTdhF1EnzntuP/Y6cFYVG0sfDAS+?= =?us-ascii?Q?prSwNv9xf6CS95X+cMrK6sFzX7AyH0J9Je7Rlno2wzxUMfbGpcjY3/tFkCG+?= =?us-ascii?Q?+5EzW26QXGbNPU49TqWxR47t7JgfNhk6HMptzM1Eo096ib/JxlTKLWgkoDw4?= =?us-ascii?Q?WZhC9tA79ZLqXATil1gcFsliQlTSIugn5SIuJTAuaAFwKUWuS2MJpTtZwPd/?= =?us-ascii?Q?eSrGhiGje5zuRZe1SbYofOy2wFp8AapA5TCOPypOVIUlD6IDUYSTEq4nCUqO?= =?us-ascii?Q?j2a2KYiLMgaY6DPqBVxfsL4O7I9xRKC+KBMz9O2kF8jQj6AWlcT2SofJWqKr?= =?us-ascii?Q?1lfWuv9ZgLhxL44+ZO6C0bPjfxpaFb720iug/Yr/cjdy6Y+50NLq65KKr5gH?= =?us-ascii?Q?Mqeo6VGimpBp+0HzUXt9mwIn/H5p8v86btSm6JdMcwhImZKTGq6nDwvMRTn9?= =?us-ascii?Q?zHE+gvBP/atRmKTEovIsWkP+pkOn1ttt5WBHom4OFmcNC8uXwuIajzeSIUBy?= =?us-ascii?Q?k/Ilx62wlNjCUbGJSJjJyZsg8AxX7doISuQ+0WDM/8Gl209+MX2ommwz/drd?= =?us-ascii?Q?Lp3zaDlkijsV+HekiuXPnWcZPEkcQvVepQpMlrHqvBW+tTCEhn7+D60Zs39w?= =?us-ascii?Q?tQUZ+DmY8e8oRIXRjH6bh/Lo2QXnaigEXdaLfCEpjE4NccWlI7CsirP/fXMC?= =?us-ascii?Q?WEcoM+SmQ169H04JRpKNuXqf0mjBh/J1UlxDiOOOxENKqmHmDmHNCBDdcOin?= =?us-ascii?Q?ROWLAJFSZOHOAf4UODhZLvrtnRVVyoAk0RDDXbfWJ+rAjyqgAIgjl9F/JO3/?= =?us-ascii?Q?Ue2wfFgvn8XR31u8WZU4RqlprVGc/NDGmcCfkVwBVkx3/zBLycKRznyK97eS?= =?us-ascii?Q?qRPOGuhvIfvZTdQ5p38L77KfW7jJsP5Y4XWqxw8XKtmQUURK/Kc8q22x0Sy6?= =?us-ascii?Q?YZalX4f9LhmRTIV5q2808SsCcIbKqd/DXXN6uoxOL4FPPJW0xEIVQtyREjMu?= =?us-ascii?Q?HUAV4DfqcbFdcA4RKi5KQoE1M45FkOq9/qgYE0Z0uL2RVf60+CA=3D?= x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?4gkf8/6Rx5WySyOpTJNFUlN634z9dBml1B74ZtHsGnQ1tpdMOflFGIa/FoDF?= =?us-ascii?Q?dVUSjWRJ0Yc7plHcQRU/VNm7DwfojmvKdnNzK0LUwa9uEJqiCc7rBoUZQtSo?= =?us-ascii?Q?fWq8/6pDpDZsyWFojgg536ZkbSYBZLPoe57lcppQN36lojXp9rQ6gvnfQ6wH?= =?us-ascii?Q?k2tgf5xO84RNrlRROWlMBQ1t/5BReVGnMI2/q2kt+SulnN8vfapF3REjrISO?= =?us-ascii?Q?kZ/tWjLJZZOSDetkU6tnSWSA/X6dxp9//Z3+8LVMQY7bICHWBWxr1qEZCrJx?= =?us-ascii?Q?EewkCwKsHVKEXVWQdWa7KZsEFXQgf1Do0/JMQaQboUo2l6nZEXeq29nJ3NOY?= =?us-ascii?Q?TQzi6aoZpRYiKsdETv8aIb6b5C/zT107eZWbd7D9r3zVu6KB8c74giM8doAJ?= =?us-ascii?Q?+GRWBRBgl6fEBp4GYknTN04IbtgGfXI60OukQiXxDgWV6U6TTfKGCuzmsIXR?= =?us-ascii?Q?iW6y5aXcUitMXrYGxLGDf5xxVMAlY9Lw6Qig2jlcilV9PlWdAMNBel+IEG89?= =?us-ascii?Q?irz3K/mXvTpBStC2msuU+d1igaDu4evjpE474NoYHyTZUDTwpXKXmba48sFZ?= =?us-ascii?Q?b6+YVY7bj53zgYPXN0pa2q7s2HRjPWR/Ay6/TfB4xEtfk/ngw0pLX0v9uJdt?= =?us-ascii?Q?JH5BsAnG5Tq7iR8GnkhdMYrJ9uAeUTfAo3F0eSEq66aukirGQ0QX27FKSTbV?= =?us-ascii?Q?YkPV/r9JjY5R9Cr2ZCbpCROWfoJWWvAsx21y1x7hdQe+vUzaq9tdMSARkLN4?= =?us-ascii?Q?F7Ok+dD1UcEjb59ciHA9T/1lcOduVXGOvmPPZcmpC7iLDKp5y9MRYx+HFiou?= =?us-ascii?Q?2FMUzZR22lGR8pu8n5VMjpc+O1V5tJfIqSxl06q82oGWueNI8GfVzZteK+ox?= =?us-ascii?Q?51uYclDNQT1asctptQ9lMGnJ+kHFtuB2A9bk3TSClzmZpiH7jup7zi4VtfjO?= =?us-ascii?Q?kaWbW8WdaahkJF4gihG27W8wIEf7/Y5ioBTI2SGd5KsKON71Co/UIlmJW/jU?= =?us-ascii?Q?UfkQngpIkJNMyGUA5KmJjOqacB72F9AH9ArHl8uUYLFp5AJwHGH2SEGdyFO4?= =?us-ascii?Q?SDaALfWHXydSbmDoYjzCqvQoy82XMZg59r6r/YaKsS/6QYtxSpFoVfp5sbdh?= =?us-ascii?Q?/dT6dTOhNf1O6wV8rZ3xq0rR+BPBusyyMtb8DDZ5WQOV2JDfv31YkFxZzh9G?= =?us-ascii?Q?Wqb8PpddmbwrHWulQvrmZNjR7KM0DAz0up1ewieYpJ3YB/0qHJ6p+Z/ey+o?= =?us-ascii?Q?=3D?= MIME-Version: 1.0 X-OriginatorOrg: sct-15-20-8534-20-msonline-outlook-c7cf3.templateTenant X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM8P223MB0365.NAMP223.PROD.OUTLOOK.COM X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-CrossTenant-Network-Message-Id: ea18053d-7ba6-406d-3c6e-08dda16208c2 X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Jun 2025 23:14:18.1793 (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: PH3PPF2DAC04883 Subject: Re: [FFmpeg-devel] [PATCH 01/11] fftools/graph/graphprint: Fix races when initializing graphprint 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 Cc: Stefano Sabatini 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: > -----Original Message----- > From: ffmpeg-devel On Behalf Of Andreas > Rheinhardt > Sent: Montag, 2. Juni 2025 00:08 > >>> - Why do you want to remove the ResourceManager AVClass? > >> > >> Because I think callers should provide their own logcontext > > > > Do we have any precedent cases for this? What would be the > > parameter type for passing it - AVClass* ? > > > > The type would be void*. AVClass* is wrong (AVClass** would be correct). > There are lots of functions with a logcontext parameter (see git grep -E > 'void \*log_?ctx' *.h) Alright, thanks. > > > > > > >>> It wasn't unused. Now the prefix is gone for log entries in > >>> decompress_gzip() > >> > >> True, this patch is just wrong (in case of decompression errors, > >> av_log() will treat ResourceManagerContext as a logging context, > >> although it is no more; this would lead to segfaults if AVDictionary is > >> already set, i.e. when initializing the second resource). > > > > Yes, I had seen it crashing once as well when moving debugger > > execution to an av_log line during the second execution. > > All good now! > > > >>> Actually, all av_log() calls should include the resman_ctx > >>> Seems this has been forgotten (well..by me) > > > > Thanks for fixing! > > > > > >>> - For the registered_formatters initialization: > >>> I used to have initialization order issues when I had tried > >>> with static initialization. That's the reason for those functions > >>> Probably you've done it differently as it seems to work so far > >> > >> What issues? > > > > Here's my comment from an earlier review: > > > > ---- > > > >>> +static const AVTextFormatter *registered_formatters[7+1]; > >> > >> maybe use a const here, also I'd be more happy if we had a more > >> dynamic registration system to avoid the hardcoded bits > > > > While trying this, I remembered that I had tried this before already, but it > causes trouble with static initialization order. Probably that's the reason > why it has been like this before already, I'm not sure? > > > > ----> > > What I had seen: The registered_formatters array was initialized before > > static initialization of all its members had been done. > > It happened at a rather early stage, so it is well possible that it is > > resolved by changes that were made meanwhile. > > FATE tests are passing, so it's probably fine, now. > > FATE is mostly useless here, given that graphprint is not covered by it. It wasn't about graphprint. At that time, graphprint wasn't even included. It was about the classic output formats, and since some of them are used for output in FATE tests, it should be a valid proof that the issues I had seen are no longer there. Yet, I have run FATE on Linux only. Unfortunately, Patchwork takes always only the first patch when sent as multiple attachments, so we can't see whether FATE is finishing well on all the builders. > > > Here's my full review: > > > > v2-0001-fftools-graph-graphprint-Fix-races-when-initializ > > > > LGTM, I wasn't aware that av_strtok() is destructive, > > maybe the doc text should mention it. s param being > > non-const might give a hint, but it's still a bit > > unexpected for anybody who doesn't know all the APIs > > by heart. > > > > Now that it's no longer updated, should print_graphs_format > > be made const? Same for print_graphs_file? > > They are updated generically, via the option system. Making them const > would lead to compiler warnings from the "{ &print_graphs_format }," > line in the options definition. Why? The chars are const, but not the pointer. I don't see a warning either (neither MSVC nor GCC) > Furthermore, the compiler would put > these variables into .rodata if they were made const, which would lead > to crashes if the user sets the relevant options. To be clear, what I meant is changing extern char *print_graphs_file = NULL; to extern const char *print_graphs_file = NULL; > > v2-0002-fftools-textformat-avtextformat-Separate-mutable- > > > > I am aware of the mixed mutability in the section > > definitions. The general idea of separation and making > > the section definitions immutable is right of course. > > > > Yet, the general goal is to build a versatile API for > > other use cases, so we should not make changes based > > on "x is currently used by a only, not by b, so make > > it local to a". > > > > The ability to limit the fields that should be output > > for each section is an essential feature that should > > be generally available for all usages, that's why it > > should rather go in the opposite direction - i.e. > > from FFprobe (where it's specific atm) to the API. > > I haven't come to do that yet. > > > > > > We should also hear what Stefano will say about it, > > as far as I understood he sees it similar, but I > > might be wrong. > > I'll of course wait for Stefano, but already want to add that nothing is > more versatile than a callback. By saying "versatile", I meant that the API is usable for many cases, but it should also reduce the burden of re-implementing common features. Specifically, in this case it would be more convenient for API consumers when they wouldn't need to care about command line parsing and populating the dictionaries for sections with the fields that should be printed. Ideally, the API would allow both ways. > > v2-0010-fftools-textformat-avtextformat-Move-avtext_print > > > > avtext_print_integers() allows to print binary data in > > a tabular layout and various formats. > > I don't see it that way. E.g. it forces you to write the offset at the > start of each line, it forces the layout of the offset upon you. One > could of course add yet more arguments to support more use cases, but > then it would be ever more of a monster. I think it would make sense to skim through the usages to see what's actually required at the moment and based on that, we can determine the best way to go forward, like a better signature and feature set. PS: I'm CCing Stefano Thanks sw _______________________________________________ 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".