[PATCH v5 13/18] libcamera: software_isp: Move black level to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Fri Sep 6 16:01:13 CEST 2024
Laurent Pinchart <laurent.pinchart at ideasonboard.com> 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.
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