[PATCH v3 2/6] ipa: simple: softisp: Extend to pass metadata
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Jan 27 07:56:59 CET 2025
Hi Milan,
Thank you for the patch.
On Mon, Jan 13, 2025 at 02:34:01PM +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.
>
> 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 | 35 +++++++++++++++----
> src/libcamera/software_isp/software_isp.cpp | 11 ++++++
> 5 files changed, 43 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index d51b03fd..3d248aba 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -83,12 +83,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);
> void inputReady(FrameBuffer *input);
> void outputReady(FrameBuffer *output);
>
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index d52e6f1a..934a6fd1 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 b26e4e15..944c644e 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -295,15 +295,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 123179b6..bb0ee23e 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -184,6 +184,7 @@ class SimplePipelineHandler;
> struct SimpleFrameInfo {
> uint32_t frame;
> Request *request;
> + bool metadataProcessed;
> };
>
> class SimpleFrames
> @@ -207,6 +208,7 @@ void SimpleFrames::create(Request *request)
> ASSERT(inserted);
> it->second.frame = frame;
> it->second.request = request;
> + it->second.metadataProcessed = false;
> }
>
> void SimpleFrames::destroy(uint32_t frame)
> @@ -357,11 +359,12 @@ private:
> void tryPipeline(unsigned int code, const Size &size);
> static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
>
> - void completeRequest(Request *request);
> + void completeRequest(Request *request, bool checkCompleted);
> 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);
> };
>
> @@ -614,6 +617,7 @@ int SimpleCameraData::init()
> });
> swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);
> swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);
> + swIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);
> swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);
> }
> }
> @@ -859,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> /* No conversion, just complete the request. */
> Request *request = buffer->request();
> pipe->completeBuffer(request, buffer);
> - completeRequest(request);
> + completeRequest(request, false);
> return;
> }
>
> @@ -877,7 +881,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
> const RequestOutputs &outputs = conversionQueue_.front();
> for (auto &[stream, buf] : outputs.outputs)
> pipe->completeBuffer(outputs.request, buf);
> - completeRequest(outputs.request);
> + completeRequest(outputs.request, false);
> conversionQueue_.pop();
>
> return;
> @@ -935,7 +939,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>
> /* Otherwise simply complete the request. */
> pipe->completeBuffer(request, buffer);
> - completeRequest(request);
> + completeRequest(request, false);
> }
>
> void SimpleCameraData::clearIncompleteRequests()
> @@ -946,11 +950,17 @@ void SimpleCameraData::clearIncompleteRequests()
> }
> }
>
> -void SimpleCameraData::completeRequest(Request *request)
> +void SimpleCameraData::completeRequest(Request *request, bool checkCompleted)
I would name the function tryCompleteRequest() to match the rkisp1
pipeline handler.
Why do you need a checkCompleted argument ? This seems fairly error
prone. Can't you follow the same logic as rkisp1 ?
Note that we plan to completely refactor the request completion logic,
but that will likely be done after this series gets merged.
> {
> + if (checkCompleted && request->hasPendingBuffers())
> + return;
> +
> SimpleFrameInfo *info = frameInfo_.find(request);
> - if (info)
> + if (info) {
> + if (checkCompleted && !info->metadataProcessed)
> + return;
> frameInfo_.destroy(info->frame);
> + }
> pipe()->completeRequest(request);
> }
>
> @@ -967,7 +977,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
> /* Complete the buffer and the request. */
> Request *request = buffer->request();
> if (pipe->completeBuffer(request, buffer))
> - completeRequest(request);
> + completeRequest(request, true);
> }
>
> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> @@ -976,6 +986,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;
> + completeRequest(info->request, true);
> +}
> +
> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)
> {
> delayedCtrls_->push(sensorControls);
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 2bea64d9..bd4c25cc 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -51,6 +51,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 are ready
> + */
> +
> /**
> * \var SoftwareIsp::setSensorControls
> * \brief A signal emitted when the values to write to the sensor controls are
> @@ -136,6 +141,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
> }
>
> ipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);
> + ipa_->metadataReady.connect(this, &SoftwareIsp::saveMetadata);
> ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);
>
> debayer_->moveToThread(&ispWorkerThread_);
> @@ -357,6 +363,11 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
> ispStatsReady.emit(frame, bufferId);
> }
>
> +void SoftwareIsp::saveMetadata(uint32_t frame, const ControlList &metadata)
This function is a bit of a misnomer, as it doesn't save metadata.
You could rename the function, or drop it and use a lambda when
connecting the signal in the SoftwareIsp constructor.
> +{
> + metadataReady.emit(frame, metadata);
> +}
> +
> void SoftwareIsp::inputReady(FrameBuffer *input)
> {
> inputBufferReady.emit(input);
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list