| Message ID | 20260501191400.985920-1-devve.3@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi, thank you for the patches. (It would be better to send the patches as a series next time so that their ordering is clear.) d3vv3 <devve.3@gmail.com> writes: > Add configurable YAML parameters maxGainR, maxGainB, and speed to AWB. > Replace the single hardcoded max gain (4.0x) with per-channel limits, > and apply exponential moving average smoothing to reduce colour > temperature oscillation between frames. > > Signed-off-by: d3vv3 <devve.3@gmail.com> > --- > src/ipa/simple/algorithms/awb.cpp | 34 ++++++++++++++++++++++++++----- > src/ipa/simple/algorithms/awb.h | 6 ++++++ > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index f5c88ea6..0f9964a4 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -14,6 +14,8 @@ > > #include <libcamera/control_ids.h> > > +#include "libcamera/internal/yaml_parser.h" > + > #include "libipa/colours.h" > #include "simple/ipa_context.h" > > @@ -23,6 +25,21 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > namespace ipa::soft::algorithms { > > +int Awb::init([[maybe_unused]] IPAContext &context, > + const ValueNode &tuningData) > +{ > + maxGainR_ = tuningData["maxGainR"].get<float>().value_or(4.0f); > + maxGainB_ = tuningData["maxGainB"].get<float>().value_or(4.0f); > + speed_ = tuningData["speed"].get<float>().value_or(1.0f); > + > + LOG(IPASoftAwb, Info) Maybe Debug would be sufficient here? Reviewed-by: Milan Zamazal <mzamazal@redhat.com> > + << "AWB: maxGainR " << maxGainR_ > + << ", maxGainB " << maxGainB_ > + << ", speed " << speed_; > + > + return 0; > +} > + > int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > @@ -84,14 +101,21 @@ void Awb::process(IPAContext &context, > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; > > /* > - * Calculate red and blue gains for AWB. > - * Clamp max gain at 4.0, this also avoids 0 division. > + * Calculate red and blue gains for AWB. Clamp max gain to avoid > + * division by zero and extreme color casts. > */ > auto &gains = context.activeState.awb.gains; > + float rawRGain = sum.r() <= sum.g() / maxGainR_ ? maxGainR_ : > + static_cast<float>(sum.g()) / sum.r(); > + float rawBGain = sum.b() <= sum.g() / maxGainB_ ? maxGainB_ : > + static_cast<float>(sum.g()) / sum.b(); > + > + /* Apply temporal smoothing to avoid rapid white balance changes. */ > + float alpha = std::clamp(speed_, 0.01f, 1.0f); > gains = { { > - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > - 1.0, > - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > + gains.r() * (1.0f - alpha) + rawRGain * alpha, > + 1.0f, > + gains.b() * (1.0f - alpha) + rawBGain * alpha, > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h > index ad993f39..0aedc1d1 100644 > --- a/src/ipa/simple/algorithms/awb.h > +++ b/src/ipa/simple/algorithms/awb.h > @@ -19,6 +19,7 @@ public: > Awb() = default; > ~Awb() = default; > > + int init(IPAContext &context, const ValueNode &tuningData) override; > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > void prepare(IPAContext &context, > const uint32_t frame, > @@ -29,6 +30,11 @@ public: > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) override; > + > +private: > + float maxGainR_; > + float maxGainB_; > + float speed_; > }; > > } /* namespace ipa::soft::algorithms */
Could you please resend v2 as a separate patch series, not in reply to v1 ? I see three patches named "v2 1/9", "v2 2/9" and "v2 3/9", as well as two other patches simply named "v2", all interleaved in the first version of the series. It's hard to tell what to review. Please format a complete series with git-format-patch, specify the "-v2" argument to properly indicate the version in the subject lines, and send all patches in one go, numbered 1/N to N/N. If applicable, a cover letter can be useful, you can generate the template by specifying the "--cover-letter" argument to git-send-email. On Wed, May 06, 2026 at 11:58:22PM +0200, d3vv3 wrote: > Add configurable YAML parameters maxGainR, maxGainB, and speed to AWB. > Replace the single hardcoded max gain (4.0x) with per-channel limits, > and apply exponential moving average smoothing to reduce colour > temperature oscillation between frames. > > Signed-off-by: d3vv3 <devve.3@gmail.com> > --- > Hi, thanks for the review. These is were my first email patches ever. I > will learn how to send them as series for the next time. > > Changed log level in init() from Info to Debug as suggested. > > src/ipa/simple/algorithms/awb.cpp | 34 ++++++++++++++++++++++++++----- > src/ipa/simple/algorithms/awb.h | 6 ++++++ > 2 files changed, 35 insertions(+), 5 deletions(-) > > diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp > index f5c88ea6..937aabc8 100644 > --- a/src/ipa/simple/algorithms/awb.cpp > +++ b/src/ipa/simple/algorithms/awb.cpp > @@ -14,6 +14,8 @@ > > #include <libcamera/control_ids.h> > > +#include "libcamera/internal/yaml_parser.h" > + > #include "libipa/colours.h" > #include "simple/ipa_context.h" > > @@ -23,6 +25,21 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > namespace ipa::soft::algorithms { > > +int Awb::init([[maybe_unused]] IPAContext &context, > + const ValueNode &tuningData) > +{ > + maxGainR_ = tuningData["maxGainR"].get<float>().value_or(4.0f); > + maxGainB_ = tuningData["maxGainB"].get<float>().value_or(4.0f); > + speed_ = tuningData["speed"].get<float>().value_or(1.0f); > + > + LOG(IPASoftAwb, Debug) > + << "AWB: maxGainR " << maxGainR_ > + << ", maxGainB " << maxGainB_ > + << ", speed " << speed_; > + > + return 0; > +} > + > int Awb::configure(IPAContext &context, > [[maybe_unused]] const IPAConfigInfo &configInfo) > { > @@ -84,14 +101,21 @@ void Awb::process(IPAContext &context, > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; > > /* > - * Calculate red and blue gains for AWB. > - * Clamp max gain at 4.0, this also avoids 0 division. > + * Calculate red and blue gains for AWB. Clamp max gain to avoid > + * division by zero and extreme color casts. > */ > auto &gains = context.activeState.awb.gains; > + float rawRGain = sum.r() <= sum.g() / maxGainR_ ? maxGainR_ : > + static_cast<float>(sum.g()) / sum.r(); > + float rawBGain = sum.b() <= sum.g() / maxGainB_ ? maxGainB_ : > + static_cast<float>(sum.g()) / sum.b(); > + > + /* Apply temporal smoothing to avoid rapid white balance changes. */ > + float alpha = std::clamp(speed_, 0.01f, 1.0f); > gains = { { > - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), > - 1.0, > - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), > + gains.r() * (1.0f - alpha) + rawRGain * alpha, > + 1.0f, > + gains.b() * (1.0f - alpha) + rawBGain * alpha, > } }; > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; > diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h > index ad993f39..0aedc1d1 100644 > --- a/src/ipa/simple/algorithms/awb.h > +++ b/src/ipa/simple/algorithms/awb.h > @@ -19,6 +19,7 @@ public: > Awb() = default; > ~Awb() = default; > > + int init(IPAContext &context, const ValueNode &tuningData) override; > int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; > void prepare(IPAContext &context, > const uint32_t frame, > @@ -29,6 +30,11 @@ public: > IPAFrameContext &frameContext, > const SwIspStats *stats, > ControlList &metadata) override; > + > +private: > + float maxGainR_; > + float maxGainB_; > + float speed_; > }; > > } /* namespace ipa::soft::algorithms */
Thanks for the feedback and patience, Laurent. I have now sent all as a patch series. I do not know how to handle patches from other people that I have built on, so I sent them as well. El jue, 7 may 2026 a las 0:44, Laurent Pinchart (< laurent.pinchart@ideasonboard.com>) escribió: > Could you please resend v2 as a separate patch series, not in reply to > v1 ? I see three patches named "v2 1/9", "v2 2/9" and "v2 3/9", as well > as two other patches simply named "v2", all interleaved in the first > version of the series. It's hard to tell what to review. > > Please format a complete series with git-format-patch, specify the "-v2" > argument to properly indicate the version in the subject lines, and send > all patches in one go, numbered 1/N to N/N. If applicable, a cover > letter can be useful, you can generate the template by specifying the > "--cover-letter" argument to git-send-email. > > On Wed, May 06, 2026 at 11:58:22PM +0200, d3vv3 wrote: > > Add configurable YAML parameters maxGainR, maxGainB, and speed to AWB. > > Replace the single hardcoded max gain (4.0x) with per-channel limits, > > and apply exponential moving average smoothing to reduce colour > > temperature oscillation between frames. > > > > Signed-off-by: d3vv3 <devve.3@gmail.com> > > --- > > Hi, thanks for the review. These is were my first email patches ever. I > > will learn how to send them as series for the next time. > > > > Changed log level in init() from Info to Debug as suggested. > > > > src/ipa/simple/algorithms/awb.cpp | 34 ++++++++++++++++++++++++++----- > > src/ipa/simple/algorithms/awb.h | 6 ++++++ > > 2 files changed, 35 insertions(+), 5 deletions(-) > > > > diff --git a/src/ipa/simple/algorithms/awb.cpp > b/src/ipa/simple/algorithms/awb.cpp > > index f5c88ea6..937aabc8 100644 > > --- a/src/ipa/simple/algorithms/awb.cpp > > +++ b/src/ipa/simple/algorithms/awb.cpp > > @@ -14,6 +14,8 @@ > > > > #include <libcamera/control_ids.h> > > > > +#include "libcamera/internal/yaml_parser.h" > > + > > #include "libipa/colours.h" > > #include "simple/ipa_context.h" > > > > @@ -23,6 +25,21 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > > > namespace ipa::soft::algorithms { > > > > +int Awb::init([[maybe_unused]] IPAContext &context, > > + const ValueNode &tuningData) > > +{ > > + maxGainR_ = tuningData["maxGainR"].get<float>().value_or(4.0f); > > + maxGainB_ = tuningData["maxGainB"].get<float>().value_or(4.0f); > > + speed_ = tuningData["speed"].get<float>().value_or(1.0f); > > + > > + LOG(IPASoftAwb, Debug) > > + << "AWB: maxGainR " << maxGainR_ > > + << ", maxGainB " << maxGainB_ > > + << ", speed " << speed_; > > + > > + return 0; > > +} > > + > > int Awb::configure(IPAContext &context, > > [[maybe_unused]] const IPAConfigInfo &configInfo) > > { > > @@ -84,14 +101,21 @@ void Awb::process(IPAContext &context, > > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - > offset; > > > > /* > > - * Calculate red and blue gains for AWB. > > - * Clamp max gain at 4.0, this also avoids 0 division. > > + * Calculate red and blue gains for AWB. Clamp max gain to avoid > > + * division by zero and extreme color casts. > > */ > > auto &gains = context.activeState.awb.gains; > > + float rawRGain = sum.r() <= sum.g() / maxGainR_ ? maxGainR_ : > > + static_cast<float>(sum.g()) / sum.r(); > > + float rawBGain = sum.b() <= sum.g() / maxGainB_ ? maxGainB_ : > > + static_cast<float>(sum.g()) / sum.b(); > > + > > + /* Apply temporal smoothing to avoid rapid white balance changes. > */ > > + float alpha = std::clamp(speed_, 0.01f, 1.0f); > > gains = { { > > - sum.r() <= sum.g() / 4 ? 4.0f : > static_cast<float>(sum.g()) / sum.r(), > > - 1.0, > > - sum.b() <= sum.g() / 4 ? 4.0f : > static_cast<float>(sum.g()) / sum.b(), > > + gains.r() * (1.0f - alpha) + rawRGain * alpha, > > + 1.0f, > > + gains.b() * (1.0f - alpha) + rawBGain * alpha, > > } }; > > > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / > gains.b() } }; > > diff --git a/src/ipa/simple/algorithms/awb.h > b/src/ipa/simple/algorithms/awb.h > > index ad993f39..0aedc1d1 100644 > > --- a/src/ipa/simple/algorithms/awb.h > > +++ b/src/ipa/simple/algorithms/awb.h > > @@ -19,6 +19,7 @@ public: > > Awb() = default; > > ~Awb() = default; > > > > + int init(IPAContext &context, const ValueNode &tuningData) > override; > > int configure(IPAContext &context, const IPAConfigInfo > &configInfo) override; > > void prepare(IPAContext &context, > > const uint32_t frame, > > @@ -29,6 +30,11 @@ public: > > IPAFrameContext &frameContext, > > const SwIspStats *stats, > > ControlList &metadata) override; > > + > > +private: > > + float maxGainR_; > > + float maxGainB_; > > + float speed_; > > }; > > > > } /* namespace ipa::soft::algorithms */ > > -- > Regards, > > Laurent Pinchart >
On Thu, May 07, 2026 at 01:09:40AM +0200, devve wrote: > Thanks for the feedback and patience, Laurent. I have now sent all as a > patch series. I do not know how to handle patches from other people that I > have built on, so I sent them as well. Usually you would mention the dependencies in the cover letter, including links (to patchwork.libcamera.org for instance). If you depend on patches posted separately from each other, including them in your series can be fine too to simplify the review. It should then be mentioned in the cover letter. > El jue, 7 may 2026 a las 0:44, Laurent Pinchart escribió: > > > Could you please resend v2 as a separate patch series, not in reply to > > v1 ? I see three patches named "v2 1/9", "v2 2/9" and "v2 3/9", as well > > as two other patches simply named "v2", all interleaved in the first > > version of the series. It's hard to tell what to review. > > > > Please format a complete series with git-format-patch, specify the "-v2" > > argument to properly indicate the version in the subject lines, and send > > all patches in one go, numbered 1/N to N/N. If applicable, a cover > > letter can be useful, you can generate the template by specifying the > > "--cover-letter" argument to git-send-email. > > > > On Wed, May 06, 2026 at 11:58:22PM +0200, d3vv3 wrote: > > > Add configurable YAML parameters maxGainR, maxGainB, and speed to AWB. > > > Replace the single hardcoded max gain (4.0x) with per-channel limits, > > > and apply exponential moving average smoothing to reduce colour > > > temperature oscillation between frames. > > > > > > Signed-off-by: d3vv3 <devve.3@gmail.com> > > > --- > > > Hi, thanks for the review. These is were my first email patches ever. I > > > will learn how to send them as series for the next time. > > > > > > Changed log level in init() from Info to Debug as suggested. > > > > > > src/ipa/simple/algorithms/awb.cpp | 34 ++++++++++++++++++++++++++----- > > > src/ipa/simple/algorithms/awb.h | 6 ++++++ > > > 2 files changed, 35 insertions(+), 5 deletions(-) > > > > > > diff --git a/src/ipa/simple/algorithms/awb.cpp > > b/src/ipa/simple/algorithms/awb.cpp > > > index f5c88ea6..937aabc8 100644 > > > --- a/src/ipa/simple/algorithms/awb.cpp > > > +++ b/src/ipa/simple/algorithms/awb.cpp > > > @@ -14,6 +14,8 @@ > > > > > > #include <libcamera/control_ids.h> > > > > > > +#include "libcamera/internal/yaml_parser.h" > > > + > > > #include "libipa/colours.h" > > > #include "simple/ipa_context.h" > > > > > > @@ -23,6 +25,21 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) > > > > > > namespace ipa::soft::algorithms { > > > > > > +int Awb::init([[maybe_unused]] IPAContext &context, > > > + const ValueNode &tuningData) > > > +{ > > > + maxGainR_ = tuningData["maxGainR"].get<float>().value_or(4.0f); > > > + maxGainB_ = tuningData["maxGainB"].get<float>().value_or(4.0f); > > > + speed_ = tuningData["speed"].get<float>().value_or(1.0f); > > > + > > > + LOG(IPASoftAwb, Debug) > > > + << "AWB: maxGainR " << maxGainR_ > > > + << ", maxGainB " << maxGainB_ > > > + << ", speed " << speed_; > > > + > > > + return 0; > > > +} > > > + > > > int Awb::configure(IPAContext &context, > > > [[maybe_unused]] const IPAConfigInfo &configInfo) > > > { > > > @@ -84,14 +101,21 @@ void Awb::process(IPAContext &context, > > > const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - > > offset; > > > > > > /* > > > - * Calculate red and blue gains for AWB. > > > - * Clamp max gain at 4.0, this also avoids 0 division. > > > + * Calculate red and blue gains for AWB. Clamp max gain to avoid > > > + * division by zero and extreme color casts. > > > */ > > > auto &gains = context.activeState.awb.gains; > > > + float rawRGain = sum.r() <= sum.g() / maxGainR_ ? maxGainR_ : > > > + static_cast<float>(sum.g()) / sum.r(); > > > + float rawBGain = sum.b() <= sum.g() / maxGainB_ ? maxGainB_ : > > > + static_cast<float>(sum.g()) / sum.b(); > > > + > > > + /* Apply temporal smoothing to avoid rapid white balance changes. > > */ > > > + float alpha = std::clamp(speed_, 0.01f, 1.0f); > > > gains = { { > > > - sum.r() <= sum.g() / 4 ? 4.0f : > > static_cast<float>(sum.g()) / sum.r(), > > > - 1.0, > > > - sum.b() <= sum.g() / 4 ? 4.0f : > > static_cast<float>(sum.g()) / sum.b(), > > > + gains.r() * (1.0f - alpha) + rawRGain * alpha, > > > + 1.0f, > > > + gains.b() * (1.0f - alpha) + rawBGain * alpha, > > > } }; > > > > > > RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / > > gains.b() } }; > > > diff --git a/src/ipa/simple/algorithms/awb.h > > b/src/ipa/simple/algorithms/awb.h > > > index ad993f39..0aedc1d1 100644 > > > --- a/src/ipa/simple/algorithms/awb.h > > > +++ b/src/ipa/simple/algorithms/awb.h > > > @@ -19,6 +19,7 @@ public: > > > Awb() = default; > > > ~Awb() = default; > > > > > > + int init(IPAContext &context, const ValueNode &tuningData) > > override; > > > int configure(IPAContext &context, const IPAConfigInfo > > &configInfo) override; > > > void prepare(IPAContext &context, > > > const uint32_t frame, > > > @@ -29,6 +30,11 @@ public: > > > IPAFrameContext &frameContext, > > > const SwIspStats *stats, > > > ControlList &metadata) override; > > > + > > > +private: > > > + float maxGainR_; > > > + float maxGainB_; > > > + float speed_; > > > }; > > > > > > } /* namespace ipa::soft::algorithms */
diff --git a/src/ipa/simple/algorithms/awb.cpp b/src/ipa/simple/algorithms/awb.cpp index f5c88ea6..0f9964a4 100644 --- a/src/ipa/simple/algorithms/awb.cpp +++ b/src/ipa/simple/algorithms/awb.cpp @@ -14,6 +14,8 @@ #include <libcamera/control_ids.h> +#include "libcamera/internal/yaml_parser.h" + #include "libipa/colours.h" #include "simple/ipa_context.h" @@ -23,6 +25,21 @@ LOG_DEFINE_CATEGORY(IPASoftAwb) namespace ipa::soft::algorithms { +int Awb::init([[maybe_unused]] IPAContext &context, + const ValueNode &tuningData) +{ + maxGainR_ = tuningData["maxGainR"].get<float>().value_or(4.0f); + maxGainB_ = tuningData["maxGainB"].get<float>().value_or(4.0f); + speed_ = tuningData["speed"].get<float>().value_or(1.0f); + + LOG(IPASoftAwb, Info) + << "AWB: maxGainR " << maxGainR_ + << ", maxGainB " << maxGainB_ + << ", speed " << speed_; + + return 0; +} + int Awb::configure(IPAContext &context, [[maybe_unused]] const IPAConfigInfo &configInfo) { @@ -84,14 +101,21 @@ void Awb::process(IPAContext &context, const RGB<uint64_t> sum = stats->sum_.max(offset + minValid) - offset; /* - * Calculate red and blue gains for AWB. - * Clamp max gain at 4.0, this also avoids 0 division. + * Calculate red and blue gains for AWB. Clamp max gain to avoid + * division by zero and extreme color casts. */ auto &gains = context.activeState.awb.gains; + float rawRGain = sum.r() <= sum.g() / maxGainR_ ? maxGainR_ : + static_cast<float>(sum.g()) / sum.r(); + float rawBGain = sum.b() <= sum.g() / maxGainB_ ? maxGainB_ : + static_cast<float>(sum.g()) / sum.b(); + + /* Apply temporal smoothing to avoid rapid white balance changes. */ + float alpha = std::clamp(speed_, 0.01f, 1.0f); gains = { { - sum.r() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.r(), - 1.0, - sum.b() <= sum.g() / 4 ? 4.0f : static_cast<float>(sum.g()) / sum.b(), + gains.r() * (1.0f - alpha) + rawRGain * alpha, + 1.0f, + gains.b() * (1.0f - alpha) + rawBGain * alpha, } }; RGB<double> rgbGains{ { 1 / gains.r(), 1 / gains.g(), 1 / gains.b() } }; diff --git a/src/ipa/simple/algorithms/awb.h b/src/ipa/simple/algorithms/awb.h index ad993f39..0aedc1d1 100644 --- a/src/ipa/simple/algorithms/awb.h +++ b/src/ipa/simple/algorithms/awb.h @@ -19,6 +19,7 @@ public: Awb() = default; ~Awb() = default; + int init(IPAContext &context, const ValueNode &tuningData) override; int configure(IPAContext &context, const IPAConfigInfo &configInfo) override; void prepare(IPAContext &context, const uint32_t frame, @@ -29,6 +30,11 @@ public: IPAFrameContext &frameContext, const SwIspStats *stats, ControlList &metadata) override; + +private: + float maxGainR_; + float maxGainB_; + float speed_; }; } /* namespace ipa::soft::algorithms */
Add configurable YAML parameters maxGainR, maxGainB, and speed to AWB. Replace the single hardcoded max gain (4.0x) with per-channel limits, and apply exponential moving average smoothing to reduce colour temperature oscillation between frames. Signed-off-by: d3vv3 <devve.3@gmail.com> --- src/ipa/simple/algorithms/awb.cpp | 34 ++++++++++++++++++++++++++----- src/ipa/simple/algorithms/awb.h | 6 ++++++ 2 files changed, 35 insertions(+), 5 deletions(-)