[RFC PATCH 2/2] libcamera: bitdepth: Adapt camera_sensor_helper to use BitDepth
Milan Zamazal
mzamazal at redhat.com
Fri Jan 3 15:18:49 CET 2025
Hi Isaac,
thank you for the patch.
Isaac Scott <isaac.scott at ideasonboard.com> writes:
> Adapt the camera_sensor_helper class to use BitDepth instead of bit
> shifting for black levels as an example of how the BitDepth
> implementation can be used.
>
> Signed-off-by: Isaac Scott <isaac.scott at ideasonboard.com>
> ---
> src/ipa/libipa/camera_sensor_helper.cpp | 18 +++++++++---------
> src/ipa/libipa/camera_sensor_helper.h | 5 +++--
> src/ipa/simple/algorithms/awb.cpp | 3 ++-
> src/ipa/simple/algorithms/blc.cpp | 10 ++++++----
> src/ipa/simple/ipa_context.h | 5 +++--
> src/ipa/simple/soft_simple.cpp | 5 ++---
> 6 files changed, 25 insertions(+), 21 deletions(-)
>
> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp
> index c6169bdc..1031dbd1 100644
> --- a/src/ipa/libipa/camera_sensor_helper.cpp
> +++ b/src/ipa/libipa/camera_sensor_helper.cpp
> @@ -407,7 +407,7 @@ public:
> CameraSensorHelperAr0144()
> {
> /* Power-on default value: 168 at 12bits. */
> - blackLevel_ = 2688;
> + blackLevel_ = 168_12bit;
> }
>
> uint32_t gainCode(double gain) const override
> @@ -525,7 +525,7 @@ public:
> CameraSensorHelperImx214()
> {
> /* From datasheet: 64 at 10bits. */
> - blackLevel_ = 4096;
> + blackLevel_ = 64_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 0, 512, -1, 512 };
> }
> @@ -538,7 +538,7 @@ public:
> CameraSensorHelperImx219()
> {
> /* From datasheet: 64 at 10bits. */
> - blackLevel_ = 4096;
> + blackLevel_ = 64_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 0, 256, -1, 256 };
> }
> @@ -551,7 +551,7 @@ public:
> CameraSensorHelperImx258()
> {
> /* From datasheet: 0x40 at 10bits. */
> - blackLevel_ = 4096;
> + blackLevel_ = 0x40_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 0, 512, -1, 512 };
> }
> @@ -564,7 +564,7 @@ public:
> CameraSensorHelperImx283()
> {
> /* From datasheet: 0x32 at 10bits. */
> - blackLevel_ = 3200;
> + blackLevel_ = 0x32_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 0, 2048, -1, 2048 };
> }
> @@ -604,7 +604,7 @@ public:
> CameraSensorHelperImx335()
> {
> /* From datasheet: 0x32 at 10bits. */
> - blackLevel_ = 3200;
> + blackLevel_ = 0x32_10bit;
> gainType_ = AnalogueGainExponential;
> gainConstants_.exp = { 1.0, expGainDb(0.3) };
> }
> @@ -665,7 +665,7 @@ public:
> CameraSensorHelperOv4689()
> {
> /* From datasheet: 0x40 at 12bits. */
> - blackLevel_ = 1024;
> + blackLevel_ = 0x40_12bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 1, 0, 0, 128 };
> }
> @@ -678,7 +678,7 @@ public:
> CameraSensorHelperOv5640()
> {
> /* From datasheet: 0x10 at 10bits. */
> - blackLevel_ = 1024;
> + blackLevel_ = 0x10_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 1, 0, 0, 16 };
> }
> @@ -713,7 +713,7 @@ public:
> CameraSensorHelperOv5675()
> {
> /* From Linux kernel driver: 0x40 at 10bits. */
> - blackLevel_ = 4096;
> + blackLevel_ = 0x40_10bit;
> gainType_ = AnalogueGainLinear;
> gainConstants_.linear = { 1, 0, 0, 128 };
> }
> diff --git a/src/ipa/libipa/camera_sensor_helper.h b/src/ipa/libipa/camera_sensor_helper.h
> index 75868205..b72606bf 100644
> --- a/src/ipa/libipa/camera_sensor_helper.h
> +++ b/src/ipa/libipa/camera_sensor_helper.h
> @@ -14,6 +14,7 @@
> #include <vector>
>
> #include <libcamera/base/class.h>
> +#include "bitdepth.h"
>
> namespace libcamera {
>
> @@ -25,7 +26,7 @@ public:
> CameraSensorHelper() = default;
> virtual ~CameraSensorHelper() = default;
>
> - std::optional<int16_t> blackLevel() const { return blackLevel_; }
> + std::optional<BitDepthValue<16>> blackLevel() const { return blackLevel_; }
> virtual uint32_t gainCode(double gain) const;
> virtual double gain(uint32_t gainCode) const;
>
> @@ -52,7 +53,7 @@ protected:
> AnalogueGainExpConstants exp;
> };
>
> - std::optional<int16_t> blackLevel_;
> + std::optional<BitDepthValue<16>> blackLevel_;
> AnalogueGainType gainType_;
> AnalogueGainConstants gainConstants_;
>
> diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp
> index 195de41d..cfce5869 100644
> --- a/src/ipa/simple/algorithms/awb.cpp
> +++ b/src/ipa/simple/algorithms/awb.cpp
> @@ -12,6 +12,7 @@
>
> #include <libcamera/base/log.h>
>
> +#include "libipa/bitdepth.h"
> #include "simple/ipa_context.h"
>
> namespace libcamera {
> @@ -36,7 +37,7 @@ void Awb::process(IPAContext &context,
> [[maybe_unused]] ControlList &metadata)
> {
> const SwIspStats::Histogram &histogram = stats->yHistogram;
> - const uint8_t blackLevel = context.activeState.blc.level;
> + const BitDepthValue<8> blackLevel = context.activeState.blc.level;
>
> /*
> * Black level must be subtracted to get the correct AWB ratios, they
> diff --git a/src/ipa/simple/algorithms/blc.cpp b/src/ipa/simple/algorithms/blc.cpp
> index b4e32fe1..7c5d3f6d 100644
> --- a/src/ipa/simple/algorithms/blc.cpp
> +++ b/src/ipa/simple/algorithms/blc.cpp
> @@ -10,6 +10,7 @@
> #include <numeric>
>
> #include <libcamera/base/log.h>
> +#include "libipa/bitdepth.h"
>
> namespace libcamera {
>
> @@ -29,7 +30,7 @@ int BlackLevel::init(IPAContext &context, const YamlObject &tuningData)
> * Convert 16 bit values from the tuning file to 8 bit black
> * level for the SoftISP.
> */
> - context.configuration.black.level = blackLevel.value() >> 8;
> + context.configuration.black.level->convert<8>();
I think it should be
context.configuration.black.level = blackLevel.convert<8>();
> }
> return 0;
> }
> @@ -38,7 +39,7 @@ int BlackLevel::configure(IPAContext &context,
> [[maybe_unused]] const IPAConfigInfo &configInfo)
> {
> context.activeState.blc.level =
> - context.configuration.black.level.value_or(255);
> + context.configuration.black.level.value_or(255_8bit);
> return 0;
> }
>
> @@ -68,14 +69,15 @@ void BlackLevel::process(IPAContext &context,
> const unsigned int pixelThreshold = ignoredPercentage * total;
> const unsigned int histogramRatio = 256 / SwIspStats::kYHistogramSize;
> const unsigned int currentBlackIdx =
> - context.activeState.blc.level / histogramRatio;
> + context.activeState.blc.level.value() / histogramRatio;
This is a typical example why I think using plain `value()' without a
specified depth may be not sufficiently clear and possibly error-prone,
since the internal depth of context.activeState.blc.level is not visible
here.
> for (unsigned int i = 0, seen = 0;
> i < currentBlackIdx && i < SwIspStats::kYHistogramSize;
> i++) {
> seen += histogram[i];
> if (seen >= pixelThreshold) {
> - context.activeState.blc.level = i * histogramRatio;
> + context.activeState.blc.level =
> + BitDepthValue<8>(i * histogramRatio);
> exposure_ = frameContext.sensor.exposure;
> gain_ = frameContext.sensor.gain;
> LOG(IPASoftBL, Debug)
> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h
> index fd121eeb..be3b967a 100644
> --- a/src/ipa/simple/ipa_context.h
> +++ b/src/ipa/simple/ipa_context.h
> @@ -12,6 +12,7 @@
> #include <stdint.h>
>
> #include <libipa/fc_queue.h>
> +#include "libipa/bitdepth.h"
>
> namespace libcamera {
>
> @@ -24,13 +25,13 @@ struct IPASessionConfiguration {
> double againMin, againMax, againMinStep;
> } agc;
> struct {
> - std::optional<uint8_t> level;
> + std::optional<BitDepthValue<8>> level;
> } black;
> };
>
> struct IPAActiveState {
> struct {
> - uint8_t level;
> + BitDepthValue<8> level;
> } blc;
>
> struct {
> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
> index ac2a9421..292fa81f 100644
> --- a/src/ipa/simple/soft_simple.cpp
> +++ b/src/ipa/simple/soft_simple.cpp
> @@ -211,11 +211,10 @@ int IPASoftSimple::configure(const IPAConfigInfo &configInfo)
> /*
> * The black level from camHelper_ is a 16 bit value, software ISP
> * works with 8 bit pixel values, both regardless of the actual
> - * sensor pixel width. Hence we obtain the pixel-based black value
> - * by dividing the value from the helper by 256.
> + * sensor pixel width.
> */
> context_.configuration.black.level =
> - camHelper_->blackLevel().value() / 256;
> + camHelper_->blackLevel();
> }
> } else {
> /*
More information about the libcamera-devel
mailing list