[PATCH v3 1/1] libcamera: software_isp: Get black level from the camera helper
Milan Zamazal
mzamazal at redhat.com
Mon Oct 14 20:53:15 CEST 2024
Robert Mader <robert.mader at collabora.com> writes:
> On 14.10.24 13:07, Milan Zamazal wrote:
>> Hi Robert,
>>
>
>> thank you for testing and review.
>>
>> Robert Mader <robert.mader at 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 at 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 at redhat.com>
>>>>> Reviewed-by: Kieran Bingham <kieran.bingham at 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
More information about the libcamera-devel
mailing list