[v5,2/2] libcamera: software_isp: Black level from tuning file
diff mbox series

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

Commit Message

Milan Zamazal Oct. 18, 2024, 9:26 a.m. UTC 
This patch allows obtaining a black level from a tuning file in addition
to the camera sensor helper.  If both of them define a black level, the
one from the tuning file takes precedence.

The use cases are:

- A user wants to use a different black level, for whatever reason.

- There is a sensor without known gains but with a known black level.
  Because a camera sensor helper cannot be defined without specifying
  gains, the only way to specify the black level is using the tuning
  file.  Software ISP uses its fallback gain handling in such a case.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
 src/ipa/simple/algorithms/blc.h   | 1 +
 src/ipa/simple/soft_simple.cpp    | 3 ++-
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Oct. 18, 2024, 1:45 p.m. UTC | #1 
Quoting Milan Zamazal (2024-10-18 10:26:28)
> This patch allows obtaining a black level from a tuning file in addition
> to the camera sensor helper.  If both of them define a black level, the
> one from the tuning file takes precedence.
> 
> The use cases are:
> 
> - A user wants to use a different black level, for whatever reason.
> 
> - There is a sensor without known gains but with a known black level.
>   Because a camera sensor helper cannot be defined without specifying
>   gains, the only way to specify the black level is using the tuning
>   file.  Software ISP uses its fallback gain handling in such a case.
> 
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
>  src/ipa/simple/algorithms/blc.h   | 1 +
>  src/ipa/simple/soft_simple.cpp    | 3 ++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index a7af2e12c..8516c4335 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -21,11 +21,20 @@ BlackLevel::BlackLevel()
>  {
>  }
>  
> +int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
> +{
> +       auto blackLevel = tuningData["blackLevel"].get<int16_t>();
> +       if (blackLevel.has_value())
> +               context.configuration.black.level = blackLevel.value() / 256;

It's not essential - but for converting between bit depths - I would
think using bit shifting would be clearer (keep this however you prefer
though).

	/*
	 * Convert 16 bit values from the tuning file to 8 bit black
	 * level for the SoftISP.
	 */
	.... = blackLevel.value() >> 8;

might be more clear than
	.... = blackLevel.value() / 256;

> +       return 0;
> +}
> +
>  int BlackLevel::configure(IPAContext &context,
>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>  {
>         context.activeState.blc.level =
>                 context.configuration.black.level.value_or(255);
> +       LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;

What's pdm: blll? Should this be removed? or changed to be Debug level ?


For the update itself with those handled, I think having the Tuning File
able to specify the black level is helpful:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>         return 0;
>  }
>  
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 828ad8b18..2cf2a8774 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -19,6 +19,7 @@ public:
>         BlackLevel();
>         ~BlackLevel() = default;
>  
> +       int init(IPAContext &context, const YamlObject &tuningData) override;
>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>         void process(IPAContext &context, const uint32_t frame,
>                      IPAFrameContext &frameContext,
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index e96e65bd1..03525b2f2 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -201,7 +201,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>                         (context_.configuration.agc.againMax -
>                          context_.configuration.agc.againMin) /
>                         100.0;
> -               if (camHelper_->blackLevel().has_value()) {
> +               if (!context_.configuration.black.level.has_value() &&
> +                   camHelper_->blackLevel().has_value()) {
>                         /*
>                          * The black level from camHelper_ is a 16 bit value, software ISP
>                          * works with 8 bit pixel values, both regardless of the actual
> -- 
> 2.44.1
>
Milan Zamazal Oct. 18, 2024, 2:02 p.m. UTC | #2 
Hi Kieran,

thank you for review.

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

> Quoting Milan Zamazal (2024-10-18 10:26:28)
>> This patch allows obtaining a black level from a tuning file in addition
>> to the camera sensor helper.  If both of them define a black level, the
>
>> one from the tuning file takes precedence.
>> 
>> The use cases are:
>> 
>> - A user wants to use a different black level, for whatever reason.
>> 
>> - There is a sensor without known gains but with a known black level.
>>   Because a camera sensor helper cannot be defined without specifying
>>   gains, the only way to specify the black level is using the tuning
>>   file.  Software ISP uses its fallback gain handling in such a case.
>> 
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
>>  src/ipa/simple/algorithms/blc.h   | 1 +
>>  src/ipa/simple/soft_simple.cpp    | 3 ++-
>>  3 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index a7af2e12c..8516c4335 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -21,11 +21,20 @@ BlackLevel::BlackLevel()
>>  {
>>  }
>>  
>> +int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
>> +{
>> +       auto blackLevel = tuningData["blackLevel"].get<int16_t>();
>> +       if (blackLevel.has_value())
>> +               context.configuration.black.level = blackLevel.value() / 256;
>
> It's not essential - but for converting between bit depths - I would
> think using bit shifting would be clearer (keep this however you prefer
> though).
>
> 	/*
> 	 * Convert 16 bit values from the tuning file to 8 bit black
> 	 * level for the SoftISP.
> 	 */
> 	.... = blackLevel.value() >> 8;
>
> might be more clear than
> 	.... = blackLevel.value() / 256;

OK.

>> +       return 0;
>> +}
>> +
>>  int BlackLevel::configure(IPAContext &context,
>>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
>>  {
>>         context.activeState.blc.level =
>>                 context.configuration.black.level.value_or(255);
>> +       LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;
>
> What's pdm: blll? Should this be removed? or changed to be Debug level ?

A forgotten statement for checking the value, should be removed.

> For the update itself with those handled, I think having the Tuning File
> able to specify the black level is helpful:
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>>         return 0;
>>  }
>>  
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 828ad8b18..2cf2a8774 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -19,6 +19,7 @@ public:
>>         BlackLevel();
>>         ~BlackLevel() = default;
>>  
>> +       int init(IPAContext &context, const YamlObject &tuningData) override;
>>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
>>         void process(IPAContext &context, const uint32_t frame,
>>                      IPAFrameContext &frameContext,
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index e96e65bd1..03525b2f2 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,7 +201,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>                         (context_.configuration.agc.againMax -
>>                          context_.configuration.agc.againMin) /
>>                         100.0;
>> -               if (camHelper_->blackLevel().has_value()) {
>> +               if (!context_.configuration.black.level.has_value() &&
>> +                   camHelper_->blackLevel().has_value()) {
>>                         /*
>>                          * The black level from camHelper_ is a 16 bit value, software ISP
>>                          * works with 8 bit pixel values, both regardless of the actual
>> -- 
>> 2.44.1
>>
Kieran Bingham Oct. 18, 2024, 2:06 p.m. UTC | #3 
Quoting Milan Zamazal (2024-10-18 15:02:54)
> Hi Kieran,
> 
> thank you for review.
> 
> Kieran Bingham <kieran.bingham@ideasonboard.com> writes:
> 
> > Quoting Milan Zamazal (2024-10-18 10:26:28)
> >> This patch allows obtaining a black level from a tuning file in addition
> >> to the camera sensor helper.  If both of them define a black level, the
> >
> >> one from the tuning file takes precedence.
> >> 
> >> The use cases are:
> >> 
> >> - A user wants to use a different black level, for whatever reason.
> >> 
> >> - There is a sensor without known gains but with a known black level.
> >>   Because a camera sensor helper cannot be defined without specifying
> >>   gains, the only way to specify the black level is using the tuning
> >>   file.  Software ISP uses its fallback gain handling in such a case.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  src/ipa/simple/algorithms/blc.cpp | 9 +++++++++
> >>  src/ipa/simple/algorithms/blc.h   | 1 +
> >>  src/ipa/simple/soft_simple.cpp    | 3 ++-
> >>  3 files changed, 12 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> >> index a7af2e12c..8516c4335 100644
> >> --- a/src/ipa/simple/algorithms/blc.cpp
> >> +++ b/src/ipa/simple/algorithms/blc.cpp
> >> @@ -21,11 +21,20 @@ BlackLevel::BlackLevel()
> >>  {
> >>  }
> >>  
> >> +int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
> >> +{
> >> +       auto blackLevel = tuningData["blackLevel"].get<int16_t>();
> >> +       if (blackLevel.has_value())
> >> +               context.configuration.black.level = blackLevel.value() / 256;
> >
> > It's not essential - but for converting between bit depths - I would
> > think using bit shifting would be clearer (keep this however you prefer
> > though).
> >
> >       /*
> >        * Convert 16 bit values from the tuning file to 8 bit black
> >        * level for the SoftISP.
> >        */
> >       .... = blackLevel.value() >> 8;
> >
> > might be more clear than
> >       .... = blackLevel.value() / 256;
> 
> OK.
> 
> >> +       return 0;
> >> +}
> >> +
> >>  int BlackLevel::configure(IPAContext &context,
> >>                           [[maybe_unused]] const IPAConfigInfo &configInfo)
> >>  {
> >>         context.activeState.blc.level =
> >>                 context.configuration.black.level.value_or(255);
> >> +       LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;
> >
> > What's pdm: blll? Should this be removed? or changed to be Debug level ?
> 
> A forgotten statement for checking the value, should be removed.

Ack, so with that removed, CI is clear - so we can merge the next
version!

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1292807

> 
> > For the update itself with those handled, I think having the Tuning File
> > able to specify the black level is helpful:
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> >>         return 0;
> >>  }
> >>  
> >> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> >> index 828ad8b18..2cf2a8774 100644
> >> --- a/src/ipa/simple/algorithms/blc.h
> >> +++ b/src/ipa/simple/algorithms/blc.h
> >> @@ -19,6 +19,7 @@ public:
> >>         BlackLevel();
> >>         ~BlackLevel() = default;
> >>  
> >> +       int init(IPAContext &context, const YamlObject &tuningData) override;
> >>         int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
> >>         void process(IPAContext &context, const uint32_t frame,
> >>                      IPAFrameContext &frameContext,
> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> >> index e96e65bd1..03525b2f2 100644
> >> --- a/src/ipa/simple/soft_simple.cpp
> >> +++ b/src/ipa/simple/soft_simple.cpp
> >> @@ -201,7 +201,8 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> >>                         (context_.configuration.agc.againMax -
> >>                          context_.configuration.agc.againMin) /
> >>                         100.0;
> >> -               if (camHelper_->blackLevel().has_value()) {
> >> +               if (!context_.configuration.black.level.has_value() &&
> >> +                   camHelper_->blackLevel().has_value()) {
> >>                         /*
> >>                          * The black level from camHelper_ is a 16 bit value, software ISP
> >>                          * works with 8 bit pixel values, both regardless of the actual
> >> -- 
> >> 2.44.1
> >>
>

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index a7af2e12c..8516c4335 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -21,11 +21,20 @@  BlackLevel::BlackLevel()
 {
 }
 
+int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
+{
+	auto blackLevel = tuningData["blackLevel"].get<int16_t>();
+	if (blackLevel.has_value())
+		context.configuration.black.level = blackLevel.value() / 256;
+	return 0;
+}
+
 int BlackLevel::configure(IPAContext &context,
 			  [[maybe_unused]] const IPAConfigInfo &configInfo)
 {
 	context.activeState.blc.level =
 		context.configuration.black.level.value_or(255);
+	LOG(IPASoftBL, Info) << "pdm: blll: " << context.activeState.blc.level;
 	return 0;
 }
 
diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
index 828ad8b18..2cf2a8774 100644
--- a/src/ipa/simple/algorithms/blc.h
+++ b/src/ipa/simple/algorithms/blc.h
@@ -19,6 +19,7 @@  public:
 	BlackLevel();
 	~BlackLevel() = default;
 
+	int init(IPAContext &context, const YamlObject &tuningData) override;
 	int configure(IPAContext &context, const IPAConfigInfo &configInfo) override;
 	void process(IPAContext &context, const uint32_t frame,
 		     IPAFrameContext &frameContext,
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index e96e65bd1..03525b2f2 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -201,7 +201,8 @@  int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
 			(context_.configuration.agc.againMax -
 			 context_.configuration.agc.againMin) /
 			100.0;
-		if (camHelper_->blackLevel().has_value()) {
+		if (!context_.configuration.black.level.has_value() &&
+		    camHelper_->blackLevel().has_value()) {
 			/*
 			 * The black level from camHelper_ is a 16 bit value, software ISP
 			 * works with 8 bit pixel values, both regardless of the actual