[PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata

[Laurent Pinchart]

Hi Milan,

On Thu, Mar 27, 2025 at 05:46:10PM +0100, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > On Wed, Mar 26, 2025 at 01:48:51PM +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      | 33 ++++++++++++++++++-
> >>  src/libcamera/software_isp/software_isp.cpp   |  9 +++++
> >>  5 files changed, 45 insertions(+), 7 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);
> >>  	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 2281c0a6..f34df926 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,13 @@ private:
> >>  	void tryPipeline(unsigned int code, const Size &size);
> >>  	static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
> >>  
> >> +	void tryCompleteRequest(Request *request);
> >>  	void completeRequest(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);
> >>  };
> >>  
> >> @@ -600,6 +604,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);
> >>  		}
> >>  	}
> >> @@ -932,6 +937,21 @@ void SimpleCameraData::clearIncompleteRequests()
> >>  	}
> >>  }
> >>  
> >> +void SimpleCameraData::tryCompleteRequest(Request *request)
> >> +{
> >> +	if (request->hasPendingBuffers())
> >> +		return;
> >> +
> >> +	SimpleFrameInfo *info = frameInfo_.find(request);
> >> +	if (!info)
> >> +		return;
> >> +
> >> +	if (!info->metadataProcessed)
> >> +		return;
> >> +
> >> +	completeRequest(request);
> >> +}
> >
> > I may not have expressed my thoughts correctly in the review of v3. What
> > I proposed was having a single function, named tryCompleteRequest(), to
> > replace your current completeRequest() implementation. The function
> > would ensure that all the pieces of data necessary to complete the
> > request are there, and then proceed to complete it.
> >
> > As far as I can tell, the only thing you will need to change in addition
> > to merging the completeRequest() function into tryCompleteRequest() 
> 
> frameInfo_ is used only with software ISP so it would have been handled
> conditionally too.  I think this is where we misunderstood each other.
> 
> Actually, a bug was introduced in v4 of the preceding patch, in that
> frameInfo_ is checked in completeRequest whether it should be present
> (with swisp) or not (without swisp).
> 
> Let's try to stop the increasing confusion.
> 
> > is to set metadataProcessed to true when the pipeline is not expected
> > to produced metadata (that is, when no soft ISP is used). This could
> > be done for instance when creating the SimpleFrameInfo entry. 
> 
> OK, let's always create SimpleFrameInfo for a request, whether software
> ISP is used or not.  This should also avoid further confusion once
> multiple streams are used (in the patches for raw streams).  And instead
> of setting metadataProcessed to true when no metadata is expected, I'd
> introduce and set an additional metadataRequired; then it is clearer
> what is what and it can be handled in tryCompleteRequest.  Now a single
> completion method, tryCompleteRequest, makes sense.
> 
> > You should also double-check the cancellation paths, to set
> > metadataProcessed to true if the frame is cancelled before being
> > processed by the soft ISP.
> 
> Not pretty but it is required only in a single place and not much
> confusing there, so OK.  At least until we possibly add more stuff to
> the frame info and the accompanying completion checks.
> 
> I hope we are now getting in sync with each other.

I believe so :-) Thanks for the clarifications. I'll review the latest
version of the series.

> >> +
> >>  void SimpleCameraData::completeRequest(Request *request)
> >>  {
> >>  	SimpleFrameInfo *info = frameInfo_.find(request);
> >> @@ -957,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);
> >> +		tryCompleteRequest(request);
> >>  }
> >>  
> >>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> >> @@ -966,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;
> >> +	tryCompleteRequest(info->request);
> >> +}
> >> +
> >>  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 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