[PATCH] pipeline: rkisp1: cproc: Fix default value handling
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 11 22:09:26 CEST 2024
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 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);
> > +}
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 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) },
> > };
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list