Message ID | 20250223230403.1226-7-laurent.pinchart@ideasonboard.com |
---|---|
State | Accepted |
Headers | show |
Series |
|
Related | show |
Hi Laurent, Thank you for the patch. On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote: > The lux value can never be negative. Pass it as an unsigned int. > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe one exception). They just produce too many ways to create hidden bugs. But so be it :-) > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > Do we need a Lux type ? And maybe a ColourTemperature type ? > --- > src/ipa/libipa/awb.h | 2 +- > src/ipa/libipa/awb_bayes.cpp | 2 +- > src/ipa/libipa/awb_bayes.h | 2 +- > src/ipa/libipa/awb_grey.cpp | 2 +- > src/ipa/libipa/awb_grey.h | 2 +- > 5 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h > index 4a1b012a4334..5c298d3b6f69 100644 > --- a/src/ipa/libipa/awb.h > +++ b/src/ipa/libipa/awb.h > @@ -35,7 +35,7 @@ public: > virtual ~AwbAlgorithm() = default; > > virtual int init(const YamlObject &tuningData) = 0; > - virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0; > + virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0; > virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0; > > const ControlInfoMap::Map &controls() const > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp > index e75bfcd693d9..9287b884cb95 100644 > --- a/src/ipa/libipa/awb_bayes.cpp > +++ b/src/ipa/libipa/awb_bayes.cpp > @@ -275,7 +275,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature) > return { { gains[0], 1.0, gains[1] } }; > } > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux) > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux) > { > ipa::Pwl prior; > if (lux > 0) { This if clause should then be removed, too. Regards, Stefan > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h > index 47db7243273f..23bf88061118 100644 > --- a/src/ipa/libipa/awb_bayes.h > +++ b/src/ipa/libipa/awb_bayes.h > @@ -34,7 +34,7 @@ public: > AwbBayes() = default; > > int init(const YamlObject &tuningData) override; > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > RGB<double> gainsFromColourTemperature(double temperatureK) override; > void handleControls(const ControlList &controls) override; > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp > index 06ffd45618d8..e979cc4d1a3e 100644 > --- a/src/ipa/libipa/awb_grey.cpp > +++ b/src/ipa/libipa/awb_grey.cpp > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData) > * > * \return The AWB result > */ > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux) > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux) > { > AwbResult result; > auto means = stats.getRGBMeans(); > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h > index 1a365e616a98..e3c34201dbc9 100644 > --- a/src/ipa/libipa/awb_grey.h > +++ b/src/ipa/libipa/awb_grey.h > @@ -23,7 +23,7 @@ public: > AwbGrey() = default; > > int init(const YamlObject &tuningData) override; > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > RGB<double> gainsFromColourTemperature(double colourTemperature) override; > > private: > -- > Regards, > > Laurent Pinchart >
Hi Stefan, On Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote: > Hi Laurent, > > Thank you for the patch. > > On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote: > > The lux value can never be negative. Pass it as an unsigned int. > > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe > one exception). They just produce too many ways to create hidden bugs. > But so be it :-) I think I recall we discussed pros and cons. It doesn't address the signedness issue in general, but how about creating a Lux type ? > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > Do we need a Lux type ? And maybe a ColourTemperature type ? > > --- > > src/ipa/libipa/awb.h | 2 +- > > src/ipa/libipa/awb_bayes.cpp | 2 +- > > src/ipa/libipa/awb_bayes.h | 2 +- > > src/ipa/libipa/awb_grey.cpp | 2 +- > > src/ipa/libipa/awb_grey.h | 2 +- > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h > > index 4a1b012a4334..5c298d3b6f69 100644 > > --- a/src/ipa/libipa/awb.h > > +++ b/src/ipa/libipa/awb.h > > @@ -35,7 +35,7 @@ public: > > virtual ~AwbAlgorithm() = default; > > > > virtual int init(const YamlObject &tuningData) = 0; > > - virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0; > > + virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0; > > virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0; > > > > const ControlInfoMap::Map &controls() const > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp > > index e75bfcd693d9..9287b884cb95 100644 > > --- a/src/ipa/libipa/awb_bayes.cpp > > +++ b/src/ipa/libipa/awb_bayes.cpp > > @@ -275,7 +275,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature) > > return { { gains[0], 1.0, gains[1] } }; > > } > > > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux) > > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux) > > { > > ipa::Pwl prior; > > if (lux > 0) { > > This if clause should then be removed, too. No, this tests for > 0, not >= 0. 0 is still a magic value. a std::optional<unsigned int> would express that better if we want to go that way. > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h > > index 47db7243273f..23bf88061118 100644 > > --- a/src/ipa/libipa/awb_bayes.h > > +++ b/src/ipa/libipa/awb_bayes.h > > @@ -34,7 +34,7 @@ public: > > AwbBayes() = default; > > > > int init(const YamlObject &tuningData) override; > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > RGB<double> gainsFromColourTemperature(double temperatureK) override; > > void handleControls(const ControlList &controls) override; > > > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp > > index 06ffd45618d8..e979cc4d1a3e 100644 > > --- a/src/ipa/libipa/awb_grey.cpp > > +++ b/src/ipa/libipa/awb_grey.cpp > > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData) > > * > > * \return The AWB result > > */ > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux) > > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux) > > { > > AwbResult result; > > auto means = stats.getRGBMeans(); > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h > > index 1a365e616a98..e3c34201dbc9 100644 > > --- a/src/ipa/libipa/awb_grey.h > > +++ b/src/ipa/libipa/awb_grey.h > > @@ -23,7 +23,7 @@ public: > > AwbGrey() = default; > > > > int init(const YamlObject &tuningData) override; > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > RGB<double> gainsFromColourTemperature(double colourTemperature) override; > > > > private:
Hi Laurent, On Mon, Feb 24, 2025 at 11:26:52AM +0200, Laurent Pinchart wrote: > Hi Stefan, > > On Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote: > > Hi Laurent, > > > > Thank you for the patch. > > > > On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote: > > > The lux value can never be negative. Pass it as an unsigned int. > > > > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe > > one exception). They just produce too many ways to create hidden bugs. > > But so be it :-) > > I think I recall we discussed pros and cons. > > It doesn't address the signedness issue in general, but how about > creating a Lux type ? No, please not. Then we also need to create a colour temperature type at no real benefit. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > Do we need a Lux type ? And maybe a ColourTemperature type ? > > > --- > > > src/ipa/libipa/awb.h | 2 +- > > > src/ipa/libipa/awb_bayes.cpp | 2 +- > > > src/ipa/libipa/awb_bayes.h | 2 +- > > > src/ipa/libipa/awb_grey.cpp | 2 +- > > > src/ipa/libipa/awb_grey.h | 2 +- > > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h > > > index 4a1b012a4334..5c298d3b6f69 100644 > > > --- a/src/ipa/libipa/awb.h > > > +++ b/src/ipa/libipa/awb.h > > > @@ -35,7 +35,7 @@ public: > > > virtual ~AwbAlgorithm() = default; > > > > > > virtual int init(const YamlObject &tuningData) = 0; > > > - virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0; > > > + virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0; > > > virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0; > > > > > > const ControlInfoMap::Map &controls() const > > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp > > > index e75bfcd693d9..9287b884cb95 100644 > > > --- a/src/ipa/libipa/awb_bayes.cpp > > > +++ b/src/ipa/libipa/awb_bayes.cpp > > > @@ -275,7 +275,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature) > > > return { { gains[0], 1.0, gains[1] } }; > > > } > > > > > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux) > > > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux) > > > { > > > ipa::Pwl prior; > > > if (lux > 0) { > > > > This if clause should then be removed, too. > > No, this tests for > 0, not >= 0. 0 is still a magic value. a > std::optional<unsigned int> would express that better if we want to go > that way. Ups. Right. Lets keep that one simple (stick to the uint) Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h > > > index 47db7243273f..23bf88061118 100644 > > > --- a/src/ipa/libipa/awb_bayes.h > > > +++ b/src/ipa/libipa/awb_bayes.h > > > @@ -34,7 +34,7 @@ public: > > > AwbBayes() = default; > > > > > > int init(const YamlObject &tuningData) override; > > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > > RGB<double> gainsFromColourTemperature(double temperatureK) override; > > > void handleControls(const ControlList &controls) override; > > > > > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp > > > index 06ffd45618d8..e979cc4d1a3e 100644 > > > --- a/src/ipa/libipa/awb_grey.cpp > > > +++ b/src/ipa/libipa/awb_grey.cpp > > > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData) > > > * > > > * \return The AWB result > > > */ > > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux) > > > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux) > > > { > > > AwbResult result; > > > auto means = stats.getRGBMeans(); > > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h > > > index 1a365e616a98..e3c34201dbc9 100644 > > > --- a/src/ipa/libipa/awb_grey.h > > > +++ b/src/ipa/libipa/awb_grey.h > > > @@ -23,7 +23,7 @@ public: > > > AwbGrey() = default; > > > > > > int init(const YamlObject &tuningData) override; > > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > > RGB<double> gainsFromColourTemperature(double colourTemperature) override; > > > > > > private: > > -- > Regards, > > Laurent Pinchart
On Mon, Feb 24, 2025 at 11:20:16AM +0100, Stefan Klug wrote: > On Mon, Feb 24, 2025 at 11:26:52AM +0200, Laurent Pinchart wrote: > > On Mon, Feb 24, 2025 at 10:13:33AM +0100, Stefan Klug wrote: > > > On Mon, Feb 24, 2025 at 01:03:54AM +0200, Laurent Pinchart wrote: > > > > The lux value can never be negative. Pass it as an unsigned int. > > > > > > I don't like unsigned ints outside lowlevel/hardware stuff (size_t maybe > > > one exception). They just produce too many ways to create hidden bugs. > > > But so be it :-) > > > > I think I recall we discussed pros and cons. > > > > It doesn't address the signedness issue in general, but how about > > creating a Lux type ? > > No, please not. Then we also need to create a colour temperature type at > no real benefit. The idea was to standardize the representation. We have integer vs. double used in different places. I'm not pushing strongly for this though. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > Do we need a Lux type ? And maybe a ColourTemperature type ? > > > > --- > > > > src/ipa/libipa/awb.h | 2 +- > > > > src/ipa/libipa/awb_bayes.cpp | 2 +- > > > > src/ipa/libipa/awb_bayes.h | 2 +- > > > > src/ipa/libipa/awb_grey.cpp | 2 +- > > > > src/ipa/libipa/awb_grey.h | 2 +- > > > > 5 files changed, 5 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h > > > > index 4a1b012a4334..5c298d3b6f69 100644 > > > > --- a/src/ipa/libipa/awb.h > > > > +++ b/src/ipa/libipa/awb.h > > > > @@ -35,7 +35,7 @@ public: > > > > virtual ~AwbAlgorithm() = default; > > > > > > > > virtual int init(const YamlObject &tuningData) = 0; > > > > - virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0; > > > > + virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0; > > > > virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0; > > > > > > > > const ControlInfoMap::Map &controls() const > > > > diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp > > > > index e75bfcd693d9..9287b884cb95 100644 > > > > --- a/src/ipa/libipa/awb_bayes.cpp > > > > +++ b/src/ipa/libipa/awb_bayes.cpp > > > > @@ -275,7 +275,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature) > > > > return { { gains[0], 1.0, gains[1] } }; > > > > } > > > > > > > > -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux) > > > > +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux) > > > > { > > > > ipa::Pwl prior; > > > > if (lux > 0) { > > > > > > This if clause should then be removed, too. > > > > No, this tests for > 0, not >= 0. 0 is still a magic value. a > > std::optional<unsigned int> would express that better if we want to go > > that way. > > Ups. Right. Lets keep that one simple (stick to the uint) > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > > diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h > > > > index 47db7243273f..23bf88061118 100644 > > > > --- a/src/ipa/libipa/awb_bayes.h > > > > +++ b/src/ipa/libipa/awb_bayes.h > > > > @@ -34,7 +34,7 @@ public: > > > > AwbBayes() = default; > > > > > > > > int init(const YamlObject &tuningData) override; > > > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > > > RGB<double> gainsFromColourTemperature(double temperatureK) override; > > > > void handleControls(const ControlList &controls) override; > > > > > > > > diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp > > > > index 06ffd45618d8..e979cc4d1a3e 100644 > > > > --- a/src/ipa/libipa/awb_grey.cpp > > > > +++ b/src/ipa/libipa/awb_grey.cpp > > > > @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData) > > > > * > > > > * \return The AWB result > > > > */ > > > > -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux) > > > > +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux) > > > > { > > > > AwbResult result; > > > > auto means = stats.getRGBMeans(); > > > > diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h > > > > index 1a365e616a98..e3c34201dbc9 100644 > > > > --- a/src/ipa/libipa/awb_grey.h > > > > +++ b/src/ipa/libipa/awb_grey.h > > > > @@ -23,7 +23,7 @@ public: > > > > AwbGrey() = default; > > > > > > > > int init(const YamlObject &tuningData) override; > > > > - AwbResult calculateAwb(const AwbStats &stats, int lux) override; > > > > + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; > > > > RGB<double> gainsFromColourTemperature(double colourTemperature) override; > > > > > > > > private:
diff --git a/src/ipa/libipa/awb.h b/src/ipa/libipa/awb.h index 4a1b012a4334..5c298d3b6f69 100644 --- a/src/ipa/libipa/awb.h +++ b/src/ipa/libipa/awb.h @@ -35,7 +35,7 @@ public: virtual ~AwbAlgorithm() = default; virtual int init(const YamlObject &tuningData) = 0; - virtual AwbResult calculateAwb(const AwbStats &stats, int lux) = 0; + virtual AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) = 0; virtual RGB<double> gainsFromColourTemperature(double colourTemperature) = 0; const ControlInfoMap::Map &controls() const diff --git a/src/ipa/libipa/awb_bayes.cpp b/src/ipa/libipa/awb_bayes.cpp index e75bfcd693d9..9287b884cb95 100644 --- a/src/ipa/libipa/awb_bayes.cpp +++ b/src/ipa/libipa/awb_bayes.cpp @@ -275,7 +275,7 @@ RGB<double> AwbBayes::gainsFromColourTemperature(double colourTemperature) return { { gains[0], 1.0, gains[1] } }; } -AwbResult AwbBayes::calculateAwb(const AwbStats &stats, int lux) +AwbResult AwbBayes::calculateAwb(const AwbStats &stats, unsigned int lux) { ipa::Pwl prior; if (lux > 0) { diff --git a/src/ipa/libipa/awb_bayes.h b/src/ipa/libipa/awb_bayes.h index 47db7243273f..23bf88061118 100644 --- a/src/ipa/libipa/awb_bayes.h +++ b/src/ipa/libipa/awb_bayes.h @@ -34,7 +34,7 @@ public: AwbBayes() = default; int init(const YamlObject &tuningData) override; - AwbResult calculateAwb(const AwbStats &stats, int lux) override; + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; RGB<double> gainsFromColourTemperature(double temperatureK) override; void handleControls(const ControlList &controls) override; diff --git a/src/ipa/libipa/awb_grey.cpp b/src/ipa/libipa/awb_grey.cpp index 06ffd45618d8..e979cc4d1a3e 100644 --- a/src/ipa/libipa/awb_grey.cpp +++ b/src/ipa/libipa/awb_grey.cpp @@ -70,7 +70,7 @@ int AwbGrey::init(const YamlObject &tuningData) * * \return The AWB result */ -AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] int lux) +AwbResult AwbGrey::calculateAwb(const AwbStats &stats, [[maybe_unused]] unsigned int lux) { AwbResult result; auto means = stats.getRGBMeans(); diff --git a/src/ipa/libipa/awb_grey.h b/src/ipa/libipa/awb_grey.h index 1a365e616a98..e3c34201dbc9 100644 --- a/src/ipa/libipa/awb_grey.h +++ b/src/ipa/libipa/awb_grey.h @@ -23,7 +23,7 @@ public: AwbGrey() = default; int init(const YamlObject &tuningData) override; - AwbResult calculateAwb(const AwbStats &stats, int lux) override; + AwbResult calculateAwb(const AwbStats &stats, unsigned int lux) override; RGB<double> gainsFromColourTemperature(double colourTemperature) override; private:
The lux value can never be negative. Pass it as an unsigned int. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- Do we need a Lux type ? And maybe a ColourTemperature type ? --- src/ipa/libipa/awb.h | 2 +- src/ipa/libipa/awb_bayes.cpp | 2 +- src/ipa/libipa/awb_bayes.h | 2 +- src/ipa/libipa/awb_grey.cpp | 2 +- src/ipa/libipa/awb_grey.h | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-)