[v2,14/19] libcamera: software_isp: Move black level to an algorithm module
diff mbox series

Message ID 20240703175119.1872585-15-mzamazal@redhat.com
State Superseded
Headers show
Series
  • Software ISP refactoring
Related show

Commit Message

Milan Zamazal July 3, 2024, 5:51 p.m. UTC 
The black level determination, already present as a separate class, can
be moved to the prepared Algorithm processing structure.  It is the
first of the current software ISP algorithms that is called in the stats
processing sequence, which means it is also the first one that should be
moved to the new structure.  Stats processing starts with calling
Algorithm-based processing so the call order of the algorithms is
retained.

Movement of this algorithm is relatively straightforward because all it
does is processing stats.

Black level is now recomputed on each stats processing.  In a future
patch, after DelayedControls are used, this will be changed to recompute
the black level only after exposure/gain changes.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/ipa/simple/algorithms/blc.cpp     | 71 ++++++++++++++++++++
 src/ipa/simple/algorithms/blc.h       | 37 +++++++++++
 src/ipa/simple/algorithms/meson.build |  1 +
 src/ipa/simple/black_level.cpp        | 93 ---------------------------
 src/ipa/simple/black_level.h          | 33 ----------
 src/ipa/simple/data/uncalibrated.yaml |  1 +
 src/ipa/simple/ipa_context.cpp        |  8 +++
 src/ipa/simple/ipa_context.h          |  5 ++
 src/ipa/simple/meson.build            |  1 -
 src/ipa/simple/soft_simple.cpp        |  8 +--
 10 files changed, 125 insertions(+), 133 deletions(-)
 create mode 100644 src/ipa/simple/algorithms/blc.cpp
 create mode 100644 src/ipa/simple/algorithms/blc.h
 delete mode 100644 src/ipa/simple/black_level.cpp
 delete mode 100644 src/ipa/simple/black_level.h

Comments

Umang Jain July 13, 2024, 4:40 p.m. UTC | #1 
Hi Milan

Thank you for the patch

On 03/07/24 11:21 pm, Milan Zamazal wrote:
> The black level determination, already present as a separate class, can
> be moved to the prepared Algorithm processing structure.  It is the
> first of the current software ISP algorithms that is called in the stats
> processing sequence, which means it is also the first one that should be
> moved to the new structure.  Stats processing starts with calling

S/  Stats/ Stats/

(extra space)
> Algorithm-based processing so the call order of the algorithms is
> retained.
>
> Movement of this algorithm is relatively straightforward because all it
> does is processing stats.
>
> Black level is now recomputed on each stats processing.  In a future
s/  In/ In/
(ditto)
> patch, after DelayedControls are used, this will be changed to recompute
> the black level only after exposure/gain changes.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>   src/ipa/simple/algorithms/blc.cpp     | 71 ++++++++++++++++++++
>   src/ipa/simple/algorithms/blc.h       | 37 +++++++++++
>   src/ipa/simple/algorithms/meson.build |  1 +
>   src/ipa/simple/black_level.cpp        | 93 ---------------------------
>   src/ipa/simple/black_level.h          | 33 ----------
>   src/ipa/simple/data/uncalibrated.yaml |  1 +
>   src/ipa/simple/ipa_context.cpp        |  8 +++
>   src/ipa/simple/ipa_context.h          |  5 ++
>   src/ipa/simple/meson.build            |  1 -
>   src/ipa/simple/soft_simple.cpp        |  8 +--
>   10 files changed, 125 insertions(+), 133 deletions(-)
>   create mode 100644 src/ipa/simple/algorithms/blc.cpp
>   create mode 100644 src/ipa/simple/algorithms/blc.h
>   delete mode 100644 src/ipa/simple/black_level.cpp
>   delete mode 100644 src/ipa/simple/black_level.h
>
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> new file mode 100644
> index 00000000..e98c5804
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -0,0 +1,71 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * black level handling

s/black/Black/
> + */
> +
> +#include "blc.h"
> +
> +#include <numeric>
> +
> +#include <libcamera/base/log.h>
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +LOG_DEFINE_CATEGORY(IPASoftBL)
> +
> +BlackLevel::BlackLevel()
> +{
> +}
> +
> +int BlackLevel::init(IPAContext &context,
> +		     [[maybe_unused]] const YamlObject &tuningData)
> +{
> +	context.activeState.black.level = 255;
> +	return 0;
> +}
> +
> +void BlackLevel::process(IPAContext &context,
> +			 [[maybe_unused]] const uint32_t frame,
> +			 [[maybe_unused]] IPAFrameContext &frameContext,
> +			 const SwIspStats *stats,
> +			 [[maybe_unused]] ControlList &metadata)
> +{
> +	const SwIspStats::Histogram &histogram = stats->yHistogram;
> +
> +	/*
> +	 * The constant is selected to be "good enough", not overly conservative or
> +	 * aggressive. There is no magic about the given value.
> +	 */
> +	constexpr float ignoredPercentage_ = 0.02;
> +	const unsigned int total =
> +		std::accumulate(begin(histogram), end(histogram), 0);
> +	const unsigned int pixelThreshold = ignoredPercentage_ * total;
> +	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
> +	const unsigned int currentBlackIdx =
> +		context.activeState.black.level / histogramRatio;
> +
> +	for (unsigned int i = 0, seen = 0;
> +	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
> +	     i++) {
> +		seen += histogram[i];
> +		if (seen >= pixelThreshold) {
> +			context.activeState.black.level = i * histogramRatio;
> +			LOG(IPASoftBL, Debug)
> +				<< "Auto-set black level: "
> +				<< i << "/" << SwIspStats::kYHistogramSize
> +				<< " (" << 100 * (seen - histogram[i]) / total << "% below, "
> +				<< 100 * seen / total << "% at or below)";
> +			break;
> +		}
> +	};
> +}
> +
> +REGISTER_IPA_ALGORITHM(BlackLevel, "BlackLevel")
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
> new file mode 100644
> index 00000000..c8b8d92d
> --- /dev/null
> +++ b/src/ipa/simple/algorithms/blc.h
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2024, Red Hat Inc.
> + *
> + * black level handling

