| Message ID | 20260910172120.73148-3-dmanresa@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On Thu, Sep 10, 2026 at 07:21:20PM +0200, D. Manresa wrote: > The black level correction algorithm hard-codes an optical black level of > 64 for the four Bayer channels. Take it from the camera sensor helper > instead, like the RkISP1 IPA does, falling back to the historical value > with a warning when the helper does not provide one. > > The helper reports the level as a 16-bit value, while the ImgU OB grid > expects units of half a 10-bit LSB: on a Dell Latitude 7275 (OV5670, > black level 64 at 10 bits) a correction of 64 leaves a residual pedestal > in the AWB statistics and in the image, which the colour gains then > amplify into a blue or cyan cast; 128 removes the pedestal and 192 clips > dark areas to zero. The 16-bit value is therefore shifted right by 5, > giving 128 for the OV5670. Dan Scally independently reached the same > conclusion on a Surface Go 2 (OV5693), where the ISP has to be given > twice the data pedestal configured in the sensor for a dark image to come > out dark. > > Measured on the same indoor scene with the uncalibrated tuning file > (NV12, limited range, black = 16): with 64 the darkest percentile of Y > is 32 and its mean chroma is U/V = 132/131; with 128 they are 15 and > 128/128. Too long, with lots of irrelevant information. > Signed-off-by: D. Manresa <dmanresa@gmail.com> > --- > src/ipa/ipu3/algorithms/blc.cpp | 60 ++++++++++++++++++++++++++++----- > src/ipa/ipu3/algorithms/blc.h | 5 +++ > 2 files changed, 56 insertions(+), 9 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/blc.cpp b/src/ipa/ipu3/algorithms/blc.cpp > index 35748fb..f91ea37 100644 > --- a/src/ipa/ipu3/algorithms/blc.cpp > +++ b/src/ipa/ipu3/algorithms/blc.cpp > @@ -7,6 +7,10 @@ > > #include "blc.h" > > +#include <libcamera/base/log.h> > + > +#include "libipa/camera_sensor_helper.h" > + > /** > * \file blc.h > * \brief IPU3 Black Level Correction control > @@ -30,10 +34,52 @@ namespace ipa::ipu3::algorithms { > * isn't currently supported. > */ > > +LOG_DEFINE_CATEGORY(IPU3Blc) > + > +/* > + * Black level used when the camera sensor helper does not provide one, > + * expressed in ImgU units (see init()). This is the historical value that > + * was hard-coded before the level came from the helper. > + */ > +static constexpr uint16_t kDefaultBlackLevel = 64; > + > BlackLevelCorrection::BlackLevelCorrection() > + : blackLevel_(kDefaultBlackLevel) > { > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int BlackLevelCorrection::init(IPAContext &context, > + [[maybe_unused]] const ValueNode &tuningData) > +{ > + std::optional<int16_t> blackLevel = context.camHelper->blackLevel(); > + if (!blackLevel) { > + LOG(IPU3Blc, Warning) > + << "No black level provided by camera sensor helper" > + << ", please fix"; > + blackLevel_ = kDefaultBlackLevel; > + } else { > + /* > + * The helper reports the black level as a 16-bit value, while > + * the ImgU OB grid expects it in units of half a 10-bit LSB. On > + * an OV5670 (black level 64 at 10 bits) a correction of 64 > + * leaves a residual pedestal of about 9/255 in the AWB > + * statistics and in the image, 128 removes it, and 192 clips > + * dark areas to zero. Experiments with the configurable data > + * pedestal of an OV5693 confirm that the ISP needs twice the > + * value programmed in the sensor's black level target. The > + * 16-bit value is therefore shifted right by 5. > + */ Same here. > + blackLevel_ = *blackLevel >> 5; > + } > + > + LOG(IPU3Blc, Debug) << "Black level " << blackLevel_; > + > + return 0; > +} > + > /** > * \brief Fill in the parameter structure, and enable black level correction > * \param[in] context The shared IPA context > @@ -49,15 +95,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context, > [[maybe_unused]] IPAFrameContext &frameContext, > ipu3_uapi_params *params) > { > - /* > - * The Optical Black Level correction values > - * \todo The correction values should come from sensor specific > - * tuning processes. This is a first rough approximation. > - */ > - params->obgrid_param.gr = 64; > - params->obgrid_param.r = 64; > - params->obgrid_param.b = 64; > - params->obgrid_param.gb = 64; > + /* The Optical Black Level correction values */ > + params->obgrid_param.gr = blackLevel_; > + params->obgrid_param.r = blackLevel_; > + params->obgrid_param.b = blackLevel_; > + params->obgrid_param.gb = blackLevel_; > > /* Enable the custom black level correction processing */ > params->use.obgrid = 1; > diff --git a/src/ipa/ipu3/algorithms/blc.h b/src/ipa/ipu3/algorithms/blc.h > index 6274804..bef756f 100644 > --- a/src/ipa/ipu3/algorithms/blc.h > +++ b/src/ipa/ipu3/algorithms/blc.h > @@ -18,9 +18,14 @@ class BlackLevelCorrection : public Algorithm > public: > BlackLevelCorrection(); > > + int init(IPAContext &context, const ValueNode &tuningData) override; > + > void prepare(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, > ipu3_uapi_params *params) override; > + > +private: > + uint16_t blackLevel_; > }; > > } /* namespace ipa::ipu3::algorithms */
diff --git a/src/ipa/ipu3/algorithms/blc.cpp b/src/ipa/ipu3/algorithms/blc.cpp index 35748fb..f91ea37 100644 --- a/src/ipa/ipu3/algorithms/blc.cpp +++ b/src/ipa/ipu3/algorithms/blc.cpp @@ -7,6 +7,10 @@ #include "blc.h" +#include <libcamera/base/log.h> + +#include "libipa/camera_sensor_helper.h" + /** * \file blc.h * \brief IPU3 Black Level Correction control @@ -30,10 +34,52 @@ namespace ipa::ipu3::algorithms { * isn't currently supported. */ +LOG_DEFINE_CATEGORY(IPU3Blc) + +/* + * Black level used when the camera sensor helper does not provide one, + * expressed in ImgU units (see init()). This is the historical value that + * was hard-coded before the level came from the helper. + */ +static constexpr uint16_t kDefaultBlackLevel = 64; + BlackLevelCorrection::BlackLevelCorrection() + : blackLevel_(kDefaultBlackLevel) { } +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int BlackLevelCorrection::init(IPAContext &context, + [[maybe_unused]] const ValueNode &tuningData) +{ + std::optional<int16_t> blackLevel = context.camHelper->blackLevel(); + if (!blackLevel) { + LOG(IPU3Blc, Warning) + << "No black level provided by camera sensor helper" + << ", please fix"; + blackLevel_ = kDefaultBlackLevel; + } else { + /* + * The helper reports the black level as a 16-bit value, while + * the ImgU OB grid expects it in units of half a 10-bit LSB. On + * an OV5670 (black level 64 at 10 bits) a correction of 64 + * leaves a residual pedestal of about 9/255 in the AWB + * statistics and in the image, 128 removes it, and 192 clips + * dark areas to zero. Experiments with the configurable data + * pedestal of an OV5693 confirm that the ISP needs twice the + * value programmed in the sensor's black level target. The + * 16-bit value is therefore shifted right by 5. + */ + blackLevel_ = *blackLevel >> 5; + } + + LOG(IPU3Blc, Debug) << "Black level " << blackLevel_; + + return 0; +} + /** * \brief Fill in the parameter structure, and enable black level correction * \param[in] context The shared IPA context @@ -49,15 +95,11 @@ void BlackLevelCorrection::prepare([[maybe_unused]] IPAContext &context, [[maybe_unused]] IPAFrameContext &frameContext, ipu3_uapi_params *params) { - /* - * The Optical Black Level correction values - * \todo The correction values should come from sensor specific - * tuning processes. This is a first rough approximation. - */ - params->obgrid_param.gr = 64; - params->obgrid_param.r = 64; - params->obgrid_param.b = 64; - params->obgrid_param.gb = 64; + /* The Optical Black Level correction values */ + params->obgrid_param.gr = blackLevel_; + params->obgrid_param.r = blackLevel_; + params->obgrid_param.b = blackLevel_; + params->obgrid_param.gb = blackLevel_; /* Enable the custom black level correction processing */ params->use.obgrid = 1; diff --git a/src/ipa/ipu3/algorithms/blc.h b/src/ipa/ipu3/algorithms/blc.h index 6274804..bef756f 100644 --- a/src/ipa/ipu3/algorithms/blc.h +++ b/src/ipa/ipu3/algorithms/blc.h @@ -18,9 +18,14 @@ class BlackLevelCorrection : public Algorithm public: BlackLevelCorrection(); + int init(IPAContext &context, const ValueNode &tuningData) override; + void prepare(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, ipu3_uapi_params *params) override; + +private: + uint16_t blackLevel_; }; } /* namespace ipa::ipu3::algorithms */
The black level correction algorithm hard-codes an optical black level of 64 for the four Bayer channels. Take it from the camera sensor helper instead, like the RkISP1 IPA does, falling back to the historical value with a warning when the helper does not provide one. The helper reports the level as a 16-bit value, while the ImgU OB grid expects units of half a 10-bit LSB: on a Dell Latitude 7275 (OV5670, black level 64 at 10 bits) a correction of 64 leaves a residual pedestal in the AWB statistics and in the image, which the colour gains then amplify into a blue or cyan cast; 128 removes the pedestal and 192 clips dark areas to zero. The 16-bit value is therefore shifted right by 5, giving 128 for the OV5670. Dan Scally independently reached the same conclusion on a Surface Go 2 (OV5693), where the ISP has to be given twice the data pedestal configured in the sensor for a dark image to come out dark. Measured on the same indoor scene with the uncalibrated tuning file (NV12, limited range, black = 16): with 64 the darkest percentile of Y is 32 and its mean chroma is U/V = 132/131; with 128 they are 15 and 128/128. Signed-off-by: D. Manresa <dmanresa@gmail.com> --- src/ipa/ipu3/algorithms/blc.cpp | 60 ++++++++++++++++++++++++++++----- src/ipa/ipu3/algorithms/blc.h | 5 +++ 2 files changed, 56 insertions(+), 9 deletions(-)