[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