[PATCH v1 3/6] pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_
Naushir Patuck
naush at raspberrypi.com
Tue May 20 10:14:33 CEST 2025
Hi Jacopo,
Thanks for the feedback!
On Tue, 20 May 2025 at 08:02, Jacopo Mondi <jacopo.mondi at ideasonboard.com>
wrote:
> 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
>
It does indeed queue all buffers from the internal pools. prepareBuffers()
is a separate call.
>
> > 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;
> }
>
>
Ack. I'll update this for the next version.
>
> > - /*
> > - * 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 ?
>
You are right, this is a bug. I should decrement invalidFrameCount to 0,
then decrement startupFrameCount. Will fix this in the next version.
Regards,
Naush
>
> > }
> > }
> >
> > 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20250520/c47af3c0/attachment.htm>
More information about the libcamera-devel
mailing list