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

[David Plowman]

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?

> +
> +       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!

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
>