[{"id":34699,"web_url":"https://patchwork.libcamera.org/comment/34699/","msgid":"<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com>","date":"2025-06-27T14:00:44","subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n> Software ISP performs performance measurement on certain part of initial\n> frames. Let's make this range configurable.\n> \n> For this purpose, this patch introduces new configuration options\n> pipelines.simple.measure.skip and pipelines.simple.measure.number.\n> Setting the latter one to 0 disables the measurement.\n> \n> Instead of the last frame, the class member and its configuration\n> specify the number of frames to measure. This is easier to use for\n> users and doesn't require to adjust two configuration parameters when\n> the number of the initially skipped frames is changed.\n> \n> The patch also changes the names of the class members to make them more\n> accurate.\n> \n> Completes software ISP TODO #7.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> Documentation/runtime_configuration.rst | 6 ++++++\n> src/libcamera/software_isp/TODO | 25 ----------------------\n> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------\n> src/libcamera/software_isp/debayer_cpu.h | 7 +++---\n> 4 files changed, 25 insertions(+), 38 deletions(-)\n> \n> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n> index f4e86ae24..de74fa732 100644\n> --- a/Documentation/runtime_configuration.rst\n> +++ b/Documentation/runtime_configuration.rst\n> @@ -52,6 +52,9 @@ file structure:\n> ...\n> simple:\n> copy_input_buffer: # true/false\n> + measure:\n> + skip: # non-negative integer, frames to skip initially\n> + number: # non-negative integer, frames to measure\n> supported_devices:\n> - driver: # driver name, e.g. `mxc-isi`\n> software_isp: # true/false\n> @@ -87,6 +90,9 @@ Configuration file example\n> min_total_unicam_buffers: 2\n> simple:\n> copy_input_buffer: false\n> + measure:\n> + skip: 50\n> + number: 30\n> supported_devices:\n> - driver: mxc-isi\n> software_isp: true\n> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n> index 2c919f442..f19e15ae2 100644\n> --- a/src/libcamera/software_isp/TODO\n> +++ b/src/libcamera/software_isp/TODO\n> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.\n> \n> ---\n> \n> -7. Performance measurement configuration\n> -\n> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n> -> /* Measure before emitting signals */\n> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> -> \ttimespec frameEndTime = {};\n> -> \tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> -> \tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> -> \tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> -> \t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> -> \t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> -> \t\tLOG(Debayer, Info)\n> -> \t\t\t<< \"Processed \" << measuredFrames\n> -> \t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> -> \t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> -> \t\t\t<< \" us/frame\";\n> -> \t}\n> -> }\n> -\n> -I wonder if there would be a way to control at runtime when/how to\n> -perform those measurements. Maybe that's a bit overkill.\n> -\n> ----\n> -\n> 8. DebayerCpu cleanups\n> \n> > >> class DebayerCpu : public Debayer, public Object\n> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n> index 85e31e98e..27113c6a5 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats, const GlobalConfigurat\n> \tenableInputMemcpy_ =\n> \t\tconfiguration.option(\"pipelines/simple/copy_input_buffer\").value_or(true);\n> \n> +\tskipBeforeMeasure_ = configuration.option(\n> +\t\t\t\t\t\t \"pipelines/simple/measure/skip\")\n> +\t\t\t\t .value_or(skipBeforeMeasure_);\n> +\tframesToMeasure_ = configuration.option(\n> +\t\t\t\t\t\t\"pipelines/simple/measure/number\")\n> +\t\t\t\t .value_or(framesToMeasure_);\n> +\n\nRegarding this and the previous patch, I think the `DebayerCpu` class should\nnot access the configuration like this.\n\nA different approach could be creating e.g.\n\n struct DebayerCpu::Params {\n bool inputMemcpy = true;\n struct {\n unsigned int skip = 0, frames = 0;\n } measure;\n };\n\nand passing that as argument to the constructor, and letting the caller extract the\nsettings from wherever it wants.\n\nAlternatively, and I think this is generally useful is other contexts, I would\nadd a `Configuration` type that contains a single `const YamlObject&`, essentially\nrepresenting a subtree of the configuration. And then this type would have the\noption(), listOption, envOption(), etc. functions. In addition it would have a\n`Configuration operator[](path)` or `Configuration child(path)` that returns a\n`Configuration` object pointing to a child. This would make it possible to not\nhave to consider the entire structure of the configuration from the root.\nEverything could deal with just its own dictionary, etc. This could then be\npassed to the constructors of various objects (pipeline handler, ipa, etc.)\n\nThoughts? I am not proposing these changes be made just now, but I think\nsomething like this could be useful in the future.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \t/* Initialize color lookup tables */\n> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n> \t\tred_[i] = green_[i] = blue_[i] = i;\n> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n> \t}\n> \n> -\tmeasuredFrames_ = 0;\n> +\tencounteredFrames_ = 0;\n> \tframeProcessTime_ = 0;\n> \n> \treturn 0;\n> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> {\n> \ttimespec frameStartTime;\n> \n> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n> +\tbool measure = framesToMeasure_ > 0 &&\n> +\t\t encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n> +\t\t ++encounteredFrames_ > skipBeforeMeasure_;\n> +\tif (measure) {\n> \t\tframeStartTime = {};\n> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n> \t}\n> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n> \tdmaSyncers.clear();\n> \n> \t/* Measure before emitting signals */\n> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n> -\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n> +\tif (measure) {\n> \t\ttimespec frameEndTime = {};\n> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n> \t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n> -\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n> -\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n> -\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n> +\t\tif (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n> \t\t\tLOG(Debayer, Info)\n> -\t\t\t\t<< \"Processed \" << measuredFrames\n> +\t\t\t\t<< \"Processed \" << framesToMeasure_\n> \t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n> +\t\t\t\t<< frameProcessTime_ / (1000 * framesToMeasure_)\n> \t\t\t\t<< \" us/frame\";\n> \t\t}\n> \t}\n> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n> index 2f35aa18b..9d343e464 100644\n> --- a/src/libcamera/software_isp/debayer_cpu.h\n> +++ b/src/libcamera/software_isp/debayer_cpu.h\n> @@ -161,11 +161,10 @@ private:\n> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n> \tbool enableInputMemcpy_;\n> \tbool swapRedBlueGains_;\n> -\tunsigned int measuredFrames_;\n> +\tunsigned int encounteredFrames_;\n> \tint64_t frameProcessTime_;\n> -\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n> -\tstatic constexpr unsigned int kFramesToSkip = 30;\n> -\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n> +\tunsigned int skipBeforeMeasure_ = 30;\n> +\tunsigned int framesToMeasure_ = 30;\n> };\n> \n> } /* namespace libcamera */","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 5FA18C3237\n\tfor ;\n\tFri, 27 Jun 2025 14:01:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8532C68DFE;\n\tFri, 27 Jun 2025 16:01:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D7D3E68DE5\n\tfor ;\n\tFri, 27 Jun 2025 16:00:52 +0200 (CEST)","from [192.168.33.12] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 31A0373B;\n\tFri, 27 Jun 2025 16:00:33 +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=\"rhWbjdwX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751032833;\n\tbh=MAfYXQcv/Bnp5c/nP5aH0ZzwIv2URi5WY5WLmru+SNs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=rhWbjdwXxrP3RTrQNXQ8zPz3I+NlZn6WzMKqqzHRBaMUdHTTUWfyn+hqCq0+sD1pA\n\tOVCWVmbOAA6QvChptwJUz30srbhV6mHHzuQHryuI4IaPYGDnLGy+lbfgHJZFaUSU2j\n\twloixJyIIqMpAPSdObw87yiKmLUjm1kNTV33oY7s=","Message-ID":"<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com>","Date":"Fri, 27 Jun 2025 16:00:44 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Cc":"Kieran Bingham ,\n\tLaurent Pinchart ","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-11-mzamazal@redhat.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250624083612.27230-11-mzamazal@redhat.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":34719,"web_url":"https://patchwork.libcamera.org/comment/34719/","msgid":"<85ldpcvp2x.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-06-27T21:42:30","subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Barnabás,\n\nBarnabás Pőcze writes:\n\n> Hi\n>\n> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n>> Software ISP performs performance measurement on certain part of initial\n>> frames. Let's make this range configurable.\n>> For this purpose, this patch introduces new configuration options\n>> pipelines.simple.measure.skip and pipelines.simple.measure.number.\n>> Setting the latter one to 0 disables the measurement.\n>> Instead of the last frame, the class member and its configuration\n>> specify the number of frames to measure. This is easier to use for\n>> users and doesn't require to adjust two configuration parameters when\n>> the number of the initially skipped frames is changed.\n>> The patch also changes the names of the class members to make them more\n>> accurate.\n>> Completes software ISP TODO #7.\n>> Signed-off-by: Milan Zamazal \n>> ---\n>> Documentation/runtime_configuration.rst | 6 ++++++\n>> src/libcamera/software_isp/TODO | 25 ----------------------\n>> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------\n>> src/libcamera/software_isp/debayer_cpu.h | 7 +++---\n>> 4 files changed, 25 insertions(+), 38 deletions(-)\n>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n>> index f4e86ae24..de74fa732 100644\n>> --- a/Documentation/runtime_configuration.rst\n>> +++ b/Documentation/runtime_configuration.rst\n>> @@ -52,6 +52,9 @@ file structure:\n>> ...\n>> simple:\n>> copy_input_buffer: # true/false\n>> + measure:\n>> + skip: # non-negative integer, frames to skip initially\n>> + number: # non-negative integer, frames to measure\n>> supported_devices:\n>> - driver: # driver name, e.g. `mxc-isi`\n>> software_isp: # true/false\n>> @@ -87,6 +90,9 @@ Configuration file example\n>> min_total_unicam_buffers: 2\n>> simple:\n>> copy_input_buffer: false\n>> + measure:\n>> + skip: 50\n>> + number: 30\n>> supported_devices:\n>> - driver: mxc-isi\n>> software_isp: true\n>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n>> index 2c919f442..f19e15ae2 100644\n>> --- a/src/libcamera/software_isp/TODO\n>> +++ b/src/libcamera/software_isp/TODO\n>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.\n>> ---\n>> -7. Performance measurement configuration\n>> -\n>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>> -> /* Measure before emitting signals */\n>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> -> \ttimespec frameEndTime = {};\n>> -> \tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>> -> \tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>> -> \tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>> -> \t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>> -> \t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>> -> \t\tLOG(Debayer, Info)\n>> -> \t\t\t<< \"Processed \" << measuredFrames\n>> -> \t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> -> \t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>> -> \t\t\t<< \" us/frame\";\n>> -> \t}\n>> -> }\n>> -\n>> -I wonder if there would be a way to control at runtime when/how to\n>> -perform those measurements. Maybe that's a bit overkill.\n>> -\n>> ----\n>> -\n>> 8. DebayerCpu cleanups\n>> > >> class DebayerCpu : public Debayer, public Object\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>> index 85e31e98e..27113c6a5 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats, const GlobalConfigurat\n>> \tenableInputMemcpy_ =\n>> \t\tconfiguration.option(\"pipelines/simple/copy_input_buffer\").value_or(true);\n>> +\tskipBeforeMeasure_ = configuration.option(\n>> +\t\t\t\t\t\t \"pipelines/simple/measure/skip\")\n>> +\t\t\t\t .value_or(skipBeforeMeasure_);\n>> +\tframesToMeasure_ = configuration.option(\n>> +\t\t\t\t\t\t\"pipelines/simple/measure/number\")\n>> +\t\t\t\t .value_or(framesToMeasure_);\n>> +\n>\n> Regarding this and the previous patch, I think the `DebayerCpu` class should\n> not access the configuration like this.\n>\n> A different approach could be creating e.g.\n>\n> struct DebayerCpu::Params {\n> bool inputMemcpy = true;\n> struct {\n> unsigned int skip = 0, frames = 0;\n> } measure;\n> };\n>\n> and passing that as argument to the constructor, and letting the caller extract the\n> settings from wherever it wants.\n\nOK, makes sense.\n\n> Alternatively, and I think this is generally useful is other contexts, I would\n> add a `Configuration` type that contains a single `const YamlObject&`, essentially\n> representing a subtree of the configuration. And then this type would have the\n> option(), listOption, envOption(), etc. functions. In addition it would have a\n> `Configuration operator[](path)` or `Configuration child(path)` that returns a\n> `Configuration` object pointing to a child. This would make it possible to not\n> have to consider the entire structure of the configuration from the root.\n> Everything could deal with just its own dictionary, etc. This could then be\n> passed to the constructors of various objects (pipeline handler, ipa, etc.)\n> \n> Thoughts?\n\nWhat would be the primary motivation? I can think about:\n\n- Not exposing the whole configuration to proprietary code.\n\n- Making the configuration lookup relative to not having update it on\n contingent restructuring (but since such a thing is an annoyance to\n users above all and it still requires changes somewhere at callers,\n this doesn't look like any important).\n\nAgainst:\n\n- Perhaps just unnecessary complication.\n\n- It obfuscates in the code where's the given configuration option in\n the file.\n\n> I am not proposing these changes be made just now, but I think\n> something like this could be useful in the future.\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n>\n>> \t/* Initialize color lookup tables */\n>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>> \t\tred_[i] = green_[i] = blue_[i] = i;\n>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n>> \t}\n>> -\tmeasuredFrames_ = 0;\n>> +\tencounteredFrames_ = 0;\n>> \tframeProcessTime_ = 0;\n>> \treturn 0;\n>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> {\n>> \ttimespec frameStartTime;\n>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n>> +\tbool measure = framesToMeasure_ > 0 &&\n>> +\t\t encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n>> +\t\t ++encounteredFrames_ > skipBeforeMeasure_;\n>> +\tif (measure) {\n>> \t\tframeStartTime = {};\n>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>> \t}\n>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>> \tdmaSyncers.clear();\n>> \t/* Measure before emitting signals */\n>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>> -\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>> +\tif (measure) {\n>> \t\ttimespec frameEndTime = {};\n>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>> \t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>> -\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>> -\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>> -\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>> +\t\tif (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n>> \t\t\tLOG(Debayer, Info)\n>> -\t\t\t\t<< \"Processed \" << measuredFrames\n>> +\t\t\t\t<< \"Processed \" << framesToMeasure_\n>> \t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>> +\t\t\t\t<< frameProcessTime_ / (1000 * framesToMeasure_)\n>> \t\t\t\t<< \" us/frame\";\n>> \t\t}\n>> \t}\n>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>> index 2f35aa18b..9d343e464 100644\n>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>> @@ -161,11 +161,10 @@ private:\n>> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>> \tbool enableInputMemcpy_;\n>> \tbool swapRedBlueGains_;\n>> -\tunsigned int measuredFrames_;\n>> +\tunsigned int encounteredFrames_;\n>> \tint64_t frameProcessTime_;\n>> -\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n>> -\tstatic constexpr unsigned int kFramesToSkip = 30;\n>> -\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n>> +\tunsigned int skipBeforeMeasure_ = 30;\n>> +\tunsigned int framesToMeasure_ = 30;\n>> };\n>> } /* namespace libcamera */","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 127FEC3237\n\tfor ;\n\tFri, 27 Jun 2025 21:42:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5C7668E04;\n\tFri, 27 Jun 2025 23:42:36 +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 A167168DE5\n\tfor ;\n\tFri, 27 Jun 2025 23:42:35 +0200 (CEST)","from mail-wm1-f71.google.com (mail-wm1-f71.google.com\n\t[209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-623-PsogPVlvPq-FSaw7z-3JoA-1; Fri, 27 Jun 2025 17:42:33 -0400","by mail-wm1-f71.google.com with SMTP id\n\t5b1f17b1804b1-450d244bfabso1958565e9.0\n\tfor ;\n\tFri, 27 Jun 2025 14:42:33 -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-45380d0f15esm79912935e9.1.2025.06.27.14.42.30\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 27 Jun 2025 14:42:30 -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=\"PGtBslbz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1751060554;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=Vv+gTtLyGiSCYrcR2a5DZNQsFBP0UnsbqdxZaBgt0k4=;\n\tb=PGtBslbz1Pv4E2m4zmNE2zbGmd3evTRW9MhWIXkgq9YEhTNUCan0FUfmc33GlpIC2gO/FZ\n\tXdFwz5mW0sEMUDc6A3N9bmk1cjOF8rhTinrkkxdM26fYPsxnEtmDbNIbJGHbI6QHusIjsG\n\tJi0f2buEOaQeucGoD/RmX2asNWNNSiA=","X-MC-Unique":"PsogPVlvPq-FSaw7z-3JoA-1","X-Mimecast-MFC-AGG-ID":"PsogPVlvPq-FSaw7z-3JoA_1751060552","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1751060552; x=1751665352;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=9tIaQOzLJBG72qmPsJo8hWmRuur/BPTQE576s8IYySk=;\n\tb=mfvMh4RyahfaF8YBbqol10c5n+odw1hPiSnTaxrRSFPbY5IozuTBOPDxI+1/dZLjq4\n\t5oV5ydafTFlTnSmKcVGKq3qB6lk1OsVVMiaR3+TN5E+sCGpkXO7D56tYXTIlK5XGYFSs\n\tLoLuWBlQXy1x8bmRhSVPZeKgrxB+ZiYiMJOcwzFqjX+a0MbuIU+K+JGLrIUreemv2CC5\n\tD+x/nchD9JEUwkOJkTDp/Wa53cBKVR8bEu3LrIp4365827ZewC7HiNS3H1+gAY7Rca2u\n\tFewsV9coQN1gXiIrrz4wuEOr6twF02PUyRtYOfnqbxszMW2Ntgxxs28XZWmETIlDezi1\n\tPqRQ==","X-Gm-Message-State":"AOJu0YykIBXMF5R71cuHgjRUtcWRIw3q+2JUd6YMRBzZamtbSKCA9dIb\n\tIzxS9mJzg64AB2ubWT9zfDym5hDxNEG6fUXS4U5adEjT5CEHtG0mUXp7+GNW8IW1SxpkuXvcDyE\n\tVlAVjqe/4EogIbYlA4wI78lap02+hqmySwUvNepE4Ymlmk0Yq5J6fOLVkl8teFFm08Gsqn+DT3E\n\t8=","X-Gm-Gg":"ASbGnct5wj46JAOpOJ+r2r9JODAoZCX/S5/dBFP7DY2CwwggyqxfFResrVYY82oEOvH\n\t/CVadUNZZsC2n9qjwwgVfa3Fcpk8OIMdD9FdPhtP8r8W2egvJDq+LsqNMiBxyphB2DIH6bucndO\n\tTjrbhnmvnQNfNwPt4GnLpI6S8e1Mq1vGCJ+8YwOzMOVXgj/gyQRbHpTcHaXnSGA1NiJjM/QK94F\n\t/bL52YcS1vzvtUbRasoexJ+Hk9VyjfdsU237SeV69GFHZnpoXbDSEcll+/OKhfuDwFBz7ft8+F6\n\tL58P+WMoRijkLyg0vOq9eK6NHXG5m8gvMG/rhv7F32itgOExEsAlZbrT5YcPrz4LKOPtZviNaGQ\n\t=","X-Received":["by 2002:a05:600c:4fc2:b0:43c:fda5:41e9 with SMTP id\n\t5b1f17b1804b1-4538eea3783mr59996405e9.31.1751060551802; \n\tFri, 27 Jun 2025 14:42:31 -0700 (PDT)","by 2002:a05:600c:4fc2:b0:43c:fda5:41e9 with SMTP id\n\t5b1f17b1804b1-4538eea3783mr59996255e9.31.1751060551296; \n\tFri, 27 Jun 2025 14:42:31 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFAtY4qYsLtSOUKH3qD8wj4WIiHfISXYC0N4AQYawvqlQ6F2FBqHGh0k60aaB0obGUAtaQDyQ==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","In-Reply-To":"<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com> (\n\t=?utf-8?b?IkJhcm5hYsOhcyBQxZFjemUiJ3M=?= message of \"Fri,\n\t27 Jun 2025 16:00:44 +0200\")","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-11-mzamazal@redhat.com>\n\t<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com>","Date":"Fri, 27 Jun 2025 23:42:30 +0200","Message-ID":"<85ldpcvp2x.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":"BXS4aHkYZivHX6dDzsUhAJzeILoMzGaZYO_5rC926pQ_1751060552","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":34756,"web_url":"https://patchwork.libcamera.org/comment/34756/","msgid":"<85plek4899.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-07-01T08:37:06","subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\n> Hi Barnabás,\n>\n> Barnabás Pőcze writes:\n>\n>> Hi\n>>\n>> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n>>> Software ISP performs performance measurement on certain part of initial\n>>> frames. Let's make this range configurable.\n>>> For this purpose, this patch introduces new configuration options\n>>> pipelines.simple.measure.skip and pipelines.simple.measure.number.\n>>> Setting the latter one to 0 disables the measurement.\n>>> Instead of the last frame, the class member and its configuration\n>>> specify the number of frames to measure. This is easier to use for\n>>> users and doesn't require to adjust two configuration parameters when\n>>> the number of the initially skipped frames is changed.\n>>> The patch also changes the names of the class members to make them more\n>>> accurate.\n>>> Completes software ISP TODO #7.\n>>> Signed-off-by: Milan Zamazal \n>>> ---\n>>> Documentation/runtime_configuration.rst | 6 ++++++\n>>> src/libcamera/software_isp/TODO | 25 ----------------------\n>>> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------\n>>> src/libcamera/software_isp/debayer_cpu.h | 7 +++---\n>>> 4 files changed, 25 insertions(+), 38 deletions(-)\n>>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n>>> index f4e86ae24..de74fa732 100644\n>>> --- a/Documentation/runtime_configuration.rst\n>>> +++ b/Documentation/runtime_configuration.rst\n>>> @@ -52,6 +52,9 @@ file structure:\n>>> ...\n>>> simple:\n>>> copy_input_buffer: # true/false\n>>> + measure:\n>>> + skip: # non-negative integer, frames to skip initially\n>>> + number: # non-negative integer, frames to measure\n>>> supported_devices:\n>>> - driver: # driver name, e.g. `mxc-isi`\n>>> software_isp: # true/false\n>>> @@ -87,6 +90,9 @@ Configuration file example\n>>> min_total_unicam_buffers: 2\n>>> simple:\n>>> copy_input_buffer: false\n>>> + measure:\n>>> + skip: 50\n>>> + number: 30\n>>> supported_devices:\n>>> - driver: mxc-isi\n>>> software_isp: true\n>>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n>>> index 2c919f442..f19e15ae2 100644\n>>> --- a/src/libcamera/software_isp/TODO\n>>> +++ b/src/libcamera/software_isp/TODO\n>>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.\n>>> ---\n>>> -7. Performance measurement configuration\n>>> -\n>>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>>> -> /* Measure before emitting signals */\n>>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>>> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>>> -> \ttimespec frameEndTime = {};\n>>> -> \tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>>> -> \tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>>> -> \tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>>> -> \t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>>> -> \t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>>> -> \t\tLOG(Debayer, Info)\n>>> -> \t\t\t<< \"Processed \" << measuredFrames\n>>> -> \t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>>> -> \t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>>> -> \t\t\t<< \" us/frame\";\n>>> -> \t}\n>>> -> }\n>>> -\n>>> -I wonder if there would be a way to control at runtime when/how to\n>>> -perform those measurements. Maybe that's a bit overkill.\n>>> -\n>>> ----\n>>> -\n>>> 8. DebayerCpu cleanups\n>>> > >> class DebayerCpu : public Debayer, public Object\n>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> index 85e31e98e..27113c6a5 100644\n>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats, const GlobalConfigurat\n>>> \tenableInputMemcpy_ =\n>>> \t\tconfiguration.option(\"pipelines/simple/copy_input_buffer\").value_or(true);\n>>> +\tskipBeforeMeasure_ = configuration.option(\n>>> +\t\t\t\t\t\t \"pipelines/simple/measure/skip\")\n>>> +\t\t\t\t .value_or(skipBeforeMeasure_);\n>>> +\tframesToMeasure_ = configuration.option(\n>>> +\t\t\t\t\t\t\"pipelines/simple/measure/number\")\n>>> +\t\t\t\t .value_or(framesToMeasure_);\n>>> +\n>>\n>> Regarding this and the previous patch, I think the `DebayerCpu` class should\n>> not access the configuration like this.\n>>\n>> A different approach could be creating e.g.\n>>\n>> struct DebayerCpu::Params {\n>> bool inputMemcpy = true;\n>> struct {\n>> unsigned int skip = 0, frames = 0;\n>> } measure;\n>> };\n>>\n>> and passing that as argument to the constructor, and letting the caller extract the\n>> settings from wherever it wants.\n>\n> OK, makes sense.\n\nOn the second thought, it doesn't. Why should the caller decide what\nthe given debayering class needs from the configuration rather than the\nclass itself? Note there will be different debayering classes (CPU,\nGPU, maybe others in future), possibly with their own specific\nconfiguration options.\n\n>> Alternatively, and I think this is generally useful is other contexts, I would\n>> add a `Configuration` type that contains a single `const YamlObject&`, essentially\n>> representing a subtree of the configuration. And then this type would have the\n>> option(), listOption, envOption(), etc. functions. In addition it would have a\n>> `Configuration operator[](path)` or `Configuration child(path)` that returns a\n>> `Configuration` object pointing to a child. This would make it possible to not\n>> have to consider the entire structure of the configuration from the root.\n>> Everything could deal with just its own dictionary, etc. This could then be\n>> passed to the constructors of various objects (pipeline handler, ipa, etc.)\n>> \n>> Thoughts?\n>\n> What would be the primary motivation? I can think about:\n>\n> - Not exposing the whole configuration to proprietary code.\n>\n> - Making the configuration lookup relative to not having update it on\n> contingent restructuring (but since such a thing is an annoyance to\n> users above all and it still requires changes somewhere at callers,\n> this doesn't look like any important).\n>\n> Against:\n>\n> - Perhaps just unnecessary complication.\n>\n> - It obfuscates in the code where's the given configuration option in\n> the file.\n>\n>> I am not proposing these changes be made just now, but I think\n>> something like this could be useful in the future.\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>\n>>> \t/* Initialize color lookup tables */\n>>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>>> \t\tred_[i] = green_[i] = blue_[i] = i;\n>>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>>> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n>>> \t}\n>>> -\tmeasuredFrames_ = 0;\n>>> +\tencounteredFrames_ = 0;\n>>> \tframeProcessTime_ = 0;\n>>> \treturn 0;\n>>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>>> {\n>>> \ttimespec frameStartTime;\n>>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n>>> +\tbool measure = framesToMeasure_ > 0 &&\n>>> +\t\t encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n>>> +\t\t ++encounteredFrames_ > skipBeforeMeasure_;\n>>> +\tif (measure) {\n>>> \t\tframeStartTime = {};\n>>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>>> \t}\n>>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>>> \tdmaSyncers.clear();\n>>> \t/* Measure before emitting signals */\n>>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>>> -\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>>> +\tif (measure) {\n>>> \t\ttimespec frameEndTime = {};\n>>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>>> \t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>>> -\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>>> -\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>>> -\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>>> +\t\tif (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n>>> \t\t\tLOG(Debayer, Info)\n>>> -\t\t\t\t<< \"Processed \" << measuredFrames\n>>> +\t\t\t\t<< \"Processed \" << framesToMeasure_\n>>> \t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>>> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>>> +\t\t\t\t<< frameProcessTime_ / (1000 * framesToMeasure_)\n>>> \t\t\t\t<< \" us/frame\";\n>>> \t\t}\n>>> \t}\n>>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>>> index 2f35aa18b..9d343e464 100644\n>>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>>> @@ -161,11 +161,10 @@ private:\n>>> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>>> \tbool enableInputMemcpy_;\n>>> \tbool swapRedBlueGains_;\n>>> -\tunsigned int measuredFrames_;\n>>> +\tunsigned int encounteredFrames_;\n>>> \tint64_t frameProcessTime_;\n>>> -\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n>>> -\tstatic constexpr unsigned int kFramesToSkip = 30;\n>>> -\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n>>> +\tunsigned int skipBeforeMeasure_ = 30;\n>>> +\tunsigned int framesToMeasure_ = 30;\n>>> };\n>>> } /* namespace libcamera */","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 B93FCBDCBF\n\tfor ;\n\tTue, 1 Jul 2025 08:37:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F56768E14;\n\tTue, 1 Jul 2025 10:37:14 +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 A71C561528\n\tfor ;\n\tTue, 1 Jul 2025 10:37:12 +0200 (CEST)","from mail-wr1-f72.google.com (mail-wr1-f72.google.com\n\t[209.85.221.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-542-8ZRuY9oTPCmHxBiUSolaKw-1; Tue, 01 Jul 2025 04:37:10 -0400","by mail-wr1-f72.google.com with SMTP id\n\tffacd0b85a97d-3a4f7ebfd00so2692401f8f.2\n\tfor ;\n\tTue, 01 Jul 2025 01:37:09 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb ([85.93.96.130])\n\tby smtp.gmail.com with ESMTPSA id\n\tffacd0b85a97d-3a88c7fa54dsm12392861f8f.23.2025.07.01.01.37.07\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 01 Jul 2025 01:37:07 -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=\"D5HIT1uD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1751359031;\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\tcontent-transfer-encoding:content-transfer-encoding:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=T5bvg8RQhO0JnY9jRYd5RLatpT5SeOzYru155lgRopg=;\n\tb=D5HIT1uDcA4gvCOhdnUgW5ML18dteP/HaTDRPIgn8UbII6FE2CQsYNGDIt7+BPvLi7mLpV\n\tRM2zqE0uUopv24ciB7uaGxYe4Sp5MOVCT1AUIY9P/i6q530Cuc0v4JApReA8hAJRSrsEPu\n\tCsY+Ng7JHL8RA7KycwaedSPUIuJ95RA=","X-MC-Unique":"8ZRuY9oTPCmHxBiUSolaKw-1","X-Mimecast-MFC-AGG-ID":"8ZRuY9oTPCmHxBiUSolaKw_1751359029","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1751359029; x=1751963829;\n\th=content-transfer-encoding:mime-version:user-agent:message-id:date\n\t:references:in-reply-to:subject:cc:to:from:x-gm-message-state:from\n\t:to:cc:subject:date:message-id:reply-to;\n\tbh=n3uROXyU90d75pCxFj+CDCbpDV98sBqVz0Rl/Yea2xM=;\n\tb=KVizF7wxEZlaYlMmykp3+qE00INvPwFXr0CLuqlgJtlw0XqqYKdpxwSG2+rWshbzON\n\tJq8FNDE8dauK9cN/0OJZg8h/gPDcJ0uVJyD9fmVL+HGUXv7aTLVT1YWAvUzijag+0Nrr\n\tlnVJFfxh80al8KJOdNKPX4hM7giByOCGKpc/kCEwOjc9Ig3B15Ahb2RLpHQT3FAXNpgO\n\tLDfOLrMk5LbBVAix4Ri//zYMmQLwBTCSfyJ5govqbuOC/SZpiwK3MloBVixPTOnprNkY\n\tnlmKwwOTnt4mJ1rWhBNQ4pbleBCcHr20gWbflzrYDGJ+vj4BImYx9z2ThhPLMSdnSyAt\n\tb6pg==","X-Gm-Message-State":"AOJu0YzOJpAK7iAQ10XBd4xXG/XOjfaaRAzmrR45bb22AibFndmsDRXZ\n\tz/HiVjbAAyQqEj1QpFmTmnBhLZOzeCtdERlowNm+BQxIGLvvWtS//XulQykwzRqGV6dmJpnAQ5w\n\t5hQ8shnBoGMJ5vWhkvD6xEVCvIXSlXVUuGKCGPeNf2Kc2jzmdf6PAFf3/4nSNthoHxb4JIeyT1E\n\tM=","X-Gm-Gg":"ASbGncvGNQ/AeCqXtyzJE9w2wiOu7swCR01tWK1HSItNMDGbJuUDEI3JiEVUaULGh3P\n\tYw7erBeEnIAEIbwYs5be69Z72RP48e9C6U0wZLdzDLZ25qZlI5CV+c1kyaFOE7KrbwTblbJOdgE\n\tyQH0SrhgGK9kL1UO9yU+yv3y03W3tP/ApYw20TbbfWb+WtsDjYLRpHZ5EteDVee3R30ZoRRotg8\n\tMq/mxHwEzfkav3pr+d2pNaYtDutpiwa0ZD+FWspVrnwjlgYbC6yPv9vlLSxTJks2EK2ennHBsBc\n\tdViZrFKRea71b6qcKcjXbL9qe2VIaFBlc6BK0fVx1fBs2GQ=","X-Received":["by 2002:a05:6000:250a:b0:3a5:1f2:68f3 with SMTP id\n\tffacd0b85a97d-3a90b6dfac9mr12016210f8f.46.1751359028633; \n\tTue, 01 Jul 2025 01:37:08 -0700 (PDT)","by 2002:a05:6000:250a:b0:3a5:1f2:68f3 with SMTP id\n\tffacd0b85a97d-3a90b6dfac9mr12016173f8f.46.1751359028042; \n\tTue, 01 Jul 2025 01:37:08 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IF/4g8tdFc5wBpiEDMyVKiAzbHN0Sq4zqCYtG5DD+HVgzN1J1nK0TCyIX+GNB3Sz6vbMY8Hhw==","From":"Milan Zamazal ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t, Laurent Pinchart\n\t","Subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","In-Reply-To":"<85ldpcvp2x.fsf@mzamazal-thinkpadp1gen7.tpbc.csb> (Milan\n\tZamazal's message of \"Fri, 27 Jun 2025 23:42:30 +0200\")","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-11-mzamazal@redhat.com>\n\t<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com>\n\t<85ldpcvp2x.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Date":"Tue, 01 Jul 2025 10:37:06 +0200","Message-ID":"<85plek4899.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":"W-1BX_Y5WoBh-HRb7KxSyjOm442lsaGmw0M0xCLtC08_1751359029","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":34757,"web_url":"https://patchwork.libcamera.org/comment/34757/","msgid":"<60de9adb-f406-4d72-b274-edc5d4dc96e5@ideasonboard.com>","date":"2025-07-01T09:05:13","subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 07. 01. 10:37 keltezéssel, Milan Zamazal írta:\n> Milan Zamazal writes:\n> \n>> Hi Barnabás,\n>>\n>> Barnabás Pőcze writes:\n>>\n>>> Hi\n>>>\n>>> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:\n>>>> Software ISP performs performance measurement on certain part of initial\n>>>> frames. Let's make this range configurable.\n>>>> For this purpose, this patch introduces new configuration options\n>>>> pipelines.simple.measure.skip and pipelines.simple.measure.number.\n>>>> Setting the latter one to 0 disables the measurement.\n>>>> Instead of the last frame, the class member and its configuration\n>>>> specify the number of frames to measure. This is easier to use for\n>>>> users and doesn't require to adjust two configuration parameters when\n>>>> the number of the initially skipped frames is changed.\n>>>> The patch also changes the names of the class members to make them more\n>>>> accurate.\n>>>> Completes software ISP TODO #7.\n>>>> Signed-off-by: Milan Zamazal \n>>>> ---\n>>>> Documentation/runtime_configuration.rst | 6 ++++++\n>>>> src/libcamera/software_isp/TODO | 25 ----------------------\n>>>> src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------\n>>>> src/libcamera/software_isp/debayer_cpu.h | 7 +++---\n>>>> 4 files changed, 25 insertions(+), 38 deletions(-)\n>>>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst\n>>>> index f4e86ae24..de74fa732 100644\n>>>> --- a/Documentation/runtime_configuration.rst\n>>>> +++ b/Documentation/runtime_configuration.rst\n>>>> @@ -52,6 +52,9 @@ file structure:\n>>>> ...\n>>>> simple:\n>>>> copy_input_buffer: # true/false\n>>>> + measure:\n>>>> + skip: # non-negative integer, frames to skip initially\n>>>> + number: # non-negative integer, frames to measure\n>>>> supported_devices:\n>>>> - driver: # driver name, e.g. `mxc-isi`\n>>>> software_isp: # true/false\n>>>> @@ -87,6 +90,9 @@ Configuration file example\n>>>> min_total_unicam_buffers: 2\n>>>> simple:\n>>>> copy_input_buffer: false\n>>>> + measure:\n>>>> + skip: 50\n>>>> + number: 30\n>>>> supported_devices:\n>>>> - driver: mxc-isi\n>>>> software_isp: true\n>>>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO\n>>>> index 2c919f442..f19e15ae2 100644\n>>>> --- a/src/libcamera/software_isp/TODO\n>>>> +++ b/src/libcamera/software_isp/TODO\n>>>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.\n>>>> ---\n>>>> -7. Performance measurement configuration\n>>>> -\n>>>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)\n>>>> -> /* Measure before emitting signals */\n>>>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>>>> -> ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>>>> -> \ttimespec frameEndTime = {};\n>>>> -> \tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>>>> -> \tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>>>> -> \tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>>>> -> \t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>>>> -> \t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>>>> -> \t\tLOG(Debayer, Info)\n>>>> -> \t\t\t<< \"Processed \" << measuredFrames\n>>>> -> \t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>>>> -> \t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>>>> -> \t\t\t<< \" us/frame\";\n>>>> -> \t}\n>>>> -> }\n>>>> -\n>>>> -I wonder if there would be a way to control at runtime when/how to\n>>>> -perform those measurements. Maybe that's a bit overkill.\n>>>> -\n>>>> ----\n>>>> -\n>>>> 8. DebayerCpu cleanups\n>>>> > >> class DebayerCpu : public Debayer, public Object\n>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> index 85e31e98e..27113c6a5 100644\n>>>> --- a/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp\n>>>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr stats, const GlobalConfigurat\n>>>> \tenableInputMemcpy_ =\n>>>> \t\tconfiguration.option(\"pipelines/simple/copy_input_buffer\").value_or(true);\n>>>> +\tskipBeforeMeasure_ = configuration.option(\n>>>> +\t\t\t\t\t\t \"pipelines/simple/measure/skip\")\n>>>> +\t\t\t\t .value_or(skipBeforeMeasure_);\n>>>> +\tframesToMeasure_ = configuration.option(\n>>>> +\t\t\t\t\t\t\"pipelines/simple/measure/number\")\n>>>> +\t\t\t\t .value_or(framesToMeasure_);\n>>>> +\n>>>\n>>> Regarding this and the previous patch, I think the `DebayerCpu` class should\n>>> not access the configuration like this.\n>>>\n>>> A different approach could be creating e.g.\n>>>\n>>> struct DebayerCpu::Params {\n>>> bool inputMemcpy = true;\n>>> struct {\n>>> unsigned int skip = 0, frames = 0;\n>>> } measure;\n>>> };\n>>>\n>>> and passing that as argument to the constructor, and letting the caller extract the\n>>> settings from wherever it wants.\n>>\n>> OK, makes sense.\n> \n> On the second thought, it doesn't. Why should the caller decide what\n> the given debayering class needs from the configuration rather than the\n> class itself? Note there will be different debayering classes (CPU,\n> GPU, maybe others in future), possibly with their own specific\n> configuration options.\n\nIt's still the class deciding, no? Just in a different, one might argue, more explicit way.\nIn any case, I think the point is that you have to draw a line somewhere because you can't\nhave everything reach into a magic object for parameters if you want reusable components. When\nI wrote this, I felt that maybe DebayerCpu is on the other side of the line and it shouldn't\naccess the configuration directly. In any case, I believe the this version is fine, so don't\nchange it unless you want to.\n\n\n> \n>>> Alternatively, and I think this is generally useful is other contexts, I would\n>>> add a `Configuration` type that contains a single `const YamlObject&`, essentially\n>>> representing a subtree of the configuration. And then this type would have the\n>>> option(), listOption, envOption(), etc. functions. In addition it would have a\n>>> `Configuration operator[](path)` or `Configuration child(path)` that returns a\n>>> `Configuration` object pointing to a child. This would make it possible to not\n>>> have to consider the entire structure of the configuration from the root.\n>>> Everything could deal with just its own dictionary, etc. This could then be\n>>> passed to the constructors of various objects (pipeline handler, ipa, etc.)\n>>>\n>>> Thoughts?\n>>\n>> What would be the primary motivation? I can think about:\n>>\n>> - Not exposing the whole configuration to proprietary code.\n>>\n>> - Making the configuration lookup relative to not having update it on\n>> contingent restructuring (but since such a thing is an annoyance to\n>> users above all and it still requires changes somewhere at callers,\n>> this doesn't look like any important).\n\nThis is the primary motivation for me. Having relative paths makes reusing\nthings simpler, e.g. if you want to configure multiple instances in different\nparts of the configuration file. Admittedly, I don't see a use-case for this\ncurrently.\n\n\n>>\n>> Against:\n>>\n>> - Perhaps just unnecessary complication.\n>>\n>> - It obfuscates in the code where's the given configuration option in\n>> the file.\n\nThat's probably the strongest argument against in my opinion. But if you want\nto reuse the same component in different parts, then you have to give up some\nif that clarity either way in my opinion.\n\n\nRegards,\nBarnabás Pőcze\n\n\n>>\n>>> I am not proposing these changes be made just now, but I think\n>>> something like this could be useful in the future.\n>>>\n>>>\n>>> Regards,\n>>> Barnabás Pőcze\n>>>\n>>>\n>>>> \t/* Initialize color lookup tables */\n>>>> \tfor (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {\n>>>> \t\tred_[i] = green_[i] = blue_[i] = i;\n>>>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,\n>>>> \t\t\tlineBuffers_[i].resize(lineBufferLength_);\n>>>> \t}\n>>>> -\tmeasuredFrames_ = 0;\n>>>> +\tencounteredFrames_ = 0;\n>>>> \tframeProcessTime_ = 0;\n>>>> \treturn 0;\n>>>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>>>> {\n>>>> \ttimespec frameStartTime;\n>>>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {\n>>>> +\tbool measure = framesToMeasure_ > 0 &&\n>>>> +\t\t encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&\n>>>> +\t\t ++encounteredFrames_ > skipBeforeMeasure_;\n>>>> +\tif (measure) {\n>>>> \t\tframeStartTime = {};\n>>>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);\n>>>> \t}\n>>>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output\n>>>> \tdmaSyncers.clear();\n>>>> \t/* Measure before emitting signals */\n>>>> -\tif (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&\n>>>> -\t ++measuredFrames_ > DebayerCpu::kFramesToSkip) {\n>>>> +\tif (measure) {\n>>>> \t\ttimespec frameEndTime = {};\n>>>> \t\tclock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);\n>>>> \t\tframeProcessTime_ += timeDiff(frameEndTime, frameStartTime);\n>>>> -\t\tif (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {\n>>>> -\t\t\tconst unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -\n>>>> -\t\t\t\t\t\t\t DebayerCpu::kFramesToSkip;\n>>>> +\t\tif (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {\n>>>> \t\t\tLOG(Debayer, Info)\n>>>> -\t\t\t\t<< \"Processed \" << measuredFrames\n>>>> +\t\t\t\t<< \"Processed \" << framesToMeasure_\n>>>> \t\t\t\t<< \" frames in \" << frameProcessTime_ / 1000 << \"us, \"\n>>>> -\t\t\t\t<< frameProcessTime_ / (1000 * measuredFrames)\n>>>> +\t\t\t\t<< frameProcessTime_ / (1000 * framesToMeasure_)\n>>>> \t\t\t\t<< \" us/frame\";\n>>>> \t\t}\n>>>> \t}\n>>>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h\n>>>> index 2f35aa18b..9d343e464 100644\n>>>> --- a/src/libcamera/software_isp/debayer_cpu.h\n>>>> +++ b/src/libcamera/software_isp/debayer_cpu.h\n>>>> @@ -161,11 +161,10 @@ private:\n>>>> \tunsigned int xShift_; /* Offset of 0/1 applied to window_.x */\n>>>> \tbool enableInputMemcpy_;\n>>>> \tbool swapRedBlueGains_;\n>>>> -\tunsigned int measuredFrames_;\n>>>> +\tunsigned int encounteredFrames_;\n>>>> \tint64_t frameProcessTime_;\n>>>> -\t/* Skip 30 frames for things to stabilize then measure 30 frames */\n>>>> -\tstatic constexpr unsigned int kFramesToSkip = 30;\n>>>> -\tstatic constexpr unsigned int kLastFrameToMeasure = 60;\n>>>> +\tunsigned int skipBeforeMeasure_ = 30;\n>>>> +\tunsigned int framesToMeasure_ = 30;\n>>>> };\n>>>> } /* namespace libcamera */\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 59B77BDCBF\n\tfor ;\n\tTue, 1 Jul 2025 09:05:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FAD668E17;\n\tTue, 1 Jul 2025 11:05:19 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DD11861528\n\tfor ;\n\tTue, 1 Jul 2025 11:05:17 +0200 (CEST)","from [192.168.33.8] (185.221.143.107.nat.pool.zt.hu\n\t[185.221.143.107])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53EAEC74;\n\tTue, 1 Jul 2025 11:04:55 +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=\"ddFkpup4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1751360695;\n\tbh=g1y3UyQladimAf8KV4B+fE/0818y28MO1nGgWGe3H1U=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=ddFkpup4e+++YyuQOYfDIleyM8qzZN5ltOjkMJbjmKXDXuO/9kwIbKEMpRcbH1Ius\n\trfrjX3nC2jxeBti1g1zdfr2XZDmfQLUSDhLYpjVavaVik5LyzXsYCNbOhnMleIuRmZ\n\t1VTod9yp3faw+lS+pXjAWZacF8daT50umWs44jSc=","Message-ID":"<60de9adb-f406-4d72-b274-edc5d4dc96e5@ideasonboard.com>","Date":"Tue, 1 Jul 2025 11:05:13 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v11 10/12] libcamera: software_isp: Make measurement\n\tconfigurable","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ,\n\tLaurent Pinchart ","References":"<20250624083612.27230-1-mzamazal@redhat.com>\n\t<20250624083612.27230-11-mzamazal@redhat.com>\n\t<57bf43cf-9360-45bc-8922-9c8bcd346b13@ideasonboard.com>\n\t<85ldpcvp2x.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>\n\t<85plek4899.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<85plek4899.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]