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 D13AB4BF1E for ; Thu, 18 Jul 2024 15:14:24 +0000 (UTC) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id 623FB68D8E3; Thu, 18 Jul 2024 18:14:21 +0300 (EEST) Received: from mail0.khirnov.net (red.khirnov.net [176.97.15.12]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 21CE868D4E7 for ; Thu, 18 Jul 2024 18:14:14 +0300 (EEST) Authentication-Results: mail0.khirnov.net; dkim=pass (2048-bit key; unprotected) header.d=khirnov.net header.i=@khirnov.net header.a=rsa-sha256 header.s=mail header.b=Z1+43lZ6; dkim-atps=neutral Received: from localhost (localhost [IPv6:::1]) by mail0.khirnov.net (Postfix) with ESMTP id C92E7240DB7 for ; Thu, 18 Jul 2024 17:14:13 +0200 (CEST) Received: from mail0.khirnov.net ([IPv6:::1]) by localhost (mail0.khirnov.net [IPv6:::1]) (amavis, port 10024) with ESMTP id yiNcLW22sK36 for ; Thu, 18 Jul 2024 17:14:13 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=khirnov.net; s=mail; t=1721315653; bh=6Gc9YZajQm8JI1BMd2nuunEyjZfmbL6Q1Gmn0QFwGC0=; h=Subject:From:To:In-Reply-To:References:Date:From; b=Z1+43lZ65+In3YFBfw0K7I0iu60NlE3TmRMJ511CR2WM1bA76BdmA8pNUOxoErMKQ yRETdlDtwFAG2Sj9lrqKE7ieyYsFYhhxYukLp4muHbepRuCCr7WVvgm72+xwfHAifB F8di61634bHsfUL3iWGgxV622aBmxzChRnJaM9Nosrmmog2AtCzXwV9ucXlPVAGtw+ fwRydLD/YkkSXedzyn+L88dB6ZmAYUTHE4IAktgWlM4CypCQ9st4klssxe0JLG5KPj odZg1nZEhtGyl0YM/liJVxIcNVyxK8PPz7CCvoUTwbjwTozvNQ1EApTNDjSaB9klkl ymOqcj0RHS3Cw== Received: from lain.khirnov.net (lain.khirnov.net [IPv6:2001:67c:1138:4306::3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "lain.khirnov.net", Issuer "smtp.khirnov.net SMTP CA" (verified OK)) by mail0.khirnov.net (Postfix) with ESMTPS id 09CB8240695 for ; Thu, 18 Jul 2024 17:14:13 +0200 (CEST) Received: by lain.khirnov.net (Postfix, from userid 1000) id DDBE91601B9; Thu, 18 Jul 2024 17:14:12 +0200 (CEST) From: Anton Khirnov To: FFmpeg development discussions and patches In-Reply-To: <20240718143151.GB4991@pb2> References: <20240716171155.31838-1-anton@khirnov.net> <20240716171155.31838-13-anton@khirnov.net> <20240717223238.GW4991@pb2> <172129080994.21847.15080640617406361149@lain.khirnov.net> <20240718143151.GB4991@pb2> Mail-Followup-To: FFmpeg development discussions and patches Date: Thu, 18 Jul 2024 17:14:12 +0200 Message-ID: <172131565287.21847.1792634968888081802@lain.khirnov.net> User-Agent: alot/0.8.1 MIME-Version: 1.0 Subject: Re: [FFmpeg-devel] [PATCH 13/39] lavc/ffv1: drop redundant PlaneContext.quant_table 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: Quoting Michael Niedermayer (2024-07-18 16:31:51) > On Thu, Jul 18, 2024 at 10:20:09AM +0200, Anton Khirnov wrote: > > Quoting Michael Niedermayer (2024-07-18 00:32:38) > > > the data for each decoder task should be together and not scattered around > > > more than needed, reducing cache efficiency > > > > > > putting all this extra code in the inner per pixel loop is not ok > > > especially not for the sake of avoiding a memcpy of a few hundread bytes multiple levels of loops outside > > > > > A nice theory, > > that doesnt sound like a friendly description And this does not look like a friendly review. I am getting a very strong impression that you're looking for reasons to object to the patches, rather seriously review them. > > but in practice > > > this patchset > > "this patchset" isnt "this patch", nor does any improvment from "this patchset" > depend on the change of "this patch" > In fact it would probably benefit from droping this patch Just what would that benefit be? Storing and copying around multiple copies of the same data is generally bad for readability, as it requires more cognitive effort to understand the code - which is why I wrote this patch in the first place. It is also inefficient in terms of overall memory use and cache utilization, but I expect the impact of that to be small. If you're concerned about dereferencing a pointer in an inner loop, I can add a temporary variable to decode_line() as below, but removing duplicated data seems like an unambiguous improvement to me. diff --git a/libavcodec/ffv1dec_template.c b/libavcodec/ffv1dec_template.c index 97a28b085a..d68bbda5be 100644 --- a/libavcodec/ffv1dec_template.c +++ b/libavcodec/ffv1dec_template.c @@ -30,6 +30,7 @@ RENAME(decode_line)(FFV1Context *f, { PlaneContext *const p = &s->plane[plane_index]; RangeCoder *const c = &s->c; + const int16_t (*quant_table)[256] = f->quant_tables[p->quant_table_index]; int x; int run_count = 0; int run_mode = 0; @@ -59,7 +60,7 @@ RENAME(decode_line)(FFV1Context *f, return AVERROR_INVALIDDATA; } - context = RENAME(get_context)(f->quant_tables[p->quant_table_index], + context = RENAME(get_context)(quant_table, sample[1] + x, sample[0] + x, sample[1] + x); if (context < 0) { context = -context; -- Anton Khirnov _______________________________________________ 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".