<div dir="ltr"><div dir="ltr">Hi Barnabás,</div><br><div class="gmail_quote gmail_quote_container"><div dir="ltr" class="gmail_attr">On Fri, 6 Jun 2025 at 12:40, Barnabás Pőcze <<a href="mailto:barnabas.pocze@ideasonboard.com">barnabas.pocze@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<br>
<br>
<br>
2025. 06. 06. 12:55 keltezéssel, Naushir Patuck írta:<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>
> Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> Reviewed-by: Jacopo Mondi <<a href="mailto:jacopo.mondi@ideasonboard.com" target="_blank">jacopo.mondi@ideasonboard.com</a>><br>
> ---<br>
> .../pipeline/rpi/common/pipeline_base.cpp | 98 ++++++++-----------<br>
> .../pipeline/rpi/common/pipeline_base.h | 5 +-<br>
> 2 files changed, 42 insertions(+), 61 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..3f0b7abdc59a 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>
> @@ -894,28 +893,12 @@ int PipelineHandlerBase::queueAllBuffers(Camera *camera)<br>
> int ret;<br>
> <br>
> for (auto const stream : data->streams_) {<br>
> - if (!(stream->getFlags() & StreamFlag::External)) {<br>
> - ret = stream->queueAllBuffers();<br>
> - if (ret < 0)<br>
> - return ret;<br>
> - } else {<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>
> + if (stream->getFlags() & StreamFlag::External)<br>
> + continue;<br>
> +<br>
> + ret = stream->queueAllBuffers();<br>
> + if (ret < 0)<br>
> + return ret;<br>
> }<br>
> <br>
> return 0;<br>
> @@ -1412,7 +1395,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 (invalidFrameCount_)<br>
> + md.status = FrameMetadata::Status::FrameError;<br>
> + else if (startupFrameCount_)<br>
> + md.status = FrameMetadata::Status::FrameStartup;<br>
> +<br>
> /*<br>
> * Tag the buffer as completed, returning it to the<br>
> * application.<br>
> @@ -1458,42 +1449,31 @@ 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>
<br>
There are two `request->metadata().clear()` calls in `{PiSP,Vc4}CameraData::tryRunPipeline()`:<br>
<br>
/*<br>
* Clear the request metadata and fill it with some initial non-IPA<br>
* related controls. We clear it first because the request metadata<br>
* may have been populated if we have dropped the previous frame.<br>
*/<br>
request->metadata().clear();<br>
<br>
<br>
Are those still needed?<br></blockquote><div><br></div><div>I believe this can be removed now. I can post a patch on top once this is merged.</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>
Regards,<br>
Barnabás Pőcze<br>
<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 (invalidFrameCount_) {<br>
> + invalidFrameCount_--;<br>
> + LOG(RPI, Debug) << "Decrementing invalid frames to "<br>
> + << invalidFrameCount_;<br>
> + } else if (startupFrameCount_) {<br>
> + startupFrameCount_--;<br>
> + LOG(RPI, Debug) << "Decrementing startup frames to "<br>
> + << startupFrameCount_;<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>
</blockquote></div></div>