| 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 > >
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(-)