[PATCH v3 1/6] libcamera: software_isp: Track frames and requests

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Mon, Jan 13, 2025 at 02:34:00PM +0100, Milan Zamazal wrote:
> Hardware pipelines track requests and other information related to
> particular frames.  This hasn't been needed in software ISP so far.  But
> in order to be able to track metadata corresponding to a given frame,
> frame-request tracking mechanism starts being useful.  This patch
> introduces the basic tracking structure, actual metadata handling is
> added in the following patch.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 81 ++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..123179b6 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline)
>  
>  class SimplePipelineHandler;
>  
> +struct SimpleFrameInfo {
> +	uint32_t frame;
> +	Request *request;
> +};
> +
> +class SimpleFrames
> +{
> +public:
> +	void create(Request *request);
> +	void destroy(uint32_t frame);
> +	void clear();
> +
> +	SimpleFrameInfo *find(uint32_t frame);
> +	SimpleFrameInfo *find(Request *request);
> +
> +private:
> +	std::map<uint32_t, SimpleFrameInfo> frameInfo_;
> +};
> +
> +void SimpleFrames::create(Request *request)
> +{
> +	uint32_t frame = request->sequence();
> +	auto [it, inserted] = frameInfo_.try_emplace(request->sequence());
> +	ASSERT(inserted);
> +	it->second.frame = frame;
> +	it->second.request = request;
> +}
> +
> +void SimpleFrames::destroy(uint32_t frame)
> +{
> +	frameInfo_.erase(frame);
> +}
> +
> +void SimpleFrames::clear()
> +{
> +	frameInfo_.clear();
> +}
> +
> +SimpleFrameInfo *SimpleFrames::find(uint32_t frame)
> +{
> +	auto info = frameInfo_.find(frame);
> +	if (info == frameInfo_.end())
> +		return nullptr;
> +	return &info->second;
> +}
> +
> +SimpleFrameInfo *SimpleFrames::find(Request *request)
> +{
> +	for (auto &itInfo : frameInfo_) {
> +		SimpleFrameInfo *info = &itInfo.second;
> +
> +		if (info->request == request)
> +			return info;
> +	}
> +
> +	return nullptr;
> +}
> +
>  struct SimplePipelineInfo {
>  	const char *driver;
>  	/*
> @@ -293,11 +351,13 @@ public:
>  
>  	std::unique_ptr<Converter> converter_;
>  	std::unique_ptr<SoftwareIsp> swIsp_;
> +	SimpleFrames frameInfo_;
>  
>  private:
>  	void tryPipeline(unsigned int code, const Size &size);
>  	static std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);
>  
> +	void completeRequest(Request *request);
>  	void conversionInputDone(FrameBuffer *buffer);
>  	void conversionOutputDone(FrameBuffer *buffer);
>  
> @@ -799,7 +859,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  			/* No conversion, just complete the request. */
>  			Request *request = buffer->request();
>  			pipe->completeBuffer(request, buffer);
> -			pipe->completeRequest(request);
> +			completeRequest(request);
>  			return;
>  		}
>  
> @@ -817,7 +877,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  		const RequestOutputs &outputs = conversionQueue_.front();
>  		for (auto &[stream, buf] : outputs.outputs)
>  			pipe->completeBuffer(outputs.request, buf);
> -		pipe->completeRequest(outputs.request);
> +		completeRequest(outputs.request);
>  		conversionQueue_.pop();
>  
>  		return;
> @@ -875,7 +935,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  
>  	/* Otherwise simply complete the request. */
>  	pipe->completeBuffer(request, buffer);
> -	pipe->completeRequest(request);
> +	completeRequest(request);
>  }
>  
>  void SimpleCameraData::clearIncompleteRequests()
> @@ -886,6 +946,14 @@ void SimpleCameraData::clearIncompleteRequests()
>  	}
>  }
>  
> +void SimpleCameraData::completeRequest(Request *request)
> +{
> +	SimpleFrameInfo *info = frameInfo_.find(request);
> +	if (info)

If there's no frame info something is really wrong. I don't think
completing the request makes much sense in that case.

> +		frameInfo_.destroy(info->frame);
> +	pipe()->completeRequest(request);
> +}
> +
>  void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)
>  {
>  	/* Queue the input buffer back for capture. */
> @@ -899,7 +967,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)
>  	/* Complete the buffer and the request. */
>  	Request *request = buffer->request();
>  	if (pipe->completeBuffer(request, buffer))
> -		pipe->completeRequest(request);
> +		completeRequest(request);
>  }
>  
>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)
> @@ -1412,6 +1480,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  
>  	video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);
>  
> +	data->frameInfo_.clear();
>  	data->clearIncompleteRequests();
>  	data->conversionBuffers_.clear();
>  
> @@ -1442,8 +1511,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)
>  
>  	if (data->useConversion_) {
>  		data->conversionQueue_.push({ request, std::move(buffers) });
> -		if (data->swIsp_)
> +		if (data->swIsp_) {
> +			data->frameInfo_.create(request);
>  			data->swIsp_->queueRequest(request->sequence(), request->controls());
> +		}
>  	}
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart