[v5,05/18] libcamera: software_isp: Make stats frame and buffer aware
diff mbox series

Message ID 20240830072554.180672-6-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP refactoring
Related show

Commit Message

Milan Zamazal Aug. 30, 2024, 7:25 a.m. UTC 
This patch adds frame and bufferId arguments to stats related calls.
Although the parameters are currently unused, because frame ids are not
tracked and used and the stats buffer is passed around directly rather
than being referred by its id, they bring the internal APIs closer to
their counterparts in hardware pipelines.

It serves as a preparation for followup patches that will introduce:

- Frame number tracking in order to switch to DelayedControls
  (software ISP TODO #11 + #12).
- A ring buffer for stats in order to improve passing the stats
  (software ISP TODO #2).

Frame and buffer ids are unrelated for the given purposes but since they
are passed together at the same places, the change is implemented as a
single patch rather than two, basically the same, patches.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 .../libcamera/internal/software_isp/software_isp.h    |  8 +++++---
 include/libcamera/ipa/soft.mojom                      |  4 +++-
 src/ipa/simple/soft_simple.cpp                        |  7 +++++--
 src/libcamera/pipeline/simple/simple.cpp              |  8 +++++---
 src/libcamera/software_isp/debayer_cpu.cpp            |  8 +++++++-
 src/libcamera/software_isp/software_isp.cpp           | 11 +++++++----
 src/libcamera/software_isp/swstats_cpu.cpp            |  6 ++++--
 src/libcamera/software_isp/swstats_cpu.h              |  4 ++--
 8 files changed, 38 insertions(+), 18 deletions(-)

Comments

Laurent Pinchart Sept. 1, 2024, 8 p.m. UTC | #1 
Hi Milan,

Thank you for the patch.

On Fri, Aug 30, 2024 at 09:25:41AM +0200, Milan Zamazal wrote:
> This patch adds frame and bufferId arguments to stats related calls.
> Although the parameters are currently unused, because frame ids are not
> tracked and used and the stats buffer is passed around directly rather
> than being referred by its id, they bring the internal APIs closer to
> their counterparts in hardware pipelines.
> 
> It serves as a preparation for followup patches that will introduce:
> 
> - Frame number tracking in order to switch to DelayedControls
>   (software ISP TODO #11 + #12).
> - A ring buffer for stats in order to improve passing the stats
>   (software ISP TODO #2).
> 
> Frame and buffer ids are unrelated for the given purposes but since they
> are passed together at the same places, the change is implemented as a
> single patch rather than two, basically the same, patches.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  .../libcamera/internal/software_isp/software_isp.h    |  8 +++++---
>  include/libcamera/ipa/soft.mojom                      |  4 +++-
>  src/ipa/simple/soft_simple.cpp                        |  7 +++++--
>  src/libcamera/pipeline/simple/simple.cpp              |  8 +++++---
>  src/libcamera/software_isp/debayer_cpu.cpp            |  8 +++++++-
>  src/libcamera/software_isp/software_isp.cpp           | 11 +++++++----
>  src/libcamera/software_isp/swstats_cpu.cpp            |  6 ++++--
>  src/libcamera/software_isp/swstats_cpu.h              |  4 ++--
>  8 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index f8e00003..3602bce8 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -11,6 +11,7 @@
>  #include <initializer_list>
>  #include <map>
>  #include <memory>
> +#include <stdint.h>
>  #include <string>
>  #include <tuple>
>  #include <vector>
> @@ -66,7 +67,8 @@ public:
>  	int exportBuffers(const Stream *stream, unsigned int count,
>  			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  
> -	void processStats(const ControlList &sensorControls);
> +	void processStats(const uint32_t frame, const uint32_t bufferId,
> +			  const ControlList &sensorControls);
>  
>  	int start();
>  	void stop();
> @@ -78,13 +80,13 @@ public:
>  
>  	Signal<FrameBuffer *> inputBufferReady;
>  	Signal<FrameBuffer *> outputBufferReady;
> -	Signal<> ispStatsReady;
> +	Signal<uint32_t, uint32_t> ispStatsReady;
>  	Signal<const ControlList &> setSensorControls;
>  
>  private:
>  	void saveIspParams();
>  	void setSensorCtrls(const ControlList &sensorControls);
> -	void statsReady();
> +	void statsReady(uint32_t frame, uint32_t bufferId);
>  	void inputReady(FrameBuffer *input);
>  	void outputReady(FrameBuffer *output);
>  
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index 0fd47bb0..cc5c37b2 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -23,7 +23,9 @@ interface IPASoftInterface {
>  	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
>  		=> (int32 ret);
>  
> -	[async] processStats(libcamera.ControlList sensorControls);
> +	[async] processStats(uint32 frame,
> +                             uint32 bufferId,
> +                             libcamera.ControlList sensorControls);

Wrong indentation (use tabs instead of spaces). I'll fix it locally if
there's no need for a v6.

>  };
>  
>  interface IPASoftEventInterface {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index 72321f44..12b5245e 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -75,7 +75,8 @@ public:
>  	int start() override;
>  	void stop() override;
>  
> -	void processStats(const ControlList &sensorControls) override;
> +	void processStats(const uint32_t frame, const uint32_t bufferId,
> +			  const ControlList &sensorControls) override;
>  
>  protected:
>  	std::string logPrefix() const override;
> @@ -249,7 +250,9 @@ void IPASoftSimple::stop()
>  {
>  }
>  
> -void IPASoftSimple::processStats(const ControlList &sensorControls)
> +void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
> +				 [[maybe_unused]] const uint32_t bufferId,
> +				 const ControlList &sensorControls)
>  {
>  	SwIspStats::Histogram histogram = stats_->yHistogram;
>  	if (ignoreUpdates_ > 0)
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 1e7ec7d9..48a568da 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -13,6 +13,7 @@
>  #include <memory>
>  #include <queue>
>  #include <set>
> +#include <stdint.h>
>  #include <string.h>
>  #include <string>
>  #include <unordered_map>
> @@ -291,7 +292,7 @@ private:
>  	void conversionInputDone(FrameBuffer *buffer);
>  	void conversionOutputDone(FrameBuffer *buffer);
>  
> -	void ispStatsReady();
> +	void ispStatsReady(uint32_t frame, uint32_t bufferId);
>  	void setSensorControls(const ControlList &sensorControls);
>  };
>  
> @@ -891,10 +892,11 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
>  		pipe->completeRequest(request);
>  }
>  
> -void SimpleCameraData::ispStatsReady()
> +void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
>  {
>  	/* \todo Use the DelayedControls class */
> -	swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
> +	swIsp_->processStats(frame, bufferId,
> +			     sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
>  						    V4L2_CID_EXPOSURE }));
>  }
>  
> diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
> index 077f7f4b..2a2e7edb 100644
> --- a/src/libcamera/software_isp/debayer_cpu.cpp
> +++ b/src/libcamera/software_isp/debayer_cpu.cpp
> @@ -777,7 +777,13 @@ void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
>  		}
>  	}
>  
> -	stats_->finishFrame();
> +	/*
> +	 * Frame and buffer ids are currently not used, so pass zeros as parameters.
> +	 *
> +	 * \todo Pass real values once frame is passed here and stats buffer passing
> +	 * is changed.
> +	 */
> +	stats_->finishFrame(0, 0);
>  	outputBufferReady.emit(output);
>  	inputBufferReady.emit(input);
>  }
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index a3ae3e22..a3855568 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -155,15 +155,18 @@ SoftwareIsp::~SoftwareIsp()
>  
>  /**
>   * \brief Process the statistics gathered
> + * \param[in] frame The frame number
> + * \param[in] bufferId ID of the statistics buffer
>   * \param[in] sensorControls The sensor controls
>   *
>   * Requests the IPA to calculate new parameters for ISP and new control
>   * values for the sensor.
>   */
> -void SoftwareIsp::processStats(const ControlList &sensorControls)
> +void SoftwareIsp::processStats(const uint32_t frame, const uint32_t bufferId,
> +			       const ControlList &sensorControls)
>  {
>  	ASSERT(ipa_);
> -	ipa_->processStats(sensorControls);
> +	ipa_->processStats(frame, bufferId, sensorControls);
>  }
>  
>  /**
> @@ -349,9 +352,9 @@ void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
>  	setSensorControls.emit(sensorControls);
>  }
>  
> -void SoftwareIsp::statsReady()
> +void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
>  {
> -	ispStatsReady.emit();
> +	ispStatsReady.emit(frame, bufferId);
>  }
>  
>  void SoftwareIsp::inputReady(FrameBuffer *input)
> diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
> index 815c4d4f..c520c806 100644
> --- a/src/libcamera/software_isp/swstats_cpu.cpp
> +++ b/src/libcamera/software_isp/swstats_cpu.cpp
> @@ -311,13 +311,15 @@ void SwStatsCpu::startFrame(void)
>  
>  /**
>   * \brief Finish statistics calculation for the current frame
> + * \param[in] frame The frame number
> + * \param[in] bufferId ID of the statistics buffer
>   *
>   * This may only be called after a successful setWindow() call.
>   */
> -void SwStatsCpu::finishFrame(void)
> +void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
>  {
>  	*sharedStats_ = stats_;
> -	statsReady.emit();
> +	statsReady.emit(frame, bufferId);
>  }
>  
>  /**
> diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h
> index 363e326f..26a2f462 100644
> --- a/src/libcamera/software_isp/swstats_cpu.h
> +++ b/src/libcamera/software_isp/swstats_cpu.h
> @@ -41,7 +41,7 @@ public:
>  	int configure(const StreamConfiguration &inputCfg);
>  	void setWindow(const Rectangle &window);
>  	void startFrame();
> -	void finishFrame();
> +	void finishFrame(uint32_t frame, uint32_t bufferId);
>  
>  	void processLine0(unsigned int y, const uint8_t *src[])
>  	{
> @@ -61,7 +61,7 @@ public:
>  		(this->*stats2_)(src);
>  	}
>  
> -	Signal<> statsReady;
> +	Signal<uint32_t, uint32_t> statsReady;
>  
>  private:
>  	using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);

Patch
diff mbox series 

diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
index f8e00003..3602bce8 100644
--- a/include/libcamera/internal/software_isp/software_isp.h
+++ b/include/libcamera/internal/software_isp/software_isp.h
@@ -11,6 +11,7 @@ 
 #include <initializer_list>
 #include <map>
 #include <memory>
+#include <stdint.h>
 #include <string>
 #include <tuple>
 #include <vector>
@@ -66,7 +67,8 @@  public:
 	int exportBuffers(const Stream *stream, unsigned int count,
 			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
 
-	void processStats(const ControlList &sensorControls);
+	void processStats(const uint32_t frame, const uint32_t bufferId,
+			  const ControlList &sensorControls);
 
 	int start();
 	void stop();
@@ -78,13 +80,13 @@  public:
 
 	Signal<FrameBuffer *> inputBufferReady;
 	Signal<FrameBuffer *> outputBufferReady;
-	Signal<> ispStatsReady;
+	Signal<uint32_t, uint32_t> ispStatsReady;
 	Signal<const ControlList &> setSensorControls;
 
 private:
 	void saveIspParams();
 	void setSensorCtrls(const ControlList &sensorControls);
-	void statsReady();
+	void statsReady(uint32_t frame, uint32_t bufferId);
 	void inputReady(FrameBuffer *input);
 	void outputReady(FrameBuffer *output);
 
diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
index 0fd47bb0..cc5c37b2 100644
--- a/include/libcamera/ipa/soft.mojom
+++ b/include/libcamera/ipa/soft.mojom
@@ -23,7 +23,9 @@  interface IPASoftInterface {
 	configure(libcamera.ControlInfoMap sensorCtrlInfoMap)
 		=> (int32 ret);
 
-	[async] processStats(libcamera.ControlList sensorControls);
+	[async] processStats(uint32 frame,
+                             uint32 bufferId,
+                             libcamera.ControlList sensorControls);
 };
 
 interface IPASoftEventInterface {
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index 72321f44..12b5245e 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -75,7 +75,8 @@  public:
 	int start() override;
 	void stop() override;
 
-	void processStats(const ControlList &sensorControls) override;
+	void processStats(const uint32_t frame, const uint32_t bufferId,
+			  const ControlList &sensorControls) override;
 
 protected:
 	std::string logPrefix() const override;
@@ -249,7 +250,9 @@  void IPASoftSimple::stop()
 {
 }
 
-void IPASoftSimple::processStats(const ControlList &sensorControls)
+void IPASoftSimple::processStats([[maybe_unused]] const uint32_t frame,
+				 [[maybe_unused]] const uint32_t bufferId,
+				 const ControlList &sensorControls)
 {
 	SwIspStats::Histogram histogram = stats_->yHistogram;
 	if (ignoreUpdates_ > 0)
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 1e7ec7d9..48a568da 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -13,6 +13,7 @@ 
 #include <memory>
 #include <queue>
 #include <set>
+#include <stdint.h>
 #include <string.h>
 #include <string>
 #include <unordered_map>
@@ -291,7 +292,7 @@  private:
 	void conversionInputDone(FrameBuffer *buffer);
 	void conversionOutputDone(FrameBuffer *buffer);
 
-	void ispStatsReady();
+	void ispStatsReady(uint32_t frame, uint32_t bufferId);
 	void setSensorControls(const ControlList &sensorControls);
 };
 
@@ -891,10 +892,11 @@  void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
 		pipe->completeRequest(request);
 }
 
-void SimpleCameraData::ispStatsReady()
+void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
 {
 	/* \todo Use the DelayedControls class */
