[PATCH v2 2/5] libcamera: software_isp: Handle queued output buffers on stop

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Mon, Feb 24, 2025 at 07:52:32PM +0100, Milan Zamazal wrote:
>> When SoftwareIsp stops, input and output buffers queued to it may not
>> yet be fully processed.  They will be eventually returned but stop means
>> stop, there should be no processing related actions invoked afterwards.
>> 
>> Let's stop forwarding processed output buffers from the SoftwareIsp
>> slots once SoftwareIsp is stopped.  Let's track the queued output
>> buffers and mark those still pending as canceled in SoftwareIsp::stop
>> and return them to the pipeline handler.
>> 
>> Dealing with input buffers is addressed in a separate patch.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  .../internal/software_isp/software_isp.h      |  1 +
>>  src/libcamera/software_isp/software_isp.cpp   | 22 ++++++++++++++++---
>>  2 files changed, 20 insertions(+), 3 deletions(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index af0dcc24..f2344355 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -101,6 +101,7 @@ private:
>>  
>>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>>  	bool running_;
>> +	std::deque<FrameBuffer *> queuedOutputBuffers_;
>
> #include <deque>
>
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 1a39f4d8..4339e547 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -17,6 +17,7 @@
>>  #include <libcamera/formats.h>
>>  #include <libcamera/stream.h>
>>  
>> +#include "libcamera/internal/framebuffer.h"
>>  #include "libcamera/internal/ipa_manager.h"
>>  #include "libcamera/internal/software_isp/debayer_params.h"
>>  
>> @@ -300,8 +301,11 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
>>  			return -EINVAL;
>>  	}
>>  
>> -	for (auto iter = outputs.begin(); iter != outputs.end(); iter++)
>> -		process(frame, input, iter->second);
>> +	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
>> +		FrameBuffer *const buffer = iter->second;
>> +		queuedOutputBuffers_.push_back(buffer);
>> +		process(frame, input, buffer);
>> +	}
>>  
>>  	return 0;
>>  }
>> @@ -331,6 +335,13 @@ void SoftwareIsp::stop()
>>  
>>  	running_ = false;
>>  	ipa_->stop();
>> +
>> +	for (auto buffer : queuedOutputBuffers_) {
>> +		FrameMetadata &metadata = buffer->_d()->metadata();
>> +		metadata.status = FrameMetadata::FrameCancelled;
>> +		outputBufferReady.emit(buffer);
>> +	}
>> +	queuedOutputBuffers_.clear();
>>  }
>>  
>>  /**
>> @@ -369,7 +380,12 @@ void SoftwareIsp::inputReady(FrameBuffer *input)
>>  
>>  void SoftwareIsp::outputReady(FrameBuffer *output)
>>  {
>> -	outputBufferReady.emit(output);
>> +	if (running_) {
>> +		queuedOutputBuffers_.erase(find(queuedOutputBuffers_.begin(),
>
> s/find/std::find/
>
> and #include <algorithm>.
>
> This being said, the software ISP always processes buffers in order. Do
> we need to call find(), can't we just call pop_front() with an assertion
> to ensure the element matches ?

I can't see any contradiction to this and it works for me so I'll do it
this way in v3.

>> +						queuedOutputBuffers_.end(),
>> +						output));
>> +		outputBufferReady.emit(output);
>> +	}
>>  }
>>  
>>  } /* namespace libcamera */