[PATCH v5 13/18] libcamera: software_isp: Move black level to an algorithm module
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Sep 6 14:57:43 CEST 2024
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 ?
> >> + 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
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list