[PATCH 1/3] ipa: simple: softisp: Extend to pass metadata

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Tue, Jun 18, 2024 at 12:25:23AM +0100, Kieran Bingham wrote:
> Extend the Simple IPA IPC to support returning a metadata controllist

s/controllist/ControlList/

> when the process call has completed.
> 
> For efficiency, use the existing signal setIspParams() directly
> to avoid having an extra async callback to return the metadata.
> 
> Merge the metadata reported by the ISP into any completing
> request to provide to the application.
> 
> This should be further extended or improved to make use of the frame
> context structures so that the metadata is tied to a specific frame and
> request completion.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  include/libcamera/internal/software_isp/software_isp.h | 5 ++++-
>  include/libcamera/ipa/soft.mojom                       | 2 +-
>  src/ipa/simple/soft_simple.cpp                         | 4 +++-
>  src/libcamera/pipeline/simple/simple.cpp               | 7 +++++--
>  src/libcamera/software_isp/software_isp.cpp            | 6 ++++--
>  5 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index c5338c05b99b..2631f40ef22a 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -80,8 +80,10 @@ public:
>  	Signal<> ispStatsReady;
>  	Signal<const ControlList &> setSensorControls;
>  
> +	const ControlList &metadata() { return metadata_; }
> +
>  private:
> -	void saveIspParams();
> +	void saveIspParams(const ControlList &metadata);
>  	void setSensorCtrls(const ControlList &sensorControls);
>  	void statsReady();
>  	void inputReady(FrameBuffer *input);
> @@ -92,6 +94,7 @@ private:
>  	SharedMemObject<DebayerParams> sharedParams_;
>  	DebayerParams debayerParams_;
>  	DmaBufAllocator dmaHeap_;
> +	ControlList metadata_;
>  
>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>  };
> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom
> index 3aa2066ebaa0..1a0221df7660 100644
> --- a/include/libcamera/ipa/soft.mojom
> +++ b/include/libcamera/ipa/soft.mojom
> @@ -24,5 +24,5 @@ interface IPASoftInterface {
>  
>  interface IPASoftEventInterface {
>  	setSensorControls(libcamera.ControlList sensorControls);
> -	setIspParams();
> +	setIspParams(libcamera.ControlList metadata);
>  };
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b7746ce09206..09c7b575301e 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -250,6 +250,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>  	SwIspStats::Histogram histogram = stats_->yHistogram;
>  	if (ignoreUpdates_ > 0)
>  		blackLevel_.update(histogram);
> +
> +	ControlList metadata(controls::controls);
>  	const uint8_t blackLevel = blackLevel_.get();
>  
>  	/*
> @@ -303,7 +305,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)
>  		params_->blue[i] = gammaTable_[idx];
>  	}
>  
> -	setIspParams.emit();
> +	setIspParams.emit(metadata);
>  
>  	/* \todo Switch to the libipa/algorithm.h API someday. */
>  
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index eb36578e320e..accdb744d61b 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -860,10 +860,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)
>  			return;
>  		}
>  
> -		if (converter_)
> +		if (converter_) {
>  			converter_->queueBuffers(buffer, conversionQueue_.front());
> -		else
> +		} else {
>  			swIsp_->queueBuffers(buffer, conversionQueue_.front());
> +			if (request)
> +				request->metadata().merge(swIsp_->metadata());

You're shoving metadata in a random request here, that really makes me
cringe. It's not just that there's no frame context tracking on the IPA
side, the pipeline handler side is wrong too.

> +		}
>  
>  		conversionQueue_.pop();
>  		return;
> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
> index 20fb6f48fdf9..efbbea2d2279 100644
> --- a/src/libcamera/software_isp/software_isp.cpp
> +++ b/src/libcamera/software_isp/software_isp.cpp
> @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)
>  SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)
>  	: dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |
>  		   DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |
> -		   DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)
> +		   DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf),
> +	  metadata_(controls::controls)
>  {
>  	/*
>  	 * debayerParams_ must be initialized because the initial value is used for
> @@ -349,9 +350,10 @@ void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)
>  			       ConnectionTypeQueued, input, output, debayerParams_);
>  }
>  
> -void SoftwareIsp::saveIspParams()
> +void SoftwareIsp::saveIspParams(const ControlList &metadata)
>  {
>  	debayerParams_ = *sharedParams_;
> +	metadata_.merge(metadata, ControlList::MergePolicy::OverwriteExisting);
>  }
>  
>  void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)

-- 
Regards,

Laurent Pinchart