| Message ID | 20241119084525.22589-1-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for the patch. Yes, we should probably have spotted this earlier. But I'll add a test for grey world AWB so that we don't get caught out like this again! On Tue, 19 Nov 2024 at 08:55, Naushir Patuck <naush@raspberrypi.com> wrote: > > If grey world AWB is setup in the tuning file, the CT curve will either > be missing or invalid. Disable biasing the statistics for the search in > such cases. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/awb.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 9d8e170d1bfe..87b1b077ffb8 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -459,10 +459,13 @@ void Awb::prepareStats() > * LSC has already been applied to the stats in this pipeline, so stop > * any LSC compensation. We also ignore config_.fast in this version. > */ > + const double biasCtR = config_.bayes && config_.ctR.size() ? > + config_.ctR.eval(config_.biasCT) : 0; > + const double biasCtB = config_.bayes && config_.ctB.size() ? > + config_.ctB.eval(config_.biasCT) : 0; I'm OK with this, though perhaps "empty()" is microscopically better than "size()" here? Actually, I think the read function ensures that the PWLs are set if bayes is, so we could leave it out completely. But either way I'm not overly bothered: Reviewed-by: David Plowman <david.plowman@raspberrypi.com> David > generateStats(zones_, statistics_, config_.minPixels, > config_.minG, getGlobalMetadata(), > - config_.biasProportion, config_.ctR.eval(config_.biasCT), > - config_.ctB.eval(config_.biasCT)); > + config_.biasProportion, biasCtR, biasCtB); > /* > * apply sensitivities, so values appear to come from our "canonical" > * sensor. > -- > 2.34.1 >
Quoting Naushir Patuck (2024-11-19 08:45:25) > If grey world AWB is setup in the tuning file, the CT curve will either > be missing or invalid. Disable biasing the statistics for the search in > such cases. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> What's the fixes tag for this (mostly so I can report this as a fix in the next release?) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/rpi/controller/rpi/awb.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index 9d8e170d1bfe..87b1b077ffb8 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -459,10 +459,13 @@ void Awb::prepareStats() > * LSC has already been applied to the stats in this pipeline, so stop > * any LSC compensation. We also ignore config_.fast in this version. > */ > + const double biasCtR = config_.bayes && config_.ctR.size() ? > + config_.ctR.eval(config_.biasCT) : 0; > + const double biasCtB = config_.bayes && config_.ctB.size() ? > + config_.ctB.eval(config_.biasCT) : 0; > generateStats(zones_, statistics_, config_.minPixels, > config_.minG, getGlobalMetadata(), > - config_.biasProportion, config_.ctR.eval(config_.biasCT), > - config_.ctB.eval(config_.biasCT)); > + config_.biasProportion, biasCtR, biasCtB); > /* > * apply sensitivities, so values appear to come from our "canonical" > * sensor. > -- > 2.34.1 >
On Tue, 19 Nov 2024 at 09:24, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2024-11-19 08:45:25) > > If grey world AWB is setup in the tuning file, the CT curve will either > > be missing or invalid. Disable biasing the statistics for the search in > > such cases. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > What's the fixes tag for this (mostly so I can report this as a fix in > the next release?) > Good point, please add if possible: Fixes: ea8fd63d936f ("ipa: rpi: awb: Add a bias to the AWB search") > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > --- > > src/ipa/rpi/controller/rpi/awb.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index 9d8e170d1bfe..87b1b077ffb8 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -459,10 +459,13 @@ void Awb::prepareStats() > > * LSC has already been applied to the stats in this pipeline, so stop > > * any LSC compensation. We also ignore config_.fast in this version. > > */ > > + const double biasCtR = config_.bayes && config_.ctR.size() ? > > + config_.ctR.eval(config_.biasCT) : 0; > > + const double biasCtB = config_.bayes && config_.ctB.size() ? > > + config_.ctB.eval(config_.biasCT) : 0; > > generateStats(zones_, statistics_, config_.minPixels, > > config_.minG, getGlobalMetadata(), > > - config_.biasProportion, config_.ctR.eval(config_.biasCT), > > - config_.ctB.eval(config_.biasCT)); > > + config_.biasProportion, biasCtR, biasCtB); > > /* > > * apply sensitivities, so values appear to come from our "canonical" > > * sensor. > > -- > > 2.34.1 > >
On Tue, Nov 19, 2024 at 09:11:08AM +0000, David Plowman wrote: > Hi Naush > > Thanks for the patch. Yes, we should probably have spotted this > earlier. But I'll add a test for grey world AWB so that we don't get > caught out like this again! > > On Tue, 19 Nov 2024 at 08:55, Naushir Patuck <naush@raspberrypi.com> wrote: > > > > If grey world AWB is setup in the tuning file, the CT curve will either > > be missing or invalid. Disable biasing the statistics for the search in > > such cases. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/awb.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index 9d8e170d1bfe..87b1b077ffb8 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -459,10 +459,13 @@ void Awb::prepareStats() > > * LSC has already been applied to the stats in this pipeline, so stop > > * any LSC compensation. We also ignore config_.fast in this version. > > */ > > + const double biasCtR = config_.bayes && config_.ctR.size() ? > > + config_.ctR.eval(config_.biasCT) : 0; > > + const double biasCtB = config_.bayes && config_.ctB.size() ? > > + config_.ctB.eval(config_.biasCT) : 0; > > I'm OK with this, though perhaps "empty()" is microscopically better > than "size()" here? Actually, I think the read function ensures that > the PWLs are set if bayes is, so we could leave it out completely. But > either way I'm not overly bothered: Yes, I think we can write const double biasCtR = config_.bayes ? config_.ctR.eval(config_.biasCT) : 0; const double biasCtB = config_.bayes ? config_.ctB.eval(config_.biasCT) : 0; > Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > > > generateStats(zones_, statistics_, config_.minPixels, > > config_.minG, getGlobalMetadata(), > > - config_.biasProportion, config_.ctR.eval(config_.biasCT), > > - config_.ctB.eval(config_.biasCT)); > > + config_.biasProportion, biasCtR, biasCtB); > > /* > > * apply sensitivities, so values appear to come from our "canonical" > > * sensor.
diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index 9d8e170d1bfe..87b1b077ffb8 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -459,10 +459,13 @@ void Awb::prepareStats() * LSC has already been applied to the stats in this pipeline, so stop * any LSC compensation. We also ignore config_.fast in this version. */ + const double biasCtR = config_.bayes && config_.ctR.size() ? + config_.ctR.eval(config_.biasCT) : 0; + const double biasCtB = config_.bayes && config_.ctB.size() ? + config_.ctB.eval(config_.biasCT) : 0; generateStats(zones_, statistics_, config_.minPixels, config_.minG, getGlobalMetadata(), - config_.biasProportion, config_.ctR.eval(config_.biasCT), - config_.ctB.eval(config_.biasCT)); + config_.biasProportion, biasCtR, biasCtB); /* * apply sensitivities, so values appear to come from our "canonical" * sensor.
If grey world AWB is setup in the tuning file, the CT curve will either be missing or invalid. Disable biasing the statistics for the search in such cases. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/rpi/controller/rpi/awb.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)