| Message ID | 20240701144122.3418955-4-stefan.klug@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan 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. > > Signed-off-by: Stefan Klug <stefan.klug@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"; > + > + 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); > + > + 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"; > + } > > tuningParameters_ = true; > > -- > 2.43.0 >
Quoting Jacopo Mondi (2024-07-02 09:00:44) > Hi Stefan > > 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. > > > > Signed-off-by: Stefan Klug <stefan.klug@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"; > > + > > + 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? 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? > > + > > + 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 ? > > + } > > > > tuningParameters_ = true; > > > > -- > > 2.43.0 > >
Hi Jacopo, On Tue, Jul 02, 2024 at 10:00:44AM +0200, Jacopo Mondi wrote: > Hi Stefan > > 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. > > > > Signed-off-by: Stefan Klug <stefan.klug@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"; > > + > > + if (!blackLevels || (tuningData.contains("R") && > > Is there any need to get context.camHelper->blackLevels() if the > values are in the config file ? The strategy now is to move away from the having the back levels in the config. After discussion with Laurent, libcamera should at least know the pedestal values. When we add additional measurements they could be added to the tuning file as offsets. So camHelper->blackLevels() is the default, the tuning file is kept for backwards compatibility and to be able to specify the values when libcamera doesn't have that data. > > > > + 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); > > + > > + if (blackLevels) > > Is this a Warning ? Same question above. I would Warn only if no BLC in > sensor helpers AND in config file. Yes, I would say so. Basically we are depricating the black levels from the tuning file and moving them into sensor helpers. The first one should be issued as it points to an incomplete implementation in libcamera. The second one only shows up, if the fisrt one didn't show up. Best regards, Stefan > > > + LOG(RkISP1Blc, Warning) > > + << "Black levels overwritten by tuning file"; > > + } > > > > tuningParameters_ = true; > > > > -- > > 2.43.0 > >
On Tue, Jul 02, 2024 at 11:51:03AM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi (2024-07-02 09:00:44) > > Hi Stefan > > > > 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. > > > > > > Signed-off-by: Stefan Klug <stefan.klug@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"; > > > + > > > + 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. > > 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?) Cheers, Stefan > > > > > + } > > > > > > tuningParameters_ = true; > > > > > > -- > > > 2.43.0 > > >
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@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; > > > >
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@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
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@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. > > 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; > > > > > >
Quoting Stefan Klug (2024-07-03 08:30:19) > 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@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. I didn't think we needed to maintain any backwards compatibilty here, as I thought we didn't have BLC fully implemented - but now I realise it was/is working on rk3399 ... so that's more awkward. It's 'unlikely' that anyone else is specifically using external tuning files, but in theory we don't know - and others could be. Then if we were going to do an ABI jump on libcamera, I would say it's even easier to justify breaking the previous values too ... but I was actually anticipating the next libcamera release in ~2 weeks to be the first opportunity to make a non-ABI breaking release! I'm so conflicted ;D We could also keep the backwards compatiblity for 'one' release if that helps with a warning, and clean the code up after the next libcamera tag? -- Kieran > > 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
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@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
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"; + + if (!blackLevels || (tuningData.contains("R") && + 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); + + if (blackLevels) + LOG(RkISP1Blc, Warning) + << "Black levels overwritten by tuning file"; + } tuningParameters_ = true;
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. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/algorithms/blc.cpp | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)