| Message ID | 20211116162615.27777-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for the patch ! On 16/11/2021 17:26, Laurent Pinchart wrote: > The kMaxLuminance constant is badly named, it's not a maximum luminance, > but the maximum integer value output by the AWB statistics engine for > per-channel averages. The constant is used in a single place, hardcoding > the value is actually more readable. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> I hesitated and made it a constexpr to avoid "magic values"... :-) Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index bd02c474611c..43a39ffd57d6 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5; > /* Number of frames to wait before calculating stats on minimum exposure */ > static constexpr uint32_t kNumStartupFrames = 10; > > -/* Maximum luminance used for brightness normalization */ > -static constexpr uint32_t kMaxLuminance = 255; > - > /* > * Normalized luma value target. > * > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext, > greenSum * frameContext.awb.gains.green * .587 + > blueSum * frameContext.awb.gains.blue * .114; > > - /* Return the normalized relative luminance. */ > - return Y_sum / (grid.height * grid.width) / kMaxLuminance; > + return Y_sum / (grid.height * grid.width) / 255; > } > > /** >
Hi Jean-Michel, On Wed, Nov 17, 2021 at 11:09:55AM +0100, Jean-Michel Hautbois wrote: > Hi Laurent, > > Thanks for the patch ! > > On 16/11/2021 17:26, Laurent Pinchart wrote: > > The kMaxLuminance constant is badly named, it's not a maximum luminance, > > but the maximum integer value output by the AWB statistics engine for > > per-channel averages. The constant is used in a single place, hardcoding > > the value is actually more readable. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I hesitated and made it a constexpr to avoid "magic values"... :-) I also prefer avoiding magic values. Here I think "kMaxLuminance" isn't a very good name. It's really the maximum value of the RGB averages computed by the ImgU in AWB statistics. As it's only used in a single function, and as I couldn't think of a good name for the constant, I decided to drop it. > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > --- > > src/ipa/ipu3/algorithms/agc.cpp | 6 +----- > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > index bd02c474611c..43a39ffd57d6 100644 > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5; > > /* Number of frames to wait before calculating stats on minimum exposure */ > > static constexpr uint32_t kNumStartupFrames = 10; > > > > -/* Maximum luminance used for brightness normalization */ > > -static constexpr uint32_t kMaxLuminance = 255; > > - > > /* > > * Normalized luma value target. > > * > > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext, > > greenSum * frameContext.awb.gains.green * .587 + > > blueSum * frameContext.awb.gains.blue * .114; > > > > - /* Return the normalized relative luminance. */ > > - return Y_sum / (grid.height * grid.width) / kMaxLuminance; > > + return Y_sum / (grid.height * grid.width) / 255; > > } > > > > /**
Quoting Laurent Pinchart (2021-11-17 10:12:56) > Hi Jean-Michel, > > On Wed, Nov 17, 2021 at 11:09:55AM +0100, Jean-Michel Hautbois wrote: > > Hi Laurent, > > > > Thanks for the patch ! > > > > On 16/11/2021 17:26, Laurent Pinchart wrote: > > > The kMaxLuminance constant is badly named, it's not a maximum luminance, > > > but the maximum integer value output by the AWB statistics engine for > > > per-channel averages. The constant is used in a single place, hardcoding > > > the value is actually more readable. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > I hesitated and made it a constexpr to avoid "magic values"... :-) > > I also prefer avoiding magic values. Here I think "kMaxLuminance" isn't > a very good name. It's really the maximum value of the RGB averages > computed by the ImgU in AWB statistics. As it's only used in a single > function, and as I couldn't think of a good name for the constant, I > decided to drop it. I thought MaxLuminance conveyed that. As in - the maximum lumincance is 255, so the maximum average possible is 255. But I don't mind it being hardcoded if that's prefered. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> > > > > > --- > > > src/ipa/ipu3/algorithms/agc.cpp | 6 +----- > > > 1 file changed, 1 insertion(+), 5 deletions(-) > > > > > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > > > index bd02c474611c..43a39ffd57d6 100644 > > > --- a/src/ipa/ipu3/algorithms/agc.cpp > > > +++ b/src/ipa/ipu3/algorithms/agc.cpp > > > @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5; > > > /* Number of frames to wait before calculating stats on minimum exposure */ > > > static constexpr uint32_t kNumStartupFrames = 10; > > > > > > -/* Maximum luminance used for brightness normalization */ > > > -static constexpr uint32_t kMaxLuminance = 255; > > > - > > > /* > > > * Normalized luma value target. > > > * > > > @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext, > > > greenSum * frameContext.awb.gains.green * .587 + > > > blueSum * frameContext.awb.gains.blue * .114; > > > > > > - /* Return the normalized relative luminance. */ > > > - return Y_sum / (grid.height * grid.width) / kMaxLuminance; > > > + return Y_sum / (grid.height * grid.width) / 255; > > > } > > > > > > /** > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index bd02c474611c..43a39ffd57d6 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -61,9 +61,6 @@ static constexpr double kEvGainTarget = 0.5; /* Number of frames to wait before calculating stats on minimum exposure */ static constexpr uint32_t kNumStartupFrames = 10; -/* Maximum luminance used for brightness normalization */ -static constexpr uint32_t kMaxLuminance = 255; - /* * Normalized luma value target. * @@ -298,8 +295,7 @@ double Agc::computeInitialY(IPAFrameContext &frameContext, greenSum * frameContext.awb.gains.green * .587 + blueSum * frameContext.awb.gains.blue * .114; - /* Return the normalized relative luminance. */ - return Y_sum / (grid.height * grid.width) / kMaxLuminance; + return Y_sum / (grid.height * grid.width) / 255; } /**
The kMaxLuminance constant is badly named, it's not a maximum luminance, but the maximum integer value output by the AWB statistics engine for per-channel averages. The constant is used in a single place, hardcoding the value is actually more readable. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-)