[PATCH v1 2/6] ipa: rpi: Replace dropFrameCount in the IPA -> PH interface

[Naushir Patuck]

Hi David,


On Mon, 19 May 2025 at 11:17, David Plowman <david.plowman at raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks for the patch.
>
> On Mon, 19 May 2025 at 10:23, Naushir Patuck <naush at raspberrypi.com>
> wrote:
> >
> > Replace the dropFrameCount parameter returned from ipa::start() to the
> > pipeline handler by startupFrameCount and invalidFrameCount. The former
> > counts the number of frames required for AWB/AGC to converge, and the
> > latter counts the number of invalid frames produced by the sensor when
> > starting up.
> >
> > In the pipeline handler, use the sum of these 2 values to replicate the
> > existing dropFrameCount behaviour.
> >
> > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.mojom            |  3 ++-
> >  src/ipa/rpi/common/ipa_base.cpp                    | 14 ++++++++------
> >  .../pipeline/rpi/common/pipeline_base.cpp          |  4 ++--
> >  3 files changed, 12 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/raspberrypi.mojom
> b/include/libcamera/ipa/raspberrypi.mojom
> > index e30c70bde08b..12b083e9d04b 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -52,7 +52,8 @@ struct ConfigResult {
> >
> >  struct StartResult {
> >         libcamera.ControlList controls;
> > -       int32 dropFrameCount;
> > +       int32 startupFrameCount;
> > +       int32 invalidFrameCount;
> >  };
> >
> >  struct PrepareParams {
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp
> b/src/ipa/rpi/common/ipa_base.cpp
> > index e0a93daa9db2..c15f8a7bf71e 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -324,6 +324,7 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >          * "mistrusted", which depends on whether this is a startup from
> cold,
> >          * or merely a mode switch in a running system.
> >          */
> > +       unsigned int agcConvergenceFrames = 0, awbConvergenceFrames = 0;
> >         frameCount_ = 0;
> >         if (firstStart_) {
> >                 dropFrameCount_ = helper_->hideFramesStartup();
> > @@ -336,7 +337,6 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >                  * (mistrustCount_) that they won't see. But if zero
> (i.e.
> >                  * no convergence necessary), no frames need to be
> dropped.
> >                  */
> > -               unsigned int agcConvergenceFrames = 0;
> >                 RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> >                         controller_.getAlgorithm("agc"));
> >                 if (agc) {
> > @@ -345,7 +345,6 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >                                 agcConvergenceFrames += mistrustCount_;
> >                 }
> >
> > -               unsigned int awbConvergenceFrames = 0;
> >                 RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
> >                         controller_.getAlgorithm("awb"));
> >                 if (awb) {
> > @@ -353,15 +352,18 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >                         if (awbConvergenceFrames)
> >                                 awbConvergenceFrames += mistrustCount_;
> >                 }
> > -
> > -               dropFrameCount_ = std::max({ dropFrameCount_,
> agcConvergenceFrames, awbConvergenceFrames });
> > -               LOG(IPARPI, Debug) << "Drop " << dropFrameCount_ << "
> frames on startup";
> >         } else {
> >                 dropFrameCount_ = helper_->hideFramesModeSwitch();
> >                 mistrustCount_ = helper_->mistrustFramesModeSwitch();
> >         }
> >
> > -       result->dropFrameCount = dropFrameCount_;
> > +       result->startupFrameCount = std::max({ agcConvergenceFrames,
> awbConvergenceFrames });
> > +       result->invalidFrameCount = dropFrameCount_;
> > +
> > +       dropFrameCount_ = std::max({ dropFrameCount_,
> agcConvergenceFrames, awbConvergenceFrames });
>
> TBH, I'm not 100% clear why we do that max operation (I realise the
> old version does it too), do you remember?
>

I think it's just accounting for the worst case where we run the prepare()
/ process() cycle on every frame during startup.


>
> > +
> > +       LOG(IPARPI, Debug) << "Startup frames: " <<
> result->startupFrameCount
> > +                          << " Invalid frames: " <<
> result->invalidFrameCount;
>
> I just wondered very slightly whether it might be slightly easier to
> follow if we don't use "dropFrameCount" or "dropFrameCount_" here,
> because in the PH it means something slightly different (is that
> right?).  So maybe replace it by "invalidFrameCount"? I know, very
> minor!
>
> >
> >         firstStart_ = false;
> >         lastRunTimestamp_ = 0;
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index 1f13e5230fae..5956485953a2 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -660,8 +660,8 @@ int PipelineHandlerBase::start(Camera *camera, const
> ControlList *controls)
> >                 data->setSensorControls(result.controls);
> >
> >         /* Configure the number of dropped frames required on startup. */
> > -       data->dropFrameCount_ = data->config_.disableStartupFrameDrops
> > -                             ? 0 : result.dropFrameCount;
>
> Can you say why we return "result.dropFrameCount" here and not
> "result.invalidFrameCount"? I've obviously forgotten what's going on
> here!
>

This "models" the existing behavior, but changes in subsequent patches to
account for result.invalidFrameCount correctly.

Regards,
Naush



>
> Assuming this is all just recollection failure on my part:
>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
>
> Thanks
> David
>
> > +       data->dropFrameCount_ = data->config_.disableStartupFrameDrops ?
> > +                       0 : result.startupFrameCount +
> result.invalidFrameCount;
> >
> >         for (auto const stream : data->streams_)
> >                 stream->resetBuffers();
> > --
> > 2.43.0
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20250520/748bc51b/attachment.htm>