[PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera sensor helper
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jul 2 16:50:25 CEST 2024
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];
}
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