[PATCH] pipeline: rkisp1: cproc: Fix default value handling
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Jun 5 10:40:04 CEST 2024
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 at 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 at 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
>
More information about the libcamera-devel
mailing list