Message ID | 20221130113727.13299-1-naush@raspberrypi.com |
---|---|
State | Accepted |
Commit | 6b03da662b8b0f919e7df230cd78ba007aafc096 |
Headers | show |
Series |
|
Related | show |
Quoting Naushir Patuck via libcamera-devel (2022-11-30 11:37:27) > Fix a bug in the IPA frame dropping (for rate control) logic, where the > metadata for the current context was copied from itself (i.e. a no-op), instead > of being copied from the previous context. > > This bug does not occur in normal conditions, only when running with a low > exposure time and unconstrained framerate, which happens in a particular > picamera2 test. > I thought that -1 being excluded from 0 on v1 would cause problems ;-) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Fixes: 546154b13433 ("pipeline: ipa: raspberrypi: Use IPA cookies") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/raspberrypi/raspberrypi.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 0f914f841e54..bead436def3c 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1062,7 +1062,7 @@ void IPARPi::prepareISP(const ISPConfig &data) > * in helper_->Prepare(). > */ > RPiController::Metadata &lastMetadata = > - rpiMetadata_[ipaContext ? ipaContext : rpiMetadata_.size()]; > + rpiMetadata_[(ipaContext ? ipaContext : rpiMetadata_.size()) - 1]; > rpiMetadata.mergeCopy(lastMetadata); > processPending_ = false; > return; > -- > 2.25.1 >
On Wed, 30 Nov 2022 at 12:24, Kieran Bingham < kieran.bingham@ideasonboard.com> wrote: > Quoting Naushir Patuck via libcamera-devel (2022-11-30 11:37:27) > > Fix a bug in the IPA frame dropping (for rate control) logic, where the > > metadata for the current context was copied from itself (i.e. a no-op), > instead > > of being copied from the previous context. > > > > This bug does not occur in normal conditions, only when running with a > low > > exposure time and unconstrained framerate, which happens in a particular > > picamera2 test. > > > > I thought that -1 being excluded from 0 on v1 would cause problems ;-) > I was sure that I fixed it, but I probably lost the change in a rebase :-) > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > Fixes: 546154b13433 ("pipeline: ipa: raspberrypi: Use IPA cookies") > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/raspberrypi/raspberrypi.cpp | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp > b/src/ipa/raspberrypi/raspberrypi.cpp > > index 0f914f841e54..bead436def3c 100644 > > --- a/src/ipa/raspberrypi/raspberrypi.cpp > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > > @@ -1062,7 +1062,7 @@ void IPARPi::prepareISP(const ISPConfig &data) > > * in helper_->Prepare(). > > */ > > RPiController::Metadata &lastMetadata = > > - rpiMetadata_[ipaContext ? ipaContext : > rpiMetadata_.size()]; > > + rpiMetadata_[(ipaContext ? ipaContext : > rpiMetadata_.size()) - 1]; > > rpiMetadata.mergeCopy(lastMetadata); > > processPending_ = false; > > return; > > -- > > 2.25.1 > > >
Hi Naush Thanks for the fix. Good that we had some tests to catch it!! On Wed, 30 Nov 2022 at 11:37, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Fix a bug in the IPA frame dropping (for rate control) logic, where the > metadata for the current context was copied from itself (i.e. a no-op), instead > of being copied from the previous context. > > This bug does not occur in normal conditions, only when running with a low > exposure time and unconstrained framerate, which happens in a particular > picamera2 test. > > Fixes: 546154b13433 ("pipeline: ipa: raspberrypi: Use IPA cookies") > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks David > --- > src/ipa/raspberrypi/raspberrypi.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp > index 0f914f841e54..bead436def3c 100644 > --- a/src/ipa/raspberrypi/raspberrypi.cpp > +++ b/src/ipa/raspberrypi/raspberrypi.cpp > @@ -1062,7 +1062,7 @@ void IPARPi::prepareISP(const ISPConfig &data) > * in helper_->Prepare(). > */ > RPiController::Metadata &lastMetadata = > - rpiMetadata_[ipaContext ? ipaContext : rpiMetadata_.size()]; > + rpiMetadata_[(ipaContext ? ipaContext : rpiMetadata_.size()) - 1]; > rpiMetadata.mergeCopy(lastMetadata); > processPending_ = false; > return; > -- > 2.25.1 >
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index 0f914f841e54..bead436def3c 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -1062,7 +1062,7 @@ void IPARPi::prepareISP(const ISPConfig &data) * in helper_->Prepare(). */ RPiController::Metadata &lastMetadata = - rpiMetadata_[ipaContext ? ipaContext : rpiMetadata_.size()]; + rpiMetadata_[(ipaContext ? ipaContext : rpiMetadata_.size()) - 1]; rpiMetadata.mergeCopy(lastMetadata); processPending_ = false; return;
Fix a bug in the IPA frame dropping (for rate control) logic, where the metadata for the current context was copied from itself (i.e. a no-op), instead of being copied from the previous context. This bug does not occur in normal conditions, only when running with a low exposure time and unconstrained framerate, which happens in a particular picamera2 test. Fixes: 546154b13433 ("pipeline: ipa: raspberrypi: Use IPA cookies") Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/raspberrypi/raspberrypi.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)