[PATCH v5 13/18] libcamera: software_isp: Move black level to an algorithm module

Milan Zamazal mzamazal at redhat.com
Mon Sep 16 17:22:24 CEST 2024


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

> On Fri, Sep 06, 2024 at 04:01:13PM +0200, Milan Zamazal wrote:
>> Laurent Pinchart writes:
>> > On Fri, Sep 06, 2024 at 11:42:05AM +0200, Milan Zamazal wrote:
>
>> >> Laurent Pinchart writes:
>> >> > On Fri, Aug 30, 2024 at 09:25:49AM +0200, 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
>> >> >> 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.
>> >> >> 
>> >> >> The comment about getting black level from the tuning files is dropped.
>> >> >> The black level will be taken from CameraSensorHelper if available and
>> >> >> that will be implemented in one of the followup patches.
>> >> >> 
>> >> >> 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 at redhat.com>
>> >> >> Reviewed-by: Umang Jain <umang.jain at ideasonboard.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..3b73d830
>> >> >> --- /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;
>> >> >
>> >> > Is init() the right place, shouldn't this be done in configure() ?
>> >> > Setting the initial value in init() means that a new capture session
>> >> > will not start with a reinitialized black level value.
>> >> 
>> >> That should be fine as long as the sensor is the same, which I suppose
>> >> is the case.
>> >
>> > Is it fine ? Should an application expect that the camera will start
>> > with identical parameters for the first capture session and for all
>> > subsequent ones ?
>> 
>> I'm not sure.  Since init() is what takes tuningData as a parameter, I'd
>> think a new instance should be created any time something substantial
>> changes (e.g. a different sensor is used).  OTOH in the patch (posted
>> separately) taking black level from CameraSensorHelper if available, I
>> move the initialization from init() to configure() because this is where
>> CameraSensorHelper is available in software ISP.
>
> init() is meant to read parameters from the tuning file, and perform
> one-time initialization of data that do not vary during the life time of
> the camera. Here you're initializing the active state, which is then
> modified by process(). The active state should be reinitialized with
> each capture session to avoid leaking state between sessions. Actually,
> if you look for instance at IPARkISP1::configure(), you will see
>
> 	/* Clear the IPA context before the streaming session. */
> 	context_.configuration = {};
> 	context_.activeState = {}; 
> 	context_.frameContexts.clear();
>
> Now, I think there are use cases for retaining some state between
> sessions, to speed up convergence of algorithms, but I think that should
> be designed and built on top of a base where the state is cleared,
> instead of leaking the state between sessions in an uncontrolled way.

Thank you for explanation.  I'll rethink it for v7, most likely simply
moving the initialization to configure() + some commentary, to not
complicated things at this stage.

>> In any case, moving it to configure() right now doesn't harm and I can
>> do it in v7 (together with moving the gamma initialization).
>> 
>> >> >> +	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.
>> >> >
>> >> > Please reflow to 80 columns.
>> >> >
>> >> >> +	 */
>> >> >> +	constexpr float ignoredPercentage_ = 0.02;
>> >> >
>> >> > It's not a member variable, so no trailing underscore.
>> >> >
>> >> >> +	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..2f73db52
>> >> >> --- /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"
>> >> >
>> >> > I think you can drop controls.h, swisp_stats.h and ipa_context.h.
>> >> > They're included only for classes and structures that are part of the
>> >> > Algorithm class API, so they're guaranteed to be included through
>> >> > algorithm.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 3c1c7262..268cff32 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 e7d8b8df..22156569 100644
>> >> >> --- a/src/ipa/simple/ipa_context.h
>> >> >> +++ b/src/ipa/simple/ipa_context.h
>> >> >> @@ -7,6 +7,8 @@
>> >> >>  
>> >> >>  #pragma once
>> >> >>  
>> >> >> +#include <stdint.h>
>> >> >> +
>> >> >>  #include <libipa/fc_queue.h>
>> >> >>  
>> >> >>  namespace libcamera {
>> >> >> @@ -17,6 +19,9 @@ struct IPASessionConfiguration {
>> >> >>  };
>> >> >>  
>> >> >>  struct IPAActiveState {
>> >> >> +	struct {
>> >> >> +		uint8_t level;
>> >> >> +	} black;
>> >> >
>> >> > How about naming this blc ?
>> >> >
>> >> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>> >> >
>> >> >>  };
>> >> >>  
>> >> >>  struct IPAFrameContext : public FrameContext {
>> >> >> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>> >> >> index dcd7c70a..2f9f15f4 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 4b9201dc..a1bd2f0c 100644
>> >> >> --- a/src/ipa/simple/soft_simple.cpp
>> >> >> +++ b/src/ipa/simple/soft_simple.cpp
>> >> >> @@ -29,7 +29,6 @@
>> >> >>  
>> >> >>  #include "libipa/camera_sensor_helper.h"
>> >> >>  
>> >> >> -#include "black_level.h"
>> >> >>  #include "module.h"
>> >> >>  
>> >> >>  namespace libcamera {
>> >> >> @@ -61,7 +60,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)
>> >> >>  	{
>> >> >> @@ -93,7 +92,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(const uint32_t frame,
>> >> >>  		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



More information about the libcamera-devel mailing list