| Message ID | 20220816053123.1100015-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, Thank you for the patch. On Tue, Aug 16, 2022 at 02:31:23PM +0900, Paul Elder via libcamera-devel wrote: > Add support for manually controlling the color gains on the rkisp1 IPA. > To that end, add and plumb the AwbEnable and ColourGains controls. As > per-frame controls aren't supported yet in the rkisp1 IPA, simply apply > and perform checks on the controls immediately. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++-- > src/ipa/rkisp1/algorithms/awb.h | 3 +++ > src/ipa/rkisp1/ipa_context.h | 1 + > src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ > 4 files changed, 44 insertions(+), 2 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp > index 9f00364d..dcb4b2c9 100644 > --- a/src/ipa/rkisp1/algorithms/awb.cpp > +++ b/src/ipa/rkisp1/algorithms/awb.cpp > @@ -12,6 +12,7 @@ > > #include <libcamera/base/log.h> > > +#include <libcamera/control_ids.h> > #include <libcamera/ipa/core_ipa_interface.h> > > /** > @@ -38,6 +39,7 @@ int Awb::configure(IPAContext &context, > context.frameContext.awb.gains.red = 1.0; > context.frameContext.awb.gains.blue = 1.0; > context.frameContext.awb.gains.green = 1.0; > + context.frameContext.awb.enabled = true; > > /* > * Define the measurement window for AWB as a centered rectangle > @@ -116,6 +118,34 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params) > params->module_ens |= RKISP1_CIF_ISP_MODULE_AWB; > } > > +/** > + * \copydoc libcamera::ipa::Algorithm::queueRequest > + */ > +void Awb::queueRequest(IPAContext &context, > + [[maybe_unused]] const uint32_t frame, > + const ControlList &controls) > +{ > + auto &awb = context.frameContext.awb; > + > + const auto &awbEnable = controls.get(controls::AwbEnable); > + if (awbEnable) { What would you think of if (awbEnable && *awbEnable != awb.enabled) { to avoid logging the enabling/disabling message for every request that has the control set even when its value doesn't change ? > + awb.enabled = *awbEnable; > + > + LOG(RkISP1Awb, Debug) > + << (*awbEnable ? "Enabling" : "Disabling") << " AWB"; > + } > + > + const auto &colourGains = controls.get(controls::ColourGains); > + if (colourGains && !awb.enabled) { > + awb.gains.red = (*colourGains)[0]; > + awb.gains.blue = (*colourGains)[1]; > + > + LOG(RkISP1Awb, Debug) > + << "Set colour gains to red: " << (*colourGains)[0] > + << " blue: " << (*colourGains)[1]; s/ blue/, blue/ << "Set colour gains to red: " << awb.gains.red << "< blue: " << awb.gains.blue; is more readable. > + } > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::process > */ > @@ -164,8 +194,10 @@ void Awb::process([[maybe_unused]] IPAContext &context, > * Gain values are unsigned integer value, range 0 to 4 with 8 bit > * fractional part. > */ > - frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); > - frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); > + if (frameContext.awb.enabled) { > + frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); > + frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); > + } There may be room for further refactoring to avoid unneeded calculations when AWB is in manual mode, but that can be done later. > /* Hardcode the green gain to 1.0. */ > frameContext.awb.gains.green = 1.0; > > diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h > index 667a8beb..4332fac7 100644 > --- a/src/ipa/rkisp1/algorithms/awb.h > +++ b/src/ipa/rkisp1/algorithms/awb.h > @@ -21,6 +21,9 @@ public: > > int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; > void prepare(IPAContext &context, rkisp1_params_cfg *params) override; > + void queueRequest(IPAContext &context, > + const uint32_t frame, > + const ControlList &controls); > void process(IPAContext &context, IPAFrameContext *frameCtx, > const rkisp1_stat_buffer *stats) override; > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 2bdb6a81..fe258b2e 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -55,6 +55,7 @@ struct IPAFrameContext { > } gains; > > double temperatureK; > + bool enabled; We already have an "enabled" in IPASessionConfiguration.awb which indicates if the AWB algorithm is enabled, without considering if it's in auto mode or manual mode. Naming this field just "enabled" may be a bit confusing. Would autoMode, autoModeEnabled or autoEnabled be better? Or is this something that we should consider later, as part of a larger naming overhaul ? In any case, the field needs to be documented in ipa_context.cpp. > } awb; > > struct { > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 17d42d38..55752988 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -89,9 +89,15 @@ private: > > namespace { > > +std::array<float, 2> minColourGains = { 0.0f, 0.0f }; > +std::array<float, 2> maxColourGains = { 3.996f, 3.996f }; constexpr for both. > + > /* List of controls handled by the RkISP1 IPA */ > const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > + { &controls::AwbEnable, ControlInfo(false, true) }, > + { &controls::ColourGains, ControlInfo(Span<float, 2>(minColourGains), > + Span<float, 2>(maxColourGains)) }, I don't think this is right, you'll end up using the explicit ControlInfo(Span<const ControlValue> values, const ControlValue &def = {}); meant for enumerated controls. You should use ControlInfo(0.0f, 3.996f, 1.0f) instead. All those comments are fairly minor. Once addressed, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > { &controls::Brightness, ControlInfo(-1.0f, 0.993f) }, > { &controls::Contrast, ControlInfo(0.0f, 1.993f) }, > { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
diff --git a/src/ipa/rkisp1/algorithms/awb.cpp b/src/ipa/rkisp1/algorithms/awb.cpp index 9f00364d..dcb4b2c9 100644 --- a/src/ipa/rkisp1/algorithms/awb.cpp +++ b/src/ipa/rkisp1/algorithms/awb.cpp @@ -12,6 +12,7 @@ #include <libcamera/base/log.h> +#include <libcamera/control_ids.h> #include <libcamera/ipa/core_ipa_interface.h> /** @@ -38,6 +39,7 @@ int Awb::configure(IPAContext &context, context.frameContext.awb.gains.red = 1.0; context.frameContext.awb.gains.blue = 1.0; context.frameContext.awb.gains.green = 1.0; + context.frameContext.awb.enabled = true; /* * Define the measurement window for AWB as a centered rectangle @@ -116,6 +118,34 @@ void Awb::prepare(IPAContext &context, rkisp1_params_cfg *params) params->module_ens |= RKISP1_CIF_ISP_MODULE_AWB; } +/** + * \copydoc libcamera::ipa::Algorithm::queueRequest + */ +void Awb::queueRequest(IPAContext &context, + [[maybe_unused]] const uint32_t frame, + const ControlList &controls) +{ + auto &awb = context.frameContext.awb; + + const auto &awbEnable = controls.get(controls::AwbEnable); + if (awbEnable) { + awb.enabled = *awbEnable; + + LOG(RkISP1Awb, Debug) + << (*awbEnable ? "Enabling" : "Disabling") << " AWB"; + } + + const auto &colourGains = controls.get(controls::ColourGains); + if (colourGains && !awb.enabled) { + awb.gains.red = (*colourGains)[0]; + awb.gains.blue = (*colourGains)[1]; + + LOG(RkISP1Awb, Debug) + << "Set colour gains to red: " << (*colourGains)[0] + << " blue: " << (*colourGains)[1]; + } +} + /** * \copydoc libcamera::ipa::Algorithm::process */ @@ -164,8 +194,10 @@ void Awb::process([[maybe_unused]] IPAContext &context, * Gain values are unsigned integer value, range 0 to 4 with 8 bit * fractional part. */ - frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); - frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); + if (frameContext.awb.enabled) { + frameContext.awb.gains.red = std::clamp(redGain, 0.0, 1023.0 / 256); + frameContext.awb.gains.blue = std::clamp(blueGain, 0.0, 1023.0 / 256); + } /* Hardcode the green gain to 1.0. */ frameContext.awb.gains.green = 1.0; diff --git a/src/ipa/rkisp1/algorithms/awb.h b/src/ipa/rkisp1/algorithms/awb.h index 667a8beb..4332fac7 100644 --- a/src/ipa/rkisp1/algorithms/awb.h +++ b/src/ipa/rkisp1/algorithms/awb.h @@ -21,6 +21,9 @@ public: int configure(IPAContext &context, const IPACameraSensorInfo &configInfo) override; void prepare(IPAContext &context, rkisp1_params_cfg *params) override; + void queueRequest(IPAContext &context, + const uint32_t frame, + const ControlList &controls); void process(IPAContext &context, IPAFrameContext *frameCtx, const rkisp1_stat_buffer *stats) override; diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 2bdb6a81..fe258b2e 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -55,6 +55,7 @@ struct IPAFrameContext { } gains; double temperatureK; + bool enabled; } awb; struct { diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 17d42d38..55752988 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -89,9 +89,15 @@ private: namespace { +std::array<float, 2> minColourGains = { 0.0f, 0.0f }; +std::array<float, 2> maxColourGains = { 3.996f, 3.996f }; + /* List of controls handled by the RkISP1 IPA */ const ControlInfoMap::Map rkisp1Controls{ { &controls::AeEnable, ControlInfo(false, true) }, + { &controls::AwbEnable, ControlInfo(false, true) }, + { &controls::ColourGains, ControlInfo(Span<float, 2>(minColourGains), + Span<float, 2>(maxColourGains)) }, { &controls::Brightness, ControlInfo(-1.0f, 0.993f) }, { &controls::Contrast, ControlInfo(0.0f, 1.993f) }, { &controls::Saturation, ControlInfo(0.0f, 1.993f) },
Add support for manually controlling the color gains on the rkisp1 IPA. To that end, add and plumb the AwbEnable and ColourGains controls. As per-frame controls aren't supported yet in the rkisp1 IPA, simply apply and perform checks on the controls immediately. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/ipa/rkisp1/algorithms/awb.cpp | 36 +++++++++++++++++++++++++++++-- src/ipa/rkisp1/algorithms/awb.h | 3 +++ src/ipa/rkisp1/ipa_context.h | 1 + src/ipa/rkisp1/rkisp1.cpp | 6 ++++++ 4 files changed, 44 insertions(+), 2 deletions(-)