[v11,10/12] libcamera: software_isp: Make measurement configurable

23639 diff mbox series

Software ISP performs performance measurement on certain part of initial
frames.  Let's make this range configurable.

For this purpose, this patch introduces new configuration options
pipelines.simple.measure.skip and pipelines.simple.measure.number.
Setting the latter one to 0 disables the measurement.

Instead of the last frame, the class member and its configuration
specify the number of frames to measure.  This is easier to use for
users and doesn't require to adjust two configuration parameters when
the number of the initially skipped frames is changed.

The patch also changes the names of the class members to make them more
accurate.

Completes software ISP TODO #7.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 Documentation/runtime_configuration.rst    |  6 ++++++
 src/libcamera/software_isp/TODO            | 25 ----------------------
 src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------
 src/libcamera/software_isp/debayer_cpu.h   |  7 +++---
 4 files changed, 25 insertions(+), 38 deletions(-)

Hi

2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
> Software ISP performs performance measurement on certain part of initial
> frames.  Let's make this range configurable.
> 
> For this purpose, this patch introduces new configuration options
> pipelines.simple.measure.skip and pipelines.simple.measure.number.
> Setting the latter one to 0 disables the measurement.
> 
> Instead of the last frame, the class member and its configuration
> specify the number of frames to measure.  This is easier to use for
> users and doesn't require to adjust two configuration parameters when
> the number of the initially skipped frames is changed.
> 
> The patch also changes the names of the class members to make them more
> accurate.
> 
> Completes software ISP TODO #7.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   Documentation/runtime_configuration.rst    |  6 ++++++
>   src/libcamera/software_isp/TODO            | 25 ----------------------
>   src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------
>   src/libcamera/software_isp/debayer_cpu.h   |  7 +++---
>   4 files changed, 25 insertions(+), 38 deletions(-)
> 
> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
> index f4e86ae24..de74fa732 100644
> --- a/Documentation/runtime_configuration.rst
> +++ b/Documentation/runtime_configuration.rst
> @@ -52,6 +52,9 @@ file structure:
>             ...
>         simple:
>           copy_input_buffer: # true/false
> +        measure:
> +          skip: # non-negative integer, frames to skip initially
> +          number: # non-negative integer, frames to measure
>           supported_devices:
>           - driver: # driver name, e.g. `mxc-isi`
>             software_isp: # true/false
> @@ -87,6 +90,9 @@ Configuration file example
>               min_total_unicam_buffers: 2
>          simple:
>            copy_input_buffer: false
> +         measure:
> +           skip: 50
> +           number: 30
>            supported_devices:
>            - driver: mxc-isi
>              software_isp: true
> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
> index 2c919f442..f19e15ae2 100644
> --- a/src/libcamera/software_isp/TODO
> +++ b/src/libcamera/software_isp/TODO
> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.
>   
>   ---
>   
> -7. Performance measurement configuration
> -
> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
> -> /* Measure before emitting signals */
> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> ->     ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
> -> 	timespec frameEndTime = {};
> -> 	clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
> -> 	frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
> -> 	if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
> -> 		const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
> -> 						    DebayerCpu::kFramesToSkip;
> -> 		LOG(Debayer, Info)
> -> 			<< "Processed " << measuredFrames
> -> 			<< " frames in " << frameProcessTime_ / 1000 << "us, "
> -> 			<< frameProcessTime_ / (1000 * measuredFrames)
> -> 			<< " us/frame";
> -> 	}
> -> }
> -
> -I wonder if there would be a way to control at runtime when/how to
> -perform those measurements. Maybe that's a bit overkill.
> -
> ----
> -
>   8. DebayerCpu cleanups
>   
>   > >> class DebayerCpu : public Debayer, public Object
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 85e31e98e..27113c6a5 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat
>   	enableInputMemcpy_ =
>   		configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true);
>   
> +	skipBeforeMeasure_ = configuration.option<unsigned int>(
> +						  "pipelines/simple/measure/skip")
> +				     .value_or(skipBeforeMeasure_);
> +	framesToMeasure_ = configuration.option<unsigned int>(
> +						"pipelines/simple/measure/number")
> +				   .value_or(framesToMeasure_);
> +

