[RFC,1/7] libcamera: software_isp: Let a single point decide about computing stats
diff mbox series

Message ID 20261021080000.16624-2-mzamazal@redhat.com
State New
Headers show
Series
  • Autofocus basics for software ISP
Related show

Commit Message

Milan Zamazal Oct. 21, 2026, 7:59 a.m. UTC 
Currently, the decision whether to compute stats for a given frame is
implemented in SwStatsCpu in several places and duplicated in
debayer_egl.cpp.  Let's decide it solely in SwStatsCpu.  This will also
help if a more flexible or complex update frequency is introduced,
e.g. to support stats for autofocus.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 .../internal/software_isp/swstats_cpu.h       | 17 ++++++-------
 src/libcamera/software_isp/debayer_egl.cpp    | 24 ++++++++-----------
 src/libcamera/software_isp/swstats_cpu.cpp    | 16 ++++++++++---
 3 files changed, 32 insertions(+), 25 deletions(-)

Comments

Robert Mader Oct. 21, 2026, 11:45 a.m. UTC | #1 
Hi,

On 21.10.26 09:59, Milan Zamazal wrote:
> Currently, the decision whether to compute stats for a given frame is
> implemented in SwStatsCpu in several places and duplicated in
> debayer_egl.cpp.  Let's decide it solely in SwStatsCpu.  This will also
> help if a more flexible or complex update frequency is introduced,
> e.g. to support stats for autofocus.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   .../internal/software_isp/swstats_cpu.h       | 17 ++++++-------
>   src/libcamera/software_isp/debayer_egl.cpp    | 24 ++++++++-----------
>   src/libcamera/software_isp/swstats_cpu.cpp    | 16 ++++++++++---
>   3 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
> index 551870921..cd5303a51 100644
> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
> @@ -38,14 +38,6 @@ public:
>   	SwStatsCpu(const CameraManager &cm);
>   	~SwStatsCpu() = default;
>   
> -	/*
> -	 * The combination of pipeline + sensor delays means that
> -	 * exposure changes can take up to 3 frames to get applied,
> -	 * Run stats once every 4 frames to ensure any previous
> -	 * exposure changes have been applied.
> -	 */
> -	static constexpr uint32_t kStatPerNumFrames = 4;
> -
>   	bool isValid() const { return sharedStats_.fd().isValid(); }
>   
>   	const SharedFD &getStatsFD() { return sharedStats_.fd(); }
> @@ -54,6 +46,7 @@ public:
>   
>   	int configure(const StreamConfiguration &inputCfg, unsigned int statsBufferCount = 1);
>   	void setWindow(const Rectangle &window);
> +	bool updating(uint32_t frame);
>   	void startFrame(uint32_t frame);
>   	void finishFrame(uint32_t frame, uint32_t bufferId);
>   	void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input);
> @@ -88,6 +81,14 @@ private:
>   	using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats);
>   	using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);
>   
> +	/*
> +	 * The combination of pipeline + sensor delays means that
> +	 * exposure changes can take up to 3 frames to get applied,
> +	 * Run stats once every 4 frames to ensure any previous
> +	 * exposure changes have been applied.
> +	 */
> +	static constexpr uint32_t kStatPerNumFrames = 4;
> +
>   	int setupStandardBayerOrder(BayerFormat::Order order);
>   	/* Bayer 8 bpp unpacked */
>   	void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats);
> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
> index 300822f9d..1a9dc1841 100644
> --- a/src/libcamera/software_isp/debayer_egl.cpp
> +++ b/src/libcamera/software_isp/debayer_egl.cpp
> @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>   	metadata.planes()[0].bytesused = output->planes()[0].length;
>   
>   	/* Calculate stats for the whole frame */
> -	if (frame % SwStatsCpu::kStatPerNumFrames) {
> -		stats_->finishFrame(frame, 0);
> -	} else {
> -		if (!inMapped) {
> -			/*
> -			 * The buffer was directly imported into EGL and thus
> -			 * not mapped for texture upload. Do it now for the
> -			 * CPU-based stats calculation.
> -			 */
> -			assert(!inDmaSyncer);
> -			inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
> -			inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
> -		}
> -		stats_->processFrame(frame, 0, inMapped.value());
> +	if (stats_->updating(frame) && !inMapped) {
> +		/*
> +		 * The buffer was directly imported into EGL and thus
> +		 * not mapped for texture upload. Do it now for the
> +		 * CPU-based stats calculation.
> +		 */
> +		assert(!inDmaSyncer);
> +		inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
> +		inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
>   	}
> +	stats_->processFrame(frame, 0, inMapped.value());

This unfortunately doesn't work: in the "!stats_->updating(frame) && 
!inMapped" case - e.g. on the second frame if dmabuf import was 
successful - inMapped is not set and we crash.

That's why I went with the somewhat ugly previous solution.

>   	inDmaSyncer.reset();
>   
>   	egl_.syncOutput();
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index 7fb77ce7d..362fb6a86 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats)
>    */
>   void SwStatsCpu::startFrame(uint32_t frame)
>   {
> -	if (frame % kStatPerNumFrames)
> +	if (!updating(frame))
>   		return;
>   
>   	if (window_.width == 0)
> @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame)
>    */
>   void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>   {
> -	bool valid = frame % kStatPerNumFrames == 0;
> +	bool valid = updating(frame);
>   
>   	if (valid) {
>   		sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 });
> @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)
>    */
>   void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input)
>   {
> -	if (frame % kStatPerNumFrames) {
> +	if (!updating(frame)) {
>   		finishFrame(frame, bufferId);
>   		return;
>   	}
> @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff
>   	bench_.finishFrame();
>   }
>   
> +/**
> + * \brief Determine whether stats update is required for the given frame
> + * \param[in] frame The frame number
> + * \return Whether the stats are updated for the given frame
> + */
> +bool SwStatsCpu::updating(uint32_t frame)
> +{
> +	return frame % kStatPerNumFrames == 0;
> +}
> +
>   } /* namespace libcamera */
Milan Zamazal Oct. 21, 2026, 1:56 p.m. UTC | #2 
Hi Robert,

