| Message ID | 20241127144655.1074720-3-isaac.scott@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Isaac, thank you for the patch. Isaac Scott <isaac.scott@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@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 { > /*
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>(); } 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; 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 { /*
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@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(-)