| Message ID | 20240605064829.81288-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
On Wed, Jun 05, 2024 at 08:48:25AM GMT, Stefan Klug wrote: > Default control values where not applied to activeState. This had no negative > side effects in first place, as the hardware defaults where used. The issue > became visible, when only one of the controls was set at runtime. In that case > the params for the other values where overwritten with 0 (reset value of > activeState) resulting in a black image. > > While at it, only add the controls to the controls map if the algorithm is > contained in the tuning file. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > --- > > This patch depends on https://patchwork.libcamera.org/patch/20194/ which needs > to be merged first. > > > src/ipa/rkisp1/algorithms/cproc.cpp | 59 +++++++++++++++++++++++++++-- > src/ipa/rkisp1/algorithms/cproc.h | 3 ++ > src/ipa/rkisp1/rkisp1.cpp | 3 -- > 3 files changed, 59 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > index 68bb8180..0f122504 100644 > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > @@ -33,6 +33,56 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1CProc) > > +static int convertBrightness(const float v) > +{ > + return std::clamp<int>(std::lround(v * 128), -128, 127); > +} > + > +static int convertContrast(const float v) > +{ > + return std::clamp<int>(std::lround(v * 128), 0, 255); > +} > + > +static int convertSaturation(const float v) > +{ > + return std::clamp<int>(std::lround(v * 128), 0, 255); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int ColorProcessing::init([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const YamlObject &tuningData) > +{ > + context.ctrlMap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, 0.0f); > + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, 1.0f); > + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, 1.0f); > + > + return 0; > +} > + > +/** > + * \brief Configure the Gamma given a configInfo > + * \param[in] context The shared IPA context > + * \param[in] configInfo The IPA configuration data > + * > + * \return 0 > + */ > +int ColorProcessing::configure([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const IPACameraSensorInfo &configInfo) > +{ > + auto &cproc = context.activeState.cproc; > + > + cproc.brightness = convertBrightness( > + context.ctrlMap[&controls::Brightness].def().get<float>()); > + cproc.contrast = convertContrast( > + context.ctrlMap[&controls::Contrast].def().get<float>()); > + cproc.saturation = convertSaturation( > + context.ctrlMap[&controls::Saturation].def().get<float>()); > + > + return 0; > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::queueRequest > */ > @@ -44,9 +94,12 @@ void ColorProcessing::queueRequest(IPAContext &context, > auto &cproc = context.activeState.cproc; > bool update = false; > > + if (frame == 0) > + update = true; > + > const auto &brightness = controls.get(controls::Brightness); > if (brightness) { > - int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127); > + int value = convertBrightness(*brightness); > if (cproc.brightness != value) { > cproc.brightness = value; > update = true; > @@ -57,7 +110,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > const auto &contrast = controls.get(controls::Contrast); > if (contrast) { > - int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255); > + int value = convertContrast(*contrast); > if (cproc.contrast != value) { > cproc.contrast = value; > update = true; > @@ -68,7 +121,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > const auto saturation = controls.get(controls::Saturation); > if (saturation) { > - int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255); > + int value = convertSaturation(*saturation); > if (cproc.saturation != value) { > cproc.saturation = value; > update = true; > diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h > index bafba5cc..e50e7200 100644 > --- a/src/ipa/rkisp1/algorithms/cproc.h > +++ b/src/ipa/rkisp1/algorithms/cproc.h > @@ -21,6 +21,9 @@ public: > ColorProcessing() = default; > ~ColorProcessing() = default; > > + int init(IPAContext &context, const YamlObject &tuningData) override; > + int configure(IPAContext &context, > + const IPACameraSensorInfo &configInfo) override; > void queueRequest(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, > const ControlList &controls) override; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 17474408..62d56a3a 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > - { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > - { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > - { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > }; > -- > 2.43.0 >
Quoting Stefan Klug (2024-06-05 07:48:25) > Default control values where not applied to activeState. This had no negative s/where/were/ > side effects in first place, as the hardware defaults where used. The issue s/in first/in the first/ s/where/were/ > became visible, when only one of the controls was set at runtime. In that case > the params for the other values where overwritten with 0 (reset value of s/where/were/ ;-) > activeState) resulting in a black image. > > While at it, only add the controls to the controls map if the algorithm is > contained in the tuning file. I really like this bit. I would have thought it could be a distinct patch but I'll not worry too much if it isn't. But this should be done for all controls in rkisp1Controls if they are used/controlled by an algorithm. Keeping the initialisation of the control and mangement of it close by is far better. (and means the control will only be reported if the algo is enabled of course) > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > This patch depends on https://patchwork.libcamera.org/patch/20194/ which needs > to be merged first. > > > src/ipa/rkisp1/algorithms/cproc.cpp | 59 +++++++++++++++++++++++++++-- > src/ipa/rkisp1/algorithms/cproc.h | 3 ++ > src/ipa/rkisp1/rkisp1.cpp | 3 -- > 3 files changed, 59 insertions(+), 6 deletions(-) > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > index 68bb8180..0f122504 100644 > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > @@ -33,6 +33,56 @@ namespace ipa::rkisp1::algorithms { > > LOG_DEFINE_CATEGORY(RkISP1CProc) > > +static int convertBrightness(const float v) > +{ > + return std::clamp<int>(std::lround(v * 128), -128, 127); > +} > + > +static int convertContrast(const float v) > +{ > + return std::clamp<int>(std::lround(v * 128), 0, 255); > +} > + > +static int convertSaturation(const float v) I wondered about having these take a ControlValue so the casting/converting could be done here. Might shorten the lines where convert* is used? But entirely optional. > +{ > + return std::clamp<int>(std::lround(v * 128), 0, 255); > +} > + > +/** > + * \copydoc libcamera::ipa::Algorithm::init > + */ > +int ColorProcessing::init([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const YamlObject &tuningData) > +{ > + context.ctrlMap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, 0.0f); > + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, 1.0f); > + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, 1.0f); I'm so happy to see the control defaults and definitions move into the file that owns and manipulates them! > + > + return 0; > +} > + > +/** > + * \brief Configure the Gamma given a configInfo Gamma ? Could be just a copydoc if there's nothing more to add. > + * \param[in] context The shared IPA context > + * \param[in] configInfo The IPA configuration data > + * > + * \return 0 > + */ > +int ColorProcessing::configure([[maybe_unused]] IPAContext &context, > + [[maybe_unused]] const IPACameraSensorInfo &configInfo) > +{ > + auto &cproc = context.activeState.cproc; > + > + cproc.brightness = convertBrightness( > + context.ctrlMap[&controls::Brightness].def().get<float>()); > + cproc.contrast = convertContrast( > + context.ctrlMap[&controls::Contrast].def().get<float>()); > + cproc.saturation = convertSaturation( > + context.ctrlMap[&controls::Saturation].def().get<float>()); > + Tempting to make the default values class consts such as static const float kDefaultBrightness = 0.0f; static const float kDefaultContrast = 1.0f; static const float kDefaultSaturuation = 1.0f; and then reference those directly to avoid the lookups to what we know are otherwise constant? I'm pretty sure handling all the above is easy so with those: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + return 0; > +} > + > /** > * \copydoc libcamera::ipa::Algorithm::queueRequest > */ > @@ -44,9 +94,12 @@ void ColorProcessing::queueRequest(IPAContext &context, > auto &cproc = context.activeState.cproc; > bool update = false; > > + if (frame == 0) > + update = true; > + > const auto &brightness = controls.get(controls::Brightness); > if (brightness) { > - int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127); > + int value = convertBrightness(*brightness); > if (cproc.brightness != value) { > cproc.brightness = value; > update = true; > @@ -57,7 +110,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > const auto &contrast = controls.get(controls::Contrast); > if (contrast) { > - int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255); > + int value = convertContrast(*contrast); > if (cproc.contrast != value) { > cproc.contrast = value; > update = true; > @@ -68,7 +121,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > const auto saturation = controls.get(controls::Saturation); > if (saturation) { > - int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255); > + int value = convertSaturation(*saturation); > if (cproc.saturation != value) { > cproc.saturation = value; > update = true; > diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h > index bafba5cc..e50e7200 100644 > --- a/src/ipa/rkisp1/algorithms/cproc.h > +++ b/src/ipa/rkisp1/algorithms/cproc.h > @@ -21,6 +21,9 @@ public: > ColorProcessing() = default; > ~ColorProcessing() = default; > > + int init(IPAContext &context, const YamlObject &tuningData) override; > + int configure(IPAContext &context, > + const IPACameraSensorInfo &configInfo) override; > void queueRequest(IPAContext &context, const uint32_t frame, > IPAFrameContext &frameContext, > const ControlList &controls) override; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 17474408..62d56a3a 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > - { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > - { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > - { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > }; > -- > 2.43.0 >
Quoting Kieran Bingham (2024-06-05 09:40:04) > Quoting Stefan Klug (2024-06-05 07:48:25) ... > and then reference those directly to avoid the lookups to what we know > are otherwise constant? > > I'm pretty sure handling all the above is easy so with those: ('with those' meaning my suggestions above are optional / up to you - not ... you must take my suggestions :D) > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Hi Kieran, thanks for the review and especially for the s/where/were/. I need to put an autocorrect there :-) The suggested change to kDefaultXXX makes it really easier to digest. For reference I added the patch that will go in after gamma below. Cheers, Stefan --- Subject: [PATCH v2] pipeline: rkisp1: cproc: Fix default value handling Default control values were not applied to activeState. This had no negative side effects in the first place, as the hardware defaults were used. The issue became visible, when only one of the controls was set at runtime. In that case the params for the other values were overwritten with 0 (reset value of activeState) resulting in a black image. While at it, only add the controls to the controls map if the algorithm is contained in the tuning file. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/rkisp1/algorithms/cproc.cpp | 58 +++++++++++++++++++++++++++-- src/ipa/rkisp1/algorithms/cproc.h | 3 ++ src/ipa/rkisp1/rkisp1.cpp | 3 -- 3 files changed, 58 insertions(+), 6 deletions(-) diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index 68bb8180..18c7b719 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -33,6 +33,55 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1CProc) +constexpr float kDefaultBrightness = 0.0f; +constexpr float kDefaultContrast = 1.0f; +constexpr float kDefaultSaturation = 1.0f; + +static int convertBrightness(const float v) +{ + return std::clamp<int>(std::lround(v * 128), -128, 127); +} + +static int convertContrast(const float v) +{ + return std::clamp<int>(std::lround(v * 128), 0, 255); +} + +static int convertSaturation(const float v) +{ + return std::clamp<int>(std::lround(v * 128), 0, 255); +} + +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int ColorProcessing::init([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const YamlObject &tuningData) +{ + auto &cmap = context.ctrlMap; + + cmap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, kDefaultBrightness); + cmap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, kDefaultContrast); + cmap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, kDefaultSaturation); + + return 0; +} + +/** + * \copydoc libcamera::ipa::Algorithm::configure + */ +int ColorProcessing::configure([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const IPACameraSensorInfo &configInfo) +{ + auto &cproc = context.activeState.cproc; + + cproc.brightness = convertBrightness(kDefaultBrightness); + cproc.contrast = convertContrast(kDefaultContrast); + cproc.saturation = convertSaturation(kDefaultSaturation); + + return 0; +} + /** * \copydoc libcamera::ipa::Algorithm::queueRequest */ @@ -44,9 +93,12 @@ void ColorProcessing::queueRequest(IPAContext &context, auto &cproc = context.activeState.cproc; bool update = false; + if (frame == 0) + update = true; + const auto &brightness = controls.get(controls::Brightness); if (brightness) { - int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127); + int value = convertBrightness(*brightness); if (cproc.brightness != value) { cproc.brightness = value; update = true; @@ -57,7 +109,7 @@ void ColorProcessing::queueRequest(IPAContext &context, const auto &contrast = controls.get(controls::Contrast); if (contrast) { - int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255); + int value = convertContrast(*contrast); if (cproc.contrast != value) { cproc.contrast = value; update = true; @@ -68,7 +120,7 @@ void ColorProcessing::queueRequest(IPAContext &context, const auto saturation = controls.get(controls::Saturation); if (saturation) { - int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255); + int value = convertSaturation(*saturation); if (cproc.saturation != value) { cproc.saturation = value; update = true; diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h index bafba5cc..e50e7200 100644 --- a/src/ipa/rkisp1/algorithms/cproc.h +++ b/src/ipa/rkisp1/algorithms/cproc.h @@ -21,6 +21,9 @@ public: ColorProcessing() = default; ~ColorProcessing() = default; + int init(IPAContext &context, const YamlObject &tuningData) override; + int configure(IPAContext &context, + const IPACameraSensorInfo &configInfo) override; void queueRequest(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, const ControlList &controls) override; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 17474408..62d56a3a 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{ { &controls::AeEnable, ControlInfo(false, true) }, { &controls::AwbEnable, ControlInfo(false, true) }, { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, - { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, - { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, - { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, };
On Wed, Jun 05, 2024 at 09:40:04AM +0100, Kieran Bingham wrote: > Quoting Stefan Klug (2024-06-05 07:48:25) > > Default control values where not applied to activeState. This had no negative > > s/where/were/ > > > side effects in first place, as the hardware defaults where used. The issue > > s/in first/in the first/ > s/where/were/ > > > became visible, when only one of the controls was set at runtime. In that case > > the params for the other values where overwritten with 0 (reset value of > > s/where/were/ ;-) > > > activeState) resulting in a black image. > > > > While at it, only add the controls to the controls map if the algorithm is > > contained in the tuning file. > > I really like this bit. I would have thought it could be a distinct > patch but I'll not worry too much if it isn't. Yes, for next time splitting such changes in two patches would be preferred. > But this should be done > for all controls in rkisp1Controls if they are used/controlled by an > algorithm. Keeping the initialisation of the control and mangement of it > close by is far better. (and means the control will only be reported if > the algo is enabled of course) > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > --- > > > > This patch depends on https://patchwork.libcamera.org/patch/20194/ which needs > > to be merged first. > > > > > > src/ipa/rkisp1/algorithms/cproc.cpp | 59 +++++++++++++++++++++++++++-- > > src/ipa/rkisp1/algorithms/cproc.h | 3 ++ > > src/ipa/rkisp1/rkisp1.cpp | 3 -- > > 3 files changed, 59 insertions(+), 6 deletions(-) > > > > diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp > > index 68bb8180..0f122504 100644 > > --- a/src/ipa/rkisp1/algorithms/cproc.cpp > > +++ b/src/ipa/rkisp1/algorithms/cproc.cpp > > @@ -33,6 +33,56 @@ namespace ipa::rkisp1::algorithms { > > > > LOG_DEFINE_CATEGORY(RkISP1CProc) > > > > +static int convertBrightness(const float v) > > +{ > > + return std::clamp<int>(std::lround(v * 128), -128, 127); > > +} > > + > > +static int convertContrast(const float v) > > +{ > > + return std::clamp<int>(std::lround(v * 128), 0, 255); > > +} > > + > > +static int convertSaturation(const float v) > > I wondered about having these take a ControlValue so the > casting/converting could be done here. Might shorten the lines where > convert* is used? But entirely optional. > > > +{ > > + return std::clamp<int>(std::lround(v * 128), 0, 255); > > +} We follow the C++ way of using anonymous namespaces to make limit symbol visibility to the TU: namespace { int convertBrightness(const float v) { return std::clamp<int>(std::lround(v * 128), -128, 127); } int convertContrast(const float v) { return std::clamp<int>(std::lround(v * 128), 0, 255); } int convertSaturation(const float v) { return std::clamp<int>(std::lround(v * 128), 0, 255); } } /* namespace */ This should encompass the constexpr kDefault* mentioned below. The convertContrast() and convertSaturation() functions are identical, how about merging them into a single convertContrastSaturation() ? > > + > > +/** > > + * \copydoc libcamera::ipa::Algorithm::init > > + */ > > +int ColorProcessing::init([[maybe_unused]] IPAContext &context, You can drop [[maybe_unused]]. > > + [[maybe_unused]] const YamlObject &tuningData) > > +{ > > + context.ctrlMap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, 0.0f); > > + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, 1.0f); > > + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, 1.0f); > > I'm so happy to see the control defaults and definitions move into the > file that owns and manipulates them! > > > + > > + return 0; > > +} > > + > > +/** > > + * \brief Configure the Gamma given a configInfo > > Gamma ? > > Could be just a copydoc if there's nothing more to add. > > > + * \param[in] context The shared IPA context > > + * \param[in] configInfo The IPA configuration data > > + * > > + * \return 0 > > + */ > > +int ColorProcessing::configure([[maybe_unused]] IPAContext &context, Same here. As this patch has been merged already, could you fix these small issues (at least the ones you agree with) in follow-up patches ? > > + [[maybe_unused]] const IPACameraSensorInfo &configInfo) > > +{ > > + auto &cproc = context.activeState.cproc; > > + > > + cproc.brightness = convertBrightness( > > + context.ctrlMap[&controls::Brightness].def().get<float>()); > > + cproc.contrast = convertContrast( > > + context.ctrlMap[&controls::Contrast].def().get<float>()); > > + cproc.saturation = convertSaturation( > > + context.ctrlMap[&controls::Saturation].def().get<float>()); > > + > > Tempting to make the default values class consts such as > static const float kDefaultBrightness = 0.0f; > static const float kDefaultContrast = 1.0f; > static const float kDefaultSaturuation = 1.0f; > > and then reference those directly to avoid the lookups to what we know > are otherwise constant? > > I'm pretty sure handling all the above is easy so with those: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > + return 0; > > +} > > + > > /** > > * \copydoc libcamera::ipa::Algorithm::queueRequest > > */ > > @@ -44,9 +94,12 @@ void ColorProcessing::queueRequest(IPAContext &context, > > auto &cproc = context.activeState.cproc; > > bool update = false; > > > > + if (frame == 0) > > + update = true; > > + > > const auto &brightness = controls.get(controls::Brightness); > > if (brightness) { > > - int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127); > > + int value = convertBrightness(*brightness); > > if (cproc.brightness != value) { > > cproc.brightness = value; > > update = true; > > @@ -57,7 +110,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > const auto &contrast = controls.get(controls::Contrast); > > if (contrast) { > > - int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255); > > + int value = convertContrast(*contrast); > > if (cproc.contrast != value) { > > cproc.contrast = value; > > update = true; > > @@ -68,7 +121,7 @@ void ColorProcessing::queueRequest(IPAContext &context, > > > > const auto saturation = controls.get(controls::Saturation); > > if (saturation) { > > - int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255); > > + int value = convertSaturation(*saturation); > > if (cproc.saturation != value) { > > cproc.saturation = value; > > update = true; > > diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h > > index bafba5cc..e50e7200 100644 > > --- a/src/ipa/rkisp1/algorithms/cproc.h > > +++ b/src/ipa/rkisp1/algorithms/cproc.h > > @@ -21,6 +21,9 @@ public: > > ColorProcessing() = default; > > ~ColorProcessing() = default; > > > > + int init(IPAContext &context, const YamlObject &tuningData) override; > > + int configure(IPAContext &context, > > + const IPACameraSensorInfo &configInfo) override; > > void queueRequest(IPAContext &context, const uint32_t frame, > > IPAFrameContext &frameContext, > > const ControlList &controls) override; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 17474408..62d56a3a 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{ > > { &controls::AeEnable, ControlInfo(false, true) }, > > { &controls::AwbEnable, ControlInfo(false, true) }, > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > - { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > - { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > - { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > };
diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp index 68bb8180..0f122504 100644 --- a/src/ipa/rkisp1/algorithms/cproc.cpp +++ b/src/ipa/rkisp1/algorithms/cproc.cpp @@ -33,6 +33,56 @@ namespace ipa::rkisp1::algorithms { LOG_DEFINE_CATEGORY(RkISP1CProc) +static int convertBrightness(const float v) +{ + return std::clamp<int>(std::lround(v * 128), -128, 127); +} + +static int convertContrast(const float v) +{ + return std::clamp<int>(std::lround(v * 128), 0, 255); +} + +static int convertSaturation(const float v) +{ + return std::clamp<int>(std::lround(v * 128), 0, 255); +} + +/** + * \copydoc libcamera::ipa::Algorithm::init + */ +int ColorProcessing::init([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const YamlObject &tuningData) +{ + context.ctrlMap[&controls::Brightness] = ControlInfo(-1.0f, 0.993f, 0.0f); + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 1.993f, 1.0f); + context.ctrlMap[&controls::Saturation] = ControlInfo(0.0f, 1.993f, 1.0f); + + return 0; +} + +/** + * \brief Configure the Gamma given a configInfo + * \param[in] context The shared IPA context + * \param[in] configInfo The IPA configuration data + * + * \return 0 + */ +int ColorProcessing::configure([[maybe_unused]] IPAContext &context, + [[maybe_unused]] const IPACameraSensorInfo &configInfo) +{ + auto &cproc = context.activeState.cproc; + + cproc.brightness = convertBrightness( + context.ctrlMap[&controls::Brightness].def().get<float>()); + cproc.contrast = convertContrast( + context.ctrlMap[&controls::Contrast].def().get<float>()); + cproc.saturation = convertSaturation( + context.ctrlMap[&controls::Saturation].def().get<float>()); + + return 0; +} + /** * \copydoc libcamera::ipa::Algorithm::queueRequest */ @@ -44,9 +94,12 @@ void ColorProcessing::queueRequest(IPAContext &context, auto &cproc = context.activeState.cproc; bool update = false; + if (frame == 0) + update = true; + const auto &brightness = controls.get(controls::Brightness); if (brightness) { - int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127); + int value = convertBrightness(*brightness); if (cproc.brightness != value) { cproc.brightness = value; update = true; @@ -57,7 +110,7 @@ void ColorProcessing::queueRequest(IPAContext &context, const auto &contrast = controls.get(controls::Contrast); if (contrast) { - int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255); + int value = convertContrast(*contrast); if (cproc.contrast != value) { cproc.contrast = value; update = true; @@ -68,7 +121,7 @@ void ColorProcessing::queueRequest(IPAContext &context, const auto saturation = controls.get(controls::Saturation); if (saturation) { - int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255); + int value = convertSaturation(*saturation); if (cproc.saturation != value) { cproc.saturation = value; update = true; diff --git a/src/ipa/rkisp1/algorithms/cproc.h b/src/ipa/rkisp1/algorithms/cproc.h index bafba5cc..e50e7200 100644 --- a/src/ipa/rkisp1/algorithms/cproc.h +++ b/src/ipa/rkisp1/algorithms/cproc.h @@ -21,6 +21,9 @@ public: ColorProcessing() = default; ~ColorProcessing() = default; + int init(IPAContext &context, const YamlObject &tuningData) override; + int configure(IPAContext &context, + const IPACameraSensorInfo &configInfo) override; void queueRequest(IPAContext &context, const uint32_t frame, IPAFrameContext &frameContext, const ControlList &controls) override; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 17474408..62d56a3a 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -109,9 +109,6 @@ const ControlInfoMap::Map rkisp1Controls{ { &controls::AeEnable, ControlInfo(false, true) }, { &controls::AwbEnable, ControlInfo(false, true) }, { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, - { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, - { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, - { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, };
Default control values where not applied to activeState. This had no negative side effects in first place, as the hardware defaults where used. The issue became visible, when only one of the controls was set at runtime. In that case the params for the other values where overwritten with 0 (reset value of activeState) resulting in a black image. While at it, only add the controls to the controls map if the algorithm is contained in the tuning file. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- This patch depends on https://patchwork.libcamera.org/patch/20194/ which needs to be merged first. src/ipa/rkisp1/algorithms/cproc.cpp | 59 +++++++++++++++++++++++++++-- src/ipa/rkisp1/algorithms/cproc.h | 3 ++ src/ipa/rkisp1/rkisp1.cpp | 3 -- 3 files changed, 59 insertions(+), 6 deletions(-)