[1/1] libcamera: software_isp: Get black level from the camera helper
diff mbox series

Message ID 20240815083722.237229-2-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Get black level from the camera helper
Related show

Commit Message

Milan Zamazal Aug. 15, 2024, 8:37 a.m. UTC 
The black level in software ISP is unconditionally guessed from the
obtained frames.  CameraSensorHelper optionally provides the black level
from camera specifications now.  Let's use the value if available.

The black level value initialization is moved from init to configure
because this is where we get the other configuration (gains) from
CameraSensorHelper in software ISP.

If the black level is not available from the given CameraSensorHelper
instance, it's still determined on the fly.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 10 +++++++---
 src/ipa/simple/algorithms/blc.h   |  3 ++-
 src/ipa/simple/ipa_context.h      |  3 ++-
 src/ipa/simple/soft_simple.cpp    |  3 +++
 4 files changed, 14 insertions(+), 5 deletions(-)

Comments

Kieran Bingham Sept. 12, 2024, 12:27 p.m. UTC | #1 
Quoting Milan Zamazal (2024-08-15 09:37:16)
> The black level in software ISP is unconditionally guessed from the
> obtained frames.  CameraSensorHelper optionally provides the black level
> from camera specifications now.  Let's use the value if available.
> 
> The black level value initialization is moved from init to configure
> because this is where we get the other configuration (gains) from
> CameraSensorHelper in software ISP.
> 
> If the black level is not available from the given CameraSensorHelper
> instance, it's still determined on the fly.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 10 +++++++---
>  src/ipa/simple/algorithms/blc.h   |  3 ++-
>  src/ipa/simple/ipa_context.h      |  3 ++-
>  src/ipa/simple/soft_simple.cpp    |  3 +++
>  4 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index d01c2c14..5dc924bb 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()
>  {
>  }
>  
> -int BlackLevel::init(IPAContext &context,
> -                    [[maybe_unused]] const YamlObject &tuningData)
> +int BlackLevel::configure(typename Module::Context &context,
> +                         [[maybe_unused]] const typename Module::Config &configInfo)
>  {
> -       context.activeState.black.level = 1.0;
> +       context.activeState.black.level =
> +               context.configuration.black.level.value_or(1.0);
>         return 0;
>  }
>  
> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,
>                          const SwIspStats *stats,
>                          [[maybe_unused]] ControlList &metadata)
>  {
> +       if (context.configuration.black.level.has_value())
> +               return;
> +
>         if (frameContext.sensor.exposure == exposure_ &&
>             frameContext.sensor.gain == gain_) {
>                 return;
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 9b9daab1..586a23d2 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -24,7 +24,8 @@ public:
>         BlackLevel();
>         ~BlackLevel() = default;
>  
> -       int init(IPAContext &context, const YamlObject &tuningData)
> +       int configure(typename Module::Context &context,
> +                     const typename Module::Config &configInfo)
>                 override;
>         void process(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 0e2096f8..08f965f4 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -8,6 +8,7 @@
>  #pragma once
>  
>  #include <array>
> +#include <optional>
>  
>  #include <libcamera/controls.h>
>  
> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {
>                 double againMin, againMax, againMinStep;
>         } agc;
>         struct {
> -               double level;
> +               std::optional<double> level;
>         } black;
>  };
>  
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index aef5f6e5..ac7a22b7 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                         (context_.configuration.agc.againMax -
>                          context_.configuration.agc.againMin) /
>                         100.0;
> +               if (camHelper_->blackLevel().has_value())
> +                       context_.configuration.black.level =
> +                               camHelper_->blackLevel().value() / 65536.0;

Shouldn't this scale the value depending on the cameras configured bit
depth or such for when you use it in your calculations on pixel values?

Why 65536? 

I've recently just pushed the IMX283 black level value as:

> +		/* From datasheet: 0x32 at 10bits. */
> +		blackLevel_ = 3200;


Do you treat black level as value between 0 and 1 ?

--
Kieran



>         } else {
>                 /*
>                  * The camera sensor gain (g) is usually not equal to the value written
> -- 
> 2.44.1
>
Milan Zamazal Sept. 16, 2024, 2:36 p.m. UTC | #2 
Hi Kieran,

thank you for review.

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-08-15 09:37:16)
>> The black level in software ISP is unconditionally guessed from the
>> obtained frames.  CameraSensorHelper optionally provides the black level
>
>> from camera specifications now.  Let's use the value if available.
>> 
>> The black level value initialization is moved from init to configure
>> because this is where we get the other configuration (gains) from
>> CameraSensorHelper in software ISP.
>> 
>> If the black level is not available from the given CameraSensorHelper
>> instance, it's still determined on the fly.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 10 +++++++---
>>  src/ipa/simple/algorithms/blc.h   |  3 ++-
>>  src/ipa/simple/ipa_context.h      |  3 ++-
>>  src/ipa/simple/soft_simple.cpp    |  3 +++
>>  4 files changed, 14 insertions(+), 5 deletions(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index d01c2c14..5dc924bb 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -21,10 +21,11 @@ BlackLevel::BlackLevel()
>>  {
>>  }
>>  
>> -int BlackLevel::init(IPAContext &context,
>> -                    [[maybe_unused]] const YamlObject &tuningData)
>> +int BlackLevel::configure(typename Module::Context &context,
>> +                         [[maybe_unused]] const typename Module::Config &configInfo)
>>  {
>> -       context.activeState.black.level = 1.0;
>> +       context.activeState.black.level =
>> +               context.configuration.black.level.value_or(1.0);
>>         return 0;
>>  }
>>  
>> @@ -34,6 +35,9 @@ void BlackLevel::process(IPAContext &context,
>>                          const SwIspStats *stats,
>>                          [[maybe_unused]] ControlList &metadata)
>>  {
>> +       if (context.configuration.black.level.has_value())
>> +               return;
>> +
>>         if (frameContext.sensor.exposure == exposure_ &&
>>             frameContext.sensor.gain == gain_) {
>>                 return;
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 9b9daab1..586a23d2 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -24,7 +24,8 @@ public:
>>         BlackLevel();
>>         ~BlackLevel() = default;
>>  
>> -       int init(IPAContext &context, const YamlObject &tuningData)
>> +       int configure(typename Module::Context &context,
>> +                     const typename Module::Config &configInfo)
>>                 override;
>>         void process(IPAContext &context, const uint32_t frame,
>>                      IPAFrameContext &frameContext,
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 0e2096f8..08f965f4 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -8,6 +8,7 @@
>>  #pragma once
>>  
>>  #include <array>
>> +#include <optional>
>>  
>>  #include <libcamera/controls.h>
>>  
>> @@ -24,7 +25,7 @@ struct IPASessionConfiguration {
>>                 double againMin, againMax, againMinStep;
>>         } agc;
>>         struct {
>> -               double level;
>> +               std::optional<double> level;
>>         } black;
>>  };
>>  
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index aef5f6e5..ac7a22b7 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,6 +201,9 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                         (context_.configuration.agc.againMax -
>>                          context_.configuration.agc.againMin) /
>>                         100.0;
>> +               if (camHelper_->blackLevel().has_value())
>> +                       context_.configuration.black.level =
>> +                               camHelper_->blackLevel().value() / 65536.0;
>
> Shouldn't this scale the value depending on the cameras configured bit
> depth or such for when you use it in your calculations on pixel values?
>
> Why 65536? 

CameraSensorHelper::blackLevel() docstring says:

  This function returns the black level of the sensor scaled to a 16bit
  pixel width.

> I've recently just pushed the IMX283 black level value as:
>
>> +		/* From datasheet: 0x32 at 10bits. */
>> +		blackLevel_ = 3200;

Which is in accordance with that docstring:
50 (== 0x32) * 64 (== 2^(16-10)) = 3200

> Do you treat black level as value between 0 and 1 ?

Yes, this is what it is expected to be after the current software ISP
refactoring patches, but it may change based on the final version.

> --
> Kieran
>
>
>
>>         } else {
>>                 /*
>>                  * The camera sensor gain (g) is usually not equal to the value written
>> -- 
>> 2.44.1
>>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index d01c2c14..5dc924bb 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -21,10 +21,11 @@  BlackLevel::BlackLevel()
 {
 }
 
-int BlackLevel::init(IPAContext &context,
-		     [[maybe_unused]] const YamlObject &tuningData)
+int BlackLevel::configure(typename Module::Context &context,
+			  [[maybe_unused]] const typename Module::Config &configInfo)
 {
-	context.activeState.black.level = 1.0;
+	context.activeState.black.level =
+		context.configuration.black.level.value_or(1.0);
 	return 0;
 }
 
@@ -34,6 +35,9 @@  void BlackLevel::process(IPAContext &context,
 			 const SwIspStats *stats,
 			 [[maybe_unused]] ControlList &metadata)
 {
+	if (context.configuration.black.level.has_value())
+		return;
+
 	if (frameContext.sensor.exposure == exposure_ &&
 	    frameContext.sensor.gain == gain_) {
 		return;
diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
index 9b9daab1..586a23d2 100644
--- a/src/ipa/simple/algorithms/blc.h
+++ b/src/ipa/simple/algorithms/blc.h
@@ -24,7 +24,8 @@  public:
 	BlackLevel();
 	~BlackLevel() = default;
 
-	int init(IPAContext &context, const YamlObject &tuningData)
+	int configure(typename Module::Context &context,
+		      const typename Module::Config &configInfo)
 		override;
 	void process(IPAContext &context, const uint32_t frame,
 		     IPAFrameContext &frameContext,
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 0e2096f8..08f965f4 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -8,6 +8,7 @@ 
 #pragma once
 
 #include <array>
+#include <optional>
 
 #include <libcamera/controls.h>
 
@@ -24,7 +25,7 @@  struct IPASessionConfiguration {
 		double againMin, againMax, againMinStep;
 	} agc;
 	struct {
-		double level;
+		std::optional<double> level;
 	} black;
 };
 
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index aef5f6e5..ac7a22b7 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -201,6 +201,9 @@  int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
 			(context_.configuration.agc.againMax -
 			 context_.configuration.agc.againMin) /
 			100.0;
+		if (camHelper_->blackLevel().has_value())
+			context_.configuration.black.level =
+				camHelper_->blackLevel().value() / 65536.0;
 	} else {
 		/*
 		 * The camera sensor gain (g) is usually not equal to the value written