[PATCH v2 3/5] libcamera: software_isp: Handle queued input 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:33PM +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 input buffers from SoftwareIsp slots
>> when SoftwareIsp is stopped.  Let's track the queued input buffers and
>> return them back for capture in SoftwareIsp::stop().
>> 
>> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
>> ---
>>  .../internal/software_isp/software_isp.h         |  1 +
>>  src/libcamera/software_isp/software_isp.cpp      | 16 +++++++++++++++-
>>  2 files changed, 16 insertions(+), 1 deletion(-)
>> 
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index f2344355..bfe34725 100644
>> --- a/include/libcamera/internal/software_isp/software_isp.h
>> +++ b/include/libcamera/internal/software_isp/software_isp.h
>> @@ -102,6 +102,7 @@ private:
>>  	std::unique_ptr<ipa::soft::IPAProxySoft> ipa_;
>>  	bool running_;
>>  	std::deque<FrameBuffer *> queuedOutputBuffers_;
>> +	std::deque<FrameBuffer *> queuedInputBuffers_;
>
> I'd move input before output.
>
>>  };
>>  
>>  } /* namespace libcamera */
>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp
>> index 4339e547..3a605ab2 100644
>> --- a/src/libcamera/software_isp/software_isp.cpp
>> +++ b/src/libcamera/software_isp/software_isp.cpp
>> @@ -301,6 +301,8 @@ int SoftwareIsp::queueBuffers(uint32_t frame, FrameBuffer *input,
>>  			return -EINVAL;
>>  	}
>>  
>> +	queuedInputBuffers_.push_back(input);
>> +
>>  	for (auto iter = outputs.begin(); iter != outputs.end(); iter++) {
>>  		FrameBuffer *const buffer = iter->second;
>>  		queuedOutputBuffers_.push_back(buffer);
>> @@ -327,6 +329,9 @@ int SoftwareIsp::start()
>>  
>>  /**
>>   * \brief Stops the Software ISP streaming operation
>> + *
>> + * All pending buffers are returned back (output buffers as canceled) before
>> + * this method finishes.
>
> Shouldn't the input buffer be cancelled too ?

Is it necessary to cancel input buffers?  Aren't they just returned for
re-use regardless of their contents (unlike the output buffers where the
validity of the contents matters)?  The input buffer lands in
V4L2VideoDevice::queueBuffer() and I can't see any use of the status
there.

>>   */
>>  void SoftwareIsp::stop()
>>  {
>> @@ -342,6 +347,10 @@ void SoftwareIsp::stop()
>>  		outputBufferReady.emit(buffer);
>>  	}
>>  	queuedOutputBuffers_.clear();
>> +
>> +	for (auto buffer : queuedInputBuffers_)
>> +		inputBufferReady.emit(buffer);
>> +	queuedInputBuffers_.clear();
>>  }
>>  
>>  /**
>> @@ -375,7 +384,12 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)
>>  
>>  void SoftwareIsp::inputReady(FrameBuffer *input)
>>  {
>> -	inputBufferReady.emit(input);
>> +	if (running_) {
>> +		queuedInputBuffers_.erase(find(queuedInputBuffers_.begin(),
>> +					       queuedInputBuffers_.end(),
>> +					       input));
>
> Same comment as for 2/5.
>
>> +		inputBufferReady.emit(input);
>> +	}
>>  }
>>  
>>  void SoftwareIsp::outputReady(FrameBuffer *output)