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

[Milan Zamazal]

Hi Barnabás,

thank you for review.

Barnabás Pőcze <pobrn at protonmail.com> writes:

> 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. 

I followed the patterns from the other pipelines but indeed, I don't
think dynamic allocation is needed here.  I'll change it in v3 as you
propose.

> 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
>> 
>>