[PATCH v3 2/6] ipa: simple: softisp: Extend to pass metadata
Milan Zamazal
mzamazal at redhat.com
Thu Jan 30 11:42:59 CET 2025
Hi Laurent,
thank you for review.
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> 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.
Probably just to save some lines.
> Can't you follow the same logic as rkisp1 ?
I'll split the function to two, tryCompleteRequest and completeRequest,
it should help.
> 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.
OK, I'll use lambda to avoid naming issues. :-)
>> +{
>> + metadataReady.emit(frame, metadata);
>> +}
>> +
>> void SoftwareIsp::inputReady(FrameBuffer *input)
>> {
>> inputBufferReady.emit(input);
More information about the libcamera-devel
mailing list