[v2] libcamera: software_isp: Reset stored exposure in black level
diff mbox series

Message ID 20250319095533.24550-1-mzamazal@redhat.com
State Accepted
Commit ceea066fa23c780eed65efbb243b216c7f511db8
Headers show
Series
  • [v2] libcamera: software_isp: Reset stored exposure in black level
Related show

Commit Message

Milan Zamazal March 19, 2025, 9:55 a.m. UTC 
Automatic black level setting in software ISP updates the determined
black level value when exposure or gain change.  It stores the last
exposure and gain values to detect the change.

BlackLevel::configure() resets the stored black level value but not the
exposure and gain values.  This can prevent updating the black value and
cause bad image output, e.g. after suspending and resuming a camera, if
exposure and gain remain unchanged.

Let's store exposure and gain in IPAActiveState.  Although the values
are not supposed to be used outside BlackLevel class, storing them in
the context has the advantage of their automatic reset together with the
other context contents and having them in `blc' struct indicates their
relationship to the black value computation.

Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
 src/ipa/simple/algorithms/blc.h   |  2 --
 src/ipa/simple/ipa_context.h      |  2 ++
 3 files changed, 7 insertions(+), 7 deletions(-)

Comments

Milan Zamazal March 24, 2025, 2:20 p.m. UTC | #1 
Hi Robert,

would you have a chance to test that this changed version also works?
(I expect it does, just testing before merging.)

Milan Zamazal <mzamazal@redhat.com> writes:

> Automatic black level setting in software ISP updates the determined
> black level value when exposure or gain change.  It stores the last
> exposure and gain values to detect the change.
>
> BlackLevel::configure() resets the stored black level value but not the
> exposure and gain values.  This can prevent updating the black value and
> cause bad image output, e.g. after suspending and resuming a camera, if
> exposure and gain remain unchanged.
>
> Let's store exposure and gain in IPAActiveState.  Although the values
> are not supposed to be used outside BlackLevel class, storing them in
> the context has the advantage of their automatic reset together with the
> other context contents and having them in `blc' struct indicates their
> relationship to the black value computation.
>
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>  src/ipa/simple/algorithms/blc.h   |  2 --
>  src/ipa/simple/ipa_context.h      |  2 ++
>  3 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 1d7d370b..089e492a 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2024, Red Hat Inc.
> + * Copyright (C) 2024-2025, Red Hat Inc.
>   *
>   * Black level handling
>   */
> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>  	if (context.configuration.black.level.has_value())
>  		return;
>  
> -	if (frameContext.sensor.exposure == exposure_ &&
> -	    frameContext.sensor.gain == gain_) {
> +	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
> +	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
>  		return;
>  	}
>  
> @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>  		seen += histogram[i];
>  		if (seen >= pixelThreshold) {
>  			context.activeState.blc.level = i * histogramRatio;
> -			exposure_ = frameContext.sensor.exposure;
> -			gain_ = frameContext.sensor.gain;
> +			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
> +			context.activeState.blc.lastGain = frameContext.sensor.gain;
>  			LOG(IPASoftBL, Debug)
>  				<< "Auto-set black level: "
>  				<< i << "/" << SwIspStats::kYHistogramSize
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 52d59cab..db9e6d63 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -30,8 +30,6 @@ public:
>  		     ControlList &metadata) override;
>  
>  private:
> -	int32_t exposure_;
> -	double gain_;
>  	std::optional<uint8_t> definedLevel_;
>  };
>  
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 4af51306..b1198af5 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>  struct IPAActiveState {
>  	struct {
>  		uint8_t level;
> +		int32_t lastExposure;
> +		double lastGain;
>  	} blc;
>  
>  	struct {
Robert Mader March 26, 2025, 3:11 p.m. UTC | #2 
Yep, works fine here -> T-b

On 24.03.25 15:22, Robert Mader wrote:
> Sure, unfortunately not before Wednesday. Made me a reminder.
>
> On 24.03.25 15:20, Milan Zamazal wrote:
>> Hi Robert,
>>
>> would you have a chance to test that this changed version also works?
>> (I expect it does, just testing before merging.)
>>
>> Milan Zamazal <mzamazal@redhat.com> writes:
>>
>>> Automatic black level setting in software ISP updates the determined
>>> black level value when exposure or gain change.  It stores the last
>>> exposure and gain values to detect the change.
>>>
>>> BlackLevel::configure() resets the stored black level value but not the
>>> exposure and gain values.  This can prevent updating the black value 
>>> and
>>> cause bad image output, e.g. after suspending and resuming a camera, if
>>> exposure and gain remain unchanged.
>>>
>>> Let's store exposure and gain in IPAActiveState.  Although the values
>>> are not supposed to be used outside BlackLevel class, storing them in
>>> the context has the advantage of their automatic reset together with 
>>> the
>>> other context contents and having them in `blc' struct indicates their
>>> relationship to the black value computation.
>>>
>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>> ---
>>>   src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>>>   src/ipa/simple/algorithms/blc.h   |  2 --
>>>   src/ipa/simple/ipa_context.h      |  2 ++
>>>   3 files changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/ipa/simple/algorithms/blc.cpp 
>>> b/src/ipa/simple/algorithms/blc.cpp
>>> index 1d7d370b..089e492a 100644
>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>> @@ -1,6 +1,6 @@
>>>   /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>   /*
>>> - * Copyright (C) 2024, Red Hat Inc.
>>> + * Copyright (C) 2024-2025, Red Hat Inc.
>>>    *
>>>    * Black level handling
>>>    */
>>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>>>       if (context.configuration.black.level.has_value())
>>>           return;
>>>   -    if (frameContext.sensor.exposure == exposure_ &&
>>> -        frameContext.sensor.gain == gain_) {
>>> +    if (frameContext.sensor.exposure == 
>>> context.activeState.blc.lastExposure &&
>>> +        frameContext.sensor.gain == 
>>> context.activeState.blc.lastGain) {
>>>           return;
>>>       }
>>>   @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>>>           seen += histogram[i];
>>>           if (seen >= pixelThreshold) {
>>>               context.activeState.blc.level = i * histogramRatio;
>>> -            exposure_ = frameContext.sensor.exposure;
>>> -            gain_ = frameContext.sensor.gain;
>>> +            context.activeState.blc.lastExposure = 
>>> frameContext.sensor.exposure;
>>> +            context.activeState.blc.lastGain = 
>>> frameContext.sensor.gain;
>>>               LOG(IPASoftBL, Debug)
>>>                   << "Auto-set black level: "
>>>                   << i << "/" << SwIspStats::kYHistogramSize
>>> diff --git a/src/ipa/simple/algorithms/blc.h 
>>> b/src/ipa/simple/algorithms/blc.h
>>> index 52d59cab..db9e6d63 100644
>>> --- a/src/ipa/simple/algorithms/blc.h
>>> +++ b/src/ipa/simple/algorithms/blc.h
>>> @@ -30,8 +30,6 @@ public:
>>>                ControlList &metadata) override;
>>>     private:
>>> -    int32_t exposure_;
>>> -    double gain_;
>>>       std::optional<uint8_t> definedLevel_;
>>>   };
>>>   diff --git a/src/ipa/simple/ipa_context.h 
>>> b/src/ipa/simple/ipa_context.h
>>> index 4af51306..b1198af5 100644
>>> --- a/src/ipa/simple/ipa_context.h
>>> +++ b/src/ipa/simple/ipa_context.h
>>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>>>   struct IPAActiveState {
>>>       struct {
>>>           uint8_t level;
>>> +        int32_t lastExposure;
>>> +        double lastGain;
>>>       } blc;
>>>         struct {
Kieran Bingham March 26, 2025, 3:18 p.m. UTC | #3 
Quoting Robert Mader (2025-03-26 15:11:53)
> Yep, works fine here -> T-b

Patch work only picks up on full tags.

Tested-by: Robert Mader <robert.mader@collabora.com>

Thanks

> 
> On 24.03.25 15:22, Robert Mader wrote:
> > Sure, unfortunately not before Wednesday. Made me a reminder.
> >
> > On 24.03.25 15:20, Milan Zamazal wrote:
> >> Hi Robert,
> >>
> >> would you have a chance to test that this changed version also works?
> >> (I expect it does, just testing before merging.)
> >>
> >> Milan Zamazal <mzamazal@redhat.com> writes:
> >>
> >>> Automatic black level setting in software ISP updates the determined
> >>> black level value when exposure or gain change.  It stores the last
> >>> exposure and gain values to detect the change.
> >>>
> >>> BlackLevel::configure() resets the stored black level value but not the
> >>> exposure and gain values.  This can prevent updating the black value 
> >>> and
> >>> cause bad image output, e.g. after suspending and resuming a camera, if
> >>> exposure and gain remain unchanged.
> >>>
> >>> Let's store exposure and gain in IPAActiveState.  Although the values
> >>> are not supposed to be used outside BlackLevel class, storing them in
> >>> the context has the advantage of their automatic reset together with 
> >>> the
> >>> other context contents and having them in `blc' struct indicates their
> >>> relationship to the black value computation.
> >>>
> >>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >>> ---
> >>>   src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
> >>>   src/ipa/simple/algorithms/blc.h   |  2 --
> >>>   src/ipa/simple/ipa_context.h      |  2 ++
> >>>   3 files changed, 7 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/src/ipa/simple/algorithms/blc.cpp 
> >>> b/src/ipa/simple/algorithms/blc.cpp
> >>> index 1d7d370b..089e492a 100644
> >>> --- a/src/ipa/simple/algorithms/blc.cpp
> >>> +++ b/src/ipa/simple/algorithms/blc.cpp
> >>> @@ -1,6 +1,6 @@
> >>>   /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>>   /*
> >>> - * Copyright (C) 2024, Red Hat Inc.
> >>> + * Copyright (C) 2024-2025, Red Hat Inc.
> >>>    *
> >>>    * Black level handling
> >>>    */
> >>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
> >>>       if (context.configuration.black.level.has_value())
> >>>           return;
> >>>   -    if (frameContext.sensor.exposure == exposure_ &&
> >>> -        frameContext.sensor.gain == gain_) {
> >>> +    if (frameContext.sensor.exposure == 
> >>> context.activeState.blc.lastExposure &&
> >>> +        frameContext.sensor.gain == 
> >>> context.activeState.blc.lastGain) {
> >>>           return;
> >>>       }
> >>>   @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
> >>>           seen += histogram[i];
> >>>           if (seen >= pixelThreshold) {
> >>>               context.activeState.blc.level = i * histogramRatio;
> >>> -            exposure_ = frameContext.sensor.exposure;
> >>> -            gain_ = frameContext.sensor.gain;
> >>> +            context.activeState.blc.lastExposure = 
> >>> frameContext.sensor.exposure;
> >>> +            context.activeState.blc.lastGain = 
> >>> frameContext.sensor.gain;
> >>>               LOG(IPASoftBL, Debug)
> >>>                   << "Auto-set black level: "
> >>>                   << i << "/" << SwIspStats::kYHistogramSize
> >>> diff --git a/src/ipa/simple/algorithms/blc.h 
> >>> b/src/ipa/simple/algorithms/blc.h
> >>> index 52d59cab..db9e6d63 100644
> >>> --- a/src/ipa/simple/algorithms/blc.h
> >>> +++ b/src/ipa/simple/algorithms/blc.h
> >>> @@ -30,8 +30,6 @@ public:
> >>>                ControlList &metadata) override;
> >>>     private:
> >>> -    int32_t exposure_;
> >>> -    double gain_;
> >>>       std::optional<uint8_t> definedLevel_;
> >>>   };
> >>>   diff --git a/src/ipa/simple/ipa_context.h 
> >>> b/src/ipa/simple/ipa_context.h
> >>> index 4af51306..b1198af5 100644
> >>> --- a/src/ipa/simple/ipa_context.h
> >>> +++ b/src/ipa/simple/ipa_context.h
> >>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
> >>>   struct IPAActiveState {
> >>>       struct {
> >>>           uint8_t level;
> >>> +        int32_t lastExposure;
> >>> +        double lastGain;
> >>>       } blc;
> >>>         struct {
> 
> -- 
> Robert Mader
> Consultant Software Developer
> 
> Collabora Ltd.
> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
> Registered in England & Wales, no. 5513718
>
Robert Mader March 26, 2025, 3:20 p.m. UTC | #4 
Oh, I wasn't aware that it automatically picks those tags up - thanks!

On 26.03.25 16:18, Kieran Bingham wrote:
> Quoting Robert Mader (2025-03-26 15:11:53)
>> Yep, works fine here -> T-b
> Patch work only picks up on full tags.
>
> Tested-by: Robert Mader <robert.mader@collabora.com>
>
> Thanks
>
>> On 24.03.25 15:22, Robert Mader wrote:
>>> Sure, unfortunately not before Wednesday. Made me a reminder.
>>>
>>> On 24.03.25 15:20, Milan Zamazal wrote:
>>>> Hi Robert,
>>>>
>>>> would you have a chance to test that this changed version also works?
>>>> (I expect it does, just testing before merging.)
>>>>
>>>> Milan Zamazal <mzamazal@redhat.com> writes:
>>>>
>>>>> Automatic black level setting in software ISP updates the determined
>>>>> black level value when exposure or gain change.  It stores the last
>>>>> exposure and gain values to detect the change.
>>>>>
>>>>> BlackLevel::configure() resets the stored black level value but not the
>>>>> exposure and gain values.  This can prevent updating the black value
>>>>> and
>>>>> cause bad image output, e.g. after suspending and resuming a camera, if
>>>>> exposure and gain remain unchanged.
>>>>>
>>>>> Let's store exposure and gain in IPAActiveState.  Although the values
>>>>> are not supposed to be used outside BlackLevel class, storing them in
>>>>> the context has the advantage of their automatic reset together with
>>>>> the
>>>>> other context contents and having them in `blc' struct indicates their
>>>>> relationship to the black value computation.
>>>>>
>>>>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
>>>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>>>>> ---
>>>>>    src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>>>>>    src/ipa/simple/algorithms/blc.h   |  2 --
>>>>>    src/ipa/simple/ipa_context.h      |  2 ++
>>>>>    3 files changed, 7 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/src/ipa/simple/algorithms/blc.cpp
>>>>> b/src/ipa/simple/algorithms/blc.cpp
>>>>> index 1d7d370b..089e492a 100644
>>>>> --- a/src/ipa/simple/algorithms/blc.cpp
>>>>> +++ b/src/ipa/simple/algorithms/blc.cpp
>>>>> @@ -1,6 +1,6 @@
>>>>>    /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>>>>    /*
>>>>> - * Copyright (C) 2024, Red Hat Inc.
>>>>> + * Copyright (C) 2024-2025, Red Hat Inc.
>>>>>     *
>>>>>     * Black level handling
>>>>>     */
>>>>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>>>>>        if (context.configuration.black.level.has_value())
>>>>>            return;
>>>>>    -    if (frameContext.sensor.exposure == exposure_ &&
>>>>> -        frameContext.sensor.gain == gain_) {
>>>>> +    if (frameContext.sensor.exposure ==
>>>>> context.activeState.blc.lastExposure &&
>>>>> +        frameContext.sensor.gain ==
>>>>> context.activeState.blc.lastGain) {
>>>>>            return;
>>>>>        }
>>>>>    @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>>>>>            seen += histogram[i];
>>>>>            if (seen >= pixelThreshold) {
>>>>>                context.activeState.blc.level = i * histogramRatio;
>>>>> -            exposure_ = frameContext.sensor.exposure;
>>>>> -            gain_ = frameContext.sensor.gain;
>>>>> +            context.activeState.blc.lastExposure =
>>>>> frameContext.sensor.exposure;
>>>>> +            context.activeState.blc.lastGain =
>>>>> frameContext.sensor.gain;
>>>>>                LOG(IPASoftBL, Debug)
>>>>>                    << "Auto-set black level: "
>>>>>                    << i << "/" << SwIspStats::kYHistogramSize
>>>>> diff --git a/src/ipa/simple/algorithms/blc.h
>>>>> b/src/ipa/simple/algorithms/blc.h
>>>>> index 52d59cab..db9e6d63 100644
>>>>> --- a/src/ipa/simple/algorithms/blc.h
>>>>> +++ b/src/ipa/simple/algorithms/blc.h
>>>>> @@ -30,8 +30,6 @@ public:
>>>>>                 ControlList &metadata) override;
>>>>>      private:
>>>>> -    int32_t exposure_;
>>>>> -    double gain_;
>>>>>        std::optional<uint8_t> definedLevel_;
>>>>>    };
>>>>>    diff --git a/src/ipa/simple/ipa_context.h
>>>>> b/src/ipa/simple/ipa_context.h
>>>>> index 4af51306..b1198af5 100644
>>>>> --- a/src/ipa/simple/ipa_context.h
>>>>> +++ b/src/ipa/simple/ipa_context.h
>>>>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>>>>>    struct IPAActiveState {
>>>>>        struct {
>>>>>            uint8_t level;
>>>>> +        int32_t lastExposure;
>>>>> +        double lastGain;
>>>>>        } blc;
>>>>>          struct {
>> -- 
>> Robert Mader
>> Consultant Software Developer
>>
>> Collabora Ltd.
>> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
>> Registered in England & Wales, no. 5513718
>>
Milan Zamazal March 28, 2025, 8:11 a.m. UTC | #5 
Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Robert Mader (2025-03-26 15:11:53)
>> Yep, works fine here -> T-b

Thank you, Robert, for testing.

> Patch work only picks up on full tags.
>
> Tested-by: Robert Mader <robert.mader@collabora.com>

I believe the patch is all right now and could get in for 0.5?

> Thanks
>
>> 
>> On 24.03.25 15:22, Robert Mader wrote:
>> > Sure, unfortunately not before Wednesday. Made me a reminder.
>> >
>> > On 24.03.25 15:20, Milan Zamazal wrote:
>> >> Hi Robert,
>> >>
>> >> would you have a chance to test that this changed version also works?
>> >> (I expect it does, just testing before merging.)
>> >>
>> >> Milan Zamazal <mzamazal@redhat.com> writes:
>> >>
>> >>> Automatic black level setting in software ISP updates the determined
>> >>> black level value when exposure or gain change.  It stores the last
>> >>> exposure and gain values to detect the change.
>> >>>
>> >>> BlackLevel::configure() resets the stored black level value but not the
>> >>> exposure and gain values.  This can prevent updating the black value 
>> >>> and
>> >>> cause bad image output, e.g. after suspending and resuming a camera, if
>> >>> exposure and gain remain unchanged.
>> >>>
>> >>> Let's store exposure and gain in IPAActiveState.  Although the values
>> >>> are not supposed to be used outside BlackLevel class, storing them in
>> >>> the context has the advantage of their automatic reset together with 
>> >>> the
>> >>> other context contents and having them in `blc' struct indicates their
>> >>> relationship to the black value computation.
>> >>>
>> >>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
>> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> >>> ---
>> >>>   src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>> >>>   src/ipa/simple/algorithms/blc.h   |  2 --
>> >>>   src/ipa/simple/ipa_context.h      |  2 ++
>> >>>   3 files changed, 7 insertions(+), 7 deletions(-)
>> >>>
>> >>> diff --git a/src/ipa/simple/algorithms/blc.cpp 
>> >>> b/src/ipa/simple/algorithms/blc.cpp
>> >>> index 1d7d370b..089e492a 100644
>> >>> --- a/src/ipa/simple/algorithms/blc.cpp
>> >>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> >>> @@ -1,6 +1,6 @@
>> >>>   /* SPDX-License-Identifier: LGPL-2.1-or-later */
>> >>>   /*
>> >>> - * Copyright (C) 2024, Red Hat Inc.
>> >>> + * Copyright (C) 2024-2025, Red Hat Inc.
>> >>>    *
>> >>>    * Black level handling
>> >>>    */
>> >>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>> >>>       if (context.configuration.black.level.has_value())
>> >>>           return;
>> >>>   -    if (frameContext.sensor.exposure == exposure_ &&
>> >>> -        frameContext.sensor.gain == gain_) {
>> >>> +    if (frameContext.sensor.exposure == 
>> >>> context.activeState.blc.lastExposure &&
>> >>> +        frameContext.sensor.gain == 
>> >>> context.activeState.blc.lastGain) {
>> >>>           return;
>> >>>       }
>> >>>   @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>> >>>           seen += histogram[i];
>> >>>           if (seen >= pixelThreshold) {
>> >>>               context.activeState.blc.level = i * histogramRatio;
>> >>> -            exposure_ = frameContext.sensor.exposure;
>> >>> -            gain_ = frameContext.sensor.gain;
>> >>> +            context.activeState.blc.lastExposure = 
>> >>> frameContext.sensor.exposure;
>> >>> +            context.activeState.blc.lastGain = 
>> >>> frameContext.sensor.gain;
>> >>>               LOG(IPASoftBL, Debug)
>> >>>                   << "Auto-set black level: "
>> >>>                   << i << "/" << SwIspStats::kYHistogramSize
>> >>> diff --git a/src/ipa/simple/algorithms/blc.h 
>> >>> b/src/ipa/simple/algorithms/blc.h
>> >>> index 52d59cab..db9e6d63 100644
>> >>> --- a/src/ipa/simple/algorithms/blc.h
>> >>> +++ b/src/ipa/simple/algorithms/blc.h
>> >>> @@ -30,8 +30,6 @@ public:
>> >>>                ControlList &metadata) override;
>> >>>     private:
>> >>> -    int32_t exposure_;
>> >>> -    double gain_;
>> >>>       std::optional<uint8_t> definedLevel_;
>> >>>   };
>> >>>   diff --git a/src/ipa/simple/ipa_context.h 
>> >>> b/src/ipa/simple/ipa_context.h
>> >>> index 4af51306..b1198af5 100644
>> >>> --- a/src/ipa/simple/ipa_context.h
>> >>> +++ b/src/ipa/simple/ipa_context.h
>> >>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>> >>>   struct IPAActiveState {
>> >>>       struct {
>> >>>           uint8_t level;
>> >>> +        int32_t lastExposure;
>> >>> +        double lastGain;
>> >>>       } blc;
>> >>>         struct {
>> 
>> -- 
>> Robert Mader
>> Consultant Software Developer
>> 
>> Collabora Ltd.
>> Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
>> Registered in England & Wales, no. 5513718
>>
Laurent Pinchart March 28, 2025, 7:21 p.m. UTC | #6 
Hi Milan,

Thank you for the patch.

On Wed, Mar 19, 2025 at 10:55:33AM +0100, Milan Zamazal wrote:
> Automatic black level setting in software ISP updates the determined
> black level value when exposure or gain change.  It stores the last
> exposure and gain values to detect the change.
> 
> BlackLevel::configure() resets the stored black level value but not the
> exposure and gain values.  This can prevent updating the black value and
> cause bad image output, e.g. after suspending and resuming a camera, if
> exposure and gain remain unchanged.

I'm a bit puzzled there. If the exposure time and gain don't change,
then the black level will stay at 0. However, the bug report mentions
very dark images. What am I missing ?

> Let's store exposure and gain in IPAActiveState.  Although the values
> are not supposed to be used outside BlackLevel class, storing them in
> the context has the advantage of their automatic reset together with the
> other context contents and having them in `blc' struct indicates their
> relationship to the black value computation.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>  src/ipa/simple/algorithms/blc.h   |  2 --
>  src/ipa/simple/ipa_context.h      |  2 ++
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 1d7d370b..089e492a 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>  /*
> - * Copyright (C) 2024, Red Hat Inc.
> + * Copyright (C) 2024-2025, Red Hat Inc.
>   *
>   * Black level handling
>   */
> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>  	if (context.configuration.black.level.has_value())
>  		return;
>  
> -	if (frameContext.sensor.exposure == exposure_ &&
> -	    frameContext.sensor.gain == gain_) {
> +	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
> +	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
>  		return;
>  	}
>  
> @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>  		seen += histogram[i];
>  		if (seen >= pixelThreshold) {
>  			context.activeState.blc.level = i * histogramRatio;
> -			exposure_ = frameContext.sensor.exposure;
> -			gain_ = frameContext.sensor.gain;
> +			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
> +			context.activeState.blc.lastGain = frameContext.sensor.gain;
>  			LOG(IPASoftBL, Debug)
>  				<< "Auto-set black level: "
>  				<< i << "/" << SwIspStats::kYHistogramSize
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> index 52d59cab..db9e6d63 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -30,8 +30,6 @@ public:
>  		     ControlList &metadata) override;
>  
>  private:
> -	int32_t exposure_;
> -	double gain_;
>  	std::optional<uint8_t> definedLevel_;
>  };
>  
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index 4af51306..b1198af5 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>  struct IPAActiveState {
>  	struct {
>  		uint8_t level;
> +		int32_t lastExposure;
> +		double lastGain;
>  	} blc;
>  
>  	struct {
Milan Zamazal March 28, 2025, 9:06 p.m. UTC | #7 
Hi Laurent,

Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:

> Hi Milan,
>
> Thank you for the patch.
>
> On Wed, Mar 19, 2025 at 10:55:33AM +0100, Milan Zamazal wrote:
>> Automatic black level setting in software ISP updates the determined
>> black level value when exposure or gain change.  It stores the last
>> exposure and gain values to detect the change.
>> 
>> BlackLevel::configure() resets the stored black level value but not the
>> exposure and gain values.  This can prevent updating the black value and
>> cause bad image output, e.g. after suspending and resuming a camera, if
>> exposure and gain remain unchanged.
>
> I'm a bit puzzled there. If the exposure time and gain don't change,
> then the black level will stay at 0. However, the bug report mentions
> very dark images. What am I missing ?

That the black level, if unspecified, is initially set to 255.  (The
point is to get the lowest black level experienced, hence we start with
the highest value.)

>> Let's store exposure and gain in IPAActiveState.  Although the values
>> are not supposed to be used outside BlackLevel class, storing them in
>> the context has the advantage of their automatic reset together with the
>> other context contents and having them in `blc' struct indicates their
>> relationship to the black value computation.
>> 
>> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
>>  src/ipa/simple/algorithms/blc.h   |  2 --
>>  src/ipa/simple/ipa_context.h      |  2 ++
>>  3 files changed, 7 insertions(+), 7 deletions(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 1d7d370b..089e492a 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -1,6 +1,6 @@
>>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
>>  /*
>> - * Copyright (C) 2024, Red Hat Inc.
>> + * Copyright (C) 2024-2025, Red Hat Inc.
>>   *
>>   * Black level handling
>>   */
>> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
>>  	if (context.configuration.black.level.has_value())
>>  		return;
>>  
>> -	if (frameContext.sensor.exposure == exposure_ &&
>> -	    frameContext.sensor.gain == gain_) {
>> +	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
>> +	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
>>  		return;
>>  	}
>>  
>> @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
>>  		seen += histogram[i];
>>  		if (seen >= pixelThreshold) {
>>  			context.activeState.blc.level = i * histogramRatio;
>> -			exposure_ = frameContext.sensor.exposure;
>> -			gain_ = frameContext.sensor.gain;
>> +			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
>> +			context.activeState.blc.lastGain = frameContext.sensor.gain;
>>  			LOG(IPASoftBL, Debug)
>>  				<< "Auto-set black level: "
>>  				<< i << "/" << SwIspStats::kYHistogramSize
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> index 52d59cab..db9e6d63 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -30,8 +30,6 @@ public:
>>  		     ControlList &metadata) override;
>>  
>>  private:
>> -	int32_t exposure_;
>> -	double gain_;
>>  	std::optional<uint8_t> definedLevel_;
>>  };
>>  
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index 4af51306..b1198af5 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
>>  struct IPAActiveState {
>>  	struct {
>>  		uint8_t level;
>> +		int32_t lastExposure;
>> +		double lastGain;
>>  	} blc;
>>  
>>  	struct {
Laurent Pinchart April 1, 2025, 2:12 a.m. UTC | #8 
On Fri, Mar 28, 2025 at 10:06:54PM +0100, Milan Zamazal wrote:
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> writes:
> > On Wed, Mar 19, 2025 at 10:55:33AM +0100, Milan Zamazal wrote:
> >> Automatic black level setting in software ISP updates the determined
> >> black level value when exposure or gain change.  It stores the last
> >> exposure and gain values to detect the change.
> >> 
> >> BlackLevel::configure() resets the stored black level value but not the
> >> exposure and gain values.  This can prevent updating the black value and
> >> cause bad image output, e.g. after suspending and resuming a camera, if
> >> exposure and gain remain unchanged.
> >
> > I'm a bit puzzled there. If the exposure time and gain don't change,
> > then the black level will stay at 0. However, the bug report mentions
> > very dark images. What am I missing ?
> 
> That the black level, if unspecified, is initially set to 255.  (The
> point is to get the lowest black level experienced, hence we start with
> the highest value.)

Aahhh of course.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >> Let's store exposure and gain in IPAActiveState.  Although the values
> >> are not supposed to be used outside BlackLevel class, storing them in
> >> the context has the advantage of their automatic reset together with the
> >> other context contents and having them in `blc' struct indicates their
> >> relationship to the black value computation.
> >> 
> >> Bug: https://bugs.libcamera.org/show_bug.cgi?id=259
> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> >> ---
> >>  src/ipa/simple/algorithms/blc.cpp | 10 +++++-----
> >>  src/ipa/simple/algorithms/blc.h   |  2 --
> >>  src/ipa/simple/ipa_context.h      |  2 ++
> >>  3 files changed, 7 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> >> index 1d7d370b..089e492a 100644
> >> --- a/src/ipa/simple/algorithms/blc.cpp
> >> +++ b/src/ipa/simple/algorithms/blc.cpp
> >> @@ -1,6 +1,6 @@
> >>  /* SPDX-License-Identifier: LGPL-2.1-or-later */
> >>  /*
> >> - * Copyright (C) 2024, Red Hat Inc.
> >> + * Copyright (C) 2024-2025, Red Hat Inc.
> >>   *
> >>   * Black level handling
> >>   */
> >> @@ -54,8 +54,8 @@ void BlackLevel::process(IPAContext &context,
> >>  	if (context.configuration.black.level.has_value())
> >>  		return;
> >>  
> >> -	if (frameContext.sensor.exposure == exposure_ &&
> >> -	    frameContext.sensor.gain == gain_) {
> >> +	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
> >> +	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
> >>  		return;
> >>  	}
> >>  
> >> @@ -79,8 +79,8 @@ void BlackLevel::process(IPAContext &context,
> >>  		seen += histogram[i];
> >>  		if (seen >= pixelThreshold) {
> >>  			context.activeState.blc.level = i * histogramRatio;
> >> -			exposure_ = frameContext.sensor.exposure;
> >> -			gain_ = frameContext.sensor.gain;
> >> +			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
> >> +			context.activeState.blc.lastGain = frameContext.sensor.gain;
> >>  			LOG(IPASoftBL, Debug)
> >>  				<< "Auto-set black level: "
> >>  				<< i << "/" << SwIspStats::kYHistogramSize
> >> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> >> index 52d59cab..db9e6d63 100644
> >> --- a/src/ipa/simple/algorithms/blc.h
> >> +++ b/src/ipa/simple/algorithms/blc.h
> >> @@ -30,8 +30,6 @@ public:
> >>  		     ControlList &metadata) override;
> >>  
> >>  private:
> >> -	int32_t exposure_;
> >> -	double gain_;
> >>  	std::optional<uint8_t> definedLevel_;
> >>  };
> >>  
> >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> >> index 4af51306..b1198af5 100644
> >> --- a/src/ipa/simple/ipa_context.h
> >> +++ b/src/ipa/simple/ipa_context.h
> >> @@ -33,6 +33,8 @@ struct IPASessionConfiguration {
> >>  struct IPAActiveState {
> >>  	struct {
> >>  		uint8_t level;
> >> +		int32_t lastExposure;
> >> +		double lastGain;
> >>  	} blc;
> >>  
> >>  	struct {

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
index 1d7d370b..089e492a 100644
--- a/src/ipa/simple/algorithms/blc.cpp
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -1,6 +1,6 @@ 
 /* SPDX-License-Identifier: LGPL-2.1-or-later */
 /*
- * Copyright (C) 2024, Red Hat Inc.
+ * Copyright (C) 2024-2025, Red Hat Inc.
  *
  * Black level handling
  */
@@ -54,8 +54,8 @@  void BlackLevel::process(IPAContext &context,
 	if (context.configuration.black.level.has_value())
 		return;
 
-	if (frameContext.sensor.exposure == exposure_ &&
-	    frameContext.sensor.gain == gain_) {
+	if (frameContext.sensor.exposure == context.activeState.blc.lastExposure &&
+	    frameContext.sensor.gain == context.activeState.blc.lastGain) {
 		return;
 	}
 
@@ -79,8 +79,8 @@  void BlackLevel::process(IPAContext &context,
 		seen += histogram[i];
 		if (seen >= pixelThreshold) {
 			context.activeState.blc.level = i * histogramRatio;
-			exposure_ = frameContext.sensor.exposure;
-			gain_ = frameContext.sensor.gain;
+			context.activeState.blc.lastExposure = frameContext.sensor.exposure;
+			context.activeState.blc.lastGain = frameContext.sensor.gain;
 			LOG(IPASoftBL, Debug)
 				<< "Auto-set black level: "
 				<< i << "/" << SwIspStats::kYHistogramSize
diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
index 52d59cab..db9e6d63 100644
--- a/src/ipa/simple/algorithms/blc.h
+++ b/src/ipa/simple/algorithms/blc.h
@@ -30,8 +30,6 @@  public:
 		     ControlList &metadata) override;
 
 private:
-	int32_t exposure_;
-	double gain_;
 	std::optional<uint8_t> definedLevel_;
 };
 
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index 4af51306..b1198af5 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -33,6 +33,8 @@  struct IPASessionConfiguration {
 struct IPAActiveState {
 	struct {
 		uint8_t level;
+		int32_t lastExposure;
+		double lastGain;
 	} blc;
 
 	struct {