[PATCH 14/19] libcamera: software_isp: Move black level to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Wed Jul 3 19:37:29 CEST 2024
Hi Umang,
thank you for review.
Umang Jain <umang.jain at ideasonboard.com> writes:
> Hi Milan
>
> On 26/06/24 12:50 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
>> 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 at redhat.com>
>> ---
>> src/ipa/simple/algorithms/blc.cpp | 79 +++++++++++++++++++++++
>> 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.h | 7 ++
>> src/ipa/simple/meson.build | 1 -
>> src/ipa/simple/soft_simple.cpp | 8 +--
>> 9 files changed, 127 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..e216138d
>> --- /dev/null
>> +++ b/src/ipa/simple/algorithms/blc.cpp
>> @@ -0,0 +1,79 @@
>> +/* 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.configuration.black.set = false;
>> + context.configuration.black.changed = true;
>> + context.configuration.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)
>> +{
>> + if (context.configuration.black.set) {
>> + context.configuration.black.changed = false;
>> + return;
>> + }
>> +
>> + 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.configuration.black.level / histogramRatio;
>> +
>> + for (unsigned int i = 0, seen = 0;
>> + i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
>> + i++) {
>> + seen += histogram[i];
>> + if (seen >= pixelThreshold) {
>> + context.configuration.black.level = i * histogramRatio;
>> + context.configuration.black.changed = true;
>> + 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.h b/src/ipa/simple/ipa_context.h
>> index bc1235b6..c79bf6f8 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 {
>> @@ -15,6 +17,11 @@ namespace libcamera {
>> namespace ipa::soft {
>> struct IPASessionConfiguration {
>> + struct {
>> + uint8_t level;
>> + bool set;
>> + bool changed;
>> + } black;
>
> Shouldn't this go under IPAActiveState instead ?
It should. I thought too much forward about the black level as a
predefined value but it changes in the current implementation. There
are other problems with this patch, I'll rework it a bit.
> Already needs to be documented as done in other IPAs.
Oh yes, will do.
Thank you for your reviews, what I haven't responded to is clear and
I'll address it in v2.
>> };
>> struct IPAActiveState {
>> diff --git a/src/ipa/simple/meson.build b/src/ipa/simple/meson.build
>> index 08d877bc..cbc928c5 100644
>> --- a/src/ipa/simple/meson.build
>> +++ b/src/ipa/simple/meson.build
>> @@ -7,7 +7,6 @@ ipa_name = 'ipa_soft_simple'
>> soft_simple_sources = files([
>> '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 7200abaf..99fb953c 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_;
>> @@ -305,9 +303,7 @@ void IPASoftSimple::processStats(
>> }
>> SwIspStats::Histogram histogram = stats_->yHistogram;
>> - if (ignoreUpdates_ > 0)
>> - blackLevel_.update(histogram);
>> - const uint8_t blackLevel = blackLevel_.get();
>> + const uint8_t blackLevel = context_.configuration.black.level;
>> /*
>> * Black level must be subtracted to get the correct AWB ratios, they
More information about the libcamera-devel
mailing list