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

[David Plowman]

Hi Naush

Thanks for the patch.

On Mon, 19 May 2025 at 10:23, Naushir Patuck <naush at raspberrypi.com> 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)
>                         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;
> -                       }
>                 }
>         }
>
> @@ -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;

Maybe "if (validFrameCount_) ..." followed by "else if
(startupFrameCount_) ..." would be slightly easier to follow? I know,
makes no difference.

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks
David

> +
>                 /*
>                  * 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_;
>         }
>  }
>
> 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
>