[v6,2/6] ipa: simple: softisp: Extend to pass metadata

23060 diff mbox series

From: Kieran Bingham <kieran.bingham@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@ideasonboard.com>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Milan Zamazal <mzamazal@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(-)

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@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@ideasonboard.com>
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Signed-off-by: Milan Zamazal <mzamazal@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@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_);