| Message ID | 20260626-ipu3-libipa-rework-v2-3-41546e23de3e@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Daniel Scally (2026-06-26 14:05:50) > The AGC algorithm stores red, blue and green gains as type double > class members. Switch to using a single member of type RGB<double> > instead to simplify the code. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > --- > Changes in v2: > > - New patch > --- > src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- > src/ipa/ipu3/algorithms/agc.h | 6 +++--- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const > sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); > } > > - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; > - double ySum = rec601LuminanceFromRGB(sum * gains); > + double ySum = rec601LuminanceFromRGB(sum * gains_); > return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; > } > > @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > ControlList &metadata) > { > Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); > - rGain_ = context.activeState.awb.gains.red; > - gGain_ = context.activeState.awb.gains.blue; > - bGain_ = context.activeState.awb.gains.green; > + gains_ = RGB<double>({ context.activeState.awb.gains.red, > + context.activeState.awb.gains.blue, > + context.activeState.awb.gains.green }); Can this construct from: gains_ = RGB<double>(context.activeState.awb.gains); ? It might not, and it's not 'wrong' so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > /* > * The Agc algorithm needs to know the effective exposure value that was > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index d274a2350485f8f9a3870b24ceb29927cb9c2bec..12ddb173ff89b7de68dd544a194292f4d14efdbb 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -13,6 +13,8 @@ > > #include <libcamera/geometry.h> > > +#include "libcamera/internal/vector.h" > + > #include "libipa/agc_mean_luminance.h" > #include "libipa/histogram.h" > > @@ -49,9 +51,7 @@ private: > double maxAnalogueGain_; > > uint32_t stride_; > - double rGain_; > - double gGain_; > - double bGain_; > + RGB<double> gains_; > ipu3_uapi_grid_config bdsGrid_; > std::vector<std::tuple<uint8_t, uint8_t, uint8_t>> rgbTriples_; > }; > > -- > 2.43.0 >
2026. 06. 29. 12:06 keltezéssel, Kieran Bingham írta: > Quoting Daniel Scally (2026-06-26 14:05:50) >> The AGC algorithm stores red, blue and green gains as type double >> class members. Switch to using a single member of type RGB<double> >> instead to simplify the code. >> >> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >> --- >> Changes in v2: >> >> - New patch >> --- >> src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- >> src/ipa/ipu3/algorithms/agc.h | 6 +++--- >> 2 files changed, 7 insertions(+), 8 deletions(-) >> >> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >> index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 >> --- a/src/ipa/ipu3/algorithms/agc.cpp >> +++ b/src/ipa/ipu3/algorithms/agc.cpp >> @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const >> sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); >> } >> >> - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; >> - double ySum = rec601LuminanceFromRGB(sum * gains); >> + double ySum = rec601LuminanceFromRGB(sum * gains_); >> return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; >> } >> >> @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, >> ControlList &metadata) >> { >> Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); >> - rGain_ = context.activeState.awb.gains.red; >> - gGain_ = context.activeState.awb.gains.blue; >> - bGain_ = context.activeState.awb.gains.green; >> + gains_ = RGB<double>({ context.activeState.awb.gains.red, >> + context.activeState.awb.gains.blue, >> + context.activeState.awb.gains.green }); > > Can this construct from: > gains_ = RGB<double>(context.activeState.awb.gains); > > ? > > It might not, and it's not 'wrong' so: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> It would require changing `ipu3::IPAActiveState::gains` to be e.g. `RGB<double>` because it is just a struct of unnamed type; this shouldn't be too hard as far as I can tell, if it's deemed to be worth it. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > >> >> /* >> * The Agc algorithm needs to know the effective exposure value that was >> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h >> index d274a2350485f8f9a3870b24ceb29927cb9c2bec..12ddb173ff89b7de68dd544a194292f4d14efdbb 100644 >> --- a/src/ipa/ipu3/algorithms/agc.h >> +++ b/src/ipa/ipu3/algorithms/agc.h >> @@ -13,6 +13,8 @@ >> >> #include <libcamera/geometry.h> >> >> +#include "libcamera/internal/vector.h" >> + >> #include "libipa/agc_mean_luminance.h" >> #include "libipa/histogram.h" >> >> @@ -49,9 +51,7 @@ private: >> double maxAnalogueGain_; >> >> uint32_t stride_; >> - double rGain_; >> - double gGain_; >> - double bGain_; >> + RGB<double> gains_; >> ipu3_uapi_grid_config bdsGrid_; >> std::vector<std::tuple<uint8_t, uint8_t, uint8_t>> rgbTriples_; >> }; >> >> -- >> 2.43.0 >>
Hi Barnabas On 30/06/2026 09:24, Barnabás Pőcze wrote: > 2026. 06. 29. 12:06 keltezéssel, Kieran Bingham írta: >> Quoting Daniel Scally (2026-06-26 14:05:50) >>> The AGC algorithm stores red, blue and green gains as type double >>> class members. Switch to using a single member of type RGB<double> >>> instead to simplify the code. >>> >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> >>> --- >>> Changes in v2: >>> >>> - New patch >>> --- >>> src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- >>> src/ipa/ipu3/algorithms/agc.h | 6 +++--- >>> 2 files changed, 7 insertions(+), 8 deletions(-) >>> >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp >>> index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 >>> --- a/src/ipa/ipu3/algorithms/agc.cpp >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp >>> @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const >>> sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); >>> } >>> - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; >>> - double ySum = rec601LuminanceFromRGB(sum * gains); >>> + double ySum = rec601LuminanceFromRGB(sum * gains_); >>> return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; >>> } >>> @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, >>> ControlList &metadata) >>> { >>> Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); >>> - rGain_ = context.activeState.awb.gains.red; >>> - gGain_ = context.activeState.awb.gains.blue; >>> - bGain_ = context.activeState.awb.gains.green; >>> + gains_ = RGB<double>({ context.activeState.awb.gains.red, >>> + context.activeState.awb.gains.blue, >>> + context.activeState.awb.gains.green }); >> >> Can this construct from: >> gains_ = RGB<double>(context.activeState.awb.gains); >> >> ? >> >> It might not, and it's not 'wrong' so: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > It would require changing `ipu3::IPAActiveState::gains` to be e.g. `RGB<double>` > because it is just a struct of unnamed type; this shouldn't be too hard as far > as I can tell, if it's deemed to be worth it. That struct is dropped in the next patch anyway, and replaced with ipa::ActiveState::awb. I can change this patch if we want, but it doesn't seem worthwhile to me. > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Thanks! Dan > > >> >> >>> /* >>> * The Agc algorithm needs to know the effective exposure value that was >>> diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h >>> index d274a2350485f8f9a3870b24ceb29927cb9c2bec..12ddb173ff89b7de68dd544a194292f4d14efdbb 100644 >>> --- a/src/ipa/ipu3/algorithms/agc.h >>> +++ b/src/ipa/ipu3/algorithms/agc.h >>> @@ -13,6 +13,8 @@ >>> #include <libcamera/geometry.h> >>> +#include "libcamera/internal/vector.h" >>> + >>> #include "libipa/agc_mean_luminance.h" >>> #include "libipa/histogram.h" >>> @@ -49,9 +51,7 @@ private: >>> double maxAnalogueGain_; >>> uint32_t stride_; >>> - double rGain_; >>> - double gGain_; >>> - double bGain_; >>> + RGB<double> gains_; >>> ipu3_uapi_grid_config bdsGrid_; >>> std::vector<std::tuple<uint8_t, uint8_t, uint8_t>> rgbTriples_; >>> }; >>> >>> -- >>> 2.43.0 >>> >
Quoting Dan Scally (2026-06-30 09:29:22) > Hi Barnabas > > On 30/06/2026 09:24, Barnabás Pőcze wrote: > > 2026. 06. 29. 12:06 keltezéssel, Kieran Bingham írta: > >> Quoting Daniel Scally (2026-06-26 14:05:50) > >>> The AGC algorithm stores red, blue and green gains as type double > >>> class members. Switch to using a single member of type RGB<double> > >>> instead to simplify the code. > >>> > >>> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> > >>> --- > >>> Changes in v2: > >>> > >>> - New patch > >>> --- > >>> src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- > >>> src/ipa/ipu3/algorithms/agc.h | 6 +++--- > >>> 2 files changed, 7 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > >>> index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 > >>> --- a/src/ipa/ipu3/algorithms/agc.cpp > >>> +++ b/src/ipa/ipu3/algorithms/agc.cpp > >>> @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const > >>> sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); > >>> } > >>> - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; > >>> - double ySum = rec601LuminanceFromRGB(sum * gains); > >>> + double ySum = rec601LuminanceFromRGB(sum * gains_); > >>> return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; > >>> } > >>> @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > >>> ControlList &metadata) > >>> { > >>> Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); > >>> - rGain_ = context.activeState.awb.gains.red; > >>> - gGain_ = context.activeState.awb.gains.blue; > >>> - bGain_ = context.activeState.awb.gains.green; > >>> + gains_ = RGB<double>({ context.activeState.awb.gains.red, > >>> + context.activeState.awb.gains.blue, > >>> + context.activeState.awb.gains.green }); > >> > >> Can this construct from: > >> gains_ = RGB<double>(context.activeState.awb.gains); > >> > >> ? > >> > >> It might not, and it's not 'wrong' so: > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > It would require changing `ipu3::IPAActiveState::gains` to be e.g. `RGB<double>` > > because it is just a struct of unnamed type; this shouldn't be too hard as far > > as I can tell, if it's deemed to be worth it. > > That struct is dropped in the next patch anyway, and replaced with ipa::ActiveState::awb. I can > change this patch if we want, but it doesn't seem worthwhile to me. absolutely, leave it as is! -- Kieran > > > > > Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >
Hi Dan On Fri, Jun 26, 2026 at 02:05:50PM +0100, Daniel Scally wrote: > The AGC algorithm stores red, blue and green gains as type double > class members. Switch to using a single member of type RGB<double> > instead to simplify the code. > > Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > Changes in v2: > > - New patch > --- > src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- > src/ipa/ipu3/algorithms/agc.h | 6 +++--- > 2 files changed, 7 insertions(+), 8 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const > sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); > } > > - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; > - double ySum = rec601LuminanceFromRGB(sum * gains); > + double ySum = rec601LuminanceFromRGB(sum * gains_); > return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; > } > > @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > ControlList &metadata) > { > Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); > - rGain_ = context.activeState.awb.gains.red; > - gGain_ = context.activeState.awb.gains.blue; > - bGain_ = context.activeState.awb.gains.green; > + gains_ = RGB<double>({ context.activeState.awb.gains.red, > + context.activeState.awb.gains.blue, > + context.activeState.awb.gains.green }); > > /* > * The Agc algorithm needs to know the effective exposure value that was > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index d274a2350485f8f9a3870b24ceb29927cb9c2bec..12ddb173ff89b7de68dd544a194292f4d14efdbb 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -13,6 +13,8 @@ > > #include <libcamera/geometry.h> > > +#include "libcamera/internal/vector.h" > + > #include "libipa/agc_mean_luminance.h" > #include "libipa/histogram.h" > > @@ -49,9 +51,7 @@ private: > double maxAnalogueGain_; > > uint32_t stride_; > - double rGain_; > - double gGain_; > - double bGain_; > + RGB<double> gains_; > ipu3_uapi_grid_config bdsGrid_; > std::vector<std::tuple<uint8_t, uint8_t, uint8_t>> rgbTriples_; > }; > > -- > 2.43.0 >
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index d6a7036c6504acb106f5c773b529ad80b8349f85..b8b880b357a4770efd6810a3bdf616dd25ce93e4 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -186,8 +186,7 @@ double Agc::estimateLuminance(double gain) const sum.b() += std::min(std::get<2>(rgbTriples_[i]) * gain, 255.0); } - RGB<double> gains{{ rGain_, gGain_, bGain_ }}; - double ySum = rec601LuminanceFromRGB(sum * gains); + double ySum = rec601LuminanceFromRGB(sum * gains_); return ySum / (bdsGrid_.height * bdsGrid_.width) / 255; } @@ -208,9 +207,9 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, ControlList &metadata) { Histogram hist = parseStatistics(stats, context.configuration.grid.bdsGrid); - rGain_ = context.activeState.awb.gains.red; - gGain_ = context.activeState.awb.gains.blue; - bGain_ = context.activeState.awb.gains.green; + gains_ = RGB<double>({ context.activeState.awb.gains.red, + context.activeState.awb.gains.blue, + context.activeState.awb.gains.green }); /* * The Agc algorithm needs to know the effective exposure value that was diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index d274a2350485f8f9a3870b24ceb29927cb9c2bec..12ddb173ff89b7de68dd544a194292f4d14efdbb 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -13,6 +13,8 @@ #include <libcamera/geometry.h> +#include "libcamera/internal/vector.h" + #include "libipa/agc_mean_luminance.h" #include "libipa/histogram.h" @@ -49,9 +51,7 @@ private: double maxAnalogueGain_; uint32_t stride_; - double rGain_; - double gGain_; - double bGain_; + RGB<double> gains_; ipu3_uapi_grid_config bdsGrid_; std::vector<std::tuple<uint8_t, uint8_t, uint8_t>> rgbTriples_; };
The AGC algorithm stores red, blue and green gains as type double class members. Switch to using a single member of type RGB<double> instead to simplify the code. Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com> --- Changes in v2: - New patch --- src/ipa/ipu3/algorithms/agc.cpp | 9 ++++----- src/ipa/ipu3/algorithms/agc.h | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-)