<div dir="ltr"><div dir="ltr"><div>Hi Jacopo,</div><div><br></div><div>Thanks for the feedback!</div></div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Tue, 20 May 2025 at 08:02, Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com">jacopo.mondi@ideasonboard.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush<br>
<br>
On Mon, May 19, 2025 at 10:20:51AM +0100, Naushir Patuck wrote:<br>
> Split the pipeline handler drop frame tracking into startup frames and<br>
> invalid frames, as reported by the IPA.<br>
><br>
> Remove the drop buffer handling logic in the pipeline handler. Now all<br>
> image buffers are returned out with the appropriate FrameStatus set<br>
> for startup or invalid frames.<br>
><br>
> Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> ---<br>
> .../pipeline/rpi/common/pipeline_base.cpp | 88 +++++++------------<br>
> .../pipeline/rpi/common/pipeline_base.h | 5 +-<br>
> 2 files changed, 37 insertions(+), 56 deletions(-)<br>
><br>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> index 5956485953a2..21f2daf5bab5 100644<br>
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp<br>
> @@ -659,9 +659,9 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls)<br>
> if (!result.controls.empty())<br>
> data->setSensorControls(result.controls);<br>
><br>
> - /* Configure the number of dropped frames required on startup. */<br>
> - data->dropFrameCount_ = data->config_.disableStartupFrameDrops ?<br>
> - 0 : result.startupFrameCount + result.invalidFrameCount;<br>
> + /* Configure the number of startup and invalid frames reported by the IPA. */<br>
> + data->startupFrameCount_ = result.startupFrameCount;<br>
> + data->invalidFrameCount_ = result.invalidFrameCount;<br>
><br>
> for (auto const stream : data->streams_)<br>
> stream->resetBuffers();<br>
> @@ -678,7 +678,6 @@ int PipelineHandlerBase::start(Camera *camera, const ControlList *controls)<br>
> data->buffersAllocated_ = true;<br>
> }<br>
><br>
> - /* We need to set the dropFrameCount_ before queueing buffers. */<br>
> ret = queueAllBuffers(camera);<br>
> if (ret) {<br>
> LOG(RPI, Error) << "Failed to queue buffers";<br>
> @@ -898,23 +897,6 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)<br>
<br>
Does this function now "queue all buffers" ? Or just prepares the<br>
internal buffer pools<br></blockquote><div><br></div><div>It does indeed queue all buffers from the internal pools. prepareBuffers() is a separate call.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> ret = stream->queueAllBuffers();<br>
> if (ret < 0)<br>
> return ret;<br>
> - } else {<br>
<br>
Without the else the above branch could be made easier to read with<br>
<br>
for (auto const stream : data->streams_) {<br>
if (stream->getFlags() & StreamFlag::External)<br>
continue;<br>
<br>
int ret = stream->queueAllBuffers();<br>
if (ret < 0)<br>
return ret;<br>
}<br>
<br></blockquote><div><br></div><div>Ack. I'll update this for the next version.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> - /*<br>
> - * For external streams, we must queue up a set of internal<br>
> - * buffers to handle the number of drop frames requested by<br>
> - * the IPA. This is done by passing nullptr in queueBuffer().<br>
> - *<br>
> - * The below queueBuffer() call will do nothing if there<br>
> - * are not enough internal buffers allocated, but this will<br>
> - * be handled by queuing the request for buffers in the<br>
> - * RPiStream object.<br>
> - */<br>
> - unsigned int i;<br>
> - for (i = 0; i < data->dropFrameCount_; i++) {<br>
> - ret = stream->queueBuffer(nullptr);<br>
> - if (ret)<br>
> - return ret;<br>
> - }<br>
> }<br>
> }<br>
><br>
> @@ -1412,7 +1394,15 @@ void CameraData::handleStreamBuffer(FrameBuffer *buffer, RPi::Stream *stream)<br>
> * buffer back to the stream.<br>
> */<br>
> Request *request = requestQueue_.empty() ? nullptr : requestQueue_.front();<br>
> - if (!dropFrameCount_ && request && request->findBuffer(stream) == buffer) {<br>
> + if (request && request->findBuffer(stream) == buffer) {<br>
> + FrameMetadata &md = buffer->_d()->metadata();<br>
> +<br>
> + /* Mark the non-converged and invalid frames in the metadata. */<br>
> + if (startupFrameCount_)<br>
> + md.status = FrameMetadata::Status::FrameStartup;<br>
> + if (invalidFrameCount_)<br>
> + md.status = FrameMetadata::Status::FrameError;<br>
> +<br>
> /*<br>
> * Tag the buffer as completed, returning it to the<br>
> * application.<br>
> @@ -1458,42 +1448,32 @@ void CameraData::handleState()<br>
><br>
> void CameraData::checkRequestCompleted()<br>
> {<br>
> - bool requestCompleted = false;<br>
> - /*<br>
> - * If we are dropping this frame, do not touch the request, simply<br>
> - * change the state to IDLE when ready.<br>
> - */<br>
> - if (!dropFrameCount_) {<br>
> - Request *request = requestQueue_.front();<br>
> - if (request->hasPendingBuffers())<br>
> - return;<br>
> + Request *request = requestQueue_.front();<br>
> + if (request->hasPendingBuffers())<br>
> + return;<br>
><br>
> - /* Must wait for metadata to be filled in before completing. */<br>
> - if (state_ != State::IpaComplete)<br>
> - return;<br>
> + /* Must wait for metadata to be filled in before completing. */<br>
> + if (state_ != State::IpaComplete)<br>
> + return;<br>
><br>
> - LOG(RPI, Debug) << "Completing request sequence: "<br>
> - << request->sequence();<br>
> + LOG(RPI, Debug) << "Completing request sequence: "<br>
> + << request->sequence();<br>
><br>
> - pipe()->completeRequest(request);<br>
> - requestQueue_.pop();<br>
> - requestCompleted = true;<br>
> - }<br>
> + pipe()->completeRequest(request);<br>
> + requestQueue_.pop();<br>
><br>
> - /*<br>
> - * Make sure we have three outputs completed in the case of a dropped<br>
> - * frame.<br>
> - */<br>
> - if (state_ == State::IpaComplete &&<br>
> - ((ispOutputCount_ == ispOutputTotal_ && dropFrameCount_) ||<br>
> - requestCompleted)) {<br>
> - LOG(RPI, Debug) << "Going into Idle state";<br>
> - state_ = State::Idle;<br>
> - if (dropFrameCount_) {<br>
> - dropFrameCount_--;<br>
> - LOG(RPI, Debug) << "Dropping frame at the request of the IPA ("<br>
> - << dropFrameCount_ << " left)";<br>
> - }<br>
> + LOG(RPI, Debug) << "Going into Idle state";<br>
> + state_ = State::Idle;<br>
> +<br>
> + if (startupFrameCount_) {<br>
> + startupFrameCount_--;<br>
> + LOG(RPI, Debug) << "Decrementing startup frames to "<br>
> + << startupFrameCount_;<br>
> + }<br>
> + if (invalidFrameCount_) {<br>
> + invalidFrameCount_--;<br>
> + LOG(RPI, Debug) << "Decrementing invalid frames to "<br>
> + << invalidFrameCount_;<br>
<br>
Assume you have<br>
invalidFrameCount_ = 2;<br>
startupFrameCount_ = 3;<br>
<br>
You want to return<br>
<br>
Frame# 1 2 3 4 5 6 7<br>
Status Invalid Invalid Startup Startup Startup Success Success<br>
<br>
right ?<br>
<br>
If you decrement both invalidFrameCount_ and startupFrameCount_ at the<br>
same time, won't you get:<br>
<br>
<br>
Frame# 1 2 3 4 5 6 7<br>
Status Invalid Invalid Startup Success Success Success Success<br>
<br>
instead ?<br></blockquote><div><br></div><div>You are right, this is a bug. I should decrement
invalidFrameCount to 0, then decrement
startupFrameCount. Will fix this in the next version.</div><div><br></div><div>Regards,</div><div>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
> }<br>
> }<br>
><br>
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h<br>
> index aae0c2f35888..6023f9f9d6b3 100644<br>
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h<br>
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h<br>
> @@ -48,7 +48,7 @@ class CameraData : public Camera::Private<br>
> public:<br>
> CameraData(PipelineHandler *pipe)<br>
> : Camera::Private(pipe), state_(State::Stopped),<br>
> - dropFrameCount_(0), buffersAllocated_(false),<br>
> + startupFrameCount_(0), invalidFrameCount_(0), buffersAllocated_(false),<br>
> ispOutputCount_(0), ispOutputTotal_(0)<br>
> {<br>
> }<br>
> @@ -151,7 +151,8 @@ public:<br>
> /* Mapping of CropParams keyed by the output stream order in CameraConfiguration */<br>
> std::map<unsigned int, CropParams> cropParams_;<br>
><br>
> - unsigned int dropFrameCount_;<br>
> + unsigned int startupFrameCount_;<br>
> + unsigned int invalidFrameCount_;<br>
><br>
> /*<br>
> * If set, this stores the value that represets a gain of one for<br>
> --<br>
> 2.43.0<br>
><br>
</blockquote></div></div>