[PATCH v3 1/1] libcamera: software_isp: Get black level from the camera helper
Robert Mader
robert.mader at collabora.com
Mon Oct 14 13:34:24 CEST 2024
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.
>
>>>> } 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