Ditto

These can be applied when merging the series, so for this patch LGTM

Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> + */
> +
> +#pragma once
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/software_isp/swisp_stats.h"
> +
> +#include "algorithm.h"
> +#include "ipa_context.h"
> +
> +namespace libcamera {
> +
> +namespace ipa::soft::algorithms {
> +
> +class BlackLevel : public Algorithm
> +{
> +public:
> +	BlackLevel();
> +	~BlackLevel() = default;
> +
> +	int init(IPAContext &context, const YamlObject &tuningData)
> +		override;
> +	void process(IPAContext &context, const uint32_t frame,
> +		     IPAFrameContext &frameContext,
> +		     const SwIspStats *stats,
> +		     ControlList &metadata) override;
> +};
> +
> +} /* namespace ipa::soft::algorithms */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
> index 31d26e43..1f63c220 100644
> --- a/src/ipa/simple/algorithms/meson.build
> +++ b/src/ipa/simple/algorithms/meson.build
> @@ -1,4 +1,5 @@
>   # SPDX-License-Identifier: CC0-1.0
>   
>   soft_simple_ipa_algorithms = files([
> +    'blc.cpp',
>   ])
> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp
> deleted file mode 100644
> index 37e0109c..00000000
> --- a/src/ipa/simple/black_level.cpp
> +++ /dev/null
> @@ -1,93 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2024, Red Hat Inc.
> - *
> - * black level handling
> - */
> -
> -#include "black_level.h"
> -
> -#include <numeric>
> -
> -#include <libcamera/base/log.h>
> -
> -namespace libcamera {
> -
> -LOG_DEFINE_CATEGORY(IPASoftBL)
> -
> -namespace ipa::soft {
> -
> -/**
> - * \class BlackLevel
> - * \brief Object providing black point level for software ISP
> - *
> - * Black level can be provided in hardware tuning files or, if no tuning file is
> - * available for the given hardware, guessed automatically, with less accuracy.
> - * As tuning files are not yet implemented for software ISP, BlackLevel
> - * currently provides only guessed black levels.
> - *
> - * This class serves for tracking black level as a property of the underlying
> - * hardware, not as means of enhancing a particular scene or image.
> - *
> - * The class is supposed to be instantiated for the given camera stream.
> - * The black level can be retrieved using BlackLevel::get() method. It is
> - * initially 0 and may change when updated using BlackLevel::update() method.
> - */
> -
> -BlackLevel::BlackLevel()
> -	: blackLevel_(255), blackLevelSet_(false)
> -{
> -}
> -
> -/**
> - * \brief Return the current black level
> - *
> - * \return The black level, in the range from 0 (minimum) to 255 (maximum).
> - * If the black level couldn't be determined yet, return 0.
> - */
> -uint8_t BlackLevel::get() const
> -{
> -	return blackLevelSet_ ? blackLevel_ : 0;
> -}
> -
> -/**
> - * \brief Update black level from the provided histogram
> - * \param[in] yHistogram The histogram to be used for updating black level
> - *
> - * The black level is property of the given hardware, not image. It is updated
> - * only if it has not been yet set or if it is lower than the lowest value seen
> - * so far.
> - */
> -void BlackLevel::update(SwIspStats::Histogram &yHistogram)
> -{
> -	/*
> -	 * The constant is selected to be "good enough", not overly conservative or
> -	 * aggressive. There is no magic about the given value.
> -	 */
> -	constexpr float ignoredPercentage_ = 0.02;
> -	const unsigned int total =
> -		std::accumulate(begin(yHistogram), end(yHistogram), 0);
> -	const unsigned int pixelThreshold = ignoredPercentage_ * total;
> -	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
> -	const unsigned int currentBlackIdx = blackLevel_ / histogramRatio;
> -
> -	for (unsigned int i = 0, seen = 0;
> -	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
> -	     i++) {
> -		seen += yHistogram[i];
> -		if (seen >= pixelThreshold) {
> -			blackLevel_ = i * histogramRatio;
> -			blackLevelSet_ = true;
> -			LOG(IPASoftBL, Debug)
> -				<< "Auto-set black level: "
> -				<< i << "/" << SwIspStats::kYHistogramSize
> -				<< " (" << 100 * (seen - yHistogram[i]) / total << "% below, "
> -				<< 100 * seen / total << "% at or below)";
> -			break;
> -		}
> -	};
> -}
> -
> -} /* namespace ipa::soft */
> -
> -} /* namespace libcamera */
> diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h
> deleted file mode 100644
> index a04230c9..00000000
> --- a/src/ipa/simple/black_level.h
> +++ /dev/null
> @@ -1,33 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2024, Red Hat Inc.
> - *
> - * black level handling
> - */
> -
> -#pragma once
> -
> -#include <array>
> -#include <stdint.h>
> -
> -#include "libcamera/internal/software_isp/swisp_stats.h"
> -
> -namespace libcamera {
> -
> -namespace ipa::soft {
> -
> -class BlackLevel
> -{
> -public:
> -	BlackLevel();
> -	uint8_t get() const;
> -	void update(SwIspStats::Histogram &yHistogram);
> -
> -private:
> -	uint8_t blackLevel_;
> -	bool blackLevelSet_;
> -};
> -
> -} /* namespace ipa::soft */
> -
> -} /* namespace libcamera */
> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
> index 2cdc39a8..e0d03d96 100644
> --- a/src/ipa/simple/data/uncalibrated.yaml
> +++ b/src/ipa/simple/data/uncalibrated.yaml
> @@ -3,4 +3,5 @@
>   ---
>   version: 1
>   algorithms:
> +  - BlackLevel:
>   ...
> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
> index 0898e591..bb5daa0d 100644
> --- a/src/ipa/simple/ipa_context.cpp
> +++ b/src/ipa/simple/ipa_context.cpp
> @@ -50,4 +50,12 @@ namespace libcamera::ipa::soft {
>    * \brief The current state of IPA algorithms
>    */
>   
> +/**
> + * \var IPAActiveState::black
> + * \brief Context for the Black Level algorithm
> + *
> + * \var IPAActiveState::black.level
> + * \brief Current determined black level
> + */
> +
>   } /* namespace libcamera::ipa::soft */
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index bc1235b6..ed07dbb8 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -8,6 +8,8 @@
>   
>   #pragma once
>   
> +#include <stdint.h>
> +
>   #include <libipa/fc_queue.h>
>   
>   namespace libcamera {
> @@ -18,6 +20,9 @@ struct IPASessionConfiguration {
>   };
>   
>   struct IPAActiveState {
> +	struct {
> +		uint8_t level;
> +	} black;
>   };
>   
>   struct IPAFrameContext : public FrameContext {
> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
> index 7515a8d8..92aa5744 100644
> --- a/src/ipa/simple/meson.build
> +++ b/src/ipa/simple/meson.build
> @@ -8,7 +8,6 @@ ipa_name = 'ipa_soft_simple'
>   soft_simple_sources = files([
>       'ipa_context.cpp',
>       'soft_simple.cpp',
> -    'black_level.cpp',
>   ])
>   
>   soft_simple_sources += soft_simple_ipa_algorithms
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index a8499f29..2fb3a473 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -28,7 +28,6 @@
>   
>   #include "libipa/camera_sensor_helper.h"
>   
> -#include "black_level.h"
>   #include "module.h"
>   
>   namespace libcamera {
> @@ -60,7 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
>   {
>   public:
>   	IPASoftSimple()
> -		: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),
> +		: params_(nullptr), stats_(nullptr),
>   		  context_({ {}, {}, { kMaxFrameContexts } }),
>   		  ignoreUpdates_(0)
>   	{
> @@ -92,7 +91,6 @@ private:
>   	SwIspStats *stats_;
>   	std::unique_ptr<CameraSensorHelper> camHelper_;
>   	ControlInfoMap sensorInfoMap_;
> -	BlackLevel blackLevel_;
>   
>   	static constexpr unsigned int kGammaLookupSize = 1024;
>   	std::array<uint8_t, kGammaLookupSize> gammaTable_;
> @@ -303,9 +301,7 @@ void IPASoftSimple::processStats(
>   		algo->process(context_, frame, frameContext, stats_, metadata);
>   
>   	SwIspStats::Histogram histogram = stats_->yHistogram;
> -	if (ignoreUpdates_ > 0)
> -		blackLevel_.update(histogram);
> -	const uint8_t blackLevel = blackLevel_.get();
> +	const uint8_t blackLevel = context_.activeState.black.level;
>   
>   	/*
>   	 * Black level must be subtracted to get the correct AWB ratios, they
Milan Zamazal July 13, 2024, 4:56 p.m. UTC | #2 
Hi Umang,

thank you for review.

Umang Jain <umang.jain@ideasonboard.com> writes:

> Hi Milan
>
> Thank you for the patch
>
> On 03/07/24 11:21 pm, Milan Zamazal wrote:
>> The black level determination, already present as a separate class, can
>> be moved to the prepared Algorithm processing structure.  It is the
>> first of the current software ISP algorithms that is called in the stats
>> processing sequence, which means it is also the first one that should be
>> moved to the new structure.  Stats processing starts with calling
>
> S/  Stats/ Stats/
>
> (extra space)

This is deliberate -- it tells Emacs about an end of sentence.  I don't
think we have a style for commit messages so I suppose I'm free to use
this style there :-).  OTOH I don't mind if it is changed when merging.

>> Algorithm-based processing so the call order of the algorithms is
>> retained.
>>
>> Movement of this algorithm is relatively straightforward because all it
>> does is processing stats.
>>
>> Black level is now recomputed on each stats processing.  In a future
> s/  In/ In/
> (ditto)
>> patch, after DelayedControls are used, this will be changed to recompute
>> the black level only after exposure/gain changes.
>>
>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
>> ---
>>   src/ipa/simple/algorithms/blc.cpp     | 71 ++++++++++++++++++++
>>   src/ipa/simple/algorithms/blc.h       | 37 +++++++++++
>>   src/ipa/simple/algorithms/meson.build |  1 +
>>   src/ipa/simple/black_level.cpp        | 93 ---------------------------
>>   src/ipa/simple/black_level.h          | 33 ----------
>>   src/ipa/simple/data/uncalibrated.yaml |  1 +
>>   src/ipa/simple/ipa_context.cpp        |  8 +++
>>   src/ipa/simple/ipa_context.h          |  5 ++
>>   src/ipa/simple/meson.build            |  1 -
>>   src/ipa/simple/soft_simple.cpp        |  8 +--
>>   10 files changed, 125 insertions(+), 133 deletions(-)
>>   create mode 100644 src/ipa/simple/algorithms/blc.cpp
>>   create mode 100644 src/ipa/simple/algorithms/blc.h
>>   delete mode 100644 src/ipa/simple/black_level.cpp
>>   delete mode 100644 src/ipa/simple/black_level.h
>>
>> diff --git a/src/ipa/simple/algorithms/blc.cpp
>> b/src/ipa/simple/algorithms/blc.cpp
>> new file mode 100644
>> index 00000000..e98c5804
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -0,0 +1,71 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * black level handling
>
> s/black/Black/
>> + */
>> +
>> +#include "blc.h"
>> +
>> +#include <numeric>
>> +
>> +#include <libcamera/base/log.h>
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +LOG_DEFINE_CATEGORY(IPASoftBL)
>> +
>> +BlackLevel::BlackLevel()
>> +{
>> +}
>> +
>> +int BlackLevel::init(IPAContext &context,
>> +		     [[maybe_unused]] const YamlObject &tuningData)
>> +{
>> +	context.activeState.black.level = 255;
>> +	return 0;
>> +}
>> +
>> +void BlackLevel::process(IPAContext &context,
>> +			 [[maybe_unused]] const uint32_t frame,
>> +			 [[maybe_unused]] IPAFrameContext &frameContext,
>> +			 const SwIspStats *stats,
>> +			 [[maybe_unused]] ControlList &metadata)
>> +{
>> +	const SwIspStats::Histogram &histogram = stats->yHistogram;
>> +
>> +	/*
>> + * The constant is selected to be "good enough", not overly conservative or
>> +	 * aggressive. There is no magic about the given value.
>> +	 */
>> +	constexpr float ignoredPercentage_ = 0.02;
>> +	const unsigned int total =
>> +		std::accumulate(begin(histogram), end(histogram), 0);
>> +	const unsigned int pixelThreshold = ignoredPercentage_ * total;
>> +	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>> +	const unsigned int currentBlackIdx =
>> +		context.activeState.black.level / histogramRatio;
>> +
>> +	for (unsigned int i = 0, seen = 0;
>> +	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>> +	     i++) {
>> +		seen += histogram[i];
>> +		if (seen >= pixelThreshold) {
>> +			context.activeState.black.level = i * histogramRatio;
>> +			LOG(IPASoftBL, Debug)
>> +				<< "Auto-set black level: "
>> +				<< i << "/" << SwIspStats::kYHistogramSize
>> + << " (" << 100 * (seen - histogram[i]) / total << "% below, "
>> +				<< 100 * seen / total << "% at or below)";
>> +			break;
>> +		}
>> +	};
>> +}
>> +
>> +REGISTER_IPA_ALGORITHM(BlackLevel, "BlackLevel")
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
>> new file mode 100644
>> index 00000000..c8b8d92d
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/blc.h
>> @@ -0,0 +1,37 @@
>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> +/*
>> + * Copyright (C) 2024, Red Hat Inc.
>> + *
>> + * black level handling
>
> Ditto
>
> These can be applied when merging the series, so for this patch LGTM
>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
>> + */
>> +
>> +#pragma once
>> +
>> +#include <libcamera/controls.h>
>> +
>> +#include "libcamera/internal/software_isp/swisp_stats.h"
>> +
>> +#include "algorithm.h"
>> +#include "ipa_context.h"
>> +
>> +namespace libcamera {
>> +
>> +namespace ipa::soft::algorithms {
>> +
>> +class BlackLevel : public Algorithm
>> +{
>> +public:
>> +	BlackLevel();
>> +	~BlackLevel() = default;
>> +
>> +	int init(IPAContext &context, const YamlObject &tuningData)
>> +		override;
>> +	void process(IPAContext &context, const uint32_t frame,
>> +		     IPAFrameContext &frameContext,
>> +		     const SwIspStats *stats,
>> +		     ControlList &metadata) override;
>> +};
>> +
>> +} /* namespace ipa::soft::algorithms */
>> +
>> +} /* namespace libcamera */
>> diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
>> index 31d26e43..1f63c220 100644
>> --- a/src/ipa/simple/algorithms/meson.build
>> +++ b/src/ipa/simple/algorithms/meson.build
>> @@ -1,4 +1,5 @@
>>   # SPDX-License-Identifier: CC0-1.0
>>     soft_simple_ipa_algorithms = files([
>> +    'blc.cpp',
>>   ])
>> diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp
>> deleted file mode 100644
>> index 37e0109c..00000000
>> --- a/src/ipa/simple/black_level.cpp
>> +++ /dev/null
>> @@ -1,93 +0,0 @@
>> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> -/*
>> - * Copyright (C) 2024, Red Hat Inc.
>> - *
>> - * black level handling
>> - */
>> -
>> -#include "black_level.h"
>> -
>> -#include <numeric>
>> -
>> -#include <libcamera/base/log.h>
>> -
>> -namespace libcamera {
>> -
>> -LOG_DEFINE_CATEGORY(IPASoftBL)
>> -
>> -namespace ipa::soft {
>> -
>> -/**
>> - * \class BlackLevel
>> - * \brief Object providing black point level for software ISP
>> - *
>> - * Black level can be provided in hardware tuning files or, if no tuning file is
>> - * available for the given hardware, guessed automatically, with less accuracy.
>> - * As tuning files are not yet implemented for software ISP, BlackLevel
>> - * currently provides only guessed black levels.
>> - *
>> - * This class serves for tracking black level as a property of the underlying
>> - * hardware, not as means of enhancing a particular scene or image.
>> - *
>> - * The class is supposed to be instantiated for the given camera stream.
>> - * The black level can be retrieved using BlackLevel::get() method. It is
>> - * initially 0 and may change when updated using BlackLevel::update() method.
>> - */
>> -
>> -BlackLevel::BlackLevel()
>> -	: blackLevel_(255), blackLevelSet_(false)
>> -{
>> -}
>> -
>> -/**
>> - * \brief Return the current black level
>> - *
>> - * \return The black level, in the range from 0 (minimum) to 255 (maximum).
>> - * If the black level couldn't be determined yet, return 0.
>> - */
>> -uint8_t BlackLevel::get() const
>> -{
>> -	return blackLevelSet_ ? blackLevel_ : 0;
>> -}
>> -
>> -/**
>> - * \brief Update black level from the provided histogram
>> - * \param[in] yHistogram The histogram to be used for updating black level
>> - *
>> - * The black level is property of the given hardware, not image. It is updated
>> - * only if it has not been yet set or if it is lower than the lowest value seen
>> - * so far.
>> - */
>> -void BlackLevel::update(SwIspStats::Histogram &yHistogram)
>> -{
>> -	/*
>> -	 * The constant is selected to be "good enough", not overly conservative or
>> -	 * aggressive. There is no magic about the given value.
>> -	 */
>> -	constexpr float ignoredPercentage_ = 0.02;
>> -	const unsigned int total =
>> -		std::accumulate(begin(yHistogram), end(yHistogram), 0);
>> -	const unsigned int pixelThreshold = ignoredPercentage_ * total;
>> -	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
>> -	const unsigned int currentBlackIdx = blackLevel_ / histogramRatio;
>> -
>> -	for (unsigned int i = 0, seen = 0;
>> -	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>> -	     i++) {
>> -		seen += yHistogram[i];
>> -		if (seen >= pixelThreshold) {
>> -			blackLevel_ = i * histogramRatio;
>> -			blackLevelSet_ = true;
>> -			LOG(IPASoftBL, Debug)
>> -				<< "Auto-set black level: "
>> -				<< i << "/" << SwIspStats::kYHistogramSize
>> -				<< " (" << 100 * (seen - yHistogram[i]) / total << "% below, "
>> -				<< 100 * seen / total << "% at or below)";
>> -			break;
>> -		}
>> -	};
>> -}
>> -
>> -} /* namespace ipa::soft */
>> -
>> -} /* namespace libcamera */
>> diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h
>> deleted file mode 100644
>> index a04230c9..00000000
>> --- a/src/ipa/simple/black_level.h
>> +++ /dev/null
>> @@ -1,33 +0,0 @@
>> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
>> -/*
>> - * Copyright (C) 2024, Red Hat Inc.
>> - *
>> - * black level handling
>> - */
>> -
>> -#pragma once
>> -
>> -#include <array>
>> -#include <stdint.h>
>> -
>> -#include "libcamera/internal/software_isp/swisp_stats.h"
>> -
>> -namespace libcamera {
>> -
>> -namespace ipa::soft {
>> -
>> -class BlackLevel
>> -{
>> -public:
>> -	BlackLevel();
>> -	uint8_t get() const;
>> -	void update(SwIspStats::Histogram &yHistogram);
>> -
>> -private:
>> -	uint8_t blackLevel_;
>> -	bool blackLevelSet_;
>> -};
>> -
>> -} /* namespace ipa::soft */
>> -
>> -} /* namespace libcamera */
>> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
>> index 2cdc39a8..e0d03d96 100644
>> --- a/src/ipa/simple/data/uncalibrated.yaml
>> +++ b/src/ipa/simple/data/uncalibrated.yaml
>> @@ -3,4 +3,5 @@
>>   ---
>>   version: 1
>>   algorithms:
>> +  - BlackLevel:
>>   ...
>> diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
>> index 0898e591..bb5daa0d 100644
>> --- a/src/ipa/simple/ipa_context.cpp
>> +++ b/src/ipa/simple/ipa_context.cpp
>> @@ -50,4 +50,12 @@ namespace libcamera::ipa::soft {
>>    * \brief The current state of IPA algorithms
>>    */
>>   +/**
>> + * \var IPAActiveState::black
>> + * \brief Context for the Black Level algorithm
>> + *
>> + * \var IPAActiveState::black.level
>> + * \brief Current determined black level
>> + */
>> +
>>   } /* namespace libcamera::ipa::soft */
>> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
>> index bc1235b6..ed07dbb8 100644
>> --- a/src/ipa/simple/ipa_context.h
>> +++ b/src/ipa/simple/ipa_context.h
>> @@ -8,6 +8,8 @@
>>     #pragma once
>>   +#include <stdint.h>
>> +
>>   #include <libipa/fc_queue.h>
>>     namespace libcamera {
>> @@ -18,6 +20,9 @@ struct IPASessionConfiguration {
>>   };
>>     struct IPAActiveState {
>> +	struct {
>> +		uint8_t level;
>> +	} black;
>>   };
>>     struct IPAFrameContext : public FrameContext {
>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>> index 7515a8d8..92aa5744 100644
>> --- a/src/ipa/simple/meson.build
>> +++ b/src/ipa/simple/meson.build
>> @@ -8,7 +8,6 @@ ipa_name = 'ipa_soft_simple'
>>   soft_simple_sources = files([
>>       'ipa_context.cpp',
>>       'soft_simple.cpp',
>> -    'black_level.cpp',
>>   ])
>>     soft_simple_sources += soft_simple_ipa_algorithms
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index a8499f29..2fb3a473 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -28,7 +28,6 @@
>>     #include "libipa/camera_sensor_helper.h"
>>   -#include "black_level.h"
>>   #include "module.h"
>>     namespace libcamera {
>> @@ -60,7 +59,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
>>   {
>>   public:
>>   	IPASoftSimple()
>> -		: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),
>> +		: params_(nullptr), stats_(nullptr),
>>   		  context_({ {}, {}, { kMaxFrameContexts } }),
>>   		  ignoreUpdates_(0)
>>   	{
>> @@ -92,7 +91,6 @@ private:
>>   	SwIspStats *stats_;
>>   	std::unique_ptr<CameraSensorHelper> camHelper_;
>>   	ControlInfoMap sensorInfoMap_;
>> -	BlackLevel blackLevel_;
>>     	static constexpr unsigned int kGammaLookupSize = 1024;
>>   	std::array<uint8_t, kGammaLookupSize> gammaTable_;
>> @@ -303,9 +301,7 @@ void IPASoftSimple::processStats(
>>   		algo->process(context_, frame, frameContext, stats_, metadata);
>>     	SwIspStats::Histogram histogram = stats_->yHistogram;
>> -	if (ignoreUpdates_ > 0)
>> -		blackLevel_.update(histogram);
>> -	const uint8_t blackLevel = blackLevel_.get();
>> +	const uint8_t blackLevel = context_.activeState.black.level;
>>     	/*
>>   	 * Black level must be subtracted to get the correct AWB ratios, they

Patch
diff mbox series 

diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
new file mode 100644
index 00000000..e98c5804
--- /dev/null
+++ b/src/ipa/simple/algorithms/blc.cpp
@@ -0,0 +1,71 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Red Hat Inc.
+ *
+ * black level handling
+ */
+
+#include "blc.h"
+
+#include <numeric>
+
+#include <libcamera/base/log.h>
+
+namespace libcamera {
+
+namespace ipa::soft::algorithms {
+
+LOG_DEFINE_CATEGORY(IPASoftBL)
+
+BlackLevel::BlackLevel()
+{
+}
+
+int BlackLevel::init(IPAContext &context,
+		     [[maybe_unused]] const YamlObject &tuningData)
+{
+	context.activeState.black.level = 255;
+	return 0;
+}
+
+void BlackLevel::process(IPAContext &context,
+			 [[maybe_unused]] const uint32_t frame,
+			 [[maybe_unused]] IPAFrameContext &frameContext,
+			 const SwIspStats *stats,
+			 [[maybe_unused]] ControlList &metadata)
+{
+	const SwIspStats::Histogram &histogram = stats->yHistogram;
+
+	/*
+	 * The constant is selected to be "good enough", not overly conservative or
+	 * aggressive. There is no magic about the given value.
+	 */
+	constexpr float ignoredPercentage_ = 0.02;
+	const unsigned int total =
+		std::accumulate(begin(histogram), end(histogram), 0);
+	const unsigned int pixelThreshold = ignoredPercentage_ * total;
+	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
+	const unsigned int currentBlackIdx =
+		context.activeState.black.level / histogramRatio;
+
+	for (unsigned int i = 0, seen = 0;
+	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
+	     i++) {
+		seen += histogram[i];
+		if (seen >= pixelThreshold) {
+			context.activeState.black.level = i * histogramRatio;
+			LOG(IPASoftBL, Debug)
+				<< "Auto-set black level: "
+				<< i << "/" << SwIspStats::kYHistogramSize
+				<< " (" << 100 * (seen - histogram[i]) / total << "% below, "
+				<< 100 * seen / total << "% at or below)";
+			break;
+		}
+	};
+}
+
+REGISTER_IPA_ALGORITHM(BlackLevel, "BlackLevel")
+
+} /* namespace ipa::soft::algorithms */
+
+} /* namespace libcamera */
diff --git a/src/ipa/simple/algorithms/blc.h b/src/ipa/simple/algorithms/blc.h
new file mode 100644
index 00000000..c8b8d92d
--- /dev/null
+++ b/src/ipa/simple/algorithms/blc.h
@@ -0,0 +1,37 @@ 
+/* SPDX-License-Identifier: LGPL-2.1-or-later */
+/*
+ * Copyright (C) 2024, Red Hat Inc.
+ *
+ * black level handling
+ */
+
+#pragma once
+
+#include <libcamera/controls.h>
+
+#include "libcamera/internal/software_isp/swisp_stats.h"
+
+#include "algorithm.h"
+#include "ipa_context.h"
+
+namespace libcamera {
+
+namespace ipa::soft::algorithms {
+
+class BlackLevel : public Algorithm
+{
+public:
+	BlackLevel();
+	~BlackLevel() = default;
+
+	int init(IPAContext &context, const YamlObject &tuningData)
+		override;
+	void process(IPAContext &context, const uint32_t frame,
+		     IPAFrameContext &frameContext,
+		     const SwIspStats *stats,
+		     ControlList &metadata) override;
+};
+
+} /* namespace ipa::soft::algorithms */
+
+} /* namespace libcamera */
diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build
index 31d26e43..1f63c220 100644
--- a/src/ipa/simple/algorithms/meson.build
+++ b/src/ipa/simple/algorithms/meson.build
@@ -1,4 +1,5 @@ 
 # SPDX-License-Identifier: CC0-1.0
 
 soft_simple_ipa_algorithms = files([
+    'blc.cpp',
 ])
diff --git a/src/ipa/simple/black_level.cpp b/src/ipa/simple/black_level.cpp
deleted file mode 100644
index 37e0109c..00000000
--- a/src/ipa/simple/black_level.cpp
+++ /dev/null
@@ -1,93 +0,0 @@ 
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2024, Red Hat Inc.
- *
- * black level handling
- */
-
-#include "black_level.h"
-
-#include <numeric>
-
-#include <libcamera/base/log.h>
-
-namespace libcamera {
-
-LOG_DEFINE_CATEGORY(IPASoftBL)
-
-namespace ipa::soft {
-
-/**
- * \class BlackLevel
- * \brief Object providing black point level for software ISP
- *
- * Black level can be provided in hardware tuning files or, if no tuning file is
- * available for the given hardware, guessed automatically, with less accuracy.
- * As tuning files are not yet implemented for software ISP, BlackLevel
- * currently provides only guessed black levels.
- *
- * This class serves for tracking black level as a property of the underlying
- * hardware, not as means of enhancing a particular scene or image.
- *
- * The class is supposed to be instantiated for the given camera stream.
- * The black level can be retrieved using BlackLevel::get() method. It is
- * initially 0 and may change when updated using BlackLevel::update() method.
- */
-
-BlackLevel::BlackLevel()
-	: blackLevel_(255), blackLevelSet_(false)
-{
-}
-
-/**
- * \brief Return the current black level
- *
- * \return The black level, in the range from 0 (minimum) to 255 (maximum).
- * If the black level couldn't be determined yet, return 0.
- */
-uint8_t BlackLevel::get() const
-{
-	return blackLevelSet_ ? blackLevel_ : 0;
-}
-
-/**
- * \brief Update black level from the provided histogram
- * \param[in] yHistogram The histogram to be used for updating black level
- *
- * The black level is property of the given hardware, not image. It is updated
- * only if it has not been yet set or if it is lower than the lowest value seen
- * so far.
- */
-void BlackLevel::update(SwIspStats::Histogram &yHistogram)
-{
-	/*
-	 * The constant is selected to be "good enough", not overly conservative or
-	 * aggressive. There is no magic about the given value.
-	 */
-	constexpr float ignoredPercentage_ = 0.02;
-	const unsigned int total =
-		std::accumulate(begin(yHistogram), end(yHistogram), 0);
-	const unsigned int pixelThreshold = ignoredPercentage_ * total;
-	const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
-	const unsigned int currentBlackIdx = blackLevel_ / histogramRatio;
-
-	for (unsigned int i = 0, seen = 0;
-	     i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
-	     i++) {
-		seen += yHistogram[i];
-		if (seen >= pixelThreshold) {
-			blackLevel_ = i * histogramRatio;
-			blackLevelSet_ = true;
-			LOG(IPASoftBL, Debug)
-				<< "Auto-set black level: "
-				<< i << "/" << SwIspStats::kYHistogramSize
-				<< " (" << 100 * (seen - yHistogram[i]) / total << "% below, "
-				<< 100 * seen / total << "% at or below)";
-			break;
-		}
-	};
-}
-
-} /* namespace ipa::soft */
-
-} /* namespace libcamera */
diff --git a/src/ipa/simple/black_level.h b/src/ipa/simple/black_level.h
deleted file mode 100644
index a04230c9..00000000
--- a/src/ipa/simple/black_level.h
+++ /dev/null
@@ -1,33 +0,0 @@ 
-/* SPDX-License-Identifier: LGPL-2.1-or-later */
-/*
- * Copyright (C) 2024, Red Hat Inc.
- *
- * black level handling
- */
-
-#pragma once
-
-#include <array>
-#include <stdint.h>
-
-#include "libcamera/internal/software_isp/swisp_stats.h"
-
-namespace libcamera {
-
-namespace ipa::soft {
-
-class BlackLevel
-{
-public:
-	BlackLevel();
-	uint8_t get() const;
-	void update(SwIspStats::Histogram &yHistogram);
-
-private:
-	uint8_t blackLevel_;
-	bool blackLevelSet_;
-};
-
-} /* namespace ipa::soft */
-
-} /* namespace libcamera */
diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
index 2cdc39a8..e0d03d96 100644
--- a/src/ipa/simple/data/uncalibrated.yaml
+++ b/src/ipa/simple/data/uncalibrated.yaml
@@ -3,4 +3,5 @@ 
 ---
 version: 1
 algorithms:
+  - BlackLevel:
 ...
diff --git a/src/ipa/simple/ipa_context.cpp b/src/ipa/simple/ipa_context.cpp
index 0898e591..bb5daa0d 100644
--- a/src/ipa/simple/ipa_context.cpp
+++ b/src/ipa/simple/ipa_context.cpp
@@ -50,4 +50,12 @@  namespace libcamera::ipa::soft {
  * \brief The current state of IPA algorithms
  */
 
+/**
+ * \var IPAActiveState::black
+ * \brief Context for the Black Level algorithm
+ *
+ * \var IPAActiveState::black.level
+ * \brief Current determined black level
+ */
+
 } /* namespace libcamera::ipa::soft */
diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
index bc1235b6..ed07dbb8 100644
--- a/src/ipa/simple/ipa_context.h
+++ b/src/ipa/simple/ipa_context.h
@@ -8,6 +8,8 @@ 
 
 #pragma once
 
+#include <stdint.h>
+
 #include <libipa/fc_queue.h>
 
 namespace libcamera {
@@ -18,6 +20,9 @@  struct IPASessionConfiguration {
 };
 
 struct IPAActiveState {
+	struct {
+		uint8_t level;
+	} black;
 };
 
 struct IPAFrameContext : public FrameContext {
diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
index 7515a8d8..92aa5744 100644
--- a/src/ipa/simple/meson.build
+++ b/src/ipa/simple/meson.build
@@ -8,7 +8,6 @@  ipa_name = 'ipa_soft_simple'
 soft_simple_sources = files([
     'ipa_context.cpp',
     'soft_simple.cpp',
-    'black_level.cpp',
 ])
 
 soft_simple_sources += soft_simple_ipa_algorithms
diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
index a8499f29..2fb3a473 100644
--- a/src/ipa/simple/soft_simple.cpp
+++ b/src/ipa/simple/soft_simple.cpp
@@ -28,7 +28,6 @@ 
 
 #include "libipa/camera_sensor_helper.h"
 
-#include "black_level.h"
 #include "module.h"
 
 namespace libcamera {
@@ -60,7 +59,7 @@  class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module
 {
 public:
 	IPASoftSimple()
-		: params_(nullptr), stats_(nullptr), blackLevel_(BlackLevel()),
+		: params_(nullptr), stats_(nullptr),
 		  context_({ {}, {}, { kMaxFrameContexts } }),
 		  ignoreUpdates_(0)
 	{
@@ -92,7 +91,6 @@  private:
 	SwIspStats *stats_;
 	std::unique_ptr<CameraSensorHelper> camHelper_;
 	ControlInfoMap sensorInfoMap_;
-	BlackLevel blackLevel_;
 
 	static constexpr unsigned int kGammaLookupSize = 1024;
 	std::array<uint8_t, kGammaLookupSize> gammaTable_;
@@ -303,9 +301,7 @@  void IPASoftSimple::processStats(
 		algo->process(context_, frame, frameContext, stats_, metadata);
 
 	SwIspStats::Histogram histogram = stats_->yHistogram;
-	if (ignoreUpdates_ > 0)
-		blackLevel_.update(histogram);
-	const uint8_t blackLevel = blackLevel_.get();
+	const uint8_t blackLevel = context_.activeState.black.level;
 
 	/*
 	 * Black level must be subtracted to get the correct AWB ratios, they