Regarding this and the previous patch, I think the `DebayerCpu` class should
not access the configuration like this.

A different approach could be creating e.g.

   struct DebayerCpu::Params {
     bool inputMemcpy = true;
     struct {
       unsigned int skip = 0, frames = 0;
     } measure;
   };

and passing that as argument to the constructor, and letting the caller extract the
settings from wherever it wants.

Alternatively, and I think this is generally useful is other contexts, I would
add a `Configuration` type that contains a single `const YamlObject&`, essentially
representing a subtree of the configuration. And then this type would have the
option(), listOption, envOption(), etc. functions. In addition it would have a
`Configuration operator[](path)` or `Configuration child(path)` that returns a
`Configuration` object pointing to a child. This would make it possible to not
have to consider the entire structure of the configuration from the root.
Everything could deal with just its own dictionary, etc. This could then be
passed to the constructors of various objects (pipeline handler, ipa, etc.)

Thoughts? I am not proposing these changes be made just now, but I think
something like this could be useful in the future.


Regards,
Barnabás Pőcze


>   	/* Initialize color lookup tables */
>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>   		red_[i] = green_[i] = blue_[i] = i;
> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>   			lineBuffers_[i].resize(lineBufferLength_);
>   	}
>   
> -	measuredFrames_ = 0;
> +	encounteredFrames_ = 0;
>   	frameProcessTime_ = 0;
>   
>   	return 0;
> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>   {
>   	timespec frameStartTime;
>   
> -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {
> +	bool measure = framesToMeasure_ > 0 &&
> +		       encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&
> +		       ++encounteredFrames_ > skipBeforeMeasure_;
> +	if (measure) {
>   		frameStartTime = {};
>   		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>   	}
> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>   	dmaSyncers.clear();
>   
>   	/* Measure before emitting signals */
> -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
> -	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
> +	if (measure) {
>   		timespec frameEndTime = {};
>   		clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
>   		frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
> -		if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
> -			const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
> -							    DebayerCpu::kFramesToSkip;
> +		if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {
>   			LOG(Debayer, Info)
> -				<< "Processed " << measuredFrames
> +				<< "Processed " << framesToMeasure_
>   				<< " frames in " << frameProcessTime_ / 1000 << "us, "
> -				<< frameProcessTime_ / (1000 * measuredFrames)
> +				<< frameProcessTime_ / (1000 * framesToMeasure_)
>   				<< " us/frame";
>   		}
>   	}
> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
> index 2f35aa18b..9d343e464 100644
> --- a/src/libcamera/software_isp/debayer_cpu.h
> +++ b/src/libcamera/software_isp/debayer_cpu.h
> @@ -161,11 +161,10 @@ private:
>   	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
>   	bool enableInputMemcpy_;
>   	bool swapRedBlueGains_;
> -	unsigned int measuredFrames_;
> +	unsigned int encounteredFrames_;
>   	int64_t frameProcessTime_;
> -	/* Skip 30 frames for things to stabilize then measure 30 frames */
> -	static constexpr unsigned int kFramesToSkip = 30;
> -	static constexpr unsigned int kLastFrameToMeasure = 60;
> +	unsigned int skipBeforeMeasure_ = 30;
> +	unsigned int framesToMeasure_ = 30;
>   };
>   
>   } /* namespace libcamera */

Hi Barnabás,

Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes:

