[PATCH v5 05/18] libcamera: software_isp: Make stats frame and buffer aware
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Sep 1 22:00:36 CEST 2024
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 at redhat.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at 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[]);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list