[PATCH v3 1/1] libcamera: software_isp: Get black level from the camera helper
Robert Mader
robert.mader at collabora.com
Tue Oct 15 12:04:39 CEST 2024
On 14.10.24 20:53, Milan Zamazal wrote:
> 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.
Agreed, will file an independent issue or mail.
>
>>>>>> } else {
>>>>>> /*
>>>>>> * The camera sensor gain (g) is usually not equal to the value written
--
Robert Mader
Consultant Software Developer
Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718
More information about the libcamera-devel
mailing list