[{"id":36007,"web_url":"https://patchwork.libcamera.org/comment/36007/","msgid":"<175899972003.416692.3669857429261291350@ping.linuxembedded.co.uk>","date":"2025-09-27T19:02:00","subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Hans de Goede (2025-09-27 19:00:04)\n> Run sw-statistics once every 4th frame, instead of every frame. There are\n> 2 reasons for this:\n> \n> 1. There really is no need to have statistics for every frame and only\n> doing this every 4th frame helps save some CPU time.\n> \n> 2. The generic nature of the simple pipeline-handler, so no information\n> about possible CSI receiver frame-delays. In combination with the software\n> ISP often being used with sensors without sensor info in the sensor-helper\n> code, so no reliable control-delay information means that the software ISP\n> is prone to AGC oscillation. Skipping statistics gathering also means\n> skipping running the AGC algorithm slowing it down, avoiding this\n> oscillation.\n> \n> Note ideally the AGC oscillation problem would be fixed by adding sensor\n> metadata support all through the stack so that the exact gain and exposure\n> used for a specific frame are reliably provided by the sensor metadata.\n> \n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v2:\n> - This is a new patch in v2 of this series\n\nTested on my X13s with no flicker.\n\nI also tested at \n - kStatPerNumFrames == 2 *Flicker*\n - kStatPerNumFrames == 3 No flicker.\n - kStatPerNumFrames == 4 No flicker.\n\nSo either 3 or 4 is good here.\n\nReviewed-by: Kieran Bingham \n\n\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++++---------\n> src/libcamera/software_isp/debayer_cpu.h | 4 ++--\n> src/libcamera/software_isp/swstats_cpu.cpp | 5 +++++\n> src/libcamera/software_isp/swstats_cpu.h | 3 +++\n> 4 files changed, 25 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index bfa60888..9010333e 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -655,7 +655,7 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n> lineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n> }\n> \n> -void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process2(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> unsigned int yEnd = window_.y + window_.height;\n> /* Holds [0] previous- [1] current- [2] next-line */\n> @@ -681,7 +681,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> for (unsigned int y = window_.y; y < yEnd; y += 2) {\n> shiftLinePointers(linePointers, src);\n> memcpyNextLine(linePointers);\n> - stats_->processLine0(y, linePointers);\n> + if (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> + stats_->processLine0(y, linePointers);\n> (this->*debayer0_)(dst, linePointers);\n> src += inputConfig_.stride;\n> dst += outputConfig_.stride;\n> @@ -696,7 +697,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> if (window_.y == 0) {\n> shiftLinePointers(linePointers, src);\n> memcpyNextLine(linePointers);\n> - stats_->processLine0(yEnd, linePointers);\n> + if (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> + stats_->processLine0(yEnd, linePointers);\n> (this->*debayer0_)(dst, linePointers);\n> src += inputConfig_.stride;\n> dst += outputConfig_.stride;\n> @@ -710,7 +712,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> }\n> }\n> \n> -void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> const unsigned int yEnd = window_.y + window_.height;\n> /*\n> @@ -733,7 +735,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> for (unsigned int y = window_.y; y < yEnd; y += 4) {\n> shiftLinePointers(linePointers, src);\n> memcpyNextLine(linePointers);\n> - stats_->processLine0(y, linePointers);\n> + if (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> + stats_->processLine0(y, linePointers);\n> (this->*debayer0_)(dst, linePointers);\n> src += inputConfig_.stride;\n> dst += outputConfig_.stride;\n> @@ -746,7 +749,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \n> shiftLinePointers(linePointers, src);\n> memcpyNextLine(linePointers);\n> - stats_->processLine2(y, linePointers);\n> + if (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> + stats_->processLine2(y, linePointers);\n> (this->*debayer2_)(dst, linePointers);\n> src += inputConfig_.stride;\n> dst += outputConfig_.stride;\n> @@ -821,12 +825,13 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> return;\n> }\n> \n> - stats_->startFrame();\n> + if (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> + stats_->startFrame();\n> \n> if (inputConfig_.patternSize.height == 2)\n> - process2(in.planes()[0].data(), out.planes()[0].data());\n> + process2(frame, in.planes()[0].data(), out.planes()[0].data());\n> else\n> - process4(in.planes()[0].data(), out.planes()[0].data());\n> + process4(frame, in.planes()[0].data(), out.planes()[0].data());\n> \n> metadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> @@ -851,7 +856,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> *\n> * \\todo Pass real bufferId once stats buffer passing is changed.\n> */\n> - stats_->finishFrame(frame, 0, true);\n> + stats_->finishFrame(frame, 0, frame % SwStatsCpu::kStatPerNumFrames == 0);\n> outputBufferReady.emit(output);\n> inputBufferReady.emit(input);\n> }\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 9d343e46..03e0d784 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -133,8 +133,8 @@ private:\n> void setupInputMemcpy(const uint8_t *linePointers[]);\n> void shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> void memcpyNextLine(const uint8_t *linePointers[]);\n> - void process2(const uint8_t *src, uint8_t *dst);\n> - void process4(const uint8_t *src, uint8_t *dst);\n> + void process2(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> + void process4(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> \n> /* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> static constexpr unsigned int kMaxLineBuffers = 5;\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index da91f912..35ba0a46 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -89,6 +89,11 @@ namespace libcamera {\n> * \\brief Signals that the statistics are ready\n> */\n> \n> +/**\n> + * \\var SwStatsCpu::kStatPerNumFrames\n> + * \\brief Run stats once every kStatPerNumFrames frames\n> + */\n> +\n> /**\n> * \\typedef SwStatsCpu::statsProcessFn\n> * \\brief Called when there is data to get statistics from\n> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h\n> index 6ac3c4de..ea0e6d5a 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.h\n> +++ b/src/libcamera/software_isp/swstats_cpu.h\n> @@ -32,6 +32,9 @@ public:\n> SwStatsCpu();\n> ~SwStatsCpu() = default;\n> \n> + /* Run stats once every 4 frames */\n> + static constexpr uint32_t kStatPerNumFrames = 4;\n> +\n> bool isValid() const { return sharedStats_.fd().isValid(); }\n> \n> const SharedFD &getStatsFD() { return sharedStats_.fd(); }\n> -- \n> 2.51.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 8BDC6BDB1C\n\tfor ;\n\tSat, 27 Sep 2025 19:02:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FC376B5F3;\n\tSat, 27 Sep 2025 21:02:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F1476B5A2\n\tfor ;\n\tSat, 27 Sep 2025 21:02:03 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 96D53B7D;\n\tSat, 27 Sep 2025 21:00:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ImTHUHX5\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1758999636;\n\tbh=JegCMv65HnbLgrDCHLaJfRLi3ZIXmW/AX5wLsTyhR/k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ImTHUHX5AC5In0fqbJSna3OOnnYSOTi/dlbw/nRRs37N3kTQqlJzG7cCdqFOsayrY\n\tyCQazovchTYOLRBw8hvm3arwpM5hjALZMALqSat1UVWa1nAXP7Q1DFGQxAsChjdK44\n\tH+aSSjsj/AEAivWiOG0+mK/Ynb87Gaxf7PkEp2Z4=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250927180004.84620-7-hansg@kernel.org>","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-7-hansg@kernel.org>","Subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","From":"Kieran Bingham ","Cc":"Hans de Goede ","To":"Hans de Goede , libcamera-devel@lists.libcamera.org","Date":"Sat, 27 Sep 2025 20:02:00 +0100","Message-ID":"<175899972003.416692.3669857429261291350@ping.linuxembedded.co.uk>","User-Agent":"alot/0.9.1","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36010,"web_url":"https://patchwork.libcamera.org/comment/36010/","msgid":"<1787b9af-afa0-4c82-a1be-6b1a576d302e@collabora.com>","date":"2025-09-27T20:01:11","subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","submitter":{"id":140,"url":"https://patchwork.libcamera.org/api/people/140/","name":"Robert Mader","email":"robert.mader@collabora.com"},"content":"Very happy to see this and would like to expand on 1: in case of the \nGPU-ISP we'll be able to avoid mmapping the input buffer for debayering \nif we can directly import it with the GPU (1). So if we also don't need \nto mmap the buffer for statistics, the saved overhead can be \nsignificant, especially on aarch64 where we need relatively expensive \nsync ioctls. So big thumbs-up for this.\n\nThanks and best regards,\n\nRobert\n\n1. PoC patch: \nhttps://gitlab.freedesktop.org/rmader/libcamera/-/commit/1f5c2764c4d42c916e52f9c06c44f4ae52519a78\n\nOn 9/27/25 20:00, Hans de Goede wrote:\n> Run sw-statistics once every 4th frame, instead of every frame. There are\n> 2 reasons for this:\n>\n> 1. There really is no need to have statistics for every frame and only\n> doing this every 4th frame helps save some CPU time.\n>\n> 2. The generic nature of the simple pipeline-handler, so no information\n> about possible CSI receiver frame-delays. In combination with the software\n> ISP often being used with sensors without sensor info in the sensor-helper\n> code, so no reliable control-delay information means that the software ISP\n> is prone to AGC oscillation. Skipping statistics gathering also means\n> skipping running the AGC algorithm slowing it down, avoiding this\n> oscillation.\n>\n> Note ideally the AGC oscillation problem would be fixed by adding sensor\n> metadata support all through the stack so that the exact gain and exposure\n> used for a specific frame are reliably provided by the sensor metadata.\n>\n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v2:\n> - This is a new patch in v2 of this series\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++++---------\n> src/libcamera/software_isp/debayer_cpu.h | 4 ++--\n> src/libcamera/software_isp/swstats_cpu.cpp | 5 +++++\n> src/libcamera/software_isp/swstats_cpu.h | 3 +++\n> 4 files changed, 25 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index bfa60888..9010333e 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -655,7 +655,7 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n> \tlineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n> }\n> \n> -void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process2(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> \tunsigned int yEnd = window_.y + window_.height;\n> \t/* Holds [0] previous- [1] current- [2] next-line */\n> @@ -681,7 +681,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \tfor (unsigned int y = window_.y; y < yEnd; y += 2) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine0(y, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -696,7 +697,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \tif (window_.y == 0) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(yEnd, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine0(yEnd, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -710,7 +712,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \t}\n> }\n> \n> -void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> \tconst unsigned int yEnd = window_.y + window_.height;\n> \t/*\n> @@ -733,7 +735,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \tfor (unsigned int y = window_.y; y < yEnd; y += 4) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine0(y, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -746,7 +749,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine2(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine2(y, linePointers);\n> \t\t(this->*debayer2_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -821,12 +825,13 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \t\treturn;\n> \t}\n> \n> -\tstats_->startFrame();\n> +\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\tstats_->startFrame();\n> \n> \tif (inputConfig_.patternSize.height == 2)\n> -\t\tprocess2(in.planes()[0].data(), out.planes()[0].data());\n> +\t\tprocess2(frame, in.planes()[0].data(), out.planes()[0].data());\n> \telse\n> -\t\tprocess4(in.planes()[0].data(), out.planes()[0].data());\n> +\t\tprocess4(frame, in.planes()[0].data(), out.planes()[0].data());\n> \n> \tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> @@ -851,7 +856,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \t *\n> \t * \\todo Pass real bufferId once stats buffer passing is changed.\n> \t */\n> -\tstats_->finishFrame(frame, 0, true);\n> +\tstats_->finishFrame(frame, 0, frame % SwStatsCpu::kStatPerNumFrames == 0);\n> \toutputBufferReady.emit(output);\n> \tinputBufferReady.emit(input);\n> }\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 9d343e46..03e0d784 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -133,8 +133,8 @@ private:\n> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> \tvoid memcpyNextLine(const uint8_t *linePointers[]);\n> -\tvoid process2(const uint8_t *src, uint8_t *dst);\n> -\tvoid process4(const uint8_t *src, uint8_t *dst);\n> +\tvoid process2(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> +\tvoid process4(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> \n> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index da91f912..35ba0a46 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -89,6 +89,11 @@ namespace libcamera {\n> * \\brief Signals that the statistics are ready\n> */\n> \n> +/**\n> + * \\var SwStatsCpu::kStatPerNumFrames\n> + * \\brief Run stats once every kStatPerNumFrames frames\n> + */\n> +\n> /**\n> * \\typedef SwStatsCpu::statsProcessFn\n> * \\brief Called when there is data to get statistics from\n> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h\n> index 6ac3c4de..ea0e6d5a 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.h\n> +++ b/src/libcamera/software_isp/swstats_cpu.h\n> @@ -32,6 +32,9 @@ public:\n> \tSwStatsCpu();\n> \t~SwStatsCpu() = default;\n> \n> +\t/* Run stats once every 4 frames */\n> +\tstatic constexpr uint32_t kStatPerNumFrames = 4;\n> +\n> \tbool isValid() const { return sharedStats_.fd().isValid(); }\n> \n> \tconst SharedFD &getStatsFD() { return sharedStats_.fd(); }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE578C328C\n\tfor ;\n\tSat, 27 Sep 2025 20:01:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6BFDE6B5F3;\n\tSat, 27 Sep 2025 22:01:20 +0200 (CEST)","from sender4-op-o12.zoho.com (sender4-op-o12.zoho.com\n\t[136.143.188.12])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C75046B5A2\n\tfor ;\n\tSat, 27 Sep 2025 22:01:17 +0200 (CEST)","by mx.zohomail.com with SMTPS id 1759003273977625.2397978817528;\n\tSat, 27 Sep 2025 13:01:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=collabora.com\n\theader.i=robert.mader@collabora.com header.b=\"S6wjUTrk\"; \n\tdkim-atps=neutral","ARC-Seal":"i=1; a=rsa-sha256; t=1759003275; cv=none; \n\td=zohomail.com; s=zohoarc; \n\tb=i5LeezvM60wBx8NKu+5Hr203MhFxHUzCtmiAcoeuIg4WH7p0OiW+WrbG86pWmfr5swljAYB8HrmPJ+X6bw0fcLpvlYk1fgt+eowwR5dAfwZCdX/UxY25PQ5dnEm2lDXWDf3AuGPvCgpL3zE9xOgCGwcuy1kZaSXYoguvXWylHHM=","ARC-Message-Signature":"i=1; a=rsa-sha256; c=relaxed/relaxed; d=zohomail.com; \n\ts=zohoarc; t=1759003275;\n\th=Content-Type:Content-Transfer-Encoding:Date:Date:From:From:In-Reply-To:MIME-Version:Message-ID:References:Subject:Subject:To:To:Message-Id:Reply-To:Cc;\n\tbh=HJYCRfEidoonXoprE4+UQUAPVKpJDl8/NeSukSR4D7s=; \n\tb=HU9AFdeaW7GVjfFYvz0FxA5nuRJqWZLAhJ55U7NclMgp3AGbepzWK6dNyJMLokuslYDo9V61YOzoHNwznfz0+59enCRyZQr/VLTlyOfgg7X1kIbEGb8jKmMmiaWWYqxzs2Fyy576qz2yS2a7hFs+N4iU6OHNk8TfXryjffgfoac=","ARC-Authentication-Results":"i=1; mx.zohomail.com;\n\tdkim=pass header.i=collabora.com;\n\tspf=pass smtp.mailfrom=robert.mader@collabora.com;\n\tdmarc=pass header.from=","DKIM-Signature":"v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; t=1759003275;\n\ts=zohomail; d=collabora.com; i=robert.mader@collabora.com;\n\th=Message-ID:Date:Date:MIME-Version:Subject:Subject:To:To:References:From:From:In-Reply-To:Content-Type:Content-Transfer-Encoding:Message-Id:Reply-To:Cc;\n\tbh=HJYCRfEidoonXoprE4+UQUAPVKpJDl8/NeSukSR4D7s=;\n\tb=S6wjUTrk25yK7i4ROsDeMuobmHtbdfCpplIS6Nuldjzc6rvNMYhHvJTAabYAj1FB\n\t9U6cpRiQWSG0kOJPXfdH3ayWY0qcbZNBOLVy3aOxv+KZasjU+Dz1k+shZ98AZR592Ya\n\txvUIsoPdYz8DWPgcqVtjGEB08JaFedEQ0BmFYQI0=","Message-ID":"<1787b9af-afa0-4c82-a1be-6b1a576d302e@collabora.com>","Date":"Sat, 27 Sep 2025 22:01:11 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","To":"libcamera-devel@lists.libcamera.org","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-7-hansg@kernel.org>","Content-Language":"en-US, de-DE, en-GB","From":"Robert Mader ","In-Reply-To":"<20250927180004.84620-7-hansg@kernel.org>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36020,"web_url":"https://patchwork.libcamera.org/comment/36020/","msgid":"<85cy79r38g.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-09-29T09:56:47","subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Hans,\n\nHans de Goede writes:\n\n> Run sw-statistics once every 4th frame, instead of every frame. There are\n> 2 reasons for this:\n>\n> 1. There really is no need to have statistics for every frame and only\n> doing this every 4th frame helps save some CPU time.\n>\n> 2. The generic nature of the simple pipeline-handler, so no information\n> about possible CSI receiver frame-delays. In combination with the software\n> ISP often being used with sensors without sensor info in the sensor-helper\n> code, so no reliable control-delay information means that the software ISP\n> is prone to AGC oscillation. Skipping statistics gathering also means\n> skipping running the AGC algorithm slowing it down, avoiding this\n> oscillation.\n>\n> Note ideally the AGC oscillation problem would be fixed by adding sensor\n> metadata support all through the stack so that the exact gain and exposure\n> used for a specific frame are reliably provided by the sensor metadata.\n>\n> Signed-off-by: Hans de Goede \n> ---\n> Changes in v2:\n> - This is a new patch in v2 of this series\n> ---\n> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++++---------\n> src/libcamera/software_isp/debayer_cpu.h | 4 ++--\n> src/libcamera/software_isp/swstats_cpu.cpp | 5 +++++\n> src/libcamera/software_isp/swstats_cpu.h | 3 +++\n> 4 files changed, 25 insertions(+), 12 deletions(-)\n>\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index bfa60888..9010333e 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -655,7 +655,7 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n> \tlineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n> }\n> \n> -void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process2(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> \tunsigned int yEnd = window_.y + window_.height;\n> \t/* Holds [0] previous- [1] current- [2] next-line */\n> @@ -681,7 +681,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \tfor (unsigned int y = window_.y; y < yEnd; y += 2) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n\nI think this condition deserves a helper rather than to be repeated\neverywhere, something like\n\n bool SwStatsCpu::applicableFrame(uint32_t frame)\n\n> +\t\t\tstats_->processLine0(y, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -696,7 +697,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \tif (window_.y == 0) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(yEnd, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine0(yEnd, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -710,7 +712,7 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n> \t}\n> }\n> \n> -void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> +void DebayerCpu::process4(uint32_t frame, const uint8_t *src, uint8_t *dst)\n> {\n> \tconst unsigned int yEnd = window_.y + window_.height;\n> \t/*\n> @@ -733,7 +735,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \tfor (unsigned int y = window_.y; y < yEnd; y += 4) {\n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine0(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine0(y, linePointers);\n> \t\t(this->*debayer0_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -746,7 +749,8 @@ void DebayerCpu::process4(const uint8_t *src, uint8_t *dst)\n> \n> \t\tshiftLinePointers(linePointers, src);\n> \t\tmemcpyNextLine(linePointers);\n> -\t\tstats_->processLine2(y, linePointers);\n> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\t\tstats_->processLine2(y, linePointers);\n> \t\t(this->*debayer2_)(dst, linePointers);\n> \t\tsrc += inputConfig_.stride;\n> \t\tdst += outputConfig_.stride;\n> @@ -821,12 +825,13 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \t\treturn;\n> \t}\n> \n> -\tstats_->startFrame();\n> +\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> +\t\tstats_->startFrame();\n> \n> \tif (inputConfig_.patternSize.height == 2)\n> -\t\tprocess2(in.planes()[0].data(), out.planes()[0].data());\n> +\t\tprocess2(frame, in.planes()[0].data(), out.planes()[0].data());\n> \telse\n> -\t\tprocess4(in.planes()[0].data(), out.planes()[0].data());\n> +\t\tprocess4(frame, in.planes()[0].data(), out.planes()[0].data());\n> \n> \tmetadata.planes()[0].bytesused = out.planes()[0].size();\n> \n> @@ -851,7 +856,7 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \t *\n> \t * \\todo Pass real bufferId once stats buffer passing is changed.\n> \t */\n> -\tstats_->finishFrame(frame, 0, true);\n> +\tstats_->finishFrame(frame, 0, frame % SwStatsCpu::kStatPerNumFrames == 0);\n> \toutputBufferReady.emit(output);\n> \tinputBufferReady.emit(input);\n> }\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 9d343e46..03e0d784 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -133,8 +133,8 @@ private:\n> \tvoid setupInputMemcpy(const uint8_t *linePointers[]);\n> \tvoid shiftLinePointers(const uint8_t *linePointers[], const uint8_t *src);\n> \tvoid memcpyNextLine(const uint8_t *linePointers[]);\n> -\tvoid process2(const uint8_t *src, uint8_t *dst);\n> -\tvoid process4(const uint8_t *src, uint8_t *dst);\n> +\tvoid process2(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> +\tvoid process4(uint32_t frame, const uint8_t *src, uint8_t *dst);\n> \n> \t/* Max. supported Bayer pattern height is 4, debayering this requires 5 lines */\n> \tstatic constexpr unsigned int kMaxLineBuffers = 5;\n> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp\n> index da91f912..35ba0a46 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -89,6 +89,11 @@ namespace libcamera {\n> * \\brief Signals that the statistics are ready\n> */\n> \n> +/**\n> + * \\var SwStatsCpu::kStatPerNumFrames\n> + * \\brief Run stats once every kStatPerNumFrames frames\n> + */\n> +\n> /**\n> * \\typedef SwStatsCpu::statsProcessFn\n> * \\brief Called when there is data to get statistics from\n> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h\n> index 6ac3c4de..ea0e6d5a 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.h\n> +++ b/src/libcamera/software_isp/swstats_cpu.h\n> @@ -32,6 +32,9 @@ public:\n> \tSwStatsCpu();\n> \t~SwStatsCpu() = default;\n> \n> +\t/* Run stats once every 4 frames */\n> +\tstatic constexpr uint32_t kStatPerNumFrames = 4;\n\nI still insist on documenting why exactly 4. It's OK to say \"this value\nseems to be a good idea because ... and it just works\". It's not OK to\nhave an arbitrary constant that nobody not involved in this review\nunderstands and has no clue whether it can be changed safely etc.\n\nTested-by: Milan Zamazal \n\n> +\n> \tbool isValid() const { return sharedStats_.fd().isValid(); }\n> \n> \tconst SharedFD &getStatsFD() { return sharedStats_.fd(); }","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 249E5C324C\n\tfor ;\n\tMon, 29 Sep 2025 09:56:57 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC28E6B5F9;\n\tMon, 29 Sep 2025 11:56:55 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1C8E66B58E\n\tfor ;\n\tMon, 29 Sep 2025 11:56:54 +0200 (CEST)","from mail-wm1-f69.google.com (mail-wm1-f69.google.com\n\t[209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-364-AuBgmLP0OKmHeUB0JD_QkQ-1; Mon, 29 Sep 2025 05:56:51 -0400","by mail-wm1-f69.google.com with SMTP id\n\t5b1f17b1804b1-46e375dab7dso22068675e9.3\n\tfor ;\n\tMon, 29 Sep 2025 02:56:51 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-46e42eee0b6sm108373125e9.10.2025.09.29.02.56.48\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 29 Sep 2025 02:56:48 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"WoZnGUgF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1759139813;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=VMS0WYcV1h5h4KQe0K6DiIAlWhVfkgZ9PTekUt4FWQw=;\n\tb=WoZnGUgFZ9aKv+NSRfZRMgsaEC7zr5HYj7ixjDmfdTDiDNTwzMSg54LolGfDbijMd9LBqV\n\tBzeexZjwYCVr4vwg2AAfzYTbwoRic1kwzIi9pM9o/Hari36g1whTAFmqMa5PO7+1UxA6aT\n\tvg7tBhQRS6eXkaTfyOna1JE7DxwNpHA=","X-MC-Unique":"AuBgmLP0OKmHeUB0JD_QkQ-1","X-Mimecast-MFC-AGG-ID":"AuBgmLP0OKmHeUB0JD_QkQ_1759139810","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1759139810; x=1759744610;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=VMS0WYcV1h5h4KQe0K6DiIAlWhVfkgZ9PTekUt4FWQw=;\n\tb=SJo0J7XgqSTfoc2t0T+c4rMW3aRFsumR9qzMRwzpgGFne42M1Aw95zZBa0OrJ/0PLU\n\tzJXntZooJZ2bHKauJHYx8Ww69fyq9BQnYXNK3DZHiRXawvp4P99lnlauTH87Kf5IhExN\n\t7naG9xwgVlZquFrFI5vKk2QP6T1/vyEdOV47mrMeLaquA9Ao6h1UQpS7wid0VEZpIywm\n\tPeU7nJfccyuzfAJiu/6JwDEwXw1J/6+wiAhFNlnB58p2SyS4NC6uhRINzDhBp3IDapX+\n\tWplrrKC2jJrTdWv3mfKDlrOIdfxztQF0o9mYtoncCcKdvSCsO0OliaEDw4+5gANQxInX\n\t2olQ==","X-Gm-Message-State":"AOJu0YwkxKmwjTy+GljO+/XFM8xL087m4Px8CZoF0S7dReO5QuIydzF8\n\tgO3sonUf0Fn53EDyMuKA6fzrmJmnUaCMUpqyhP8f3EMISYFIzFGCl/yWaVqQzpFIg3/NP2m2pdi\n\tsT34ddZeD/seCXp5tpuhocGHSQ6UaBUeWMrbwWzkSoUi2V+FflsTgQ/GnigX4/a91GNpLxXAu+K\n\tF320Kwf4YiGSVyObH8U0YP1h5LZORoDvExFg1OwAKwVwrFoi7DXRKFfqQkgRI=","X-Gm-Gg":"ASbGncsPsLi1AwmLVvEiPpD5h1VXnYOAggUmcSXLeUghlb2ZeS39sR2lWgl/azU6mhN\n\t2WvGom4CD935oJt7iS72YXnNwk5pWk+iBJ1ZxgoAk8gQMrMA8rBxvY03j2HEIUWXeFpHc39bDxU\n\tHq5FjTZ57fRNCKH3Q5iNLqn0ztp1lDHzx0IuvRnxBgn8iFG07FoJsg2pImiKjYCsKz49+HgEjU3\n\tj/98kd8fslV2ODYUEKZqxCCsOOQt3EibWO71vPRixf3JDypJtgHXppkHFJTQE/ynBnt6rrMsXwr\n\tg0nFOnoGRbaEJcBsLNCiDezaTv3jkfHguGM41mdHVJoeTpB89nnlrFFaTT8xDTPhoRY37ErM7nB\n\tU0JX9OIxb5QHRPayesA==","X-Received":["by 2002:a05:600c:a43:b0:45d:d97c:236c with SMTP id\n\t5b1f17b1804b1-46e329eb0e6mr151173115e9.21.1759139809634; \n\tMon, 29 Sep 2025 02:56:49 -0700 (PDT)","by 2002:a05:600c:a43:b0:45d:d97c:236c with SMTP id\n\t5b1f17b1804b1-46e329eb0e6mr151172835e9.21.1759139808960; \n\tMon, 29 Sep 2025 02:56:48 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEJfqPYs9FxHVE4MKCV1nT5hDPJ6iCpd4igsb3bBFGcd1QxI36hMthlldXWPJiRQQIn6aUJmA==","From":"Milan Zamazal ","To":"Hans de Goede ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","In-Reply-To":"<20250927180004.84620-7-hansg@kernel.org> (Hans de Goede's\n\tmessage of \"Sat, 27 Sep 2025 20:00:04 +0200\")","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-7-hansg@kernel.org>","Date":"Mon, 29 Sep 2025 11:56:47 +0200","Message-ID":"<85cy79r38g.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"whrhrbnHdHJmaRtkrtQ1cInFCdFF0xQ2TELOe4EYUfQ_1759139810","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36061,"web_url":"https://patchwork.libcamera.org/comment/36061/","msgid":"<2b21277f-fcfe-4d0b-9494-67b2830cdf31@kernel.org>","date":"2025-09-30T14:29:37","subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","submitter":{"id":239,"url":"https://patchwork.libcamera.org/api/people/239/","name":"Hans de Goede","email":"hansg@kernel.org"},"content":"Hi,\n\nOn 29-Sep-25 11:56, Milan Zamazal wrote:\n> Hi Hans,\n> \n> Hans de Goede writes:\n> \n>> Run sw-statistics once every 4th frame, instead of every frame. There are\n>> 2 reasons for this:\n>>\n>> 1. There really is no need to have statistics for every frame and only\n>> doing this every 4th frame helps save some CPU time.\n>>\n>> 2. The generic nature of the simple pipeline-handler, so no information\n>> about possible CSI receiver frame-delays. In combination with the software\n>> ISP often being used with sensors without sensor info in the sensor-helper\n>> code, so no reliable control-delay information means that the software ISP\n>> is prone to AGC oscillation. Skipping statistics gathering also means\n>> skipping running the AGC algorithm slowing it down, avoiding this\n>> oscillation.\n>>\n>> Note ideally the AGC oscillation problem would be fixed by adding sensor\n>> metadata support all through the stack so that the exact gain and exposure\n>> used for a specific frame are reliably provided by the sensor metadata.\n>>\n>> Signed-off-by: Hans de Goede \n>> ---\n>> Changes in v2:\n>> - This is a new patch in v2 of this series\n>> ---\n>> src/libcamera/software_isp/debayer_cpu.cpp | 25 +++++++++++++---------\n>> src/libcamera/software_isp/debayer_cpu.h | 4 ++--\n>> src/libcamera/software_isp/swstats_cpu.cpp | 5 +++++\n>> src/libcamera/software_isp/swstats_cpu.h | 3 +++\n>> 4 files changed, 25 insertions(+), 12 deletions(-)\n>>\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index bfa60888..9010333e 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -655,7 +655,7 @@ void DebayerCpu::memcpyNextLine(const uint8_t *linePointers[])\n>> \tlineBufferIndex_ = (lineBufferIndex_ + 1) % (patternHeight + 1);\n>> }\n>> \n>> -void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n>> +void DebayerCpu::process2(uint32_t frame, const uint8_t *src, uint8_t *dst)\n>> {\n>> \tunsigned int yEnd = window_.y + window_.height;\n>> \t/* Holds [0] previous- [1] current- [2] next-line */\n>> @@ -681,7 +681,8 @@ void DebayerCpu::process2(const uint8_t *src, uint8_t *dst)\n>> \tfor (unsigned int y = window_.y; y < yEnd; y += 2) {\n>> \t\tshiftLinePointers(linePointers, src);\n>> \t\tmemcpyNextLine(linePointers);\n>> -\t\tstats_->processLine0(y, linePointers);\n>> +\t\tif (frame % SwStatsCpu::kStatPerNumFrames == 0)\n> \n> I think this condition deserves a helper rather than to be repeated\n> everywhere, something like\n> \n> bool SwStatsCpu::applicableFrame(uint32_t frame)\n\nThank you for the suggestion. While implementing this I've chosen\na different route for v4:\n\nChanges in v4:\n- Pass frame number to SwStatsCpu::startFrame() SwStatsCpu::processLine?()\n and move all skipping handling to inside the SwStatsCpu class\n\n\n> \n>> +\t\t\tstats_->processLine0(y, linePointers);\n>> \t\t(this->*debayer0_)(dst, linePointers);\n>> \t\tsrc += inputConfig_.stride;\n>> \t\tdst += outputConfig_.stride;\n\n...\n\n>> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h\n>> index 6ac3c4de..ea0e6d5a 100644\n>> --- a/src/libcamera/software_isp/swstats_cpu.h\n>> +++ b/src/libcamera/software_isp/swstats_cpu.h\n>> @@ -32,6 +32,9 @@ public:\n>> \tSwStatsCpu();\n>> \t~SwStatsCpu() = default;\n>> \n>> +\t/* Run stats once every 4 frames */\n>> +\tstatic constexpr uint32_t kStatPerNumFrames = 4;\n> \n> I still insist on documenting why exactly 4. It's OK to say \"this value\n> seems to be a good idea because ... and it just works\". It's not OK to\n> have an arbitrary constant that nobody not involved in this review\n> understands and has no clue whether it can be changed safely etc.\n\nSorry I missed your previous comments about this magic value needing\nsome comment explaining it. I've fixed this for v4.\n\nRegards,\n\nHans","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 18E41C324C\n\tfor ;\n\tTue, 30 Sep 2025 14:29:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F2716B599;\n\tTue, 30 Sep 2025 16:29:44 +0200 (CEST)","from tor.source.kernel.org (tor.source.kernel.org [172.105.4.254])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6E99662C35\n\tfor ;\n\tTue, 30 Sep 2025 16:29:41 +0200 (CEST)","from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58])\n\tby tor.source.kernel.org (Postfix) with ESMTP id 4D803605AE;\n\tTue, 30 Sep 2025 14:29:40 +0000 (UTC)","by smtp.kernel.org (Postfix) with ESMTPSA id 6A9FFC4CEF0;\n\tTue, 30 Sep 2025 14:29:39 +0000 (UTC)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=kernel.org header.i=@kernel.org\n\theader.b=\"rd0jlCZo\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org;\n\ts=k20201202; t=1759242580;\n\tbh=B3FvapD47Z+5B24n2W5svJc7XQEBSFD2BMM49lwX0IA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rd0jlCZoS3RkUWp7o7XQc6vcX3r3mPp+GX9PPhUIda+VIrdv2CN+s3t8ai7IuCHLY\n\tWVSe5ptrsa3ftjFNatRzZmG3NnpQzO94meKWRFjSWn/03zFIwKfb6fhuTmBUFwm5ie\n\tyrJ/R8ji0jo05hf1kBMJRdgT/LKw7Ejf2ymAmlUzPJ04p9j1uDD9Qzw00+F28Lb876\n\tuqngKbLVQ5LU1KOX5+Nc1Iq6LgRTxCRnGUhtF0l5NIFFqroQxUUt/l+UfobheSytkh\n\tpeQ8T0PzBXbSpghLOyS6BG/fP9scjNjtyNOoWqyXCd2JJdPu8ia7l8xd9arXF2D6wz\n\tPWvSCp4A1Vp5w==","Message-ID":"<2b21277f-fcfe-4d0b-9494-67b2830cdf31@kernel.org>","Date":"Tue, 30 Sep 2025 16:29:37 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250927180004.84620-1-hansg@kernel.org>\n\t<20250927180004.84620-7-hansg@kernel.org>\n\t<85cy79r38g.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","From":"Hans de Goede ","Content-Language":"en-US, nl","In-Reply-To":"<85cy79r38g.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Content-Type":"text/plain; charset=UTF-8","Content-Transfer-Encoding":"7bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]