thank you for review.

Robert Mader <robert.mader@collabora.com> writes:

> Hi,
>
> On 21.10.26 09:59, Milan Zamazal wrote:
>> Currently, the decision whether to compute stats for a given frame is
>> implemented in SwStatsCpu in several places and duplicated in
>> debayer_egl.cpp.  Let's decide it solely in SwStatsCpu.  This will also
>> help if a more flexible or complex update frequency is introduced,
>> e.g. to support stats for autofocus.
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   .../internal/software_isp/swstats_cpu.h       | 17 ++++++-------
>>   src/libcamera/software_isp/debayer_egl.cpp    | 24 ++++++++-----------
>>   src/libcamera/software_isp/swstats_cpu.cpp    | 16 ++++++++++---
>>   3 files changed, 32 insertions(+), 25 deletions(-)
>>
>> diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
>> index 551870921..cd5303a51 100644
>> --- a/include/libcamera/internal/software_isp/swstats_cpu.h
>> +++ b/include/libcamera/internal/software_isp/swstats_cpu.h
>> @@ -38,14 +38,6 @@ public:
>>   	SwStatsCpu(const CameraManager &cm);
>>   	~SwStatsCpu() = default;
>>   -	/*
>> -	 * The combination of pipeline + sensor delays means that
>> -	 * exposure changes can take up to 3 frames to get applied,
>> -	 * Run stats once every 4 frames to ensure any previous
>> -	 * exposure changes have been applied.
>> -	 */
>> -	static constexpr uint32_t kStatPerNumFrames = 4;
>> -
>>   	bool isValid() const { return sharedStats_.fd().isValid(); }
>>     	const SharedFD &getStatsFD() { return sharedStats_.fd(); }
>> @@ -54,6 +46,7 @@ public:
>>     	int configure(const StreamConfiguration &inputCfg, unsigned int
>> statsBufferCount = 1);
>>   	void setWindow(const Rectangle &window);
>> +	bool updating(uint32_t frame);
>>   	void startFrame(uint32_t frame);
>>   	void finishFrame(uint32_t frame, uint32_t bufferId);
>>   	void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input);
>> @@ -88,6 +81,14 @@ private:
>>   	using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats);
>>   	using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);
>>   +	/*
>> +	 * The combination of pipeline + sensor delays means that
>> +	 * exposure changes can take up to 3 frames to get applied,
>> +	 * Run stats once every 4 frames to ensure any previous
>> +	 * exposure changes have been applied.
>> +	 */
>> +	static constexpr uint32_t kStatPerNumFrames = 4;
>> +
>>   	int setupStandardBayerOrder(BayerFormat::Order order);
>>   	/* Bayer 8 bpp unpacked */
>>   	void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats);
>> diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
>> index 300822f9d..1a9dc1841 100644
>> --- a/src/libcamera/software_isp/debayer_egl.cpp
>> +++ b/src/libcamera/software_isp/debayer_egl.cpp
>> @@ -630,21 +630,17 @@ void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
>>   	metadata.planes()[0].bytesused = output->planes()[0].length;
>>     	/* Calculate stats for the whole frame */
>> -	if (frame % SwStatsCpu::kStatPerNumFrames) {
>> -		stats_->finishFrame(frame, 0);
>> -	} else {
>> -		if (!inMapped) {
>> -			/*
>> -			 * The buffer was directly imported into EGL and thus
>> -			 * not mapped for texture upload. Do it now for the
>> -			 * CPU-based stats calculation.
>> -			 */
>> -			assert(!inDmaSyncer);
>> -			inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
>> -			inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
>> -		}
>> -		stats_->processFrame(frame, 0, inMapped.value());
>> +	if (stats_->updating(frame) && !inMapped) {
>> +		/*
>> +		 * The buffer was directly imported into EGL and thus
>> +		 * not mapped for texture upload. Do it now for the
>> +		 * CPU-based stats calculation.
>> +		 */
>> +		assert(!inDmaSyncer);
>> +		inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
>> +		inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
>>   	}
>> +	stats_->processFrame(frame, 0, inMapped.value());
>
> This unfortunately doesn't work: in the "!stats_->updating(frame) && !inMapped"
> case - e.g. on the second frame if dmabuf import was successful - inMapped is
> not set and we crash.
>
> That's why I went with the somewhat ugly previous solution.

Oops, I'll fix it.

BTW, in case you'd be keen on running it, this commit is what I use for
now to get some primitive stats and autofocus actually doing anything:
https://gitlab.freedesktop.org/mzamazal/libcamera-softisp/-/commit/a1b3135c4175bfb9e823166467418e24014c7fc8

>
>>   	inDmaSyncer.reset();
>>     	egl_.syncOutput();
>> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
>> index 7fb77ce7d..362fb6a86 100644
>> --- a/src/libcamera/software_isp/swstats_cpu.cpp
>> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
>> @@ -383,7 +383,7 @@ void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats)
>>    */
>>   void SwStatsCpu::startFrame(uint32_t frame)
>>   {
>> -	if (frame % kStatPerNumFrames)
>> +	if (!updating(frame))
>>   		return;
>>     	if (window_.width == 0)
>> @@ -404,7 +404,7 @@ void SwStatsCpu::startFrame(uint32_t frame)
>>    */
>>   void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>>   {
>> -	bool valid = frame % kStatPerNumFrames == 0;
>> +	bool valid = updating(frame);
>>     	if (valid) {
>>   		sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 });
>> @@ -604,7 +604,7 @@ void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)
>>    */
>>   void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input)
>>   {
>> -	if (frame % kStatPerNumFrames) {
>> +	if (!updating(frame)) {
>>   		finishFrame(frame, bufferId);
>>   		return;
>>   	}
>> @@ -616,4 +616,14 @@ void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff
>>   	bench_.finishFrame();
>>   }
>>   +/**
>> + * \brief Determine whether stats update is required for the given frame
>> + * \param[in] frame The frame number
>> + * \return Whether the stats are updated for the given frame
>> + */
>> +bool SwStatsCpu::updating(uint32_t frame)
>> +{
>> +	return frame % kStatPerNumFrames == 0;
>> +}
>> +
>>   } /* namespace libcamera */

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/swstats_cpu.h b/include/libcamera/internal/software_isp/swstats_cpu.h
index 551870921..cd5303a51 100644
--- a/include/libcamera/internal/software_isp/swstats_cpu.h
+++ b/include/libcamera/internal/software_isp/swstats_cpu.h
@@ -38,14 +38,6 @@  public:
 	SwStatsCpu(const CameraManager &cm);
 	~SwStatsCpu() = default;
 
