[PATCH v6 2/6] ipa: simple: softisp: Extend to pass metadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Mar 28 01:20:25 CET 2025
Hi Milan,
Thank you for the patch.
On Thu, Mar 27, 2025 at 07:59:40PM +0100, Milan Zamazal wrote:
> From: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Extend the Simple IPA IPC to support returning a metadata ControlList
> when the process call has completed.
>
> A new signal from the IPA is introduced to report the metadata,
> similarly to what the hardware pipelines do.
>
> Merge the metadata reported by the ISP into any completing request to
> provide to the application. Completion of a request is delayed until
> this is done; this doesn't apply to canceled requests.
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
> .../internal/software_isp/software_isp.h | 2 +
> include/libcamera/ipa/soft.mojom | 1 +
> src/ipa/simple/soft_simple.cpp | 7 +--
> src/libcamera/pipeline/simple/simple.cpp | 48 ++++++++++++++-----
> src/libcamera/software_isp/software_isp.cpp | 9 ++++
> 5 files changed, 49 insertions(+), 18 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index 0026cec2..1ee37dc7 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -85,12 +85,14 @@ public:
> Signal<FrameBuffer *> inputBufferReady;
> Signal<FrameBuffer *> outputBufferReady;
> Signal<uint32_t, uint32_t> ispStatsReady;
> + Signal<uint32_t, const ControlList &> metadataReady;
> Signal<const ControlList &> setSensorControls;
>
> private:
> void saveIspParams();
> void setSensorCtrls(const ControlList &sensorControls);
> void statsReady(uint32_t frame, uint32_t bufferId);
> + void saveMetadata(uint32_t frame, const ControlList &metadata);
This function isn't implemented, probably a leftover ?
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
The series passed CI, I've dropped the saveMetadata() function and
pushed. One more milestone merged for the soft ISP :-) Thanks for all
your work.
> void inputReady(FrameBuffer *input);
> void outputReady(FrameBuffer *output);
>
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index ede74413..a8e6ecda 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -33,4 +33,5 @@ interface IPASoftInterface {
> interface IPASoftEventInterface {
> setSensorControls(libcamera.ControlList sensorControls);
> setIspParams();
> + metadataReady(uint32 frame, libcamera.ControlList metadata);
> };
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index a87c6cdd..4ef67c43 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -299,15 +299,10 @@ void IPASoftSimple::processStats(const uint32_t frame,
> int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();
> frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;
>
> - /*
> - * Software ISP currently does not produce any metadata. Use an empty
> - * ControlList for now.
> - *
> - * \todo Implement proper metadata handling
> - */
> ControlList metadata(controls::controls);
> for (auto const &algo : algorithms())
> algo->process(context_, frame, frameContext, stats_, metadata);
> + metadataReady.emit(frame, metadata);
>
> /* Sanity check */
> if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 004a8d53..fd0ccdca 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -182,19 +182,21 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
> class SimplePipelineHandler;
>
> struct SimpleFrameInfo {
> - SimpleFrameInfo(uint32_t f, Request *r)
> - : frame(f), request(r)
> + SimpleFrameInfo(uint32_t f, Request *r, bool m)
> + : frame(f), request(r), metadataRequired(m), metadataProcessed(false)
> {
> }
>
> uint32_t frame;
> Request *request;
> + bool metadataRequired;
> + bool metadataProcessed;
> };
>
> class SimpleFrames
> {
> public:
> - void create(Request *request);
> + void create(Request *request, bool metadataRequested);
> void destroy(uint32_t frame);
> void clear();
>
> @@ -204,10 +206,10 @@ private:
> std::map<uint32_t, SimpleFrameInfo> frameInfo_;
> };
>
> -void SimpleFrames::create(Request *request)
> +void SimpleFrames::create(Request *request, bool metadataRequired)
> {
> const uint32_t frame = request->sequence();
> - auto [it, inserted] = frameInfo_.try_emplace(frame, frame, request);
> + auto [it, inserted] = frameInfo_.try_emplace(frame, frame, request, metadataRequired);
> ASSERT(inserted);
> }
>
> @@ -347,11 +349,12 @@ private:
> void tryPipeline(unsigned int code, const Size &size);
> static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
>
> - void completeRequest(Request *request);
> + void tryCompleteRequest(Request *request);
> void conversionInputDone(FrameBuffer *buffer);
> void conversionOutputDone(FrameBuffer *buffer);
>
> void ispStatsReady(uint32_t frame, uint32_t bufferId);
> + void metadataReady(uint32_t frame, const ControlList &metadata);
> void setSensorControls(const ControlList &sensorControls);
> };
>
> @@ -590,6 +593,7 @@ int SimpleCameraData::init()
> swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);
> swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);
> swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);
> + swIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);
> swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);
> }
> }
> @@ -835,7 +839,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> /* No conversion, just complete the request. */
> Request *request = buffer->request();
> pipe->completeBuffer(request, buffer);
> - completeRequest(request);
> + tryCompleteRequest(request);
> return;
> }
>
> @@ -853,7 +857,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> const RequestOutputs &outputs = conversionQueue_.front();
> for (auto &[stream, buf] : outputs.outputs)
> pipe->completeBuffer(outputs.request, buf);
> - completeRequest(outputs.request);
> + SimpleFrameInfo *info = frameInfo_.find(outputs.request->sequence());
> + if (info)
> + info->metadataRequired = false;
> + tryCompleteRequest(outputs.request);
> conversionQueue_.pop();
>
> return;
> @@ -911,7 +918,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>
> /* Otherwise simply complete the request. */
> pipe->completeBuffer(request, buffer);
> - completeRequest(request);
> + tryCompleteRequest(request);
> }
>
> void SimpleCameraData::clearIncompleteRequests()
> @@ -922,14 +929,20 @@ void SimpleCameraData::clearIncompleteRequests()
> }
> }
>
> -void SimpleCameraData::completeRequest(Request *request)
> +void SimpleCameraData::tryCompleteRequest(Request *request)
> {
> + if (request->hasPendingBuffers())
> + return;
> +
> SimpleFrameInfo *info = frameInfo_.find(request->sequence());
> if (!info) {
> /* Something is really wrong, let's return. */
> return;
> }
>
> + if (info->metadataRequired && !info->metadataProcessed)
> + return;
> +
> frameInfo_.destroy(info->frame);
> pipe()->completeRequest(request);
> }
> @@ -947,7 +960,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
> /* Complete the buffer and the request. */
> Request *request = buffer->request();
> if (pipe->completeBuffer(request, buffer))
> - completeRequest(request);
> + tryCompleteRequest(request);
> }
>
> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> @@ -956,6 +969,17 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> delayedCtrls_->get(frame));
> }
>
> +void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata)
> +{
> + SimpleFrameInfo *info = frameInfo_.find(frame);
> + if (!info)
> + return;
> +
> + info->request->metadata().merge(metadata);
> + info->metadataProcessed = true;
> + tryCompleteRequest(info->request);
> +}
> +
> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
> {
> delayedCtrls_->push(sensorControls);
> @@ -1489,7 +1513,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
> }
> }
>
> - data->frameInfo_.create(request);
> + data->frameInfo_.create(request, !!data->swIsp_);
> if (data->useConversion_) {
> data->conversionQueue_.push({ request, std::move(buffers) });
> if (data->swIsp_)
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 4a74dcb6..6baa3fec 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -55,6 +55,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
> * \brief A signal emitted when the statistics for IPA are ready
> */
>
> +/**
> + * \var SoftwareIsp::metadataReady
> + * \brief A signal emitted when the metadata for IPA is ready
> + */
> +
> /**
> * \var SoftwareIsp::setSensorControls
> * \brief A signal emitted when the values to write to the sensor controls are
> @@ -141,6 +146,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
> }
>
> ipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);
> + ipa_->metadataReady.connect(this,
> + [this](uint32_t frame, const ControlList &metadata) {
> + metadataReady.emit(frame, metadata);
> + });
> ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);
>
> debayer_->moveToThread(&ispWorkerThread_);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list