[v3,3/6] pipeline: ipa: rpi: Split RPiCameraData::dropFrameCount_

23477 diff mbox series

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@raspberrypi.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 .../pipeline/rpi/common/pipeline_base.cpp     | 98 ++++++++-----------
 .../pipeline/rpi/common/pipeline_base.h       |  5 +-
 2 files changed, 42 insertions(+), 61 deletions(-)

Hi Barnabás,

On Fri, 6 Jun 2025 at 12:40, Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
wrote:

> Hi
>
>
> 2025. 06. 06. 12:55 keltezéssel, Naushir Patuck írta:
> > 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@raspberrypi.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >   .../pipeline/rpi/common/pipeline_base.cpp     | 98 ++++++++-----------
> >   .../pipeline/rpi/common/pipeline_base.h       |  5 +-
> >   2 files changed, 42 insertions(+), 61 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 5956485953a2..3f0b7abdc59a 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";
> > @@ -894,28 +893,12 @@ int PipelineHandlerBase::queueAllBuffers(Camera
> *camera)
> >       int ret;
> >
> >       for (auto const stream : data->streams_) {
> > -             if (!(stream->getFlags() & StreamFlag::External)) {
> > -                     ret = stream->queueAllBuffers();
> > -                     if (ret < 0)
> > -                             return ret;
> > -             } else {
> > -                     /*
> > -                      * 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;
> > -                     }
> > -             }
> > +             if (stream->getFlags() & StreamFlag::External)
> > +                     continue;
> > +
> > +             ret = stream->queueAllBuffers();
> > +             if (ret < 0)
> > +                     return ret;
> >       }
> >
> >       return 0;
> > @@ -1412,7 +1395,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 (invalidFrameCount_)
> > +                     md.status = FrameMetadata::Status::FrameError;
> > +             else if (startupFrameCount_)
> > +                     md.status = FrameMetadata::Status::FrameStartup;
> > +
> >               /*
> >                * Tag the buffer as completed, returning it to the
> >                * application.
> > @@ -1458,42 +1449,31 @@ 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_) {
>
> There are two `request->metadata().clear()` calls in
> `{PiSP,Vc4}CameraData::tryRunPipeline()`:
>
>         /*
>          * Clear the request metadata and fill it with some initial non-IPA
>          * related controls. We clear it first because the request metadata
>          * may have been populated if we have dropped the previous frame.
>          */
>         request->metadata().clear();
>
>
> Are those still needed?
>

I believe this can be removed now.  I can post a patch on top once this is
merged.
.
Regards,
Naush



>
>
> Regards,
> Barnabás Pőcze
>
> > -             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 (invalidFrameCount_) {
> > +             invalidFrameCount_--;
> > +             LOG(RPI, Debug) << "Decrementing invalid frames to "
> > +                             << invalidFrameCount_;
> > +     } else if (startupFrameCount_) {
> > +             startupFrameCount_--;
> > +             LOG(RPI, Debug) << "Decrementing startup frames to "
> > +                             << startupFrameCount_;
> >       }
> >   }
> >
> > 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
>
>