[v6,18/18] libcamera: software_isp: Update black level only on exposure changes

21191 diff mbox series

The black level is likely to get updated, if ever, only after exposure
or gain changes.  Don't compute its possible updates if exposure and
gain are unchanged.

It's probably not worth trying to implement something more
sophisticated.  Better to spend the effort on supporting tuning files.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp | 9 ++++++++-
 src/ipa/simple/algorithms/blc.h   | 4 ++++
 2 files changed, 12 insertions(+), 1 deletion(-)

Quoting Milan Zamazal (2024-09-06 13:09:27)
> The black level is likely to get updated, if ever, only after exposure
> or gain changes.  Don't compute its possible updates if exposure and
> gain are unchanged.
> 
> It's probably not worth trying to implement something more
> sophisticated.  Better to spend the effort on supporting tuning files.
> 

Fine with me. Indeed I think we just go with static/constant black
levels when we know more information about the sensor.

I'll be curious to see how close this implementation measures against
the 'known' black level values, or measured blacks if we do calibration
of this.

My worry on this automatic detection of black levels is what impact
there would be if the scene is always bright ... but it hasn't been an
issue yet - and I'm sure it will continue to develop so I won't impeded
here:


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/ipa/simple/algorithms/blc.cpp | 9 ++++++++-
>  src/ipa/simple/algorithms/blc.h   | 4 ++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index 1ceae85d..5e0a36d9 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -30,10 +30,15 @@ int BlackLevel::init(IPAContext &context,
>  
>  void BlackLevel::process(IPAContext &context,
>                          [[maybe_unused]] const uint32_t frame,
> -                        [[maybe_unused]] IPAFrameContext &frameContext,
> +                        IPAFrameContext &frameContext,
>                          const SwIspStats *stats,
>                          [[maybe_unused]] ControlList &metadata)
>  {
> +       if (frameContext.sensor.exposure == exposure_ &&
> +           frameContext.sensor.gain == gain_) {
> +               return;
> +       }
> +
>         const SwIspStats::Histogram &histogram = stats->yHistogram;
>  
>         /*
> @@ -54,6 +59,8 @@ void BlackLevel::process(IPAContext &context,
>                 if (seen >= pixelThreshold) {
>                         context.activeState.blc.level =
>                                 static_cast<double>(i) / SwIspStats::kYHistogramSize;
> +                       exposure_ = frameContext.sensor.exposure;
> +                       gain_ = 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 c2140b4b..fc38375e 100644
> --- a/src/ipa/simple/algorithms/blc.h
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -25,6 +25,10 @@ public:
>                      IPAFrameContext &frameContext,
>                      const SwIspStats *stats,
>                      ControlList &metadata) override;
> +
> +private:
> +       uint32_t exposure_;
> +       double gain_;
>  };
>  
>  } /* namespace ipa::soft::algorithms */
> -- 
> 2.44.1
>

Kieran Bingham <kieran.bingham@ideasonboard.com> writes:

> Quoting Milan Zamazal (2024-09-06 13:09:27)
>> The black level is likely to get updated, if ever, only after exposure
>> or gain changes.  Don't compute its possible updates if exposure and
>
>> gain are unchanged.
>> 
>> It's probably not worth trying to implement something more
>> sophisticated.  Better to spend the effort on supporting tuning files.
>> 
>
> Fine with me. Indeed I think we just go with static/constant black
> levels when we know more information about the sensor.
>
> I'll be curious to see how close this implementation measures against
> the 'known' black level values, or measured blacks if we do calibration
> of this.

In my environment, it's accurate, within the 256 levels resolution.
Sometimes it doesn't reach the right value immediately but adjusts to it
very soon.  This isn't representative but I haven't heard complaints
from anybody so far.  And the aim is not to substitute datasheets or
calibration but to serve as a reasonable fallback.

> My worry on this automatic detection of black levels is what impact
> there would be if the scene is always bright ... 

Sessions start with minimum exposure, so hardly anything can be too
bright initially.  If this wasn't the case then I guess the effect would
be similar to autoexposure adjustments -- bringing the bright scene
towards middle brightness but with more contrast.

> but it hasn't been an issue yet - and I'm sure it will continue to
> develop so I won't impeded here:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>  src/ipa/simple/algorithms/blc.cpp | 9 ++++++++-
>>  src/ipa/simple/algorithms/blc.h   | 4 ++++
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
>> index 1ceae85d..5e0a36d9 100644
>> --- a/src/ipa/simple/algorithms/blc.cpp
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -30,10 +30,15 @@ int BlackLevel::init(IPAContext &context,
>>  
>>  void BlackLevel::process(IPAContext &context,
>>                          [[maybe_unused]] const uint32_t frame,
>> -                        [[maybe_unused]] IPAFrameContext &frameContext,
>> +                        IPAFrameContext &frameContext,
>>                          const SwIspStats *stats,
>>                          [[maybe_unused]] ControlList &metadata)
>>  {
>> +       if (frameContext.sensor.exposure == exposure_ &&
>> +           frameContext.sensor.gain == gain_) {
>> +               return;
>> +       }
>> +
>>         const SwIspStats::Histogram &histogram = stats->yHistogram;
>>  
>>         /*
>> @@ -54,6 +59,8 @@ void BlackLevel::process(IPAContext &context,
>>                 if (seen >= pixelThreshold) {
>>                         context.activeState.blc.level =
>>                                 static_cast<double>(i) / SwIspStats::kYHistogramSize;
>> +                       exposure_ = frameContext.sensor.exposure;
>> +                       gain_ = 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 c2140b4b..fc38375e 100644
>> --- a/src/ipa/simple/algorithms/blc.h
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -25,6 +25,10 @@ public:
>>                      IPAFrameContext &frameContext,
>>                      const SwIspStats *stats,
>>                      ControlList &metadata) override;
>> +
>> +private:
>> +       uint32_t exposure_;
>> +       double gain_;
>>  };
>>  
>>  } /* namespace ipa::soft::algorithms */
>> -- 
>> 2.44.1
>>