| Message ID | 20260805100502.1947139-3-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Naushir Patuck (2026-08-05 11:03:54) > The current logic in the IPA gates the controller/algorithms from > running on a the frame if mistrustCount_ > 0. This logic is somwhat > incorrect, as the image data is ineed valid, just the embedded data and s/ineed/indeed/ > 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. I guess delayed controls is the 'best we can do', but if mistrustCount is 2 - aren't the first couple of frames quite unavoidably difficult to map even with delayed controls ? > Bug: https://github.com/raspberrypi/libcamera/issues/345 I feel like my tag doesn't have quite the same value here, but I've read through the code, and it seems to match my understanding of the commit message, and I see we're fixing a reported bug, and I think given the bug report - as long as we process the requests that's what matters so if it helps: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Signed-off-by: Naushir Patuck <naush@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 74517503ebc2..46f35470be99 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -354,26 +354,17 @@ void IpaBase::start(const ControlList &controls, StartResult *result) > mistrustCount_ = helper_->mistrustMetadataStartup(); > /* > * 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_->mistrustFramesModeSwitch(); > @@ -469,8 +460,11 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > /* > * 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); > @@ -526,7 +520,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) > 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 >
Hi Kieran, On Wed, 5 Aug 2026 at 13:24, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2026-08-05 11:03:54) > > The current logic in the IPA gates the controller/algorithms from > > running on a the frame if mistrustCount_ > 0. This logic is somwhat s/somwhat/somewhat/ > > incorrect, as the image data is ineed valid, just the embedded data and > > s/ineed/indeed/ > > > 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. > > I guess delayed controls is the 'best we can do', but if mistrustCount > is 2 - aren't the first couple of frames quite unavoidably difficult to > map even with delayed controls ? Delayed controls is indeed the best we can do, but it's what we use when embedded data is not present, so I've got confidence on the correctness of the values. Also for mistrustCount > 2, i don't thing it matters, delayed controls will strill track as it does normally. > > > Bug: https://github.com/raspberrypi/libcamera/issues/345 > > I feel like my tag doesn't have quite the same value here, but I've read > through the code, and it seems to match my understanding of the commit > message, and I see we're fixing a reported bug, and I think given the > bug report - as long as we process the requests that's what matters so > if it helps: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> That always helps, thanks! :) > > > > Signed-off-by: Naushir Patuck <naush@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 74517503ebc2..46f35470be99 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -354,26 +354,17 @@ void IpaBase::start(const ControlList &controls, StartResult *result) > > mistrustCount_ = helper_->mistrustMetadataStartup(); > > /* > > * 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_->mistrustFramesModeSwitch(); > > @@ -469,8 +460,11 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > > /* > > * 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); > > @@ -526,7 +520,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) > > 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 > >
Hi Naush Thanks for the patch. On Wed, 5 Aug 2026 at 11:05, Naushir Patuck <naush@raspberrypi.com> wrote: > > The current logic in the IPA gates the controller/algorithms from > running on a the frame if mistrustCount_ > 0. This logic is somwhat > incorrect, as the image data is ineed 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 Does this want a full stop? > 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> > --- > 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 74517503ebc2..46f35470be99 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -354,26 +354,17 @@ void IpaBase::start(const ControlList &controls, StartResult *result) > mistrustCount_ = helper_->mistrustMetadataStartup(); > /* > * 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_->mistrustFramesModeSwitch(); > @@ -469,8 +460,11 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) > /* > * 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); > @@ -526,7 +520,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) > 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 > Yes, I think I'm convinced. Presumably for the first <exposure/gain delay> frames the correct exposure/gain values are self-evidently the values we wrote when we started the sensor, which will match what's in the delayed controls, right? So this should be 100% reliable. (Famous last words...!) Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David
diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index 74517503ebc2..46f35470be99 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -354,26 +354,17 @@ void IpaBase::start(const ControlList &controls, StartResult *result) mistrustCount_ = helper_->mistrustMetadataStartup(); /* * 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_->mistrustFramesModeSwitch(); @@ -469,8 +460,11 @@ void IpaBase::prepareIsp(const PrepareParams ¶ms) /* * 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); @@ -526,7 +520,7 @@ void IpaBase::processStats(const ProcessParams ¶ms) 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!";
The current logic in the IPA gates the controller/algorithms from running on a the frame if mistrustCount_ > 0. This logic is somwhat incorrect, as the image data is ineed 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> --- src/ipa/rpi/common/ipa_base.cpp | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)