| Message ID | 20250130181449.130492-3-mzamazal@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Milan, Thank you for the patch. On Thu, Jan 30, 2025 at 07:14:39PM +0100, Milan Zamazal wrote: > Rather than using a custom struct to represent RGB values, let's use the > corresponding type. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/awb.cpp | 14 ++++++++------ > src/ipa/simple/algorithms/lut.cpp | 6 +++--- > src/ipa/simple/ipa_context.h | 7 ++----- > 3 files changed, 13 insertions(+), 14 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index 1efc7090..9c85b3f8 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -25,7 +25,7 @@ int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > auto &gains = context.activeState.awb.gains; > - gains.red = gains.green = gains.blue = 1.0; > + gains = { { 1.0, 1.0, 1.0 } }; > > return 0; > } > @@ -56,15 +56,17 @@ void Awb::process(IPAContext &context, > * Clamp max gain at 4.0, this also avoids 0 division. > */ > auto &gains = context.activeState.awb.gains; > - gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR; > - gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB; > - /* Green gain is fixed to 1.0 */ > + gains = { { > + sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR, > + 1.0, > + sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB, > + } }; > > - RGB<double> rgbGains{ { 1 / gains.red, 1 / gains.green, 1 / gains.blue } }; > + RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > context.activeState.awb.temperatureK = estimateCCT(rgbGains); I've sent "[PATCH] ipa: libipa: vector: Add element-wise division operator" (https://patchwork.libcamera.org/patch/22721/). If it gets merged first, I'll replace this with context.activeState.awb.temperatureK = estimateCCT(1.0 / gains); when merging your series. Otherwise I'll update the patch with this change. > > LOG(IPASoftAwb, Debug) > - << "gain R/B: " << gains.red << "/" << gains.blue > + << "gain R/B: " << gains.r() << "/" << gains.b() > << "; temperature: " << context.activeState.awb.temperatureK; You can simplify this to LOG(IPASoftAwb, Debug) << "gains: " << gains << "; temperature: " << context.activeState.awb.temperatureK; > } > > diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp > index d75ff710..bd37a955 100644 > --- a/src/ipa/simple/algorithms/lut.cpp > +++ b/src/ipa/simple/algorithms/lut.cpp > @@ -104,13 +104,13 @@ void Lut::prepare(IPAContext &context, > gammaTableSize; > /* Apply gamma after gain! */ > unsigned int idx; > - idx = std::min({ static_cast<unsigned int>(i * gains.red / div), > + idx = std::min({ static_cast<unsigned int>(i * gains.r() / div), > gammaTableSize - 1 }); > params->red[i] = gammaTable[idx]; > - idx = std::min({ static_cast<unsigned int>(i * gains.green / div), > + idx = std::min({ static_cast<unsigned int>(i * gains.g() / div), > gammaTableSize - 1 }); > params->green[i] = gammaTable[idx]; > - idx = std::min({ static_cast<unsigned int>(i * gains.blue / div), > + idx = std::min({ static_cast<unsigned int>(i * gains.b() / div), > gammaTableSize - 1 }); > params->blue[i] = gammaTable[idx]; This could be written const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); params->red[i] = gammaTable[static_cast<unsigned int>(lutGains.r())]; params->green[i] = gammaTable[static_cast<unsigned int>(lutGains.g())]; params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; I wonder, should we add a Vector::cast<T> operator ? Then we could write const RGB<unsigned int> lutGains = (gains * i / div).cast<unsigned int>().min(gammaTableSize - 1); params->red[i] = gammaTable[lutGains.r())]; params->green[i] = gammaTable[lutGains.g()]; params->blue[i] = gammaTable[lutGains.b())]; Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > } > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 607af45a..df0552db 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -14,6 +14,7 @@ > #include <libcamera/controls.h> > > #include <libipa/fc_queue.h> > +#include <libipa/vector.h> > > namespace libcamera { > > @@ -36,11 +37,7 @@ struct IPAActiveState { > } blc; > > struct { > - struct { > - double red; > - double green; > - double blue; > - } gains; > + RGB<double> gains; > unsigned int temperatureK; > } awb; >
Hi Laurent, thank you for review. Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes: > Hi Milan, > > Thank you for the patch. > > On Thu, Jan 30, 2025 at 07:14:39PM +0100, Milan Zamazal wrote: >> Rather than using a custom struct to represent RGB values, let's use the >> corresponding type. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/awb.cpp | 14 ++++++++------ >> src/ipa/simple/algorithms/lut.cpp | 6 +++--- >> src/ipa/simple/ipa_context.h | 7 ++----- >> 3 files changed, 13 insertions(+), 14 deletions(-) >> >> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp >> index 1efc7090..9c85b3f8 100644 >> --- a/src/ipa/simple/algorithms/awb.cpp >> +++ b/src/ipa/simple/algorithms/awb.cpp >> @@ -25,7 +25,7 @@ int Awb::configure(IPAContext &context, >> [[maybe_unused]] const IPAConfigInfo &configInfo) >> { >> auto &gains = context.activeState.awb.gains; >> - gains.red = gains.green = gains.blue = 1.0; >> + gains = { { 1.0, 1.0, 1.0 } }; >> >> return 0; >> } >> @@ -56,15 +56,17 @@ void Awb::process(IPAContext &context, >> * Clamp max gain at 4.0, this also avoids 0 division. >> */ >> auto &gains = context.activeState.awb.gains; >> - gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR; >> - gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB; >> - /* Green gain is fixed to 1.0 */ >> + gains = { { >> + sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR, >> + 1.0, >> + sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB, >> + } }; >> >> - RGB<double> rgbGains{ { 1 / gains.red, 1 / gains.green, 1 / gains.blue } }; >> + RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; >> context.activeState.awb.temperatureK = estimateCCT(rgbGains); > > I've sent "[PATCH] ipa: libipa: vector: Add element-wise division > operator" (https://patchwork.libcamera.org/patch/22721/). If it gets > merged first, I'll replace this with > > context.activeState.awb.temperatureK = estimateCCT(1.0 / gains); > > when merging your series. Otherwise I'll update the patch with this > change. OK. >> LOG(IPASoftAwb, Debug) >> - << "gain R/B: " << gains.red << "/" << gains.blue >> + << "gain R/B: " << gains.r() << "/" << gains.b() >> << "; temperature: " << context.activeState.awb.temperatureK; > > You can simplify this to > > LOG(IPASoftAwb, Debug) > << "gains: " << gains << "; temperature: " > << context.activeState.awb.temperatureK; > OK. >> } >> >> diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp >> index d75ff710..bd37a955 100644 >> --- a/src/ipa/simple/algorithms/lut.cpp >> +++ b/src/ipa/simple/algorithms/lut.cpp >> @@ -104,13 +104,13 @@ void Lut::prepare(IPAContext &context, >> gammaTableSize; >> /* Apply gamma after gain! */ >> unsigned int idx; >> - idx = std::min({ static_cast<unsigned int>(i * gains.red / div), >> + idx = std::min({ static_cast<unsigned int>(i * gains.r() / div), >> gammaTableSize - 1 }); >> params->red[i] = gammaTable[idx]; >> - idx = std::min({ static_cast<unsigned int>(i * gains.green / div), >> + idx = std::min({ static_cast<unsigned int>(i * gains.g() / div), >> gammaTableSize - 1 }); >> params->green[i] = gammaTable[idx]; >> - idx = std::min({ static_cast<unsigned int>(i * gains.blue / div), >> + idx = std::min({ static_cast<unsigned int>(i * gains.b() / div), >> gammaTableSize - 1 }); >> params->blue[i] = gammaTable[idx]; > > This could be written > > const RGB<double> lutGains = (gains * i / div).min(gammaTableSize - 1); > params->red[i] = gammaTable[static_cast<unsigned int>(lutGains.r())]; > params->green[i] = gammaTable[static_cast<unsigned int>(lutGains.g())]; > params->blue[i] = gammaTable[static_cast<unsigned int>(lutGains.b())]; Indeed, nicer. > I wonder, should we add a Vector::cast<T> operator ? Then we could write > > const RGB<unsigned int> lutGains = > (gains * i / div).cast<unsigned int>().min(gammaTableSize - 1); > params->red[i] = gammaTable[lutGains.r())]; > params->green[i] = gammaTable[lutGains.g()]; > params->blue[i] = gammaTable[lutGains.b())]; Maybe yes if it was a repeating pattern but not worth at the moment. > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> } >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 607af45a..df0552db 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -14,6 +14,7 @@ >> #include <libcamera/controls.h> >> >> #include <libipa/fc_queue.h> >> +#include <libipa/vector.h> >> >> namespace libcamera { >> >> @@ -36,11 +37,7 @@ struct IPAActiveState { >> } blc; >> >> struct { >> - struct { >> - double red; >> - double green; >> - double blue; >> - } gains; >> + RGB<double> gains; >> unsigned int temperatureK; >> } awb; >>
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index 1efc7090..9c85b3f8 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -25,7 +25,7 @@ int Awb::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { auto &gains = context.activeState.awb.gains; - gains.red = gains.green = gains.blue = 1.0; + gains = { { 1.0, 1.0, 1.0 } }; return 0; } @@ -56,15 +56,17 @@ void Awb::process(IPAContext &context, * Clamp max gain at 4.0, this also avoids 0 division. */ auto &gains = context.activeState.awb.gains; - gains.red = sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR; - gains.blue = sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB; - /* Green gain is fixed to 1.0 */ + gains = { { + sumR <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumR, + 1.0, + sumB <= sumG / 4 ? 4.0 : static_cast<double>(sumG) / sumB, + } }; - RGB<double> rgbGains{ { 1 / gains.red, 1 / gains.green, 1 / gains.blue } }; + RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; context.activeState.awb.temperatureK = estimateCCT(rgbGains); LOG(IPASoftAwb, Debug) - << "gain R/B: " << gains.red << "/" << gains.blue + << "gain R/B: " << gains.r() << "/" << gains.b() << "; temperature: " << context.activeState.awb.temperatureK; } diff --git a/src/ipa/simple/algorithms/lut.cpp b/src/ipa/simple/algorithms/lut.cpp index d75ff710..bd37a955 100644 --- a/src/ipa/simple/algorithms/lut.cpp +++ b/src/ipa/simple/algorithms/lut.cpp @@ -104,13 +104,13 @@ void Lut::prepare(IPAContext &context, gammaTableSize; /* Apply gamma after gain! */ unsigned int idx; - idx = std::min({ static_cast<unsigned int>(i * gains.red / div), + idx = std::min({ static_cast<unsigned int>(i * gains.r() / div), gammaTableSize - 1 }); params->red[i] = gammaTable[idx]; - idx = std::min({ static_cast<unsigned int>(i * gains.green / div), + idx = std::min({ static_cast<unsigned int>(i * gains.g() / div), gammaTableSize - 1 }); params->green[i] = gammaTable[idx]; - idx = std::min({ static_cast<unsigned int>(i * gains.blue / div), + idx = std::min({ static_cast<unsigned int>(i * gains.b() / div), gammaTableSize - 1 }); params->blue[i] = gammaTable[idx]; } diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 607af45a..df0552db 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -14,6 +14,7 @@ #include <libcamera/controls.h> #include <libipa/fc_queue.h> +#include <libipa/vector.h> namespace libcamera { @@ -36,11 +37,7 @@ struct IPAActiveState { } blc; struct { - struct { - double red; - double green; - double blue; - } gains; + RGB<double> gains; unsigned int temperatureK; } awb;
Rather than using a custom struct to represent RGB values, let's use the corresponding type. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- src/ipa/simple/algorithms/awb.cpp | 14 ++++++++------ src/ipa/simple/algorithms/lut.cpp | 6 +++--- src/ipa/simple/ipa_context.h | 7 ++----- 3 files changed, 13 insertions(+), 14 deletions(-)