> Hi
>
> 2025. 06. 24. 10:36 keltezéssel, Milan Zamazal írta:
>> Software ISP performs performance measurement on certain part of initial
>> frames.  Let's make this range configurable.
>> For this purpose, this patch introduces new configuration options
>> pipelines.simple.measure.skip and pipelines.simple.measure.number.
>> Setting the latter one to 0 disables the measurement.
>> Instead of the last frame, the class member and its configuration
>> specify the number of frames to measure.  This is easier to use for
>> users and doesn't require to adjust two configuration parameters when
>> the number of the initially skipped frames is changed.
>> The patch also changes the names of the class members to make them more
>> accurate.
>> Completes software ISP TODO #7.
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   Documentation/runtime_configuration.rst    |  6 ++++++
>>   src/libcamera/software_isp/TODO            | 25 ----------------------
>>   src/libcamera/software_isp/debayer_cpu.cpp | 25 ++++++++++++++--------
>>   src/libcamera/software_isp/debayer_cpu.h   |  7 +++---
>>   4 files changed, 25 insertions(+), 38 deletions(-)
>> diff --git a/Documentation/runtime_configuration.rst b/Documentation/runtime_configuration.rst
>> index f4e86ae24..de74fa732 100644
>> --- a/Documentation/runtime_configuration.rst
>> +++ b/Documentation/runtime_configuration.rst
>> @@ -52,6 +52,9 @@ file structure:
>>             ...
>>         simple:
>>           copy_input_buffer: # true/false
>> +        measure:
>> +          skip: # non-negative integer, frames to skip initially
>> +          number: # non-negative integer, frames to measure
>>           supported_devices:
>>           - driver: # driver name, e.g. `mxc-isi`
>>             software_isp: # true/false
>> @@ -87,6 +90,9 @@ Configuration file example
>>               min_total_unicam_buffers: 2
>>          simple:
>>            copy_input_buffer: false
>> +         measure:
>> +           skip: 50
>> +           number: 30
>>            supported_devices:
>>            - driver: mxc-isi
>>              software_isp: true
>> diff --git a/src/libcamera/software_isp/TODO b/src/libcamera/software_isp/TODO
>> index 2c919f442..f19e15ae2 100644
>> --- a/src/libcamera/software_isp/TODO
>> +++ b/src/libcamera/software_isp/TODO
>> @@ -71,31 +71,6 @@ per-frame buffers like we do for hardware ISPs.
>>     ---
>>   -7. Performance measurement configuration
>> -
>> -> void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams params)
>> -> /* Measure before emitting signals */
>> -> if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>> ->     ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
>> -> 	timespec frameEndTime = {};
>> -> 	clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
>> -> 	frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
>> -> 	if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
>> -> 		const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
>> -> 						    DebayerCpu::kFramesToSkip;
>> -> 		LOG(Debayer, Info)
>> -> 			<< "Processed " << measuredFrames
>> -> 			<< " frames in " << frameProcessTime_ / 1000 << "us, "
>> -> 			<< frameProcessTime_ / (1000 * measuredFrames)
>> -> 			<< " us/frame";
>> -> 	}
>> -> }
>> -
>> -I wonder if there would be a way to control at runtime when/how to
>> -perform those measurements. Maybe that's a bit overkill.
>> -
>> ----
>> -
>>   8. DebayerCpu cleanups
>>     > >> class DebayerCpu : public Debayer, public Object
>> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
>> index 85e31e98e..27113c6a5 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.cpp
>> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
>> @@ -55,6 +55,13 @@ DebayerCpu::DebayerCpu(std::unique_ptr<SwStatsCpu> stats, const GlobalConfigurat
>>   	enableInputMemcpy_ =
>>   		configuration.option<bool>("pipelines/simple/copy_input_buffer").value_or(true);
>>   +	skipBeforeMeasure_ = configuration.option<unsigned int>(
>> +						  "pipelines/simple/measure/skip")
>> +				     .value_or(skipBeforeMeasure_);
>> +	framesToMeasure_ = configuration.option<unsigned int>(
>> +						"pipelines/simple/measure/number")
>> +				   .value_or(framesToMeasure_);
>> +
>
> Regarding this and the previous patch, I think the `DebayerCpu` class should
> not access the configuration like this.
>
> A different approach could be creating e.g.
>
>   struct DebayerCpu::Params {
>     bool inputMemcpy = true;
>     struct {
>       unsigned int skip = 0, frames = 0;
>     } measure;
>   };
>
> and passing that as argument to the constructor, and letting the caller extract the
> settings from wherever it wants.

OK, makes sense.

> Alternatively, and I think this is generally useful is other contexts, I would
> add a `Configuration` type that contains a single `const YamlObject&`, essentially
> representing a subtree of the configuration. And then this type would have the
> option(), listOption, envOption(), etc. functions. In addition it would have a
> `Configuration operator[](path)` or `Configuration child(path)` that returns a
> `Configuration` object pointing to a child. This would make it possible to not
> have to consider the entire structure of the configuration from the root.
> Everything could deal with just its own dictionary, etc. This could then be
> passed to the constructors of various objects (pipeline handler, ipa, etc.)
> 
> Thoughts?

What would be the primary motivation?  I can think about:

- Not exposing the whole configuration to proprietary code.

- Making the configuration lookup relative to not having update it on
  contingent restructuring (but since such a thing is an annoyance to
  users above all and it still requires changes somewhere at callers,
  this doesn't look like any important).

Against:

- Perhaps just unnecessary complication.

- It obfuscates in the code where's the given configuration option in
  the file.

> I am not proposing these changes be made just now, but I think
> something like this could be useful in the future.
>
>
> Regards,
> Barnabás Pőcze
>
>
>>   	/* Initialize color lookup tables */
>>   	for (unsigned int i = 0; i < DebayerParams::kRGBLookupSize; i++) {
>>   		red_[i] = green_[i] = blue_[i] = i;
>> @@ -557,7 +564,7 @@ int DebayerCpu::configure(const StreamConfiguration &inputCfg,
>>   			lineBuffers_[i].resize(lineBufferLength_);
>>   	}
>>   -	measuredFrames_ = 0;
>> +	encounteredFrames_ = 0;
>>   	frameProcessTime_ = 0;
>>     	return 0;
>> @@ -763,7 +770,10 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>>   {
>>   	timespec frameStartTime;
>>   -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure) {
>> +	bool measure = framesToMeasure_ > 0 &&
>> +		       encounteredFrames_ < skipBeforeMeasure_ + framesToMeasure_ &&
>> +		       ++encounteredFrames_ > skipBeforeMeasure_;
>> +	if (measure) {
>>   		frameStartTime = {};
>>   		clock_gettime(CLOCK_MONOTONIC_RAW, &frameStartTime);
>>   	}
>> @@ -820,18 +830,15 @@ void DebayerCpu::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>>   	dmaSyncers.clear();
>>     	/* Measure before emitting signals */
>> -	if (measuredFrames_ < DebayerCpu::kLastFrameToMeasure &&
>> -	    ++measuredFrames_ > DebayerCpu::kFramesToSkip) {
>> +	if (measure) {
>>   		timespec frameEndTime = {};
>>   		clock_gettime(CLOCK_MONOTONIC_RAW, &frameEndTime);
>>   		frameProcessTime_ += timeDiff(frameEndTime, frameStartTime);
>> -		if (measuredFrames_ == DebayerCpu::kLastFrameToMeasure) {
>> -			const unsigned int measuredFrames = DebayerCpu::kLastFrameToMeasure -
>> -							    DebayerCpu::kFramesToSkip;
>> +		if (encounteredFrames_ == skipBeforeMeasure_ + framesToMeasure_) {
>>   			LOG(Debayer, Info)
>> -				<< "Processed " << measuredFrames
>> +				<< "Processed " << framesToMeasure_
>>   				<< " frames in " << frameProcessTime_ / 1000 << "us, "
>> -				<< frameProcessTime_ / (1000 * measuredFrames)
>> +				<< frameProcessTime_ / (1000 * framesToMeasure_)
>>   				<< " us/frame";
>>   		}
>>   	}
>> diff --git a/src/libcamera/software_isp/debayer_cpu.h b/src/libcamera/software_isp/debayer_cpu.h
>> index 2f35aa18b..9d343e464 100644
>> --- a/src/libcamera/software_isp/debayer_cpu.h
>> +++ b/src/libcamera/software_isp/debayer_cpu.h
>> @@ -161,11 +161,10 @@ private:
>>   	unsigned int xShift_; /* Offset of 0/1 applied to window_.x */
>>   	bool enableInputMemcpy_;
>>   	bool swapRedBlueGains_;
>> -	unsigned int measuredFrames_;
>> +	unsigned int encounteredFrames_;
>>   	int64_t frameProcessTime_;
>> -	/* Skip 30 frames for things to stabilize then measure 30 frames */
>> -	static constexpr unsigned int kFramesToSkip = 30;
>> -	static constexpr unsigned int kLastFrameToMeasure = 60;
>> +	unsigned int skipBeforeMeasure_ = 30;
>> +	unsigned int framesToMeasure_ = 30;
>>   };
>>     } /* namespace libcamera */