-	swIsp_->processStats(sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
+	swIsp_->processStats(frame, bufferId,
+			     sensor_->getControls({ V4L2_CID_ANALOGUE_GAIN,
 						    V4L2_CID_EXPOSURE }));
 }
 
diff --git a/src/libcamera/software_isp/debayer_cpu.cpp b/src/libcamera/software_isp/debayer_cpu.cpp
index 077f7f4b..2a2e7edb 100644
--- a/src/libcamera/software_isp/debayer_cpu.cpp
+++ b/src/libcamera/software_isp/debayer_cpu.cpp
@@ -777,7 +777,13 @@  void DebayerCpu::process(FrameBuffer *input, FrameBuffer *output, DebayerParams
 		}
 	}
 
-	stats_->finishFrame();
+	/*
+	 * Frame and buffer ids are currently not used, so pass zeros as parameters.
+	 *
+	 * \todo Pass real values once frame is passed here and stats buffer passing
+	 * is changed.
+	 */
+	stats_->finishFrame(0, 0);
 	outputBufferReady.emit(output);
 	inputBufferReady.emit(input);
 }
diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
index a3ae3e22..a3855568 100644
--- a/src/libcamera/software_isp/software_isp.cpp
+++ b/src/libcamera/software_isp/software_isp.cpp
@@ -155,15 +155,18 @@  SoftwareIsp::~SoftwareIsp()
 
 /**
  * \brief Process the statistics gathered
+ * \param[in] frame The frame number
+ * \param[in] bufferId ID of the statistics buffer
  * \param[in] sensorControls The sensor controls
  *
  * Requests the IPA to calculate new parameters for ISP and new control
  * values for the sensor.
  */
