[v2,2/4] ipa: rpi: Allow black levels to vary with analogue gain
diff mbox series

Message ID 20260806152701.4179-3-david.plowman@raspberrypi.com
State New
Headers show
Series
  • Black level and other fixes for IMX662
Related show

Commit Message

David Plowman Aug. 6, 2026, 3:19 p.m. UTC
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(-)

Comments

Jacopo Mondi Aug. 6, 2026, 3:54 p.m. UTC | #1
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 &params)
>  		<< " 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
>
David Plowman Aug. 6, 2026, 4:31 p.m. UTC | #2
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 &params)
> >               << " 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
> >
Jacopo Mondi Aug. 10, 2026, 9:03 a.m. UTC | #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 &params)
> > >               << " 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
> > >

Patch
diff mbox series

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 &params)
 		<< " 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 */