[PATCH 3/5] ipa: rkisp1: blc: Query black levels from camera sensor helper
Stefan Klug
stefan.klug at ideasonboard.com
Wed Jul 3 09:30:19 CEST 2024
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?
Best regards,
Stefan
>
> 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