[{"id":36064,"web_url":"https://patchwork.libcamera.org/comment/36064/","msgid":"<85h5wjc8fj.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-10-01T08:50:24","subject":"Re: [PATCH v4 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":"Hans de Goede <hansg@kernel.org> 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> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Tested-by: Milan Zamazal <mzamazal@redhat.com>\n> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Hans de Goede <hansg@kernel.org>\n\nI'm still not excited about `frame % kStatPerNumFrames' spread in\nmultiple places but I can live with it.\n\nReviewed-by: Milan Zamazal <mzamazal@redhat.com>\n\n> ---\n> Changes in v4:\n> - Document why to skip 3 frames / why once every 4 frames\n> - Pass frame number to SwStatsCpu::startFrame() SwStatsCpu::processLine?()\n>   and move all skipping handling to inside the SwStatsCpu class\n> ---\n>  src/libcamera/software_isp/debayer_cpu.cpp | 18 +++++++++---------\n>  src/libcamera/software_isp/debayer_cpu.h   |  4 ++--\n>  src/libcamera/software_isp/swstats_cpu.cpp | 19 +++++++++++++++----\n>  src/libcamera/software_isp/swstats_cpu.h   | 20 +++++++++++++++++---\n>  4 files changed, 43 insertions(+), 18 deletions(-)\n>\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 2dc85e5e0..d5fd0ae73 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,7 @@ 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\tstats_->processLine0(frame, y, linePointers);\n>  \t\t(this->*debayer0_)(dst, linePointers);\n>  \t\tsrc += inputConfig_.stride;\n>  \t\tdst += outputConfig_.stride;\n> @@ -696,7 +696,7 @@ 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\tstats_->processLine0(frame, yEnd, linePointers);\n>  \t\t(this->*debayer0_)(dst, linePointers);\n>  \t\tsrc += inputConfig_.stride;\n>  \t\tdst += outputConfig_.stride;\n> @@ -710,7 +710,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 +733,7 @@ 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\tstats_->processLine0(frame, y, linePointers);\n>  \t\t(this->*debayer0_)(dst, linePointers);\n>  \t\tsrc += inputConfig_.stride;\n>  \t\tdst += outputConfig_.stride;\n> @@ -746,7 +746,7 @@ 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\tstats_->processLine2(frame, y, linePointers);\n>  \t\t(this->*debayer2_)(dst, linePointers);\n>  \t\tsrc += inputConfig_.stride;\n>  \t\tdst += outputConfig_.stride;\n> @@ -821,12 +821,12 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>  \t\treturn;\n>  \t}\n>  \n> -\tstats_->startFrame();\n> +\tstats_->startFrame(frame);\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> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 9d343e464..03e0d7843 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 eb416dfdc..634ebfc3c 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.cpp\n> +++ b/src/libcamera/software_isp/swstats_cpu.cpp\n> @@ -62,8 +62,9 @@ namespace libcamera {\n>   */\n>  \n>  /**\n> - * \\fn void SwStatsCpu::processLine0(unsigned int y, const uint8_t *src[])\n> + * \\fn void SwStatsCpu::processLine0(uint32_t frame, unsigned int y, const uint8_t *src[])\n>   * \\brief Process line 0\n> + * \\param[in] frame The frame number\n>   * \\param[in] y The y coordinate.\n>   * \\param[in] src The input data.\n>   *\n> @@ -74,8 +75,9 @@ namespace libcamera {\n>   */\n>  \n>  /**\n> - * \\fn void SwStatsCpu::processLine2(unsigned int y, const uint8_t *src[])\n> + * \\fn void SwStatsCpu::processLine2(uint32_t frame, unsigned int y, const uint8_t *src[])\n>   * \\brief Process line 2 and 3\n> + * \\param[in] frame The frame number\n>   * \\param[in] y The y coordinate.\n>   * \\param[in] src The input data.\n>   *\n> @@ -89,6 +91,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> @@ -295,11 +302,15 @@ void SwStatsCpu::statsGBRG10PLine0(const uint8_t *src[])\n>  \n>  /**\n>   * \\brief Reset state to start statistics gathering for a new frame\n> + * \\param[in] frame The frame number\n>   *\n>   * This may only be called after a successful setWindow() call.\n>   */\n> -void SwStatsCpu::startFrame(void)\n> +void SwStatsCpu::startFrame(uint32_t frame)\n>  {\n> +\tif (frame % kStatPerNumFrames)\n> +\t\treturn;\n> +\n>  \tif (window_.width == 0)\n>  \t\tLOG(SwStatsCpu, Error) << \"Calling startFrame() without setWindow()\";\n>  \n> @@ -318,7 +329,7 @@ void SwStatsCpu::startFrame(void)\n>   */\n>  void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)\n>  {\n> -\tstats_.valid = true;\n> +\tstats_.valid = frame % kStatPerNumFrames == 0;\n>  \t*sharedStats_ = stats_;\n>  \tstatsReady.emit(frame, bufferId);\n>  }\n> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h\n> index 26a2f462e..fae575f85 100644\n> --- a/src/libcamera/software_isp/swstats_cpu.h\n> +++ b/src/libcamera/software_isp/swstats_cpu.h\n> @@ -32,6 +32,14 @@ public:\n>  \tSwStatsCpu();\n>  \t~SwStatsCpu() = default;\n>  \n> +\t/*\n> +\t * The combination of pipeline + sensor delays means that\n> +\t * exposure changes can take up to 3 frames to get applied,\n> +\t * Run stats once every 4 frames to ensure any previous\n> +\t * exposure changes have been applied.\n> +\t */\n> +\tstatic constexpr uint32_t kStatPerNumFrames = 4;\n> +\n>  \tbool isValid() const { return sharedStats_.fd().isValid(); }\n>  \n>  \tconst SharedFD &getStatsFD() { return sharedStats_.fd(); }\n> @@ -40,11 +48,14 @@ public:\n>  \n>  \tint configure(const StreamConfiguration &inputCfg);\n>  \tvoid setWindow(const Rectangle &window);\n> -\tvoid startFrame();\n> +\tvoid startFrame(uint32_t frame);\n>  \tvoid finishFrame(uint32_t frame, uint32_t bufferId);\n>  \n> -\tvoid processLine0(unsigned int y, const uint8_t *src[])\n> +\tvoid processLine0(uint32_t frame, unsigned int y, const uint8_t *src[])\n>  \t{\n> +\t\tif (frame % kStatPerNumFrames)\n> +\t\t\treturn;\n> +\n>  \t\tif ((y & ySkipMask_) || y < static_cast<unsigned int>(window_.y) ||\n>  \t\t    y >= (window_.y + window_.height))\n>  \t\t\treturn;\n> @@ -52,8 +63,11 @@ public:\n>  \t\t(this->*stats0_)(src);\n>  \t}\n>  \n> -\tvoid processLine2(unsigned int y, const uint8_t *src[])\n> +\tvoid processLine2(uint32_t frame, unsigned int y, const uint8_t *src[])\n>  \t{\n> +\t\tif (frame % kStatPerNumFrames)\n> +\t\t\treturn;\n> +\n>  \t\tif ((y & ySkipMask_) || y < static_cast<unsigned int>(window_.y) ||\n>  \t\t    y >= (window_.y + window_.height))\n>  \t\t\treturn;","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 43DEDC328C\n\tfor <parsemail@patchwork.libcamera.org>;\n\tWed,  1 Oct 2025 08:50:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D8596B5A2;\n\tWed,  1 Oct 2025 10:50:34 +0200 (CEST)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0E67F69318\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed,  1 Oct 2025 10:50:30 +0200 (CEST)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-339-4PG_u-QFPYmN0b1nI10aDA-1; Wed, 01 Oct 2025 04:50:28 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-3f3787688b0so4183925f8f.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tWed, 01 Oct 2025 01:50:27 -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\tffacd0b85a97d-4151d0ae6f9sm21682675f8f.51.2025.10.01.01.50.24\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 01 Oct 2025 01:50:25 -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=\"QYx0I5pA\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1759308629;\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=mtsT7O0LW3IHWUmI5hE1xNMtlKm5Atn20x1prK7d1AQ=;\n\tb=QYx0I5pAeFDl6nk/7uZeYj7Y1KDYmxUMFc2bxzPmqf45z6ew5b8stASTC2+ToHw6ywC5Y4\n\tmCguxR3OK5ubQuh7mP4+3DSRq3dWicJQ9uYDwKK1SMq9wFZpqYvQoXHz9dTCTxPFfDtEo1\n\tOK0GkZAXMIcrT7ckqk3GBimd5khiw1A=","X-MC-Unique":"4PG_u-QFPYmN0b1nI10aDA-1","X-Mimecast-MFC-AGG-ID":"4PG_u-QFPYmN0b1nI10aDA_1759308627","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1759308627; x=1759913427;\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=mtsT7O0LW3IHWUmI5hE1xNMtlKm5Atn20x1prK7d1AQ=;\n\tb=GwRLpDWl3ICDTsBNAV/pDHj55MHpP8IHUCm7EkRSXSjPijcaHafOmKRyOFOZIF294/\n\tHVATIdu4z+UvDo1VxErL0cE4Na/TEwelg9jRNCDk8wGdw0hP6kfKzqf62iWWtRzFR1Wm\n\t8O7CwptLE3kOgw01unl4dxC1k0oajQi7n2mwgwcQdehvEpY38RRub0REr4Yosz5Byk82\n\tJh9NAMkmeVgWkAT6MRx7nvEzDd4Rn5fKixM+YlNnbZM9zY/tR3El8wxJ0kT03+Z5gouP\n\tvnstBTWvqCBOOu+04jruMbxCNBP72/NYvv20x86FgPHTILtds9uynvzI3UGFDFs7BZ9f\n\tyMXg==","X-Gm-Message-State":"AOJu0Yy/aDjDTXHbgxz4gC59/I7Qgo3/UvGlgZFoyOTpFIDHULB+AY7y\n\t//NY+BTYGB0i4wls0QXerYa1gKS120wr1I74yPxhuPfFXS2V//KtUoKfXW69+IvWMmaJvVj8aWE\n\tLmiyhsm8hOJkhqw2e8yqDXRZu+ZQumBTsFTlk9Qbr/EUYBIbt+FtTc/LrJLa1XWCi0I9QAjNWCf\n\to=","X-Gm-Gg":"ASbGncsalMCX1u47v1enPySAYYeY09jfN7Q6Ro26Llz6rtD50xKKjUFV7VzpZLFsEvA\n\tNAANhu9lcNrPFv4iG26oCzp1vWVOBSBMzhnVOX1iQX/d2rDp/pVgTUGGfG6qKfnigBTCM9m/41Y\n\taRipewblcwDiwHtFhqmqIsDQfw3pMmTGj65lekjHC+iCJcr6BVqQeMHzLzMA2aEeyKHH/Hbc/Uz\n\tkZPjGuWNkS/QRqknDTOyNQ4aNkkuY7PvAsc4D2yg+DNx04KVsrEi5EGm8XsN1HBH9MjSg79nXqv\n\t+L4WxThaAz8Cuqq8Wlz9jVX1AaxhG/oqkxjv7IcSSBtz7luy/wZQco+7VP+Ubim2sqHdMJKvanj\n\tp2kQmsoEqYVcpvhMo5c+XXtg=","X-Received":["by 2002:a05:6000:4007:b0:400:4507:474 with SMTP id\n\tffacd0b85a97d-425577f3872mr1726051f8f.18.1759308626921; \n\tWed, 01 Oct 2025 01:50:26 -0700 (PDT)","by 2002:a05:6000:4007:b0:400:4507:474 with SMTP id\n\tffacd0b85a97d-425577f3872mr1726029f8f.18.1759308626365; \n\tWed, 01 Oct 2025 01:50:26 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEV84ZyPJwMc0Du8U61fuSR/tK3yRCvQPKtEveCvAoZH2O/7muLZk5LFMKCi478w1zbwzLarQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Hans de Goede <hansg@kernel.org>","Cc":"libcamera-devel@lists.libcamera.org,  Kieran Bingham\n\t<kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH v4 6/6] libcamera: software_isp: Run sw-statistics once\n\tevery 4th frame","In-Reply-To":"<20250930150428.11101-7-hansg@kernel.org> (Hans de Goede's\n\tmessage of \"Tue, 30 Sep 2025 17:04:28 +0200\")","References":"<20250930150428.11101-1-hansg@kernel.org>\n\t<20250930150428.11101-7-hansg@kernel.org>","Date":"Wed, 01 Oct 2025 10:50:24 +0200","Message-ID":"<85h5wjc8fj.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":"U2n8Uiufjoo1JF9kTuN7OelfnBil5NMJxsO67D_c1R4_1759308627","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]