| Message ID | 20240930084040.2919-2-naush@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush Thanks for this. Looks sensible! On Mon, 30 Sept 2024 at 09:40, Naushir Patuck <naush@raspberrypi.com> wrote: > > A default CT of 4500K is used in a couple of places. Add a constexpr > value for the default CT value and use it instead. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> Thanks! David > --- > src/ipa/rpi/controller/rpi/awb.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index f45525bce2d1..24f296fc66fa 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -20,6 +20,8 @@ using namespace libcamera; > > LOG_DEFINE_CATEGORY(RPiAwb) > > +constexpr double DefaultCT = 4500.0; > + > #define NAME "rpi.awb" > > /* > @@ -214,7 +216,7 @@ void Awb::initialise() > syncResults_.gainB = 1.0 / config_.ctB.eval(syncResults_.temperatureK); > } else { > /* random values just to stop the world blowing up */ > - syncResults_.temperatureK = 4500; > + syncResults_.temperatureK = DefaultCT; > syncResults_.gainR = syncResults_.gainG = syncResults_.gainB = 1.0; > } > prevSyncResults_ = syncResults_; > @@ -716,7 +718,7 @@ void Awb::awbGrey() > sumR += *ri, sumB += *bi; > double gainR = sumR.G / (sumR.R + 1), > gainB = sumB.G / (sumB.B + 1); > - asyncResults_.temperatureK = 4500; /* don't know what it is */ > + asyncResults_.temperatureK = DefaultCT; /* don't know what it is */ > asyncResults_.gainR = gainR; > asyncResults_.gainG = 1.0; > asyncResults_.gainB = gainB; > -- > 2.34.1 >
Hi Naush, Thank you for the patch. On Mon, Sep 30, 2024 at 09:40:39AM +0100, Naushir Patuck wrote: > A default CT of 4500K is used in a couple of places. Add a constexpr > value for the default CT value and use it instead. > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/awb.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > index f45525bce2d1..24f296fc66fa 100644 > --- a/src/ipa/rpi/controller/rpi/awb.cpp > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > @@ -20,6 +20,8 @@ using namespace libcamera; > > LOG_DEFINE_CATEGORY(RPiAwb) > > +constexpr double DefaultCT = 4500.0; s/DefaultCT/kDefaultCT/ > + > #define NAME "rpi.awb" > > /* > @@ -214,7 +216,7 @@ void Awb::initialise() > syncResults_.gainB = 1.0 / config_.ctB.eval(syncResults_.temperatureK); > } else { > /* random values just to stop the world blowing up */ > - syncResults_.temperatureK = 4500; > + syncResults_.temperatureK = DefaultCT; > syncResults_.gainR = syncResults_.gainG = syncResults_.gainB = 1.0; > } > prevSyncResults_ = syncResults_; > @@ -716,7 +718,7 @@ void Awb::awbGrey() > sumR += *ri, sumB += *bi; > double gainR = sumR.G / (sumR.R + 1), > gainB = sumB.G / (sumB.B + 1); > - asyncResults_.temperatureK = 4500; /* don't know what it is */ > + asyncResults_.temperatureK = DefaultCT; /* don't know what it is */ "don't know what it is" sounds like the person who wrote the code doesn't know what it does :-) Do I understand correctly that with the grey world model you can't estimate the colour temperature ? If so, while at it, I'd write /* * The grey world model can't estimate the colour temperature, use a * default value. */ Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > asyncResults_.gainR = gainR; > asyncResults_.gainG = 1.0; > asyncResults_.gainB = gainB;
Hi Laurent, Thank you for the feedback. On Mon, 30 Sept 2024 at 16:20, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Mon, Sep 30, 2024 at 09:40:39AM +0100, Naushir Patuck wrote: > > A default CT of 4500K is used in a couple of places. Add a constexpr > > value for the default CT value and use it instead. > > > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/awb.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp > > index f45525bce2d1..24f296fc66fa 100644 > > --- a/src/ipa/rpi/controller/rpi/awb.cpp > > +++ b/src/ipa/rpi/controller/rpi/awb.cpp > > @@ -20,6 +20,8 @@ using namespace libcamera; > > > > LOG_DEFINE_CATEGORY(RPiAwb) > > > > +constexpr double DefaultCT = 4500.0; > > s/DefaultCT/kDefaultCT/ I'd prefer to keep it as-is because it's consistent with the rest of the algorithm code in RPi land. > > > + > > #define NAME "rpi.awb" > > > > /* > > @@ -214,7 +216,7 @@ void Awb::initialise() > > syncResults_.gainB = 1.0 / config_.ctB.eval(syncResults_.temperatureK); > > } else { > > /* random values just to stop the world blowing up */ > > - syncResults_.temperatureK = 4500; > > + syncResults_.temperatureK = DefaultCT; > > syncResults_.gainR = syncResults_.gainG = syncResults_.gainB = 1.0; > > } > > prevSyncResults_ = syncResults_; > > @@ -716,7 +718,7 @@ void Awb::awbGrey() > > sumR += *ri, sumB += *bi; > > double gainR = sumR.G / (sumR.R + 1), > > gainB = sumB.G / (sumB.B + 1); > > - asyncResults_.temperatureK = 4500; /* don't know what it is */ > > + asyncResults_.temperatureK = DefaultCT; /* don't know what it is */ > > "don't know what it is" sounds like the person who wrote the code > doesn't know what it does :-) Hmmm, I really should remove that comment! > > Do I understand correctly that with the grey world model you can't > estimate the colour temperature ? If so, while at it, I'd write > > /* > * The grey world model can't estimate the colour temperature, use a > * default value. > */ > Ack, I'll do that. Naush > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > asyncResults_.gainR = gainR; > > asyncResults_.gainG = 1.0; > > asyncResults_.gainB = gainB; > > -- > Regards, > > Laurent Pinchart
diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp index f45525bce2d1..24f296fc66fa 100644 --- a/src/ipa/rpi/controller/rpi/awb.cpp +++ b/src/ipa/rpi/controller/rpi/awb.cpp @@ -20,6 +20,8 @@ using namespace libcamera; LOG_DEFINE_CATEGORY(RPiAwb) +constexpr double DefaultCT = 4500.0; + #define NAME "rpi.awb" /* @@ -214,7 +216,7 @@ void Awb::initialise() syncResults_.gainB = 1.0 / config_.ctB.eval(syncResults_.temperatureK); } else { /* random values just to stop the world blowing up */ - syncResults_.temperatureK = 4500; + syncResults_.temperatureK = DefaultCT; syncResults_.gainR = syncResults_.gainG = syncResults_.gainB = 1.0; } prevSyncResults_ = syncResults_; @@ -716,7 +718,7 @@ void Awb::awbGrey() sumR += *ri, sumB += *bi; double gainR = sumR.G / (sumR.R + 1), gainB = sumB.G / (sumB.B + 1); - asyncResults_.temperatureK = 4500; /* don't know what it is */ + asyncResults_.temperatureK = DefaultCT; /* don't know what it is */ asyncResults_.gainR = gainR; asyncResults_.gainG = 1.0; asyncResults_.gainB = gainB;
A default CT of 4500K is used in a couple of places. Add a constexpr value for the default CT value and use it instead. Signed-off-by: Naushir Patuck <naush@raspberrypi.com> --- src/ipa/rpi/controller/rpi/awb.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)