-	/*
-	 * The combination of pipeline + sensor delays means that
-	 * exposure changes can take up to 3 frames to get applied,
-	 * Run stats once every 4 frames to ensure any previous
-	 * exposure changes have been applied.
-	 */
-	static constexpr uint32_t kStatPerNumFrames = 4;
-
 	bool isValid() const { return sharedStats_.fd().isValid(); }
 
 	const SharedFD &getStatsFD() { return sharedStats_.fd(); }
@@ -54,6 +46,7 @@  public:
 
 	int configure(const StreamConfiguration &inputCfg, unsigned int statsBufferCount = 1);
 	void setWindow(const Rectangle &window);
+	bool updating(uint32_t frame);
 	void startFrame(uint32_t frame);
 	void finishFrame(uint32_t frame, uint32_t bufferId);
 	void processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input);
@@ -88,6 +81,14 @@  private:
 	using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[], SwIspStats &stats);
 	using processFrameFn = void (SwStatsCpu::*)(MappedFrameBuffer &in);
 
+	/*
+	 * The combination of pipeline + sensor delays means that
+	 * exposure changes can take up to 3 frames to get applied,
+	 * Run stats once every 4 frames to ensure any previous
+	 * exposure changes have been applied.
+	 */
+	static constexpr uint32_t kStatPerNumFrames = 4;
+
 	int setupStandardBayerOrder(BayerFormat::Order order);
 	/* Bayer 8 bpp unpacked */
 	void statsBGGR8Line0(const uint8_t *src[], SwIspStats &stats);
diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp
index 300822f9d..1a9dc1841 100644
--- a/src/libcamera/software_isp/debayer_egl.cpp
+++ b/src/libcamera/software_isp/debayer_egl.cpp
@@ -630,21 +630,17 @@  void DebayerEGL::process(uint32_t frame, FrameBuffer *input, FrameBuffer *output
 	metadata.planes()[0].bytesused = output->planes()[0].length;
 
 	/* Calculate stats for the whole frame */
-	if (frame % SwStatsCpu::kStatPerNumFrames) {
-		stats_->finishFrame(frame, 0);
-	} else {
-		if (!inMapped) {
-			/*
-			 * The buffer was directly imported into EGL and thus
-			 * not mapped for texture upload. Do it now for the
-			 * CPU-based stats calculation.
-			 */
-			assert(!inDmaSyncer);
-			inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
-			inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
-		}
-		stats_->processFrame(frame, 0, inMapped.value());
+	if (stats_->updating(frame) && !inMapped) {
+		/*
+		 * The buffer was directly imported into EGL and thus
+		 * not mapped for texture upload. Do it now for the
+		 * CPU-based stats calculation.
+		 */
+		assert(!inDmaSyncer);
+		inDmaSyncer.emplace(input->planes()[0].fd, DmaSyncer::SyncType::Read);
+		inMapped.emplace(input, MappedFrameBuffer::MapFlag::Read);
 	}
+	stats_->processFrame(frame, 0, inMapped.value());
 	inDmaSyncer.reset();
 
 	egl_.syncOutput();
diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
index 7fb77ce7d..362fb6a86 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -383,7 +383,7 @@  void SwStatsCpu::statsGBRG12PLine0(const uint8_t *src[], SwIspStats &stats)
  */
 void SwStatsCpu::startFrame(uint32_t frame)
 {
-	if (frame % kStatPerNumFrames)
+	if (!updating(frame))
 		return;
 
 	if (window_.width == 0)
@@ -404,7 +404,7 @@  void SwStatsCpu::startFrame(uint32_t frame)
  */
 void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
 {
-	bool valid = frame % kStatPerNumFrames == 0;
+	bool valid = updating(frame);
 
 	if (valid) {
 		sharedStats_->sum_ = RGB<uint64_t>({ 0, 0, 0 });
@@ -604,7 +604,7 @@  void SwStatsCpu::processBayerFrame2(MappedFrameBuffer &in)
  */
 void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuffer &input)
 {
-	if (frame % kStatPerNumFrames) {
+	if (!updating(frame)) {
 		finishFrame(frame, bufferId);
 		return;
 	}
@@ -616,4 +616,14 @@  void SwStatsCpu::processFrame(uint32_t frame, uint32_t bufferId, MappedFrameBuff
 	bench_.finishFrame();
 }
 
+/**
+ * \brief Determine whether stats update is required for the given frame
+ * \param[in] frame The frame number
+ * \return Whether the stats are updated for the given frame
+ */
+bool SwStatsCpu::updating(uint32_t frame)
+{
+	return frame % kStatPerNumFrames == 0;
+}
+
 } /* namespace libcamera */