[PATCH v2 1/5] libcamera: software_isp: Track frames and requests

[Barnabás Pőcze]

Hi


2024. december 19., csütörtök 22:10 keltezéssel, Milan Zamazal <mzamazal at redhat.com> írta:

> 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 | 87 ++++++++++++++++++++++--
>  1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..07c1cf1f 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -181,6 +181,70 @@ 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();
> +	SimpleFrameInfo *info = new SimpleFrameInfo;

Any reason for dynamic allocation? I took a cursory look over the later changes
and didn't see anything. If there is indeed no reason, please do the following:

  std::map<uint32_t, SimpleFrameInfo> frameInfo_;
  ...
  auto [it, inserted] = frameInfo_.try_emplace(request->sequence());
  ASSERT(inserted); // or whatever is appropriate when the sequence number already exists
  
  it->second.... = ...;


Regards,
Barnabás Pőcze


> +	info->frame = frame;
> +	info->request = request;
> +	frameInfo_[frame] = info;
> +}
> +
> +void SimpleFrames::destroy(uint32_t frame)
> +{
> +	SimpleFrameInfo *info = find(frame);
> +	if (!info)
> +		return;
> +	delete info;
> +	frameInfo_.erase(frame);
> +}
> +
> +void SimpleFrames::clear()
> +{
> +	for (const auto &info : frameInfo_)
> +		delete info.second;
> +	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 +357,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 +865,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 +883,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 +941,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 +952,14 @@ void SimpleCameraData::clearIncompleteRequests()
>  	}
>  }
> 
> +void SimpleCameraData::completeRequest(Request *request)
> +{
> +	SimpleFrameInfo *info = frameInfo_.find(request);
> +	if (info)
> +		frameInfo_.destroy(info->frame);
> +	pipe()->completeRequest(request);
> +}
> +
>  void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)
>  {
>  	/* Queue the input buffer back for capture. */
> @@ -899,7 +973,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 +1486,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)
> 
>  	video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);
> 
> +	data->frameInfo_.clear();
>  	data->clearIncompleteRequests();
>  	data->conversionBuffers_.clear();
> 
> @@ -1442,8 +1517,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;
> --
> 2.44.2
> 
>