| Message ID | 20251011160335.50578-8-kieran.bingham@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi 2025. 10. 11. 18:03 keltezéssel, Kieran Bingham írta: > Move the black level handling entirely into the blc module. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/simple/algorithms/blc.cpp | 11 +++++++++++ > src/ipa/simple/soft_simple.cpp | 10 ---------- > 2 files changed, 11 insertions(+), 10 deletions(-) > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > index 370385afc683..060006d350ee 100644 > --- a/src/ipa/simple/algorithms/blc.cpp > +++ b/src/ipa/simple/algorithms/blc.cpp > @@ -40,10 +40,21 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context, > int BlackLevel::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > + /* > + * The black level from camHelper_ is a 16 bit value, software ISP > + * works with 8 bit pixel values, both regardless of the actual > + * sensor pixel width. Hence we obtain the pixel-based black value > + * by dividing the value from the helper by 256. > + */ > + context.configuration.black.level = > + context.camHelper->blackLevel().value() / 256; > + This no longer checks if `blackLevel()` returns an empty optional or not. It also does not check if `!!context.camHelper`. Is that intentional? Also maybe it makes sense to move `context.configuration.black` into the `BlackLevel` class since it is not used outside. Regards, Barnabás Pőcze > if (definedLevel_.has_value()) > context.configuration.black.level = definedLevel_; > + > context.activeState.blc.level = > context.configuration.black.level.value_or(16); > + > return 0; > } > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index 68ddf71e9f24..caae2c586e9b 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -227,16 +227,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo, > (context_.configuration.agc.againMax - > context_.configuration.agc.againMin) / > 100.0; > - if (context_.camHelper->blackLevel().has_value()) { > - /* > - * The black level from camHelper_ is a 16 bit value, software ISP > - * works with 8 bit pixel values, both regardless of the actual > - * sensor pixel width. Hence we obtain the pixel-based black value > - * by dividing the value from the helper by 256. > - */ > - context_.configuration.black.level = > - context_.camHelper->blackLevel().value() / 256; > - } > } else { > /* > * The camera sensor gain (g) is usually not equal to the value written
Quoting Barnabás Pőcze (2025-10-11 20:01:41) > Hi > > > 2025. 10. 11. 18:03 keltezéssel, Kieran Bingham írta: > > Move the black level handling entirely into the blc module. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/ipa/simple/algorithms/blc.cpp | 11 +++++++++++ > > src/ipa/simple/soft_simple.cpp | 10 ---------- > > 2 files changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp > > index 370385afc683..060006d350ee 100644 > > --- a/src/ipa/simple/algorithms/blc.cpp > > +++ b/src/ipa/simple/algorithms/blc.cpp > > @@ -40,10 +40,21 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context, > > int BlackLevel::configure(IPAContext &context, > > [[maybe_unused]] const IPAConfigInfo &configInfo) > > { > > + /* > > + * The black level from camHelper_ is a 16 bit value, software ISP > > + * works with 8 bit pixel values, both regardless of the actual > > + * sensor pixel width. Hence we obtain the pixel-based black value > > + * by dividing the value from the helper by 256. > > + */ > > + context.configuration.black.level = > > + context.camHelper->blackLevel().value() / 256; > > + > > This no longer checks if `blackLevel()` returns an empty optional or not. > It also does not check if `!!context.camHelper`. Is that intentional? Eeep. Good spot. It is intentional when this patch is tied with the other patches in the series I'm working on where I bring in the assumption that CameraHelpers are mandatory (to bring in the full AgcMeanLuminance and DelayedControls timings) ... but indeed it would break here, so it can't be merged on it's own. But one of my main RFC curiosities is if it's still reasonable to put the CamHelpers into the IPAContext to make it easier to get access to things deeper in the algorithms, or maybe we need to move more > Also maybe it makes sense to move `context.configuration.black` into the > `BlackLevel` class since it is not used outside. Yes, excellent spot. That can already be done. Maybe that and bringing back the checks on having a camHelper means we can tidy this up already and keep current behaviour. > Regards, > Barnabás Pőcze Thanks -- Kieran > > if (definedLevel_.has_value()) > > context.configuration.black.level = definedLevel_; > > + > > context.activeState.blc.level = > > context.configuration.black.level.value_or(16); > > + > > return 0; > > } > > > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > > index 68ddf71e9f24..caae2c586e9b 100644 > > --- a/src/ipa/simple/soft_simple.cpp > > +++ b/src/ipa/simple/soft_simple.cpp > > @@ -227,16 +227,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo, > > (context_.configuration.agc.againMax - > > context_.configuration.agc.againMin) / > > 100.0; > > - if (context_.camHelper->blackLevel().has_value()) { > > - /* > > - * The black level from camHelper_ is a 16 bit value, software ISP > > - * works with 8 bit pixel values, both regardless of the actual > > - * sensor pixel width. Hence we obtain the pixel-based black value > > - * by dividing the value from the helper by 256. > > - */ > > - context_.configuration.black.level = > > - context_.camHelper->blackLevel().value() / 256; > > - } > > } else { > > /* > > * The camera sensor gain (g) is usually not equal to the value written >
diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp index 370385afc683..060006d350ee 100644 --- a/src/ipa/simple/algorithms/blc.cpp +++ b/src/ipa/simple/algorithms/blc.cpp @@ -40,10 +40,21 @@ int BlackLevel::init([[maybe_unused]] IPAContext &context, int BlackLevel::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { + /* + * The black level from camHelper_ is a 16 bit value, software ISP + * works with 8 bit pixel values, both regardless of the actual + * sensor pixel width. Hence we obtain the pixel-based black value + * by dividing the value from the helper by 256. + */ + context.configuration.black.level = + context.camHelper->blackLevel().value() / 256; + if (definedLevel_.has_value()) context.configuration.black.level = definedLevel_; + context.activeState.blc.level = context.configuration.black.level.value_or(16); + return 0; } diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index 68ddf71e9f24..caae2c586e9b 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -227,16 +227,6 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo, (context_.configuration.agc.againMax - context_.configuration.agc.againMin) / 100.0; - if (context_.camHelper->blackLevel().has_value()) { - /* - * The black level from camHelper_ is a 16 bit value, software ISP - * works with 8 bit pixel values, both regardless of the actual - * sensor pixel width. Hence we obtain the pixel-based black value - * by dividing the value from the helper by 256. - */ - context_.configuration.black.level = - context_.camHelper->blackLevel().value() / 256; - } } else { /* * The camera sensor gain (g) is usually not equal to the value written
Move the black level handling entirely into the blc module. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/simple/algorithms/blc.cpp | 11 +++++++++++ src/ipa/simple/soft_simple.cpp | 10 ---------- 2 files changed, 11 insertions(+), 10 deletions(-)