[PATCH v5 13/18] libcamera: software_isp: Move black level to an algorithm module
Milan Zamazal
mzamazal at redhat.com
Fri Sep 6 11:42:05 CEST 2024
Hi Laurent,
thank you for review.
Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> Hi Milan,
>
> Thank you for the patch.
>
> 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.
>> + 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