[PATCH v1 3/6] pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_

[Jacopo Mondi]

Hi Naush

On Mon, May 19, 2025 at 10:20:51AM +0100, Naushir Patuck wrote:
> Split the pipeline handler drop frame tracking into startup frames and
> invalid frames, as reported by the IPA.
>
> Remove the drop buffer handling logic in the pipeline handler. Now all
> image buffers are returned out with the appropriate FrameStatus set
> for startup or invalid frames.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  .../pipeline/rpi/common/pipeline_base.cpp     | 88 +++++++------------
>  .../pipeline/rpi/common/pipeline_base.h       |  5 +-
>  2 files changed, 37 insertions(+), 56 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index 5956485953a2..21f2daf5bab5 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -659,9 +659,9 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls)
>  	if (!result.controls.empty())
>  		data->setSensorControls(result.controls);
>
> -	/* Configure the number of dropped frames required on startup. */
> -	data->dropFrameCount_ = data->config_.disableStartupFrameDrops ?
> -			0 : result.startupFrameCount + result.invalidFrameCount;
> +	/* Configure the number of startup and invalid frames reported by the IPA. */
> +	data->startupFrameCount_ = result.startupFrameCount;
> +	data->invalidFrameCount_ = result.invalidFrameCount;
>
>  	for (auto const stream : data->streams_)
>  		stream->resetBuffers();
> @@ -678,7 +678,6 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls)
>  		data->buffersAllocated_ = true;
>  	}
>
> -	/* We need to set the dropFrameCount_ before queueing buffers. */
>  	ret = queueAllBuffers(camera);
>  	if (ret) {
>  		LOG(RPI, Error) << "Failed to queue buffers";
> @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)

Does this function now "queue all buffers" ? Or just prepares the
internal buffer pools

>  			ret = stream->queueAllBuffers();
>  			if (ret < 0)
>  				return ret;
> -		} else {

Without the else the above branch could be made easier to read with

	for (auto const stream : data->streams_) {
		if (stream->getFlags() & StreamFlag::External)
                        continue;

                int ret = stream->queueAllBuffers();
                if (ret < 0)
                        return ret;
        }


> -			/*
> -			 * For external streams, we must queue up a set of internal
> -			 * buffers to handle the number of drop frames requested by
> -			 * the IPA. This is done by passing nullptr in queueBuffer().
> -			 *
> -			 * The below queueBuffer() call will do nothing if there
> -			 * are not enough internal buffers allocated, but this will
> -			 * be handled by queuing the request for buffers in the
> -			 * RPiStream object.
> -			 */
> -			unsigned int i;
> -			for (i = 0; i < data->dropFrameCount_; i++) {
> -				ret = stream->queueBuffer(nullptr);
> -				if (ret)
> -					return ret;
> -			}
>  		}
>  	}
>
> @@ -1412,7 +1394,15 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)
>  	 * buffer back to the stream.
>  	 */
>  	Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();
> -	if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {
> +	if (request && request->findBuffer(stream) == buffer) {
> +		FrameMetadata &md = buffer->_d()->metadata();
> +
> +		/* Mark the non-converged and invalid frames in the metadata. */
> +		if (startupFrameCount_)
> +			md.status = FrameMetadata::Status::FrameStartup;
> +		if (invalidFrameCount_)
> +			md.status = FrameMetadata::Status::FrameError;
> +
>  		/*
>  		 * Tag the buffer as completed, returning it to the
>  		 * application.
> @@ -1458,42 +1448,32 @@ void CameraData::handleState()
>
>  void CameraData::checkRequestCompleted()
>  {
> -	bool requestCompleted = false;
> -	/*
> -	 * If we are dropping this frame, do not touch the request, simply
> -	 * change the state to IDLE when ready.
> -	 */
> -	if (!dropFrameCount_) {
> -		Request *request = requestQueue_.front();
> -		if (request->hasPendingBuffers())
> -			return;
> +	Request *request = requestQueue_.front();
> +	if (request->hasPendingBuffers())
> +		return;
>
> -		/* Must wait for metadata to be filled in before completing. */
> -		if (state_ != State::IpaComplete)
> -			return;
> +	/* Must wait for metadata to be filled in before completing. */
> +	if (state_ != State::IpaComplete)
> +		return;
>
> -		LOG(RPI, Debug) << "Completing request sequence: "
> -				<< request->sequence();
> +	LOG(RPI, Debug) << "Completing request sequence: "
> +			<< request->sequence();
>
> -		pipe()->completeRequest(request);
> -		requestQueue_.pop();
> -		requestCompleted = true;
> -	}
> +	pipe()->completeRequest(request);
> +	requestQueue_.pop();
>
> -	/*
> -	 * Make sure we have three outputs completed in the case of a dropped
> -	 * frame.
> -	 */
> -	if (state_ == State::IpaComplete &&
> -	    ((ispOutputCount_ == ispOutputTotal_ && dropFrameCount_) ||
> -	     requestCompleted)) {
> -		LOG(RPI, Debug) << "Going into Idle state";
> -		state_ = State::Idle;
> -		if (dropFrameCount_) {
> -			dropFrameCount_--;
> -			LOG(RPI, Debug) << "Dropping frame at the request of the IPA ("
> -					<< dropFrameCount_ << " left)";
> -		}
> +	LOG(RPI, Debug) << "Going into Idle state";
> +	state_ = State::Idle;
> +
> +	if (startupFrameCount_) {
> +		startupFrameCount_--;
> +		LOG(RPI, Debug) << "Decrementing startup frames to "
> +				<< startupFrameCount_;
> +	}
> +	if (invalidFrameCount_) {
> +		invalidFrameCount_--;
> +		LOG(RPI, Debug) << "Decrementing invalid frames to "
> +				<< invalidFrameCount_;

Assume you have
        invalidFrameCount_ = 2;
        startupFrameCount_ = 3;

You want to return

Frame#       1       2       3       4       5       6       7
Status    Invalid  Invalid Startup Startup Startup Success Success

right ?

If you decrement both invalidFrameCount_ and startupFrameCount_ at the
same time, won't you get:


Frame#       1       2       3       4       5       6       7
Status    Invalid  Invalid Startup Success Success Success Success

instead ?

>  	}
>  }
>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index aae0c2f35888..6023f9f9d6b3 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -48,7 +48,7 @@ class CameraData : public Camera::Private
>  public:
>  	CameraData(PipelineHandler *pipe)
>  		: Camera::Private(pipe), state_(State::Stopped),
> -		  dropFrameCount_(0), buffersAllocated_(false),
> +		  startupFrameCount_(0), invalidFrameCount_(0), buffersAllocated_(false),
>  		  ispOutputCount_(0), ispOutputTotal_(0)
>  	{
>  	}
> @@ -151,7 +151,8 @@ public:
>  	/* Mapping of CropParams keyed by the output stream order in CameraConfiguration */
>  	std::map<unsigned int, CropParams> cropParams_;
>
> -	unsigned int dropFrameCount_;
> +	unsigned int startupFrameCount_;
> +	unsigned int invalidFrameCount_;
>
>  	/*
>  	 * If set, this stores the value that represets a gain of one for
> --
> 2.43.0
>