[PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera sensor helper
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jul 3 10:30:13 CEST 2024
Quoting Laurent Pinchart (2024-07-03 09:02:06)
> On Wed, Jul 03, 2024 at 09:30:19AM +0200, Stefan Klug wrote:
> > Hi Laurent,
> >
> > On Tue, Jul 02, 2024 at 05:50:25PM +0300, Laurent Pinchart wrote:
> > > On Tue, Jul 02, 2024 at 03:27:56PM +0200, Stefan Klug wrote:
> > > > On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote:
> > > > > Quoting Jacopo Mondi (2024-07-02 09:00:44)
> > > > > > On Mon, Jul 01, 2024 at 04:38:26PM GMT, Stefan Klug wrote:
> > > > > > > The black levels from the camera sensor helper are then used to do the
> > > > > > > black level correction. Black levels can still be overwritten by the
> > > > > > > tuning file.
> > >
> > > The commit message should be readable independently from the subject
> > > line. The first sentence here sounds weird when you do so. I feel
> > > obliged to point to the obligatory https://cbea.ms/git-commit/ and to
> > > emphasize the "why" of rule 7 :-)
> > >
> > > > > > >
> > > > > > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > > > > > ---
> > > > > > > src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++----
> > > > > > > 1 file changed, 24 insertions(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/ipa/rkisp1/algorithms/blc.cpp b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > > > > index d2e743541c99..0c39c3b47da5 100644
> > > > > > > --- a/src/ipa/rkisp1/algorithms/blc.cpp
> > > > > > > +++ b/src/ipa/rkisp1/algorithms/blc.cpp
> > > > > > > @@ -46,10 +46,30 @@ BlackLevelCorrection::BlackLevelCorrection()
> > > > > > > int BlackLevelCorrection::init([[maybe_unused]] IPAContext &context,
> > > > > > > const YamlObject &tuningData)
> > > > > > > {
> > > > > > > - blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> > > > > > > - blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> > > > > > > - blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> > > > > > > - blackLevelBlue_ = tuningData["B"].get<int16_t>(256);
> > > > > > > + auto blackLevels = context.camHelper->blackLevels();
> > > > > > > + if (blackLevels) {
> > > > > > > + Span<const int32_t, 4> levels = *blackLevels;
> > > > > > > + blackLevelRed_ = levels[0];
> > > > > > > + blackLevelGreenR_ = levels[1];
> > > > > > > + blackLevelGreenB_ = levels[2];
> > > > > > > + blackLevelBlue_ = levels[3];
> > > > > > > + } else
> > > > > > > + LOG(RkISP1Blc, Warning)
> > > > > > > + << "No black levels provided by camera sensor helper";
> > >
> > > << "No black levels provided by camera sensor helper, please fix";
> > >
> > > Another form of \todo. Maybe we ned a Fixme log level :-)
> > >
> > > > > > > +
> > > > > > > + if (!blackLevels || (tuningData.contains("R") &&
> > > > > >
> > > > > > Is there any need to get context.camHelper->blackLevels() if the
> > > > > > values are in the config file ?
> > > > > >
> > > > > > > + tuningData.contains("Gr") &&
> > > > > > > + tuningData.contains("Gb") &&
> > > > > > > + tuningData.contains("B"))) {
> > > > > > > + blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> > > > > > > + blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> > > > > > > + blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> > > > > > > + blackLevelBlue_ = tuningData["B"].get<int16_t>(256);
> > > > >
> > > > > Why 256 here? Does a value have to be specified?
> > > >
> > > > Digging into the git history... these were taken from the imx219 tuning
> > > > file (and scaled to 12bits) as sensible defaults. We could either keep
> > > > them or error out. My strategy was to stay compatible with existing
> > > > tuning files. If we go the "Either correct data or fail" route, we
> > > > should remove the defaults.
> > >
> > > We use defaults here in two cases:
> > >
> > > - The camera sensor helper doesn't provide black levels, so we fall back
> > > to the tuning file for backward compatibility. We should keep the
> > > current behaviour in that case.
> > >
> > > - The keys are provided in the tuning file but the data is incorrect.
> > > That's an error. We can error out, or pick a default, I don't mind
> > > much either way. Given that the first case points to keeping the
> > > current behaviour, using the same defaults without printing any
> > > message seems the simplest option.
> > >
> > > > > Maybe we should move a parse black level helper out to some common yaml
> > > > > helpers (Do we have anywhere for that yet, that can read defined
> > > > > libcamera types from yaml?) ... and it should only return all values if
> > > > > all of them are present and parsed correctly - otherwise R=3200, Gr=256,
> > > > > Gb=256, B=256 probably wouldn't make sense?
> > > >
> > > > No that wouldn't make much sense, but is the old behaviour :-). I can
> > > > have a look there.
> > > >
> > > > > > > +
> > > > > > > + if (blackLevels)
> > > > > >
> > > > > > Is this a Warning ? Same question above. I would Warn only if no BLC in
> > > > > > sensor helpers AND in config file.
> > > > > >
> > > > > > > + LOG(RkISP1Blc, Warning)
> > > > > > > + << "Black levels overwritten by tuning file";
> > > > >
> > > > > Yes, I think the tuning file should take precedence, so I don't think
> > > > > it's a warning. Perhaps a debug print to say which values get loaded and
> > > > > where from ?
> > > >
> > > > As said in my answer to Jacopo, the warning was on purpose, but I'm fine
> > > > with making it a debug output (or info?)
> > >
> > > Maybe we can make it clearer:
> > >
> > > LOG(RkISP1Blc, Warning)
> > > << "Deprecated: black levels overwritten by tuning file";
> > >
> > > Do we need a Deprecated log level ? :-)
> > >
> > > And now that I wrote that, I wonder if we need to preserve this
> > > behaviour. The goal, when I discussed it with Stefan, was to avoid
> > > breaking existing setups using out-of-tree tuning data that contains
> > > black levels different from the data pedestal provided by the camera
> > > sensor helpers. One reason for doing is is to take dark currents into
> > > account, and I'm not sure anyone would have done this kind of tuning
> > > given that we didn't provide any tooling to measure the black level.
> > > Another reason would be to tweak the values "just because". Given how
> > > we're evolving the black level compensation implementation (not just in
> > > the algorithm but also in the tuning tools), I'm tempted to say we
> > > shouldn't preserve backward compatibility here.
> > >
> > > Also, would the following express the logic more clearly ?
> > >
> > > std::optional<int16_t> levelRed = tuningData.contains("R").get<int16_t>;
> > > std::optional<int16_t> levelGreenR = tuningData.contains("Gr").get<int16_t>;
> > > std::optional<int16_t> levelGreenB = tuningData.contains("Gb").get<int16_t>;
> > > std::optional<int16_t> levelBlue = tuningData.contains("B").get<int16_t>;
> > >
> > > bool tuningHasLevels = levelRed && levelGreenR && levelGreenB && levelBlue;
> > >
> > > auto blackLevels = context.camHelper->blackLevels();
> > >
> > > if (!blackLevels) {
> > > /*
> > > * Not all camera sensor helpers have been updated with black
> > > * levels. Print a warning and fall back to the levels from the
> > > * tuning data to preserve backward compatibility. This should
> > > * be removed once allhelpers provide the data.
> > > */
> > > LOG(RkISP1Blc, Warning)
> > > << "No black levels provided by camera sensor helper, please fix";
> > >
> > > blackLevelRed_ = levelRed.value_or(256);
> > > blackLevelGreenR_ = levelGreenR.value_or(256);
> > > blackLevelGreenB_ = levelGreenB.value_or(256);
> > > blackLevelBlue_ = levelBlue.value_or(256);
> > > } else if (tuningHasLevels) {
> > > /*
> > > * If black levels are provided in the tuning file, use them to
> > > * avoid breaking existing camera tuning. This is deprecated and
> > > * will be removed.
> > > */
> > > LOG(RkISP1Blc, Warning)
> > > << "Deprecated: black levels overwritten by tuning file";
> > >
> > > blackLevelRed_ = *levelRed;
> > > blackLevelGreenR_ = *levelGreenR;
> > > blackLevelGreenB_ = *levelGreenB;
> > > blackLevelBlue_ = *levelBlue;
> > > } else {
> > > Span<const int32_t, 4> levels = *blackLevels;
> > > blackLevelRed_ = levels[0];
> > > blackLevelGreenR_ = levels[1];
> > > blackLevelGreenB_ = levels[2];
> > > blackLevelBlue_ = levels[3];
> > > }
> >
> > I like that proposal. It is definitely easier to follow. While
> > implementing that, I realized that we are breaking backwards
> > compatibility anyways as we should interpret the values differently
> > (based on 16bit width instead of 12bit). To stay really backwards
> > compatible we could add a "referenceBitDepth" field to the tuning data
> > and fall back to 12bit if that is not set (I implemented that in the
> > companding branch). But honestly I would rather switch to 16bit and
> > break the old files instead of carrying around that additional flag.
> >
> > When we break the old files, we could directly go to the solution you
> > proposed below.
> >
> > Opinions?
>
> I'm fine with that, but I think we should then add black levels to the
> camera sensor helpers for all sensors that currently have the levels set
> in tuning files. That's OV4689, OV5640 and IMX219.
>
To be clear, following my reply on this too - I don't object to going
straight to the 'right' implementation here.
Storing Black levels as 16 bit consistently makes more sense to me than
varied bit depths.
--
Kieran
> > > If we decide to drop the second case then it could be simplified to
> > >
> > > auto blackLevels = context.camHelper->blackLevels();
> > >
> > > if (blackLevels) {
> > > Span<const int32_t, 4> levels = *blackLevels;
> > > blackLevelRed_ = levels[0];
> > > blackLevelGreenR_ = levels[1];
> > > blackLevelGreenB_ = levels[2];
> > > blackLevelBlue_ = levels[3];
> > > } else {
> > > /*
> > > * Not all camera sensor helpers have been updated with black
> > > * levels. Print a warning and fall back to the levels from the
> > > * tuning data to preserve backward compatibility. This should
> > > * be removed once allhelpers provide the data.
> > > */
> > > LOG(RkISP1Blc, Warning)
> > > << "No black levels provided by camera sensor helper, please fix";
> > >
> > > blackLevelRed_ = tuningData["R"].get<int16_t>(256);
> > > blackLevelGreenR_ = tuningData["Gr"].get<int16_t>(256);
> > > blackLevelGreenB_ = tuningData["Gb"].get<int16_t>(256);
> > > blackLevelBlue_ = tuningData["B"].get<int16_t>(256);
> > > }
> > >
> > > > > > > + }
> > > > > > >
> > > > > > > tuningParameters_ = true;
> > > > > > >
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list