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

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

Commit Message

Milan Zamazal Oct. 11, 2024, 3:39 p.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.

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>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/simple/algorithms/blc.cpp | 6 +++++-
 src/ipa/simple/ipa_context.h      | 4 ++++
 src/ipa/simple/soft_simple.cpp    | 9 +++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)

Comments

Robert Mader Oct. 12, 2024, 7:49 p.m. UTC | #1 
I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases 
using 4096 made the resulting image fully black when covering the 
sensor, which IIUC is the desired effect. Not putting any values or 
using lower ones such as 3200 results in a greenish image - so there's 
definitely an improvement here. Thus fell free to add a Tested-by: 
Robert Mader <robert.mader@collabora.com>

At least in the case of the IMX355 the output overall becomes much 
darker - too dark IMO. Is that expected - and can/will it be compensated 
by improvements of the AWB algorithm?

P.S.: I guess I can submit a follow-up adding the black level for the 
IMX355 - the IMX363 driver hasn't been submitted to upstream yet.

On 11.10.24 17:39, Milan Zamazal wrote:
> 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.
>
> 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>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>   src/ipa/simple/ipa_context.h      | 4 ++++
>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>   3 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index b9f2aaa6d..a7af2e12c 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>   int BlackLevel::configure(IPAContext &context,
>   			  [[maybe_unused]] const IPAConfigInfo &configInfo)
>   {
> -	context.activeState.blc.level = 255;
> +	context.activeState.blc.level =
> +		context.configuration.black.level.value_or(255);
>   	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/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 3519f20f6..fd121eebe 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 <stdint.h>
>   
>   #include <libipa/fc_queue.h>
> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>   		int32_t exposureMin, exposureMax;
>   		double againMin, againMax, againMinStep;
>   	} agc;
> +	struct {
> +		std::optional<uint8_t> level;
> +	} black;
>   };
>   
>   struct IPAActiveState {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index b28c7039f..079409f6d 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>   			(context_.configuration.agc.againMax -
>   			 context_.configuration.agc.againMin) /
>   			100.0;
> +		if (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
> +			 * sensor pixel width. Hence we obtain the pixel-based black value
> +			 * by dividing the value from the helper by 256.
> +			 */
> +			context_.configuration.black.level =
> +				camHelper_->blackLevel().value() / 256;
>   	} else {
>   		/*
>   		 * The camera sensor gain (g) is usually not equal to the value written
Robert Mader Oct. 13, 2024, 11:23 a.m. UTC | #2 
Turns out I have to ask for/suggest some more changes:

On 12.10.24 21:49, Robert Mader wrote:
> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases 
> using 4096 made the resulting image fully black when covering the 
> sensor, which IIUC is the desired effect. Not putting any values or 
> using lower ones such as 3200 results in a greenish image - so there's 
> definitely an improvement here. Thus fell free to add a Tested-by: 
> Robert Mader <robert.mader@collabora.com>
>
> At least in the case of the IMX355 the output overall becomes much 
> darker - too dark IMO. Is that expected - and can/will it be 
> compensated by improvements of the AWB algorithm?
>
> P.S.: I guess I can submit a follow-up adding the black level for the 
> IMX355 - the IMX363 driver hasn't been submitted to upstream yet.
>
> On 11.10.24 17:39, Milan Zamazal wrote:
>> 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.
>>
>> 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>
>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>   src/ipa/simple/ipa_context.h      | 4 ++++
>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>   3 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp 
>> b/src/ipa/simple/algorithms/blc.cpp
>> index b9f2aaa6d..a7af2e12c 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>   int BlackLevel::configure(IPAContext &context,
>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)
>>   {
>> -    context.activeState.blc.level = 255;
>> +    context.activeState.blc.level =
>> +        context.configuration.black.level.value_or(255);
>>       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/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 3519f20f6..fd121eebe 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 <stdint.h>
>>     #include <libipa/fc_queue.h>
>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>           int32_t exposureMin, exposureMax;
>>           double againMin, againMax, againMinStep;
>>       } agc;
>> +    struct {
>> +        std::optional<uint8_t> level;
>> +    } black;
>>   };
>>     struct IPAActiveState {
>> diff --git a/src/ipa/simple/soft_simple.cpp 
>> b/src/ipa/simple/soft_simple.cpp
>> index b28c7039f..079409f6d 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo 
>> &configInfo)
>>               (context_.configuration.agc.againMax -
>>                context_.configuration.agc.againMin) /
>>               100.0;
>> +        if (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
>> +             * sensor pixel width. Hence we obtain the pixel-based 
>> black value
>> +             * by dividing the value from the helper by 256.
>> +             */
>> +            context_.configuration.black.level =
>> +                camHelper_->blackLevel().value() / 256;

Style nit: braces around this block would be good now that there's the 
comment.

More importantly though: Assuming setting a value for blackLevel_ 
without providing values for gain makes sense, then it would be great to 
adapt the code above to not assume so. It currently only works in builds 
with asserts disabled - which I used yesterday when trying it. There are 
two more similar places in processStats() (camHelper_ ? ...).

Forcing the non-cam-helper code for gain seems to fix the regression I 
mentioned in the previous mail.

>>       } else {
>>           /*
>>            * The camera sensor gain (g) is usually not equal to the 
>> value written
>
Milan Zamazal Oct. 14, 2024, 11:07 a.m. UTC | #3 
Hi Robert,

thank you for testing and review.

Robert Mader <robert.mader@collabora.com> writes:

> Turns out I have to ask for/suggest some more changes:
>
> On 12.10.24 21:49, Robert Mader wrote:
>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made
>> the resulting image fully black when covering the sensor, which IIUC is the desired
>> effect. 

Yes.

>> Not putting any values or using lower ones such as 3200 results in a
>> greenish image - so there's definitely an improvement here. Thus fell
>> free to add a Tested-by: Robert Mader <robert.mader@collabora.com>
>>
>> At least in the case of the IMX355 the output overall becomes much darker - too dark
>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB
>> algorithm?

No.  Blacks and shadows should be darker but overall the image should
have about the same brightness.  This is handled by auto-exposure, which
is applied after subtracting the black level.

>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the
>> IMX363 driver hasn't been submitted to upstream yet.
>>
>> On 11.10.24 17:39, Milan Zamazal wrote:
>>> 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.
>>>
>>> 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>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>   src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>>   src/ipa/simple/ipa_context.h      | 4 ++++
>>>   src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>>   3 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>> index b9f2aaa6d..a7af2e12c 100644
>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>>   int BlackLevel::configure(IPAContext &context,
>>>                 [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>   {
>>> -    context.activeState.blc.level = 255;
>>> +    context.activeState.blc.level =
>>> +        context.configuration.black.level.value_or(255);
>>>       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/ipa_context.h b/src/ipa/simple/ipa_context.h
>>> index 3519f20f6..fd121eebe 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 <stdint.h>
>>>     #include <libipa/fc_queue.h>
>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>>           int32_t exposureMin, exposureMax;
>>>           double againMin, againMax, againMinStep;
>>>       } agc;
>>> +    struct {
>>> +        std::optional<uint8_t> level;
>>> +    } black;
>>>   };
>>>     struct IPAActiveState {
>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>> index b28c7039f..079409f6d 100644
>>> --- a/src/ipa/simple/soft_simple.cpp
>>> +++ b/src/ipa/simple/soft_simple.cpp
>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>>               (context_.configuration.agc.againMax -
>>>                context_.configuration.agc.againMin) /
>>>               100.0;
>>> +        if (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
>>> +             * sensor pixel width. Hence we obtain the pixel-based black value
>>> +             * by dividing the value from the helper by 256.
>>> +             */
>>> +            context_.configuration.black.level =
>>> +                camHelper_->blackLevel().value() / 256;
>
> Style nit: braces around this block would be good now that there's the comment.

OK.

> More importantly though: Assuming setting a value for blackLevel_ without providing
> values for gain makes sense, then it would be great to adapt the code above to not
> assume so. It currently only works in builds with asserts disabled - which I used
> yesterday when trying it. There are two more similar places in processStats()
> (camHelper_ ? ...).
>
> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the
> previous mail.

I'm not sure I understand what you mean exactly.  What the patch does is
that if a camera sensor helper is available for the given sensor and it
provides black level then that black level is used; otherwise black
level auto-detection is used (as before).  If there is no helper for the
given sensor, nothing changes.  Do you mean there is some trouble in the
current code in such a case (it seems to work fine for me)?  Or do you
talk about a situation when the helper provides black level but not
the corresponding gains?  And what assertion fails?

>>>       } else {
>>>           /*
>>>            * The camera sensor gain (g) is usually not equal to the value written
>>
Robert Mader Oct. 14, 2024, 11:34 a.m. UTC | #4 
On 14.10.24 13:07, Milan Zamazal wrote:
> Hi Robert,
>
> thank you for testing and review.
>
> Robert Mader <robert.mader@collabora.com> writes:
>
>> Turns out I have to ask for/suggest some more changes:
>>
>> On 12.10.24 21:49, Robert Mader wrote:
>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made
>>> the resulting image fully black when covering the sensor, which IIUC is the desired
>>> effect.
> Yes.
>
>>> Not putting any values or using lower ones such as 3200 results in a
>>> greenish image - so there's definitely an improvement here. Thus fell
>>> free to add a Tested-by: Robert Mader <robert.mader@collabora.com>
>>>
>>> At least in the case of the IMX355 the output overall becomes much darker - too dark
>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB
>>> algorithm?
> No.  Blacks and shadows should be darker but overall the image should
> have about the same brightness.  This is handled by auto-exposure, which
> is applied after subtracting the black level.
Indeed, and I can confirm that this works correctly once the issue below 
is addressed.
>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the
>>> IMX363 driver hasn't been submitted to upstream yet.
>>>
>>> On 11.10.24 17:39, Milan Zamazal wrote:
>>>> 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.
>>>>
>>>> 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>
>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>> ---
>>>>    src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>>>    src/ipa/simple/ipa_context.h      | 4 ++++
>>>>    src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>>>    3 files changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>>> index b9f2aaa6d..a7af2e12c 100644
>>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>>>    int BlackLevel::configure(IPAContext &context,
>>>>                  [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>>    {
>>>> -    context.activeState.blc.level = 255;
>>>> +    context.activeState.blc.level =
>>>> +        context.configuration.black.level.value_or(255);
>>>>        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/ipa_context.h b/src/ipa/simple/ipa_context.h
>>>> index 3519f20f6..fd121eebe 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 <stdint.h>
>>>>      #include <libipa/fc_queue.h>
>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>>>            int32_t exposureMin, exposureMax;
>>>>            double againMin, againMax, againMinStep;
>>>>        } agc;
>>>> +    struct {
>>>> +        std::optional<uint8_t> level;
>>>> +    } black;
>>>>    };
>>>>      struct IPAActiveState {
>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>> index b28c7039f..079409f6d 100644
>>>> --- a/src/ipa/simple/soft_simple.cpp
>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>>>                (context_.configuration.agc.againMax -
>>>>                 context_.configuration.agc.againMin) /
>>>>                100.0;
>>>> +        if (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
>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value
>>>> +             * by dividing the value from the helper by 256.
>>>> +             */
>>>> +            context_.configuration.black.level =
>>>> +                camHelper_->blackLevel().value() / 256;
>> Style nit: braces around this block would be good now that there's the comment.
> OK.
>
>> More importantly though: Assuming setting a value for blackLevel_ without providing
>> values for gain makes sense, then it would be great to adapt the code above to not
>> assume so. It currently only works in builds with asserts disabled - which I used
>> yesterday when trying it. There are two more similar places in processStats()
>> (camHelper_ ? ...).
>>
>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the
>> previous mail.
> I'm not sure I understand what you mean exactly.  What the patch does is
> that if a camera sensor helper is available for the given sensor and it
> provides black level then that black level is used; otherwise black
> level auto-detection is used (as before).  If there is no helper for the
> given sensor, nothing changes.  Do you mean there is some trouble in the
> current code in such a case (it seems to work fine for me)?  Or do you
> talk about a situation when the helper provides black level but not
> the corresponding gains?  And what assertion fails?

Sorry for being unclear here. There is a problem with the existing code 
that IMO should get addressed before adding the new black level related 
code.

That is the `if (camHelper_)` block here and two `camHelper_ ? ...` 
cases further below where it is assumed that the presence of 
`camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to 
succeed.

In order to test the new black level code added in this series, I added 
the following camera helpers:

> +class CameraSensorHelperImx355 : public CameraSensorHelper
> +{
> +public:
> +       CameraSensorHelperImx355()
> +       {
> +               blackLevel_ = 4096;
> +       }
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx355", CameraSensorHelperImx355)
> +
> +class CameraSensorHelperImx363 : public CameraSensorHelper
> +{
> +public:
> +       CameraSensorHelperImx363()
> +       {
> +               blackLevel_ = 4096;
> +       }
> +};
> +REGISTER_CAMERA_SENSOR_HELPER("imx363", CameraSensorHelperImx363)

With those, `camHelper_` is set and the pre-existing code 
unconditionally calls `camHelper_->gain()`. When assertions are on, one 
is triggered / playback is broken. When assertions are disabled, 
playback works but the output images are much darker, as described 
above. That can be fixed by force-disabling the calls to 
`camHelper_->gain()` and using the `camHelper_` code instead.

This is not strictly related to this series and could be fixed 
independently, but I guess it makes sense to fix things in an additional 
commit here.

AFAICS something similar to the new `if 
(camHelper_->blackLevel().has_value())` should be added for gain.

>
>>>>        } else {
>>>>            /*
>>>>             * The camera sensor gain (g) is usually not equal to the value written
Milan Zamazal Oct. 14, 2024, 6:53 p.m. UTC | #5 
Robert Mader <robert.mader@collabora.com> writes:

> On 14.10.24 13:07, Milan Zamazal wrote:
>> Hi Robert,
>>
>
>> thank you for testing and review.
>>
>> Robert Mader <robert.mader@collabora.com> writes:
>>
>>> Turns out I have to ask for/suggest some more changes:
>>>
>>> On 12.10.24 21:49, Robert Mader wrote:
>>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made
>>>> the resulting image fully black when covering the sensor, which IIUC is the desired
>>>> effect.
>> Yes.
>>
>>>> Not putting any values or using lower ones such as 3200 results in a
>>>> greenish image - so there's definitely an improvement here. Thus fell
>>>> free to add a Tested-by: Robert Mader <robert.mader@collabora.com>
>>>>
>>>> At least in the case of the IMX355 the output overall becomes much darker - too dark
>>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB
>>>> algorithm?
>> No.  Blacks and shadows should be darker but overall the image should
>> have about the same brightness.  This is handled by auto-exposure, which
>> is applied after subtracting the black level.
> Indeed, and I can confirm that this works correctly once the issue below is addressed.
>>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the
>>>> IMX363 driver hasn't been submitted to upstream yet.
>>>>
>>>> On 11.10.24 17:39, Milan Zamazal wrote:
>>>>> 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.
>>>>>
>>>>> 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>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>> ---
>>>>>    src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>>>>    src/ipa/simple/ipa_context.h      | 4 ++++
>>>>>    src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>>>>    3 files changed, 18 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>>>> index b9f2aaa6d..a7af2e12c 100644
>>>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>>>>    int BlackLevel::configure(IPAContext &context,
>>>>>                  [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>>>    {
>>>>> -    context.activeState.blc.level = 255;
>>>>> +    context.activeState.blc.level =
>>>>> +        context.configuration.black.level.value_or(255);
>>>>>        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/ipa_context.h b/src/ipa/simple/ipa_context.h
>>>>> index 3519f20f6..fd121eebe 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 <stdint.h>
>>>>>      #include <libipa/fc_queue.h>
>>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>>>>            int32_t exposureMin, exposureMax;
>>>>>            double againMin, againMax, againMinStep;
>>>>>        } agc;
>>>>> +    struct {
>>>>> +        std::optional<uint8_t> level;
>>>>> +    } black;
>>>>>    };
>>>>>      struct IPAActiveState {
>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>>> index b28c7039f..079409f6d 100644
>>>>> --- a/src/ipa/simple/soft_simple.cpp
>>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>>>>                (context_.configuration.agc.againMax -
>>>>>                 context_.configuration.agc.againMin) /
>>>>>                100.0;
>>>>> +        if (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
>>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value
>>>>> +             * by dividing the value from the helper by 256.
>>>>> +             */
>>>>> +            context_.configuration.black.level =
>>>>> +                camHelper_->blackLevel().value() / 256;
>>> Style nit: braces around this block would be good now that there's the comment.
>> OK.
>>
>>> More importantly though: Assuming setting a value for blackLevel_ without providing
>>> values for gain makes sense, then it would be great to adapt the code above to not
>>> assume so. It currently only works in builds with asserts disabled - which I used
>>> yesterday when trying it. There are two more similar places in processStats()
>>> (camHelper_ ? ...).
>>>
>>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the
>>> previous mail.
>> I'm not sure I understand what you mean exactly.  What the patch does is
>> that if a camera sensor helper is available for the given sensor and it
>> provides black level then that black level is used; otherwise black
>> level auto-detection is used (as before).  If there is no helper for the
>> given sensor, nothing changes.  Do you mean there is some trouble in the
>> current code in such a case (it seems to work fine for me)?  Or do you
>> talk about a situation when the helper provides black level but not
>> the corresponding gains?  And what assertion fails?
>
> Sorry for being unclear here. There is a problem with the existing code that IMO should get addressed
> before adding the new black level related code.
>
> That is the `if (camHelper_)` block here and two `camHelper_ ? ...` cases further below where it is
> assumed that the presence of `camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to
> succeed.
>
> In order to test the new black level code added in this series, I added the following camera helpers:
>
>> +class CameraSensorHelperImx355 : public CameraSensorHelper
>> +{
>> +public:
>> +       CameraSensorHelperImx355()
>> +       {
>> +               blackLevel_ = 4096;
>> +       }
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx355", CameraSensorHelperImx355)
>> +
>> +class CameraSensorHelperImx363 : public CameraSensorHelper
>> +{
>> +public:
>> +       CameraSensorHelperImx363()
>> +       {
>> +               blackLevel_ = 4096;
>> +       }
>> +};
>> +REGISTER_CAMERA_SENSOR_HELPER("imx363", CameraSensorHelperImx363)
>
> With those, `camHelper_` is set and the pre-existing code unconditionally calls
> `camHelper_->gain()`. When assertions are on, one is triggered / playback is broken. When assertions are
> disabled, playback works but the output images are much darker, as described above. That can be fixed by
> force-disabling the calls to `camHelper_->gain()` and using the `camHelper_` code instead.
>
> This is not strictly related to this series and could be fixed independently, but I guess it makes sense
> to fix things in an additional commit here.
>
> AFAICS something similar to the new `if (camHelper_->blackLevel().has_value())` should be added for gain.

I see, thank you for explanation.  I can reproduce the problem with dark
output.  It happens with or without this patch.  I don't get any
assertion error and I wouldn't expect one -- if you mean the assertions
in CameraSensorHelper::gain*, they pass with the default values.  But
0/0 is returned, which is of course a wrong value.

This also means that it is not expected that the helper doesn't define
gains (no surprise considering handling the gain constants was its only
purpose originally).  So there is a question whether a camera sensor
helper that doesn't define the gains is a valid helper.  I'd say why not
but other pipelines would have problems with such a helper too.  If we
want to support this then it'll require more changes.

Anyway, although it's an interesting issue, it has nothing to do with
this patch.  If you think there is a valid reason to define camera
sensor helpers without defining gains, I'd suggest opening a separate
thread about it or to file a bug.

>>>>>        } else {
>>>>>            /*
>>>>>             * The camera sensor gain (g) is usually not equal to the value written
Robert Mader Oct. 15, 2024, 10:04 a.m. UTC | #6 
On 14.10.24 20:53, Milan Zamazal wrote:
> Robert Mader <robert.mader@collabora.com> writes:
>
>> On 14.10.24 13:07, Milan Zamazal wrote:
>>> Hi Robert,
>>>
>>> thank you for testing and review.
>>>
>>> Robert Mader <robert.mader@collabora.com> writes:
>>>
>>>> Turns out I have to ask for/suggest some more changes:
>>>>
>>>> On 12.10.24 21:49, Robert Mader wrote:
>>>>> I gave this a try on a Pixel 3a (IMX 355 and IMX 363). In both cases using 4096 made
>>>>> the resulting image fully black when covering the sensor, which IIUC is the desired
>>>>> effect.
>>> Yes.
>>>
>>>>> Not putting any values or using lower ones such as 3200 results in a
>>>>> greenish image - so there's definitely an improvement here. Thus fell
>>>>> free to add a Tested-by: Robert Mader <robert.mader@collabora.com>
>>>>>
>>>>> At least in the case of the IMX355 the output overall becomes much darker - too dark
>>>>> IMO. Is that expected - and can/will it be compensated by improvements of the AWB
>>>>> algorithm?
>>> No.  Blacks and shadows should be darker but overall the image should
>>> have about the same brightness.  This is handled by auto-exposure, which
>>> is applied after subtracting the black level.
>> Indeed, and I can confirm that this works correctly once the issue below is addressed.
>>>>> P.S.: I guess I can submit a follow-up adding the black level for the IMX355 - the
>>>>> IMX363 driver hasn't been submitted to upstream yet.
>>>>>
>>>>> On 11.10.24 17:39, Milan Zamazal wrote:
>>>>>> 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.
>>>>>>
>>>>>> 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>
>>>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>>>>> ---
>>>>>>     src/ipa/simple/algorithms/blc.cpp | 6 +++++-
>>>>>>     src/ipa/simple/ipa_context.h      | 4 ++++
>>>>>>     src/ipa/simple/soft_simple.cpp    | 9 +++++++++
>>>>>>     3 files changed, 18 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>>>>>> index b9f2aaa6d..a7af2e12c 100644
>>>>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>>>>> @@ -24,7 +24,8 @@ BlackLevel::BlackLevel()
>>>>>>     int BlackLevel::configure(IPAContext &context,
>>>>>>                   [[maybe_unused]] const IPAConfigInfo &configInfo)
>>>>>>     {
>>>>>> -    context.activeState.blc.level = 255;
>>>>>> +    context.activeState.blc.level =
>>>>>> +        context.configuration.black.level.value_or(255);
>>>>>>         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/ipa_context.h b/src/ipa/simple/ipa_context.h
>>>>>> index 3519f20f6..fd121eebe 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 <stdint.h>
>>>>>>       #include <libipa/fc_queue.h>
>>>>>> @@ -22,6 +23,9 @@ struct IPASessionConfiguration {
>>>>>>             int32_t exposureMin, exposureMax;
>>>>>>             double againMin, againMax, againMinStep;
>>>>>>         } agc;
>>>>>> +    struct {
>>>>>> +        std::optional<uint8_t> level;
>>>>>> +    } black;
>>>>>>     };
>>>>>>       struct IPAActiveState {
>>>>>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>>>>>> index b28c7039f..079409f6d 100644
>>>>>> --- a/src/ipa/simple/soft_simple.cpp
>>>>>> +++ b/src/ipa/simple/soft_simple.cpp
>>>>>> @@ -201,6 +201,15 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
>>>>>>                 (context_.configuration.agc.againMax -
>>>>>>                  context_.configuration.agc.againMin) /
>>>>>>                 100.0;
>>>>>> +        if (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
>>>>>> +             * sensor pixel width. Hence we obtain the pixel-based black value
>>>>>> +             * by dividing the value from the helper by 256.
>>>>>> +             */
>>>>>> +            context_.configuration.black.level =
>>>>>> +                camHelper_->blackLevel().value() / 256;
>>>> Style nit: braces around this block would be good now that there's the comment.
>>> OK.
>>>
>>>> More importantly though: Assuming setting a value for blackLevel_ without providing
>>>> values for gain makes sense, then it would be great to adapt the code above to not
>>>> assume so. It currently only works in builds with asserts disabled - which I used
>>>> yesterday when trying it. There are two more similar places in processStats()
>>>> (camHelper_ ? ...).
>>>>
>>>> Forcing the non-cam-helper code for gain seems to fix the regression I mentioned in the
>>>> previous mail.
>>> I'm not sure I understand what you mean exactly.  What the patch does is
>>> that if a camera sensor helper is available for the given sensor and it
>>> provides black level then that black level is used; otherwise black
>>> level auto-detection is used (as before).  If there is no helper for the
>>> given sensor, nothing changes.  Do you mean there is some trouble in the
>>> current code in such a case (it seems to work fine for me)?  Or do you
>>> talk about a situation when the helper provides black level but not
>>> the corresponding gains?  And what assertion fails?
>> Sorry for being unclear here. There is a problem with the existing code that IMO should get addressed
>> before adding the new black level related code.
>>
>> That is the `if (camHelper_)` block here and two `camHelper_ ? ...` cases further below where it is
>> assumed that the presence of `camHelper_` allows `camHelper_->gain()` and `camHelper_->gainCode()` to
>> succeed.
>>
>> In order to test the new black level code added in this series, I added the following camera helpers:
>>
>>> +class CameraSensorHelperImx355 : public CameraSensorHelper
>>> +{
>>> +public:
>>> +       CameraSensorHelperImx355()
>>> +       {
>>> +               blackLevel_ = 4096;
>>> +       }
>>> +};
>>> +REGISTER_CAMERA_SENSOR_HELPER("imx355", CameraSensorHelperImx355)
>>> +
>>> +class CameraSensorHelperImx363 : public CameraSensorHelper
>>> +{
>>> +public:
>>> +       CameraSensorHelperImx363()
>>> +       {
>>> +               blackLevel_ = 4096;
>>> +       }
>>> +};
>>> +REGISTER_CAMERA_SENSOR_HELPER("imx363", CameraSensorHelperImx363)
>> With those, `camHelper_` is set and the pre-existing code unconditionally calls
>> `camHelper_->gain()`. When assertions are on, one is triggered / playback is broken. When assertions are
>> disabled, playback works but the output images are much darker, as described above. That can be fixed by
>> force-disabling the calls to `camHelper_->gain()` and using the `camHelper_` code instead.
>>
>> This is not strictly related to this series and could be fixed independently, but I guess it makes sense
>> to fix things in an additional commit here.
>>
>> AFAICS something similar to the new `if (camHelper_->blackLevel().has_value())` should be added for gain.
> I see, thank you for explanation.  I can reproduce the problem with dark
> output.  It happens with or without this patch.  I don't get any
> assertion error and I wouldn't expect one -- if you mean the assertions
> in CameraSensorHelper::gain*, they pass with the default values.  But
> 0/0 is returned, which is of course a wrong value.
>
> This also means that it is not expected that the helper doesn't define
> gains (no surprise considering handling the gain constants was its only
> purpose originally).  So there is a question whether a camera sensor
> helper that doesn't define the gains is a valid helper.  I'd say why not
> but other pipelines would have problems with such a helper too.  If we
> want to support this then it'll require more changes.
>
> Anyway, although it's an interesting issue, it has nothing to do with
> this patch.  If you think there is a valid reason to define camera
> sensor helpers without defining gains, I'd suggest opening a separate
> thread about it or to file a bug.
Agreed, will file an independent issue or mail.
>
>>>>>>         } else {
>>>>>>             /*
>>>>>>              * The camera sensor gain (g) is usually not equal to the value written

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index b9f2aaa6d..a7af2e12c 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -24,7 +24,8 @@  BlackLevel::BlackLevel()
 int BlackLevel::configure(IPAContext &context,
 			  [[maybe_unused]] const IPAConfigInfo &configInfo)
 {
-	context.activeState.blc.level = 255;
+	context.activeState.blc.level =
+		context.configuration.black.level.value_or(255);
 	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/ipa_context.h b/src/ipa/simple/ipa_context.h
index 3519f20f6..fd121eebe 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 <stdint.h>
 
 #include <libipa/fc_queue.h>
@@ -22,6 +23,9 @@  struct IPASessionConfiguration {
 		int32_t exposureMin, exposureMax;
 		double againMin, againMax, againMinStep;
 	} agc;
+	struct {
+		std::optional<uint8_t> level;
+	} black;
 };
 
 struct IPAActiveState {
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index b28c7039f..079409f6d 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -201,6 +201,15 @@  int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
 			(context_.configuration.agc.againMax -
 			 context_.configuration.agc.againMin) /
 			100.0;
+		if (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
+			 * sensor pixel width. Hence we obtain the pixel-based black value
+			 * by dividing the value from the helper by 256.
+			 */
+			context_.configuration.black.level =
+				camHelper_->blackLevel().value() / 256;
 	} else {
 		/*
 		 * The camera sensor gain (g) is usually not equal to the value written