| Message ID | 20260803123012.10175-3-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On Mon, 3 Aug 2026 at 13:30, David Plowman <david.plowman@raspberrypi.com> 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> > We must remember to update the ctt to measure this! Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > --- > src/ipa/rpi/controller/rpi/black_level.cpp | 33 +++++++++++++++++++--- > src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ > 2 files changed, 36 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..876bc661d 100644 > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > @@ -41,12 +41,29 @@ 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); > + > return 0; > } > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t > &blackLevelG, > uint16_t &blackLevelB) > { > + if (!blackLevelFuncR_.empty()) > + blackLevelR_ = blackLevelFuncR_.eval(1.0); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = blackLevelFuncG_.eval(1.0); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = blackLevelFuncB_.eval(1.0); > + > blackLevelR = blackLevelR_; > blackLevelG = blackLevelG_; > blackLevelB = blackLevelB_; > @@ -54,10 +71,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(deviceStatus.analogueGain); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = > blackLevelFuncG_.eval(deviceStatus.analogueGain); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = > blackLevelFuncB_.eval(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 > >
Quoting Naushir Patuck (2026-08-05 13:05:09) > Hi David, > > On Mon, 3 Aug 2026 at 13:30, David Plowman <david.plowman@raspberrypi.com> > 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> > > > > We must remember to update the ctt to measure this! > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> Is this the first sensor you've come across to need gain based black level? I assume this is now different to the 'pedestal' removed, and this is actually the black level measured from the device. -- Kieran > > --- > > src/ipa/rpi/controller/rpi/black_level.cpp | 33 +++++++++++++++++++--- > > src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ > > 2 files changed, 36 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..876bc661d 100644 > > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > > @@ -41,12 +41,29 @@ 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); > > + > > return 0; > > } > > > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t > > &blackLevelG, > > uint16_t &blackLevelB) > > { > > + if (!blackLevelFuncR_.empty()) > > + blackLevelR_ = blackLevelFuncR_.eval(1.0); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = blackLevelFuncG_.eval(1.0); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = blackLevelFuncB_.eval(1.0); > > + > > blackLevelR = blackLevelR_; > > blackLevelG = blackLevelG_; > > blackLevelB = blackLevelB_; > > @@ -54,10 +71,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(deviceStatus.analogueGain); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = > > blackLevelFuncG_.eval(deviceStatus.analogueGain); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = > > blackLevelFuncB_.eval(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 Kieran On Wed, 5 Aug 2026 at 14:19, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Quoting Naushir Patuck (2026-08-05 13:05:09) > > Hi David, > > > > On Mon, 3 Aug 2026 at 13:30, David Plowman <david.plowman@raspberrypi.com> > > 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> > > > > > > > We must remember to update the ctt to measure this! > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > Is this the first sensor you've come across to need gain based black > level? > > I assume this is now different to the 'pedestal' removed, and this is > actually the black level measured from the device. Yes, this is the first sensor where we've noticed an effect like this. We measured the new values from the device. David > -- > Kieran > > > > --- > > > src/ipa/rpi/controller/rpi/black_level.cpp | 33 +++++++++++++++++++--- > > > src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ > > > 2 files changed, 36 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..876bc661d 100644 > > > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > > > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > > > @@ -41,12 +41,29 @@ 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); > > > + > > > return 0; > > > } > > > > > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t > > > &blackLevelG, > > > uint16_t &blackLevelB) > > > { > > > + if (!blackLevelFuncR_.empty()) > > > + blackLevelR_ = blackLevelFuncR_.eval(1.0); > > > + > > > + if (!blackLevelFuncG_.empty()) > > > + blackLevelG_ = blackLevelFuncG_.eval(1.0); > > > + > > > + if (!blackLevelFuncB_.empty()) > > > + blackLevelB_ = blackLevelFuncB_.eval(1.0); > > > + > > > blackLevelR = blackLevelR_; > > > blackLevelG = blackLevelG_; > > > blackLevelB = blackLevelB_; > > > @@ -54,10 +71,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(deviceStatus.analogueGain); > > > + > > > + if (!blackLevelFuncG_.empty()) > > > + blackLevelG_ = > > > blackLevelFuncG_.eval(deviceStatus.analogueGain); > > > + > > > + if (!blackLevelFuncB_.empty()) > > > + blackLevelB_ = > > > blackLevelFuncB_.eval(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 Mon, Aug 03, 2026 at 01:07:53PM +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 | 33 +++++++++++++++++++--- > src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ > 2 files changed, 36 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..876bc661d 100644 > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > @@ -41,12 +41,29 @@ 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); > + What are your expectations on the tuning file content ? Is it legal to specify black_level_r/black_level_func/black_level_func_r at the same time ? Do you want to error out if both black_level_func and black_level_func_r are there ? Do black_level_func_r take precedence over black_level_r ? In other words: do you want to be a bit more strict in validating the tuning file content, or are you happy with this ? > return 0; > } > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > uint16_t &blackLevelB) > { > + if (!blackLevelFuncR_.empty()) > + blackLevelR_ = blackLevelFuncR_.eval(1.0); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = blackLevelFuncG_.eval(1.0); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = blackLevelFuncB_.eval(1.0); > + > blackLevelR = blackLevelR_; > blackLevelG = blackLevelG_; > blackLevelB = blackLevelB_; > @@ -54,10 +71,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(deviceStatus.analogueGain); > + > + if (!blackLevelFuncG_.empty()) > + blackLevelG_ = blackLevelFuncG_.eval(deviceStatus.analogueGain); > + > + if (!blackLevelFuncB_.empty()) > + blackLevelB_ = blackLevelFuncB_.eval(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 Yes, good questions, I think. The functions always take precedence if they've been defined. I agree some kind of warning wouldn't go amiss if there are extraneous parameters, just for those folks who like to fiddle with tuning files by hand. So I'll take a quick look at that. Thanks David On Thu, 6 Aug 2026 at 08:52, Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi David > > On Mon, Aug 03, 2026 at 01:07:53PM +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 | 33 +++++++++++++++++++--- > > src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ > > 2 files changed, 36 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..876bc661d 100644 > > --- a/src/ipa/rpi/controller/rpi/black_level.cpp > > +++ b/src/ipa/rpi/controller/rpi/black_level.cpp > > @@ -41,12 +41,29 @@ 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); > > + > > What are your expectations on the tuning file content ? > > Is it legal to specify > black_level_r/black_level_func/black_level_func_r at the same time ? > > Do you want to error out if both black_level_func and > black_level_func_r are there ? > > Do black_level_func_r take precedence over black_level_r ? > > In other words: do you want to be a bit more strict in validating the > tuning file content, or are you happy with this ? > > > > return 0; > > } > > > > void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, > > uint16_t &blackLevelB) > > { > > + if (!blackLevelFuncR_.empty()) > > + blackLevelR_ = blackLevelFuncR_.eval(1.0); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = blackLevelFuncG_.eval(1.0); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = blackLevelFuncB_.eval(1.0); > > + > > blackLevelR = blackLevelR_; > > blackLevelG = blackLevelG_; > > blackLevelB = blackLevelB_; > > @@ -54,10 +71,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(deviceStatus.analogueGain); > > + > > + if (!blackLevelFuncG_.empty()) > > + blackLevelG_ = blackLevelFuncG_.eval(deviceStatus.analogueGain); > > + > > + if (!blackLevelFuncB_.empty()) > > + blackLevelB_ = blackLevelFuncB_.eval(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..876bc661d 100644 --- a/src/ipa/rpi/controller/rpi/black_level.cpp +++ b/src/ipa/rpi/controller/rpi/black_level.cpp @@ -41,12 +41,29 @@ 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); + return 0; } void BlackLevel::initialValues(uint16_t &blackLevelR, uint16_t &blackLevelG, uint16_t &blackLevelB) { + if (!blackLevelFuncR_.empty()) + blackLevelR_ = blackLevelFuncR_.eval(1.0); + + if (!blackLevelFuncG_.empty()) + blackLevelG_ = blackLevelFuncG_.eval(1.0); + + if (!blackLevelFuncB_.empty()) + blackLevelB_ = blackLevelFuncB_.eval(1.0); + blackLevelR = blackLevelR_; blackLevelG = blackLevelG_; blackLevelB = blackLevelB_; @@ -54,10 +71,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(deviceStatus.analogueGain); + + if (!blackLevelFuncG_.empty()) + blackLevelG_ = blackLevelFuncG_.eval(deviceStatus.analogueGain); + + if (!blackLevelFuncB_.empty()) + blackLevelB_ = blackLevelFuncB_.eval(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 | 33 +++++++++++++++++++--- src/ipa/rpi/controller/rpi/black_level.h | 7 +++++ 2 files changed, 36 insertions(+), 4 deletions(-)