| Message ID | 20260805183517.228901-4-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 08. 05. 20:35 keltezéssel, Milan Zamazal írta: > From: Xander Pronk <xander.c.pronk@gmail.com> > > The algorithm is based on the common libipa lens shading correction > implementation. The grid values obtained from the libipa algorithm are > passed to the debayer algorithm as an array and used as an an RGB > texture. > > Notes on the implementation: > > - The overall idea is to keep things simple, to not make the LSC > computation unnecessarily expensive. > > - LscAlgorithm accepts only quantised types. UQ<2,6> is used, to be > converted to float in debayering. > > - The limit of 100 degrees to consider a temperature change noticeable > is arbitrary. > > Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info> > Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info> > Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com> > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > src/ipa/simple/algorithms/lsc.cpp | 98 +++++++++++++++++++++++++++ > src/ipa/simple/algorithms/lsc.h | 55 +++++++++++++++ > src/ipa/simple/algorithms/meson.build | 1 + > src/ipa/simple/ipa_context.h | 5 ++ > 4 files changed, 159 insertions(+) > create mode 100644 src/ipa/simple/algorithms/lsc.cpp > create mode 100644 src/ipa/simple/algorithms/lsc.h > > diff --git a/src/ipa/simple/algorithms/lsc.cpp b/src/ipa/simple/algorithms/lsc.cpp > new file mode 100644 > index 000000000..02d8cf48c > --- /dev/null > +++ b/src/ipa/simple/algorithms/lsc.cpp > @@ -0,0 +1,98 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Lens shading correction > + */ > + > +#include "lsc.h" > + > +#include <libcamera/base/log.h> > + > +namespace libcamera { > + > +namespace ipa::soft::algorithms { > + > +LOG_DEFINE_CATEGORY(IPASoftLsc) > + > +int Lsc::init(IPAContext &context, const ValueNode &tuningData) > +{ > + static constexpr unsigned int kGridSize = DebayerParams::kLscGridSize; > + > + for (unsigned int i = 0; i < kGridSize; i++) > + gridPos_.push_back(static_cast<double>(i) / (kGridSize - 1)); > + > + return lscAlgo_.init(tuningData, context.ctrlMap, > + { .keys = { "r", "g", "b" }, > + .numHSamples = kGridSize, > + .numVSamples = kGridSize, > + .sensorSize = context.sensorInfo.activeAreaSize }); > +} > + > +int Lsc::configure(IPAContext &context, > + [[maybe_unused]] const IPAConfigInfo &configInfo) > +{ > + return lscAlgo_.configure(context.activeState.lsc, > + context.sensorInfo.analogCrop, > + gridPos_, gridPos_); > +} > + > +void Lsc::prepare([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, > + DebayerParams *params) > +{ > + unsigned int ct = frameContext.awb.colourTemperature; > + constexpr unsigned int minTemperatureChange = 100; > + > + if (!frameContext.lsc.enabled) { > + if (lastAppliedCt_ != 0 || params->lscLutVersion == 0) { Is this sufficient to handle start-stop sequences, (re)configurations? As mentioned elsewhere, the handling of `lscLutVersion` does not seem entirely robust to me, maybe I'm missing something? > + params->lscLut.fill(64); /* UQ<2, 6>(64) == 1.0 */ > + params->lscLutVersion++; > + lastAppliedCt_ = 0; > + } > + return; > + } > + > + if (utils::abs_diff(ct, lastAppliedCt_) < minTemperatureChange) > + return; > + > + const auto &set = lscAlgo_.interpolateComponents(ct); > + > + const auto &red = set.at("r"); > + const auto &green = set.at("g"); > + const auto &blue = set.at("b"); > + > + DebayerParams::LscLookupTable lut; Cam the copy be avoided by using a reference here? > + constexpr unsigned int gridSize = DebayerParams::kLscGridSize; > + for (unsigned int i = 0, j = 0; i < gridSize * gridSize; i++) { > + lut[j++] = red[i]; > + lut[j++] = green[i]; > + lut[j++] = blue[i]; > + lut[j++] = 0; /* padding */ > + } > + params->lscLut = lut; > + params->lscLutVersion++; > + > + lastAppliedCt_ = ct; > +} > + > +void Lsc::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, const ControlList &controls) > +{ > + lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc, > + controls); > +} > + > +void Lsc::process([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + IPAFrameContext &frameContext, > + [[maybe_unused]] const SwIspStats *stats, > + ControlList &metadata) > +{ > + lscAlgo_.process(frameContext.lsc, metadata); > +} > + > +REGISTER_IPA_ALGORITHM(Lsc, "Lsc") > + > +} /* namespace ipa::soft::algorithms */ > + > +} /* namespace libcamera */ > [...]
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 08. 05. 20:35 keltezéssel, Milan Zamazal írta: >> From: Xander Pronk <xander.c.pronk@gmail.com> >> The algorithm is based on the common libipa lens shading correction >> implementation. The grid values obtained from the libipa algorithm are >> passed to the debayer algorithm as an array and used as an an RGB >> texture. >> Notes on the implementation: >> - The overall idea is to keep things simple, to not make the LSC >> computation unnecessarily expensive. >> - LscAlgorithm accepts only quantised types. UQ<2,6> is used, to be >> converted to float in debayering. >> - The limit of 100 degrees to consider a temperature change noticeable >> is arbitrary. >> Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info> >> Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info> >> Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> src/ipa/simple/algorithms/lsc.cpp | 98 +++++++++++++++++++++++++++ >> src/ipa/simple/algorithms/lsc.h | 55 +++++++++++++++ >> src/ipa/simple/algorithms/meson.build | 1 + >> src/ipa/simple/ipa_context.h | 5 ++ >> 4 files changed, 159 insertions(+) >> create mode 100644 src/ipa/simple/algorithms/lsc.cpp >> create mode 100644 src/ipa/simple/algorithms/lsc.h >> diff --git a/src/ipa/simple/algorithms/lsc.cpp b/src/ipa/simple/algorithms/lsc.cpp >> new file mode 100644 >> index 000000000..02d8cf48c >> --- /dev/null >> +++ b/src/ipa/simple/algorithms/lsc.cpp >> @@ -0,0 +1,98 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Lens shading correction >> + */ >> + >> +#include "lsc.h" >> + >> +#include <libcamera/base/log.h> >> + >> +namespace libcamera { >> + >> +namespace ipa::soft::algorithms { >> + >> +LOG_DEFINE_CATEGORY(IPASoftLsc) >> + >> +int Lsc::init(IPAContext &context, const ValueNode &tuningData) >> +{ >> + static constexpr unsigned int kGridSize = DebayerParams::kLscGridSize; >> + >> + for (unsigned int i = 0; i < kGridSize; i++) >> + gridPos_.push_back(static_cast<double>(i) / (kGridSize - 1)); >> + >> + return lscAlgo_.init(tuningData, context.ctrlMap, >> + { .keys = { "r", "g", "b" }, >> + .numHSamples = kGridSize, >> + .numVSamples = kGridSize, >> + .sensorSize = context.sensorInfo.activeAreaSize }); >> +} >> + >> +int Lsc::configure(IPAContext &context, >> + [[maybe_unused]] const IPAConfigInfo &configInfo) >> +{ >> + return lscAlgo_.configure(context.activeState.lsc, >> + context.sensorInfo.analogCrop, >> + gridPos_, gridPos_); >> +} >> + >> +void Lsc::prepare([[maybe_unused]] IPAContext &context, >> + [[maybe_unused]] const uint32_t frame, >> + IPAFrameContext &frameContext, >> + DebayerParams *params) >> +{ >> + unsigned int ct = frameContext.awb.colourTemperature; >> + constexpr unsigned int minTemperatureChange = 100; >> + >> + if (!frameContext.lsc.enabled) { >> + if (lastAppliedCt_ != 0 || params->lscLutVersion == 0) { > > Is this sufficient to handle start-stop sequences, (re)configurations? This resets the LUT to no-op in case LSC is not enabled and either the LUT may not be initialised at all (params->lscLutVersion == 0) or non-zero lastAppliedCt_ indicates the LSC is not set to the no-op values. (We never actually disable LSC, we set the table to no-change values instead.) The version is incremented to signal the change to the debayering. I can't see a problem here. > As mentioned elsewhere, the handling of `lscLutVersion` does not seem > entirely robust to me, maybe I'm missing something? On the IPA side, lscLutVersion is reset only on init (it's not reset in this version of the patch, but I think it should be, so I'll change it in v12). On the debayering side, its stored lscLutVersion_ will be changed in v12 to std::optional and set to an empty value initially and on stop, so that the texture is re-created when needed. Debayering must be reinitialised on init (a new instance created). No version collision is possible. Can you still see anything suspicious? > >> + params->lscLut.fill(64); /* UQ<2, 6>(64) == 1.0 */ >> + params->lscLutVersion++; >> + lastAppliedCt_ = 0; >> + } >> + return; >> + } >> + >> + if (utils::abs_diff(ct, lastAppliedCt_) < minTemperatureChange) >> + return; >> + >> + const auto &set = lscAlgo_.interpolateComponents(ct); >> + >> + const auto &red = set.at("r"); >> + const auto &green = set.at("g"); >> + const auto &blue = set.at("b"); >> + >> + DebayerParams::LscLookupTable lut; > > Cam the copy be avoided by using a reference here? Yes. >> + constexpr unsigned int gridSize = DebayerParams::kLscGridSize; >> + for (unsigned int i = 0, j = 0; i < gridSize * gridSize; i++) { >> + lut[j++] = red[i]; >> + lut[j++] = green[i]; >> + lut[j++] = blue[i]; >> + lut[j++] = 0; /* padding */ >> + } >> + params->lscLut = lut; >> + params->lscLutVersion++; >> + >> + lastAppliedCt_ = ct; >> +} >> + >> +void Lsc::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, >> + IPAFrameContext &frameContext, const ControlList &controls) >> +{ >> + lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc, >> + controls); >> +} >> + >> +void Lsc::process([[maybe_unused]] IPAContext &context, >> + [[maybe_unused]] const uint32_t frame, >> + IPAFrameContext &frameContext, >> + [[maybe_unused]] const SwIspStats *stats, >> + ControlList &metadata) >> +{ >> + lscAlgo_.process(frameContext.lsc, metadata); >> +} >> + >> +REGISTER_IPA_ALGORITHM(Lsc, "Lsc") >> + >> +} /* namespace ipa::soft::algorithms */ >> + >> +} /* namespace libcamera */ >> [...]
2026. 08. 06. 20:24 keltezéssel, Milan Zamazal írta: > Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > >> 2026. 08. 05. 20:35 keltezéssel, Milan Zamazal írta: >>> From: Xander Pronk <xander.c.pronk@gmail.com> >>> The algorithm is based on the common libipa lens shading correction >>> implementation. The grid values obtained from the libipa algorithm are >>> passed to the debayer algorithm as an array and used as an an RGB >>> texture. >>> Notes on the implementation: >>> - The overall idea is to keep things simple, to not make the LSC >>> computation unnecessarily expensive. >>> - LscAlgorithm accepts only quantised types. UQ<2,6> is used, to be >>> converted to float in debayering. >>> - The limit of 100 degrees to consider a temperature change noticeable >>> is arbitrary. >>> Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info> >>> Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info> >>> Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com> >>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>> --- >>> src/ipa/simple/algorithms/lsc.cpp | 98 +++++++++++++++++++++++++++ >>> src/ipa/simple/algorithms/lsc.h | 55 +++++++++++++++ >>> src/ipa/simple/algorithms/meson.build | 1 + >>> src/ipa/simple/ipa_context.h | 5 ++ >>> 4 files changed, 159 insertions(+) >>> create mode 100644 src/ipa/simple/algorithms/lsc.cpp >>> create mode 100644 src/ipa/simple/algorithms/lsc.h >>> diff --git a/src/ipa/simple/algorithms/lsc.cpp b/src/ipa/simple/algorithms/lsc.cpp >>> new file mode 100644 >>> index 000000000..02d8cf48c >>> --- /dev/null >>> +++ b/src/ipa/simple/algorithms/lsc.cpp >>> @@ -0,0 +1,98 @@ >>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>> +/* >>> + * Lens shading correction >>> + */ >>> + >>> +#include "lsc.h" >>> + >>> +#include <libcamera/base/log.h> >>> + >>> +namespace libcamera { >>> + >>> +namespace ipa::soft::algorithms { >>> + >>> +LOG_DEFINE_CATEGORY(IPASoftLsc) >>> + >>> +int Lsc::init(IPAContext &context, const ValueNode &tuningData) >>> +{ >>> + static constexpr unsigned int kGridSize = DebayerParams::kLscGridSize; >>> + >>> + for (unsigned int i = 0; i < kGridSize; i++) >>> + gridPos_.push_back(static_cast<double>(i) / (kGridSize - 1)); >>> + >>> + return lscAlgo_.init(tuningData, context.ctrlMap, >>> + { .keys = { "r", "g", "b" }, >>> + .numHSamples = kGridSize, >>> + .numVSamples = kGridSize, >>> + .sensorSize = context.sensorInfo.activeAreaSize }); >>> +} >>> + >>> +int Lsc::configure(IPAContext &context, >>> + [[maybe_unused]] const IPAConfigInfo &configInfo) >>> +{ >>> + return lscAlgo_.configure(context.activeState.lsc, >>> + context.sensorInfo.analogCrop, >>> + gridPos_, gridPos_); >>> +} >>> + >>> +void Lsc::prepare([[maybe_unused]] IPAContext &context, >>> + [[maybe_unused]] const uint32_t frame, >>> + IPAFrameContext &frameContext, >>> + DebayerParams *params) >>> +{ >>> + unsigned int ct = frameContext.awb.colourTemperature; >>> + constexpr unsigned int minTemperatureChange = 100; >>> + >>> + if (!frameContext.lsc.enabled) { >>> + if (lastAppliedCt_ != 0 || params->lscLutVersion == 0) { >> >> Is this sufficient to handle start-stop sequences, (re)configurations? > > This resets the LUT to no-op in case LSC is not enabled and either the > LUT may not be initialised at all (params->lscLutVersion == 0) or I see, I think we should put that initialization into `DebayerParams`, for example: struct DebayerParams { ... static constexpr auto identityLscLut = [] { LscLookupTable ret = {}; for (size_t i = 0; kLscValuesPerCell * i < ret.size(); i++) { ret[i * kLscValuesPerCell + 0] = 64; /* == UQ<2, 6>(1.0f).quantized() */ ret[i * kLscValuesPerCell + 1] = 64; ret[i * kLscValuesPerCell + 2] = 64; } return ret; }(); LscLookupTable lscLut = identityLscLut; uint64_t lscLutVersion = 0; }; this should remove the need for the `params->lscLutVersion == 0` branch. Also see commit 0d3e543b4d6690f65070b95450aecb5792d0d51f, why I think this initialization should be here and not in `IPASoftSimple::init()`. And then you can simply do `params->lscLut = DebayerParams::identityLscLut;` as well. > non-zero lastAppliedCt_ indicates the LSC is not set to the no-op > values. (We never actually disable LSC, we set the table to no-change > values instead.) The version is incremented to signal the change to the > debayering. I can't see a problem here. > >> As mentioned elsewhere, the handling of `lscLutVersion` does not seem >> entirely robust to me, maybe I'm missing something? > > On the IPA side, lscLutVersion is reset only on init (it's not reset in > this version of the patch, but I think it should be, so I'll change it > in v12). The initializer in `DebayerParams` should take care of that, no? > > On the debayering side, its stored lscLutVersion_ will be changed in v12 > to std::optional and set to an empty value initially and on stop, so > that the texture is re-created when needed. Debayering must be > reinitialised on init (a new instance created). No version collision is > possible. > > Can you still see anything suspicious? I think I have convinced myself it should work. Although I can see that the lack of v4l2_isp-like parameter buffers is making things more difficult. I was about to suggest some kind of reset of `lastAppliedCt_` in start/stop, but then realized that it must not be reset and must instead be "persistent" to always be able to track the state of the lsc lut in the single parameter buffer, if I'm not mistaken? > >> >>> + params->lscLut.fill(64); /* UQ<2, 6>(64) == 1.0 */ >>> + params->lscLutVersion++; >>> + lastAppliedCt_ = 0; >>> + } >>> + return; >>> + } >>> + >>> + if (utils::abs_diff(ct, lastAppliedCt_) < minTemperatureChange) >>> + return; >>> + >>> + const auto &set = lscAlgo_.interpolateComponents(ct); >>> + >>> + const auto &red = set.at("r"); >>> + const auto &green = set.at("g"); >>> + const auto &blue = set.at("b"); >>> + >>> + DebayerParams::LscLookupTable lut; >> >> Cam the copy be avoided by using a reference here? > > Yes. > >>> + constexpr unsigned int gridSize = DebayerParams::kLscGridSize; >>> + for (unsigned int i = 0, j = 0; i < gridSize * gridSize; i++) { >>> + lut[j++] = red[i]; >>> + lut[j++] = green[i]; >>> + lut[j++] = blue[i]; >>> + lut[j++] = 0; /* padding */ >>> + } >>> + params->lscLut = lut; >>> + params->lscLutVersion++; >>> + >>> + lastAppliedCt_ = ct; >>> +} >>> + >>> +void Lsc::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, >>> + IPAFrameContext &frameContext, const ControlList &controls) >>> +{ >>> + lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc, >>> + controls); >>> +} >>> + >>> +void Lsc::process([[maybe_unused]] IPAContext &context, >>> + [[maybe_unused]] const uint32_t frame, >>> + IPAFrameContext &frameContext, >>> + [[maybe_unused]] const SwIspStats *stats, >>> + ControlList &metadata) >>> +{ >>> + lscAlgo_.process(frameContext.lsc, metadata); >>> +} >>> + >>> +REGISTER_IPA_ALGORITHM(Lsc, "Lsc") >>> + >>> +} /* namespace ipa::soft::algorithms */ >>> + >>> +} /* namespace libcamera */ >>> [...] >
Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: > 2026. 08. 06. 20:24 keltezéssel, Milan Zamazal írta: >> Barnabás Pőcze <barnabas.pocze@ideasonboard.com> writes: >> >>> 2026. 08. 05. 20:35 keltezéssel, Milan Zamazal írta: >>>> From: Xander Pronk <xander.c.pronk@gmail.com> >>>> The algorithm is based on the common libipa lens shading correction >>>> implementation. The grid values obtained from the libipa algorithm are >>>> passed to the debayer algorithm as an array and used as an an RGB >>>> texture. >>>> Notes on the implementation: >>>> - The overall idea is to keep things simple, to not make the LSC >>>> computation unnecessarily expensive. >>>> - LscAlgorithm accepts only quantised types. UQ<2,6> is used, to be >>>> converted to float in debayering. >>>> - The limit of 100 degrees to consider a temperature change noticeable >>>> is arbitrary. >>>> Co-developed-by: Rick ten Wolde <rick_libcamera@wolde.info> >>>> Signed-off-by: Rick ten Wolde <rick_libcamera@wolde.info> >>>> Signed-off-by: Xander Pronk <xander.c.pronk@gmail.com> >>>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >>>> --- >>>> src/ipa/simple/algorithms/lsc.cpp | 98 +++++++++++++++++++++++++++ >>>> src/ipa/simple/algorithms/lsc.h | 55 +++++++++++++++ >>>> src/ipa/simple/algorithms/meson.build | 1 + >>>> src/ipa/simple/ipa_context.h | 5 ++ >>>> 4 files changed, 159 insertions(+) >>>> create mode 100644 src/ipa/simple/algorithms/lsc.cpp >>>> create mode 100644 src/ipa/simple/algorithms/lsc.h >>>> diff --git a/src/ipa/simple/algorithms/lsc.cpp b/src/ipa/simple/algorithms/lsc.cpp >>>> new file mode 100644 >>>> index 000000000..02d8cf48c >>>> --- /dev/null >>>> +++ b/src/ipa/simple/algorithms/lsc.cpp >>>> @@ -0,0 +1,98 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Lens shading correction >>>> + */ >>>> + >>>> +#include "lsc.h" >>>> + >>>> +#include <libcamera/base/log.h> >>>> + >>>> +namespace libcamera { >>>> + >>>> +namespace ipa::soft::algorithms { >>>> + >>>> +LOG_DEFINE_CATEGORY(IPASoftLsc) >>>> + >>>> +int Lsc::init(IPAContext &context, const ValueNode &tuningData) >>>> +{ >>>> + static constexpr unsigned int kGridSize = DebayerParams::kLscGridSize; >>>> + >>>> + for (unsigned int i = 0; i < kGridSize; i++) >>>> + gridPos_.push_back(static_cast<double>(i) / (kGridSize - 1)); >>>> + >>>> + return lscAlgo_.init(tuningData, context.ctrlMap, >>>> + { .keys = { "r", "g", "b" }, >>>> + .numHSamples = kGridSize, >>>> + .numVSamples = kGridSize, >>>> + .sensorSize = context.sensorInfo.activeAreaSize }); >>>> +} >>>> + >>>> +int Lsc::configure(IPAContext &context, >>>> + [[maybe_unused]] const IPAConfigInfo &configInfo) >>>> +{ >>>> + return lscAlgo_.configure(context.activeState.lsc, >>>> + context.sensorInfo.analogCrop, >>>> + gridPos_, gridPos_); >>>> +} >>>> + >>>> +void Lsc::prepare([[maybe_unused]] IPAContext &context, >>>> + [[maybe_unused]] const uint32_t frame, >>>> + IPAFrameContext &frameContext, >>>> + DebayerParams *params) >>>> +{ >>>> + unsigned int ct = frameContext.awb.colourTemperature; >>>> + constexpr unsigned int minTemperatureChange = 100; >>>> + >>>> + if (!frameContext.lsc.enabled) { >>>> + if (lastAppliedCt_ != 0 || params->lscLutVersion == 0) { >>> >>> Is this sufficient to handle start-stop sequences, (re)configurations? >> This resets the LUT to no-op in case LSC is not enabled and either the >> LUT may not be initialised at all (params->lscLutVersion == 0) or > > I see, I think we should put that initialization into `DebayerParams`, > for example: > > struct DebayerParams { > ... > static constexpr auto identityLscLut = [] { > LscLookupTable ret = {}; > for (size_t i = 0; kLscValuesPerCell * i < ret.size(); i++) { > ret[i * kLscValuesPerCell + 0] = 64; /* == UQ<2, 6>(1.0f).quantized() */ > ret[i * kLscValuesPerCell + 1] = 64; > ret[i * kLscValuesPerCell + 2] = 64; > } > return ret; > }(); > LscLookupTable lscLut = identityLscLut; > uint64_t lscLutVersion = 0; > }; > > this should remove the need for the `params->lscLutVersion == 0` branch. > Also see commit 0d3e543b4d6690f65070b95450aecb5792d0d51f, why I think > this initialization should be here and not in `IPASoftSimple::init()`. > > And then you can simply do `params->lscLut = DebayerParams::identityLscLut;` as well. Better, indeed, thank you for the tip. >> non-zero lastAppliedCt_ indicates the LSC is not set to the no-op >> values. (We never actually disable LSC, we set the table to no-change >> values instead.) The version is incremented to signal the change to the >> debayering. I can't see a problem here. >> >>> As mentioned elsewhere, the handling of `lscLutVersion` does not seem >>> entirely robust to me, maybe I'm missing something? >> On the IPA side, lscLutVersion is reset only on init (it's not reset in >> this version of the patch, but I think it should be, so I'll change it >> in v12). > > The initializer in `DebayerParams` should take care of that, no? Honestly, I'm not sure what and when all the instances are recreated. Better to reset all the parameters at the same place. If it's not needed, let's change it separately. >> On the debayering side, its stored lscLutVersion_ will be changed in v12 >> to std::optional and set to an empty value initially and on stop, so >> that the texture is re-created when needed. Debayering must be >> reinitialised on init (a new instance created). No version collision is >> possible. >> Can you still see anything suspicious? > > I think I have convinced myself it should work. Although I can see that > the lack of v4l2_isp-like parameter buffers is making things more difficult. > > I was about to suggest some kind of reset of `lastAppliedCt_` in start/stop, > but then realized that it must not be reset and must instead be "persistent" to > always be able to track the state of the lsc lut in the single parameter buffer, > if I'm not mistaken? You're right. Start/stop in software ISP resets just the frame contexts; IIRC it's intentional to keep the IPA state unchanged over stop+start. >> >>> >>>> + params->lscLut.fill(64); /* UQ<2, 6>(64) == 1.0 */ >>>> + params->lscLutVersion++; >>>> + lastAppliedCt_ = 0; >>>> + } >>>> + return; >>>> + } >>>> + >>>> + if (utils::abs_diff(ct, lastAppliedCt_) < minTemperatureChange) >>>> + return; >>>> + >>>> + const auto &set = lscAlgo_.interpolateComponents(ct); >>>> + >>>> + const auto &red = set.at("r"); >>>> + const auto &green = set.at("g"); >>>> + const auto &blue = set.at("b"); >>>> + >>>> + DebayerParams::LscLookupTable lut; >>> >>> Cam the copy be avoided by using a reference here? >> Yes. >> >>>> + constexpr unsigned int gridSize = DebayerParams::kLscGridSize; >>>> + for (unsigned int i = 0, j = 0; i < gridSize * gridSize; i++) { >>>> + lut[j++] = red[i]; >>>> + lut[j++] = green[i]; >>>> + lut[j++] = blue[i]; >>>> + lut[j++] = 0; /* padding */ >>>> + } >>>> + params->lscLut = lut; >>>> + params->lscLutVersion++; >>>> + >>>> + lastAppliedCt_ = ct; >>>> +} >>>> + >>>> +void Lsc::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, >>>> + IPAFrameContext &frameContext, const ControlList &controls) >>>> +{ >>>> + lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc, >>>> + controls); >>>> +} >>>> + >>>> +void Lsc::process([[maybe_unused]] IPAContext &context, >>>> + [[maybe_unused]] const uint32_t frame, >>>> + IPAFrameContext &frameContext, >>>> + [[maybe_unused]] const SwIspStats *stats, >>>> + ControlList &metadata) >>>> +{ >>>> + lscAlgo_.process(frameContext.lsc, metadata); >>>> +} >>>> + >>>> +REGISTER_IPA_ALGORITHM(Lsc, "Lsc") >>>> + >>>> +} /* namespace ipa::soft::algorithms */ >>>> + >>>> +} /* namespace libcamera */ >>>> [...] >>
diff --git a/src/ipa/simple/algorithms/lsc.cpp b/src/ipa/simple/algorithms/lsc.cpp new file mode 100644 index 000000000..02d8cf48c --- /dev/null +++ b/src/ipa/simple/algorithms/lsc.cpp @@ -0,0 +1,98 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Lens shading correction + */ + +#include "lsc.h" + +#include <libcamera/base/log.h> + +namespace libcamera { + +namespace ipa::soft::algorithms { + +LOG_DEFINE_CATEGORY(IPASoftLsc) + +int Lsc::init(IPAContext &context, const ValueNode &tuningData) +{ + static constexpr unsigned int kGridSize = DebayerParams::kLscGridSize; + + for (unsigned int i = 0; i < kGridSize; i++) + gridPos_.push_back(static_cast<double>(i) / (kGridSize - 1)); + + return lscAlgo_.init(tuningData, context.ctrlMap, + { .keys = { "r", "g", "b" }, + .numHSamples = kGridSize, + .numVSamples = kGridSize, + .sensorSize = context.sensorInfo.activeAreaSize }); +} + +int Lsc::configure(IPAContext &context, + [[maybe_unused]] const IPAConfigInfo &configInfo) +{ + return lscAlgo_.configure(context.activeState.lsc, + context.sensorInfo.analogCrop, + gridPos_, gridPos_); +} + +void Lsc::prepare([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, + DebayerParams *params) +{ + unsigned int ct = frameContext.awb.colourTemperature; + constexpr unsigned int minTemperatureChange = 100; + + if (!frameContext.lsc.enabled) { + if (lastAppliedCt_ != 0 || params->lscLutVersion == 0) { + params->lscLut.fill(64); /* UQ<2, 6>(64) == 1.0 */ + params->lscLutVersion++; + lastAppliedCt_ = 0; + } + return; + } + + if (utils::abs_diff(ct, lastAppliedCt_) < minTemperatureChange) + return; + + const auto &set = lscAlgo_.interpolateComponents(ct); + + const auto &red = set.at("r"); + const auto &green = set.at("g"); + const auto &blue = set.at("b"); + + DebayerParams::LscLookupTable lut; + constexpr unsigned int gridSize = DebayerParams::kLscGridSize; + for (unsigned int i = 0, j = 0; i < gridSize * gridSize; i++) { + lut[j++] = red[i]; + lut[j++] = green[i]; + lut[j++] = blue[i]; + lut[j++] = 0; /* padding */ + } + params->lscLut = lut; + params->lscLutVersion++; + + lastAppliedCt_ = ct; +} + +void Lsc::queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, const ControlList &controls) +{ + lscAlgo_.queueRequest(context.activeState.lsc, frameContext.lsc, + controls); +} + +void Lsc::process([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, + [[maybe_unused]] const SwIspStats *stats, + ControlList &metadata) +{ + lscAlgo_.process(frameContext.lsc, metadata); +} + +REGISTER_IPA_ALGORITHM(Lsc, "Lsc") + +} /* namespace ipa::soft::algorithms */ + +} /* namespace libcamera */ diff --git a/src/ipa/simple/algorithms/lsc.h b/src/ipa/simple/algorithms/lsc.h new file mode 100644 index 000000000..13d9f2bca --- /dev/null +++ b/src/ipa/simple/algorithms/lsc.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Lens shading correction + */ + +#pragma once + +#include <libipa/interpolator.h> + +#include "libipa/fixedpoint.h" +#include "libipa/lsc.h" + +#include "algorithm.h" +#include "ipa_context.h" + +namespace libcamera { + +namespace ipa { + +namespace soft::algorithms { + +class Lsc : public Algorithm +{ +public: + Lsc() = default; + ~Lsc() = default; + + int init(IPAContext &context, const ValueNode &tuningData) override; + int configure(IPAContext &context, + const IPAConfigInfo &configInfo) override; + void queueRequest(IPAContext &context, [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, const ControlList &controls) override; + void prepare(IPAContext &context, + const uint32_t frame, + IPAFrameContext &frameContext, + DebayerParams *params) override; + void process([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const uint32_t frame, + IPAFrameContext &frameContext, + [[maybe_unused]] const SwIspStats *stats, + ControlList &metadata) override; + +private: + LscAlgorithm<UQ<2, 6>> lscAlgo_; + + std::vector<double> gridPos_; + + unsigned int lastAppliedCt_ = 0; +}; + +} /* namespace soft::algorithms */ + +} /* namespace ipa */ + +} /* namespace libcamera */ diff --git a/src/ipa/simple/algorithms/meson.build b/src/ipa/simple/algorithms/meson.build index 73c637220..c9f6e5590 100644 --- a/src/ipa/simple/algorithms/meson.build +++ b/src/ipa/simple/algorithms/meson.build @@ -6,4 +6,5 @@ soft_simple_ipa_algorithms = files([ 'agc.cpp', 'blc.cpp', 'ccm.cpp', + 'lsc.cpp', ]) diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index ff312ae8f..23c2cfd0a 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -19,6 +19,7 @@ #include <libipa/awb.h> #include <libipa/ccm.h> #include <libipa/fc_queue.h> +#include "libipa/lsc.h" #include "core_ipa_interface.h" @@ -61,6 +62,8 @@ struct IPAActiveState { std::optional<float> contrast; std::optional<float> saturation; } knobs; + + ipa::lsc::ActiveState lsc; }; struct IPAFrameContext : public FrameContext { @@ -75,6 +78,7 @@ struct IPAFrameContext : public FrameContext { float gamma; std::optional<float> contrast; std::optional<float> saturation; + ipa::lsc::FrameContext lsc; }; struct IPAContext { @@ -89,6 +93,7 @@ struct IPAContext { FCQueue<IPAFrameContext> frameContexts; ControlInfoMap::Map ctrlMap; bool ccmEnabled = false; + ipa::lsc::ActiveState lsc; }; } /* namespace ipa::soft */