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

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Wed, Mar 26, 2025 at 01:48:50PM +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 | 85 ++++++++++++++++++++++--
>  1 file changed, 80 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf3..2281c0a6 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 {

If you add a constructor here:

	SimpleFrameInfo(uint32_t f, Request *r)
		: frame(f), request(r)
	{
	}

...

> +	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());

	const uint32_t frame = request->sequence();
	auto [it, inserted] = frameInfo_.try_emplace(frame);

> +	ASSERT(inserted);
> +	it->second.frame = frame;
> +	it->second.request = request;

... then you can write

	auto [it, inserted] = frameInfo_.try_emplace(frame, frame, request);
	ASSERT(inserted);

> +}
> +
> +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;
> +	}

That's fairly inefficient. Can't you drop this function and replace

	find(request)

with

	find(request->sequence())

? Or keep it as a convenience and simply

	return find(request->sequence());

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +
> +	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);
>  
> @@ -785,7 +845,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;
>  		}
>  
> @@ -803,7 +863,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;
> @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)
>  
>  	/* Otherwise simply complete the request. */
>  	pipe->completeBuffer(request, buffer);
> -	pipe->completeRequest(request);
> +	completeRequest(request);
>  }
>  
>  void SimpleCameraData::clearIncompleteRequests()
> @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests()
>  	}
>  }
>  
> +void SimpleCameraData::completeRequest(Request *request)
> +{
> +	SimpleFrameInfo *info = frameInfo_.find(request);
> +	if (!info) {
> +		/* Something is really wrong, let's return. */
> +		return;
> +	}
> +
> +	frameInfo_.destroy(info->frame);
> +	pipe()->completeRequest(request);
> +}
> +
>  void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)
>  {
>  	/* Queue the input buffer back for capture. */
> @@ -885,7 +957,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)
> @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
>  
>  	video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);
>  
> +	data->frameInfo_.clear();
>  	data->clearIncompleteRequests();
>  	data->conversionBuffers_.clear();
>  
> @@ -1428,8 +1501,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