| Message ID | 20260806152701.4179-3-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi David On Thu, Aug 06, 2026 at 04:19:42PM +0100, David Plowman wrote: > Optionally, a PWL (piecewise linear) function can be specified as the > black level instead of a constant value. This PWL is evaluated at the > current analogue gain for each frame. > > Tuning files will also accept "black_level_func" as a shorthand for > all three channels. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/black_level.cpp | 45 ++++++++++++++++++++-- > src/ipa/rpi/controller/rpi/black_level.h | 7 ++++ > 2 files changed, 48 insertions(+), 4 deletions(-) > > diff --git a/src/ipa/rpi/controller/rpi/black_level.cpp b/src/ipa/rpi/controller/rpi/black_level.cpp > index 42ea15050..92a258048 100644 > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > @@ -41,12 +41,41 @@ int BlackLevel::read(const libcamera::ValueNode ¶ms) > << " Read black levels red " << blackLevelR_ > << " green " << blackLevelG_ > << " blue " << blackLevelB_; > + > + /* Allow "black_level_func" as a shorthand for all 3 colours. */ > + libcamera::ipa::Pwl blackLevelFunc; > + blackLevelFunc = params["black_level_func"].get<ipa::Pwl>(ipa::Pwl{}); > + blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); > + blackLevelFuncG_ = params["black_level_func_g"].get<ipa::Pwl>(blackLevelFunc); > + blackLevelFuncB_ = params["black_level_func_b"].get<ipa::Pwl>(blackLevelFunc); > + > + /* Warn the user if they've defined constant black levels but which are unused. */ > + if (!blackLevelFuncR_.empty() && !blackLevelFuncG_.empty() && > + !blackLevelFuncB_.empty() && params.contains("black_level")) If I read the above blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); right, isn't blackLevelFuncR_ populated with blackLevelFunc if "black_level_func_r" is not available ? Does it mean the above condition will always evaluate to true when "black_level_func" is specified ? > + LOG(RPiBlackLevel, Warning) << "\"black_level\" specified but not used"; > + > + if (!blackLevelFuncR_.empty() && params.contains("black_level_r")) > + LOG(RPiBlackLevel, Warning) << "\"black_level_r\" specified but not used"; > + if (!blackLevelFuncG_.empty() && params.contains("black_level_g")) > + LOG(RPiBlackLevel, Warning) << "\"black_level_g\" specified but not used"; > + if (!blackLevelFuncB_.empty() && params.contains("black_level_b")) > + LOG(RPiBlackLevel, Warning) << "\"black_level_b\" specified but not used"; > + > return 0; > } > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > uint16_t &blackLevelB) > { > + if (!blackLevelFuncR_.empty()) > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(1.0)); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(1.0)); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(1.0)); > + > blackLevelR = blackLevelR_; > blackLevelG = blackLevelG_; > blackLevelB = blackLevelB_; > @@ -54,10 +83,18 @@ void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > void BlackLevel::prepare(Metadata *imageMetadata) > { > - /* > - * Possibly we should think about doing this in a switchMode or > - * something? > - */ > + DeviceStatus deviceStatus; > + if (!imageMetadata->get("device.status", deviceStatus)) { > + if (!blackLevelFuncR_.empty()) > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(deviceStatus.analogueGain)); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(deviceStatus.analogueGain)); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(deviceStatus.analogueGain)); > + } > + > struct BlackLevelStatus status; > status.blackLevelR = blackLevelR_; > status.blackLevelG = blackLevelG_; > diff --git a/src/ipa/rpi/controller/rpi/black_level.h b/src/ipa/rpi/controller/rpi/black_level.h > index dbf29b282..34bb81548 100644 > --- a/src/ipa/rpi/controller/rpi/black_level.h > +++ b/src/ipa/rpi/controller/rpi/black_level.h > @@ -6,6 +6,8 @@ > */ > #pragma once > > +#include <libipa/pwl.h> > + > #include "../black_level_algorithm.h" > #include "../black_level_status.h" > > @@ -27,6 +29,11 @@ private: > double blackLevelR_; > double blackLevelG_; > double blackLevelB_; > + > + /* Black levels can vary with analogue gain instead of being constant. */ > + libcamera::ipa::Pwl blackLevelFuncR_; > + libcamera::ipa::Pwl blackLevelFuncG_; > + libcamera::ipa::Pwl blackLevelFuncB_; > }; > > } /* namespace RPiController */ > -- > 2.47.3 >
Hi Jacopo On Thu, 6 Aug 2026 at 16:54, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Thu, Aug 06, 2026 at 04:19:42PM +0100, David Plowman wrote: > > Optionally, a PWL (piecewise linear) function can be specified as the > > black level instead of a constant value. This PWL is evaluated at the > > current analogue gain for each frame. > > > > Tuning files will also accept "black_level_func" as a shorthand for > > all three channels. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/ipa/rpi/controller/rpi/black_level.cpp | 45 ++++++++++++++++++++-- > > src/ipa/rpi/controller/rpi/black_level.h | 7 ++++ > > 2 files changed, 48 insertions(+), 4 deletions(-) > > > > diff --git a/src/ipa/rpi/controller/rpi/black_level.cpp b/src/ipa/rpi/controller/rpi/black_level.cpp > > index 42ea15050..92a258048 100644 > > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > > @@ -41,12 +41,41 @@ int BlackLevel::read(const libcamera::ValueNode ¶ms) > > << " Read black levels red " << blackLevelR_ > > << " green " << blackLevelG_ > > << " blue " << blackLevelB_; > > + > > + /* Allow "black_level_func" as a shorthand for all 3 colours. */ > > + libcamera::ipa::Pwl blackLevelFunc; > > + blackLevelFunc = params["black_level_func"].get<ipa::Pwl>(ipa::Pwl{}); > > + blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); > > + blackLevelFuncG_ = params["black_level_func_g"].get<ipa::Pwl>(blackLevelFunc); > > + blackLevelFuncB_ = params["black_level_func_b"].get<ipa::Pwl>(blackLevelFunc); > > + > > + /* Warn the user if they've defined constant black levels but which are unused. */ > > + if (!blackLevelFuncR_.empty() && !blackLevelFuncG_.empty() && > > + !blackLevelFuncB_.empty() && params.contains("black_level")) > > If I read the above > > blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); > > right, isn't blackLevelFuncR_ populated with blackLevelFunc if > "black_level_func_r" is not available ? > > Does it mean the above condition will always evaluate to true when > "black_level_func" is specified ? Indeed, the logic here makes my head hurt a bit. But I *think* it's correct... If you specify "black_level_func", then all of ..funcR_, funcG_ and funcB_ are set. So it the params contain a constant "black_level", then that's now unused. But you can *not* specify "black_level_func", and give the individual ".._func_R" etc. separately. If you give all of them then again, "black_level", if present, would be unused. Conversely if, at the end of reading everything, any of the functions is unset, then "black_level", if it was given, will be used. I suppose there's a case where "black_level" is given, and then all of "black_level_r", "black_level_g" and "black_level_b" are given too. Then "black_level" is again unused. But we never warned about that before, so I guess it's OK to ignore that case now? The same is true of the "func" versions. Though at this point, my head is starting to hurt again... Anyway, please shout again if you think I may have it wrong! David > > > > + LOG(RPiBlackLevel, Warning) << "\"black_level\" specified but not used"; > > + > > + if (!blackLevelFuncR_.empty() && params.contains("black_level_r")) > > + LOG(RPiBlackLevel, Warning) << "\"black_level_r\" specified but not used"; > > + if (!blackLevelFuncG_.empty() && params.contains("black_level_g")) > > + LOG(RPiBlackLevel, Warning) << "\"black_level_g\" specified but not used"; > > + if (!blackLevelFuncB_.empty() && params.contains("black_level_b")) > > + LOG(RPiBlackLevel, Warning) << "\"black_level_b\" specified but not used"; > > + > > return 0; > > } > > > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > uint16_t &blackLevelB) > > { > > + if (!blackLevelFuncR_.empty()) > > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(1.0)); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(1.0)); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(1.0)); > > + > > blackLevelR = blackLevelR_; > > blackLevelG = blackLevelG_; > > blackLevelB = blackLevelB_; > > @@ -54,10 +83,18 @@ void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > > > void BlackLevel::prepare(Metadata *imageMetadata) > > { > > - /* > > - * Possibly we should think about doing this in a switchMode or > > - * something? > > - */ > > + DeviceStatus deviceStatus; > > + if (!imageMetadata->get("device.status", deviceStatus)) { > > + if (!blackLevelFuncR_.empty()) > > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(deviceStatus.analogueGain)); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(deviceStatus.analogueGain)); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(deviceStatus.analogueGain)); > > + } > > + > > struct BlackLevelStatus status; > > status.blackLevelR = blackLevelR_; > > status.blackLevelG = blackLevelG_; > > diff --git a/src/ipa/rpi/controller/rpi/black_level.h b/src/ipa/rpi/controller/rpi/black_level.h > > index dbf29b282..34bb81548 100644 > > --- a/src/ipa/rpi/controller/rpi/black_level.h > > +++ b/src/ipa/rpi/controller/rpi/black_level.h > > @@ -6,6 +6,8 @@ > > */ > > #pragma once > > > > +#include <libipa/pwl.h> > > + > > #include "../black_level_algorithm.h" > > #include "../black_level_status.h" > > > > @@ -27,6 +29,11 @@ private: > > double blackLevelR_; > > double blackLevelG_; > > double blackLevelB_; > > + > > + /* Black levels can vary with analogue gain instead of being constant. */ > > + libcamera::ipa::Pwl blackLevelFuncR_; > > + libcamera::ipa::Pwl blackLevelFuncG_; > > + libcamera::ipa::Pwl blackLevelFuncB_; > > }; > > > > } /* namespace RPiController */ > > -- > > 2.47.3 > >
Hi David, On Thu, Aug 06, 2026 at 05:31:40PM +0100, David Plowman wrote: > Hi Jacopo > > On Thu, 6 Aug 2026 at 16:54, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi David > > > > On Thu, Aug 06, 2026 at 04:19:42PM +0100, David Plowman wrote: > > > Optionally, a PWL (piecewise linear) function can be specified as the > > > black level instead of a constant value. This PWL is evaluated at the > > > current analogue gain for each frame. > > > > > > Tuning files will also accept "black_level_func" as a shorthand for > > > all three channels. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > src/ipa/rpi/controller/rpi/black_level.cpp | 45 ++++++++++++++++++++-- > > > src/ipa/rpi/controller/rpi/black_level.h | 7 ++++ > > > 2 files changed, 48 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/ipa/rpi/controller/rpi/black_level.cpp b/src/ipa/rpi/controller/rpi/black_level.cpp > > > index 42ea15050..92a258048 100644 > > > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > > > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > > > @@ -41,12 +41,41 @@ int BlackLevel::read(const libcamera::ValueNode ¶ms) > > > << " Read black levels red " << blackLevelR_ > > > << " green " << blackLevelG_ > > > << " blue " << blackLevelB_; > > > + > > > + /* Allow "black_level_func" as a shorthand for all 3 colours. */ > > > + libcamera::ipa::Pwl blackLevelFunc; > > > + blackLevelFunc = params["black_level_func"].get<ipa::Pwl>(ipa::Pwl{}); > > > + blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); > > > + blackLevelFuncG_ = params["black_level_func_g"].get<ipa::Pwl>(blackLevelFunc); > > > + blackLevelFuncB_ = params["black_level_func_b"].get<ipa::Pwl>(blackLevelFunc); > > > + > > > + /* Warn the user if they've defined constant black levels but which are unused. */ > > > + if (!blackLevelFuncR_.empty() && !blackLevelFuncG_.empty() && > > > + !blackLevelFuncB_.empty() && params.contains("black_level")) > > > > If I read the above > > > > blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); > > > > right, isn't blackLevelFuncR_ populated with blackLevelFunc if > > "black_level_func_r" is not available ? > > > > Does it mean the above condition will always evaluate to true when > > "black_level_func" is specified ? Uh sorry, I misread the "&& params.contains("black_level"))" with "&& params.contains("black_level_func"))". That's why I was suggesting that the above condition will always evaluate to true. > > Indeed, the logic here makes my head hurt a bit. But I *think* it's correct... > > If you specify "black_level_func", then all of ..funcR_, funcG_ and > funcB_ are set. So it the params contain a constant "black_level", > then that's now unused. Indeed > > But you can *not* specify "black_level_func", and give the individual Nothing prevents it at the moment if I'm not mistaken. It's just that each individual "_r", "_b" or "_g" will take precedence over the generic "black_level_func". I think it's fine, if that's what you want. > ".._func_R" etc. separately. If you give all of them then again, > "black_level", if present, would be unused. > > Conversely if, at the end of reading everything, any of the functions > is unset, then "black_level", if it was given, will be used. > > I suppose there's a case where "black_level" is given, and then all of > "black_level_r", "black_level_g" and "black_level_b" are given too. > Then "black_level" is again unused. But we never warned about that > before, so I guess it's OK to ignore that case now? The same is true > of the "func" versions. > > Though at this point, my head is starting to hurt again... > > Anyway, please shout again if you think I may have it wrong! I think the only thing I'm not particularly happy with is that even if you specify "_func", you still get: DEBUG RPiBlackLevel black_level.cpp:40 Read black levels red 4096 green 4096 blue 4096 Because of the defaulting to 4096 in: uint16_t blackLevel = params["black_level"].get<uint16_t>(4096); even if the tuning file only contains _func entries. I think it would be better to define a priority between the pwl version and the fixed value version. Do you want to prioritize "_func" ? Then try to parse it first. If there's any "_func" in the tuning file stop there and warn like you're already doing here if the file contains any constant black level entry. If parsing "_func" fails, simply fall back to parsing the fixed values. Something like (if PWL has to be preferred) /* * Prefer dynamic black level adjustment by parsing a PWL that describes * it. Allow "black_level_func" as a shorthand for all 3 colours. */ libcamera::ipa::Pwl blackLevelFunc = params["black_level_func"].get<ipa::Pwl>(ipa::Pwl{}); blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); blackLevelFuncG_ = params["black_level_func_g"].get<ipa::Pwl>(blackLevelFunc); blackLevelFuncB_ = params["black_level_func_b"].get<ipa::Pwl>(blackLevelFunc); /* Warn the user if they've defined constant black levels but which are unused. */ if (!blackLevelFuncR_.empty()) { if (params.contains("black_level") || params.contains("black_level_r") || params.contains("black_level_g") || params.contains("black_level_b")) LOG(RPiBlackLevel, Warning) << "Constant black level specified but not used"; LOG(RPiBlackLevel, Warning) << "Using dynamic black level adjustment"; return 0; } /* * Fall back to constant black level values. Use 64 in 10 bits scaled * to 16 bits as a default. */ uint16_t blackLevel = params["black_level"].get<uint16_t>(4096); blackLevelR_ = params["black_level_r"].get<uint16_t>(blackLevel); blackLevelG_ = params["black_level_g"].get<uint16_t>(blackLevel); blackLevelB_ = params["black_level_b"].get<uint16_t>(blackLevel); LOG(RPiBlackLevel, Debug) << " Read black levels red " << blackLevelR_ << " green " << blackLevelG_ << " blue " << blackLevelB_; return 0; It won't protect against users only defining "func_r" and not "func_b" and "func_g" (and not "func"). But it's up to you to decide how strict with validation you want to be. > > David > > > > > > > > + LOG(RPiBlackLevel, Warning) << "\"black_level\" specified but not used"; > > > + > > > + if (!blackLevelFuncR_.empty() && params.contains("black_level_r")) > > > + LOG(RPiBlackLevel, Warning) << "\"black_level_r\" specified but not used"; > > > + if (!blackLevelFuncG_.empty() && params.contains("black_level_g")) > > > + LOG(RPiBlackLevel, Warning) << "\"black_level_g\" specified but not used"; > > > + if (!blackLevelFuncB_.empty() && params.contains("black_level_b")) > > > + LOG(RPiBlackLevel, Warning) << "\"black_level_b\" specified but not used"; > > > + > > > return 0; > > > } > > > > > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > > uint16_t &blackLevelB) > > > { > > > + if (!blackLevelFuncR_.empty()) > > > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(1.0)); > > > + > > > + if (!blackLevelFuncG_.empty()) > > > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(1.0)); > > > + > > > + if (!blackLevelFuncB_.empty()) > > > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(1.0)); > > > + > > > blackLevelR = blackLevelR_; > > > blackLevelG = blackLevelG_; > > > blackLevelB = blackLevelB_; > > > @@ -54,10 +83,18 @@ void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > > > > > void BlackLevel::prepare(Metadata *imageMetadata) > > > { > > > - /* > > > - * Possibly we should think about doing this in a switchMode or > > > - * something? > > > - */ > > > + DeviceStatus deviceStatus; > > > + if (!imageMetadata->get("device.status", deviceStatus)) { > > > + if (!blackLevelFuncR_.empty()) > > > + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(deviceStatus.analogueGain)); > > > + > > > + if (!blackLevelFuncG_.empty()) > > > + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(deviceStatus.analogueGain)); > > > + > > > + if (!blackLevelFuncB_.empty()) > > > + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(deviceStatus.analogueGain)); > > > + } > > > + > > > struct BlackLevelStatus status; > > > status.blackLevelR = blackLevelR_; > > > status.blackLevelG = blackLevelG_; > > > diff --git a/src/ipa/rpi/controller/rpi/black_level.h b/src/ipa/rpi/controller/rpi/black_level.h > > > index dbf29b282..34bb81548 100644 > > > --- a/src/ipa/rpi/controller/rpi/black_level.h > > > +++ b/src/ipa/rpi/controller/rpi/black_level.h > > > @@ -6,6 +6,8 @@ > > > */ > > > #pragma once > > > > > > +#include <libipa/pwl.h> > > > + > > > #include "../black_level_algorithm.h" > > > #include "../black_level_status.h" > > > > > > @@ -27,6 +29,11 @@ private: > > > double blackLevelR_; > > > double blackLevelG_; > > > double blackLevelB_; > > > + > > > + /* Black levels can vary with analogue gain instead of being constant. */ > > > + libcamera::ipa::Pwl blackLevelFuncR_; > > > + libcamera::ipa::Pwl blackLevelFuncG_; > > > + libcamera::ipa::Pwl blackLevelFuncB_; > > > }; > > > > > > } /* namespace RPiController */ > > > -- > > > 2.47.3 > > >
diff --git a/src/ipa/rpi/controller/rpi/black_level.cpp b/src/ipa/rpi/controller/rpi/black_level.cpp index 42ea15050..92a258048 100644 --- a/src/ipa/rpi/controller/rpi/black_level.cpp +++ b/src/ipa/rpi/controller/rpi/black_level.cpp @@ -41,12 +41,41 @@ int BlackLevel::read(const libcamera::ValueNode ¶ms) << " Read black levels red " << blackLevelR_ << " green " << blackLevelG_ << " blue " << blackLevelB_; + + /* Allow "black_level_func" as a shorthand for all 3 colours. */ + libcamera::ipa::Pwl blackLevelFunc; + blackLevelFunc = params["black_level_func"].get<ipa::Pwl>(ipa::Pwl{}); + blackLevelFuncR_ = params["black_level_func_r"].get<ipa::Pwl>(blackLevelFunc); + blackLevelFuncG_ = params["black_level_func_g"].get<ipa::Pwl>(blackLevelFunc); + blackLevelFuncB_ = params["black_level_func_b"].get<ipa::Pwl>(blackLevelFunc); + + /* Warn the user if they've defined constant black levels but which are unused. */ + if (!blackLevelFuncR_.empty() && !blackLevelFuncG_.empty() && + !blackLevelFuncB_.empty() && params.contains("black_level")) + LOG(RPiBlackLevel, Warning) << "\"black_level\" specified but not used"; + + if (!blackLevelFuncR_.empty() && params.contains("black_level_r")) + LOG(RPiBlackLevel, Warning) << "\"black_level_r\" specified but not used"; + if (!blackLevelFuncG_.empty() && params.contains("black_level_g")) + LOG(RPiBlackLevel, Warning) << "\"black_level_g\" specified but not used"; + if (!blackLevelFuncB_.empty() && params.contains("black_level_b")) + LOG(RPiBlackLevel, Warning) << "\"black_level_b\" specified but not used"; + return 0; } void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, uint16_t &blackLevelB) { + if (!blackLevelFuncR_.empty()) + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(1.0)); + + if (!blackLevelFuncG_.empty()) + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(1.0)); + + if (!blackLevelFuncB_.empty()) + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(1.0)); + blackLevelR = blackLevelR_; blackLevelG = blackLevelG_; blackLevelB = blackLevelB_; @@ -54,10 +83,18 @@ void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, void BlackLevel::prepare(Metadata *imageMetadata) { - /* - * Possibly we should think about doing this in a switchMode or - * something? - */ + DeviceStatus deviceStatus; + if (!imageMetadata->get("device.status", deviceStatus)) { + if (!blackLevelFuncR_.empty()) + blackLevelR_ = blackLevelFuncR_.eval(blackLevelFuncR_.domain().clamp(deviceStatus.analogueGain)); + + if (!blackLevelFuncG_.empty()) + blackLevelG_ = blackLevelFuncG_.eval(blackLevelFuncG_.domain().clamp(deviceStatus.analogueGain)); + + if (!blackLevelFuncB_.empty()) + blackLevelB_ = blackLevelFuncB_.eval(blackLevelFuncB_.domain().clamp(deviceStatus.analogueGain)); + } + struct BlackLevelStatus status; status.blackLevelR = blackLevelR_; status.blackLevelG = blackLevelG_; diff --git a/src/ipa/rpi/controller/rpi/black_level.h b/src/ipa/rpi/controller/rpi/black_level.h index dbf29b282..34bb81548 100644 --- a/src/ipa/rpi/controller/rpi/black_level.h +++ b/src/ipa/rpi/controller/rpi/black_level.h @@ -6,6 +6,8 @@ */ #pragma once +#include <libipa/pwl.h> + #include "../black_level_algorithm.h" #include "../black_level_status.h" @@ -27,6 +29,11 @@ private: double blackLevelR_; double blackLevelG_; double blackLevelB_; + + /* Black levels can vary with analogue gain instead of being constant. */ + libcamera::ipa::Pwl blackLevelFuncR_; + libcamera::ipa::Pwl blackLevelFuncG_; + libcamera::ipa::Pwl blackLevelFuncB_; }; } /* namespace RPiController */
Optionally, a PWL (piecewise linear) function can be specified as the black level instead of a constant value. This PWL is evaluated at the current analogue gain for each frame. Tuning files will also accept "black_level_func" as a shorthand for all three channels. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/ipa/rpi/controller/rpi/black_level.cpp | 45 ++++++++++++++++++++-- src/ipa/rpi/controller/rpi/black_level.h | 7 ++++ 2 files changed, 48 insertions(+), 4 deletions(-)