-void SoftwareIsp::processStats(const ControlList &sensorControls)
+void SoftwareIsp::processStats(const uint32_t frame, const uint32_t bufferId,
+			       const ControlList &sensorControls)
 {
 	ASSERT(ipa_);
-	ipa_->processStats(sensorControls);
+	ipa_->processStats(frame, bufferId, sensorControls);
 }
 
 /**
@@ -349,9 +352,9 @@  void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)
 	setSensorControls.emit(sensorControls);
 }
 
-void SoftwareIsp::statsReady()
+void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
 {
-	ispStatsReady.emit();
+	ispStatsReady.emit(frame, bufferId);
 }
 
 void SoftwareIsp::inputReady(FrameBuffer *input)
diff --git a/src/libcamera/software_isp/swstats_cpu.cpp b/src/libcamera/software_isp/swstats_cpu.cpp
index 815c4d4f..c520c806 100644
--- a/src/libcamera/software_isp/swstats_cpu.cpp
+++ b/src/libcamera/software_isp/swstats_cpu.cpp
@@ -311,13 +311,15 @@  void SwStatsCpu::startFrame(void)
 
 /**
  * \brief Finish statistics calculation for the current frame
+ * \param[in] frame The frame number
+ * \param[in] bufferId ID of the statistics buffer
  *
  * This may only be called after a successful setWindow() call.
  */
-void SwStatsCpu::finishFrame(void)
+void SwStatsCpu::finishFrame(uint32_t frame, uint32_t bufferId)
 {
 	*sharedStats_ = stats_;
-	statsReady.emit();
+	statsReady.emit(frame, bufferId);
 }
 
 /**
diff --git a/src/libcamera/software_isp/swstats_cpu.h b/src/libcamera/software_isp/swstats_cpu.h
index 363e326f..26a2f462 100644
--- a/src/libcamera/software_isp/swstats_cpu.h
+++ b/src/libcamera/software_isp/swstats_cpu.h
@@ -41,7 +41,7 @@  public:
 	int configure(const StreamConfiguration &inputCfg);
 	void setWindow(const Rectangle &window);
 	void startFrame();
-	void finishFrame();
+	void finishFrame(uint32_t frame, uint32_t bufferId);
 
 	void processLine0(unsigned int y, const uint8_t *src[])
 	{
@@ -61,7 +61,7 @@  public:
 		(this->*stats2_)(src);
 	}
 
-	Signal<> statsReady;
+	Signal<uint32_t, uint32_t> statsReady;
 
 private:
 	using statsProcessFn = void (SwStatsCpu::*)(const uint8_t *src[]);