[v2,2/2] ipa: rpi: Gate CamHelper::prepare() by the mistrust count

27619 diff mbox series

The current logic in the IPA gates the controller/algorithms from
running on a the frame if mistrustCount_ > 0. This logic is somewhat
incorrect, as the image data is indeed valid, just the embedded data and
other bits of metadata are possibly invalid.

This commit changes the gating so that the controller/algorithms do run
on frames where mistrustCount_ > 0, and CamHelper::prepare() gets gated.
CamHelper::prepare() is where the embedded data parsing occurs, and if
it does not run, we simply use the DelayedControls values provided by
the pipeline handler.

Bug: https://github.com/raspberrypi/libcamera/issues/345
Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/common/ipa_base.cpp | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

Hi Naush,

Quoting Naushir Patuck (2026-08-05 15:09:22)
> The current logic in the IPA gates the controller/algorithms from
> running on a the frame if mistrustCount_ > 0. This logic is somewhat
> incorrect, as the image data is indeed valid, just the embedded data and
> other bits of metadata are possibly invalid.
> 
> This commit changes the gating so that the controller/algorithms do run
> on frames where mistrustCount_ > 0, and CamHelper::prepare() gets gated.
> CamHelper::prepare() is where the embedded data parsing occurs, and if
> it does not run, we simply use the DelayedControls values provided by
> the pipeline handler.
> 
> Bug: https://github.com/raspberrypi/libcamera/issues/345

the CI Lint phase reminds me that 'Bug' is not a defined trailer for us
anymore.

We have these trailers accepted by checkstyle:

    known_trailers = {
        'Acked-by': email_regex,
        'Closes': link_regex,
        'Co-developed-by': email_regex,
        'Fixes': commit_regex,
        'Link': link_regex,
        'Reported-by': validate_reported_by,
        'Reviewed-by': email_regex,
        'Signed-off-by': email_regex,
        'Suggested-by': email_regex,
        'Tested-by': email_regex,
    }



Would you prefer Link: or Closes: ?

I'll update when merging.

--
Kieran


> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index aa17d39fea7b..7e00c2799b4e 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -355,26 +355,17 @@ void IpaBase::start(const ControlList &controls, StartResult *result)
>  
>                 /*
>                  * Query the AGC/AWB for how many frames they may take to
> -                * converge sufficiently. Where these numbers are non-zero
> -                * we must allow for the frames with bad metadata
> -                * (mistrustCount_) that they won't see. But if zero (i.e.
> -                * no convergence necessary), no frames need to be dropped.
> +                * converge sufficiently.
>                  */
>                 RPiController::AgcAlgorithm *agc = dynamic_cast<RPiController::AgcAlgorithm *>(
>                         controller_.getAlgorithm("agc"));
> -               if (agc) {
> +               if (agc)
>                         agcConvergenceFrames = agc->getConvergenceFrames();
> -                       if (agcConvergenceFrames)
> -                               agcConvergenceFrames += mistrustCount_;
> -               }
>  
>                 RPiController::AwbAlgorithm *awb = dynamic_cast<RPiController::AwbAlgorithm *>(
>                         controller_.getAlgorithm("awb"));
> -               if (awb) {
> +               if (awb)
>                         awbConvergenceFrames = awb->getConvergenceFrames();
> -                       if (awbConvergenceFrames)
> -                               awbConvergenceFrames += mistrustCount_;
> -               }
>         } else {
>                 invalidCount_ = helper_->hideFramesModeSwitch();
>                 mistrustCount_ = helper_->mistrustMetadataModeSwitch();
> @@ -470,8 +461,11 @@ void IpaBase::prepareIsp(const PrepareParams &params)
>         /*
>          * This may overwrite the DeviceStatus using values from the sensor
>          * metadata, and may also do additional custom processing.
> +        *
> +        * Only call CamHelper::prepare() when we know the metadata can be trusted.
>          */
> -       helper_->prepare(embeddedBuffer, rpiMetadata);
> +       if (frameCount_ >= mistrustCount_)
> +               helper_->prepare(embeddedBuffer, rpiMetadata);
>  
>         bool delayedRequestControls = false;
>         delayedMetadata.get<bool>("ipa.request_controls", delayedRequestControls);
> @@ -527,7 +521,7 @@ void IpaBase::processStats(const ProcessParams &params)
>         unsigned int ipaContext = params.ipaContext % rpiMetadata_.size();
>         RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
>  
> -       if (processPending_ && frameCount_ >= mistrustCount_) {
> +       if (processPending_) {
>                 auto it = buffers_.find(params.buffers.stats);
>                 if (it == buffers_.end()) {
>                         LOG(IPARPI, Error) << "Could not find stats buffer!";
> -- 
> 2.53.0
>

On Wed, 5 Aug 2026, 4:48 pm Kieran Bingham, <kieran.bingham@ideasonboard.com>
wrote:

> Hi Naush,
>
> Quoting Naushir Patuck (2026-08-05 15:09:22)
> > The current logic in the IPA gates the controller/algorithms from
> > running on a the frame if mistrustCount_ > 0. This logic is somewhat
> > incorrect, as the image data is indeed valid, just the embedded data and
> > other bits of metadata are possibly invalid.
> >
> > This commit changes the gating so that the controller/algorithms do run
> > on frames where mistrustCount_ > 0, and CamHelper::prepare() gets gated.
> > CamHelper::prepare() is where the embedded data parsing occurs, and if
> > it does not run, we simply use the DelayedControls values provided by
> > the pipeline handler.
> >
> > Bug: https://github.com/raspberrypi/libcamera/issues/345
>
> the CI Lint phase reminds me that 'Bug' is not a defined trailer for us
> anymore.
>
> We have these trailers accepted by checkstyle:
>
>     known_trailers = {
>         'Acked-by': email_regex,
>         'Closes': link_regex,
>         'Co-developed-by': email_regex,
>         'Fixes': commit_regex,
>         'Link': link_regex,
>         'Reported-by': validate_reported_by,
>         'Reviewed-by': email_regex,
>         'Signed-off-by': email_regex,
>         'Suggested-by': email_regex,
>         'Tested-by': email_regex,
>     }
>
>
>
> Would you prefer Link: or Closes: ?
>

Maybe Closes:?


> I'll update when merging.
>
> --
> Kieran
>
>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  src/ipa/rpi/common/ipa_base.cpp | 22 ++++++++--------------
> >  1 file changed, 8 insertions(+), 14 deletions(-)
> >
> > diff --git a/src/ipa/rpi/common/ipa_base.cpp
> b/src/ipa/rpi/common/ipa_base.cpp
> > index aa17d39fea7b..7e00c2799b4e 100644
> > --- a/src/ipa/rpi/common/ipa_base.cpp
> > +++ b/src/ipa/rpi/common/ipa_base.cpp
> > @@ -355,26 +355,17 @@ void IpaBase::start(const ControlList &controls,
> StartResult *result)
> >
> >                 /*
> >                  * Query the AGC/AWB for how many frames they may take to
> > -                * converge sufficiently. Where these numbers are
> non-zero
> > -                * we must allow for the frames with bad metadata
> > -                * (mistrustCount_) that they won't see. But if zero
> (i.e.
> > -                * no convergence necessary), no frames need to be
> dropped.
> > +                * converge sufficiently.
> >                  */
> >                 RPiController::AgcAlgorithm *agc =
> dynamic_cast<RPiController::AgcAlgorithm *>(
> >                         controller_.getAlgorithm("agc"));
> > -               if (agc) {
> > +               if (agc)
> >                         agcConvergenceFrames =
> agc->getConvergenceFrames();
> > -                       if (agcConvergenceFrames)
> > -                               agcConvergenceFrames += mistrustCount_;
> > -               }
> >
> >                 RPiController::AwbAlgorithm *awb =
> dynamic_cast<RPiController::AwbAlgorithm *>(
> >                         controller_.getAlgorithm("awb"));
> > -               if (awb) {
> > +               if (awb)
> >                         awbConvergenceFrames =
> awb->getConvergenceFrames();
> > -                       if (awbConvergenceFrames)
> > -                               awbConvergenceFrames += mistrustCount_;
> > -               }
> >         } else {
> >                 invalidCount_ = helper_->hideFramesModeSwitch();
> >                 mistrustCount_ = helper_->mistrustMetadataModeSwitch();
> > @@ -470,8 +461,11 @@ void IpaBase::prepareIsp(const PrepareParams
> &params)
> >         /*
> >          * This may overwrite the DeviceStatus using values from the
> sensor
> >          * metadata, and may also do additional custom processing.
> > +        *
> > +        * Only call CamHelper::prepare() when we know the metadata can
> be trusted.
> >          */
> > -       helper_->prepare(embeddedBuffer, rpiMetadata);
> > +       if (frameCount_ >= mistrustCount_)
> > +               helper_->prepare(embeddedBuffer, rpiMetadata);
> >
> >         bool delayedRequestControls = false;
> >         delayedMetadata.get<bool>("ipa.request_controls",
> delayedRequestControls);
> > @@ -527,7 +521,7 @@ void IpaBase::processStats(const ProcessParams
> &params)
> >         unsigned int ipaContext = params.ipaContext %
> rpiMetadata_.size();
> >         RPiController::Metadata &rpiMetadata = rpiMetadata_[ipaContext];
> >
> > -       if (processPending_ && frameCount_ >= mistrustCount_) {
> > +       if (processPending_) {
> >                 auto it = buffers_.find(params.buffers.stats);
> >                 if (it == buffers_.end()) {
> >                         LOG(IPARPI, Error) << "Could not find stats
> buffer!";
> > --
> > 2.53.0
> >
>