[libcamera-devel] [PATCH v4 21/32] ipa: rkisp1: cproc: Store per-frame information in frame context
Kieran Bingham
kieran.bingham at ideasonboard.com
Wed Sep 21 01:02:32 CEST 2022
Quoting Laurent Pinchart via libcamera-devel (2022-09-08 02:41:49)
> Rework the algorithm's usage of the active state, to store the value of
> controls for the last queued request in the queueRequest() function, and
> store a copy of the values in the corresponding frame context. The
> latter is used in the prepare() function to populate the ISP parameters
> with values corresponding to the right frame.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> src/ipa/rkisp1/algorithms/cproc.cpp | 51 ++++++++++++++++++-----------
> src/ipa/rkisp1/ipa_context.cpp | 21 ++++++++++--
> src/ipa/rkisp1/ipa_context.h | 8 ++++-
> 3 files changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/src/ipa/rkisp1/algorithms/cproc.cpp b/src/ipa/rkisp1/algorithms/cproc.cpp
> index 22a70e0b70c7..9c442cd56b3f 100644
> --- a/src/ipa/rkisp1/algorithms/cproc.cpp
> +++ b/src/ipa/rkisp1/algorithms/cproc.cpp
> @@ -38,55 +38,66 @@ LOG_DEFINE_CATEGORY(RkISP1CProc)
> */
> void ColorProcessing::queueRequest(IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> - [[maybe_unused]] RkISP1FrameContext &frameContext,
> + RkISP1FrameContext &frameContext,
> const ControlList &controls)
> {
> auto &cproc = context.activeState.cproc;
> + bool update = false;
>
> const auto &brightness = controls.get(controls::Brightness);
> if (brightness) {
> - cproc.brightness = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
> - cproc.updateParams = true;
> + int value = std::clamp<int>(std::lround(*brightness * 128), -128, 127);
> + if (cproc.brightness != value) {
> + cproc.brightness = value;
> + update = true;
> + }
>
> - LOG(RkISP1CProc, Debug) << "Set brightness to " << *brightness;
> + LOG(RkISP1CProc, Debug) << "Set brightness to " << value;
> }
>
> const auto &contrast = controls.get(controls::Contrast);
> if (contrast) {
> - cproc.contrast = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
> - cproc.updateParams = true;
> + int value = std::clamp<int>(std::lround(*contrast * 128), 0, 255);
> + if (cproc.contrast != value) {
> + cproc.contrast = value;
> + update = true;
> + }
>
> - LOG(RkISP1CProc, Debug) << "Set contrast to " << *contrast;
> + LOG(RkISP1CProc, Debug) << "Set contrast to " << value;
> }
>
> const auto saturation = controls.get(controls::Saturation);
> if (saturation) {
> - cproc.saturation = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
> - cproc.updateParams = true;
> + int value = std::clamp<int>(std::lround(*saturation * 128), 0, 255);
> + if (cproc.saturation != value) {
> + cproc.saturation = value;
> + update = true;
> + }
>
> - LOG(RkISP1CProc, Debug) << "Set saturation to " << *saturation;
> + LOG(RkISP1CProc, Debug) << "Set saturation to " << value;
> }
> +
> + frameContext.cproc.brightness = cproc.brightness;
> + frameContext.cproc.contrast = cproc.contrast;
> + frameContext.cproc.saturation = cproc.saturation;
> + frameContext.cproc.update = update;
I think this all sounds right.
I wonder what common patterns will emerge from these conversions ...
> }
>
> /**
> * \copydoc libcamera::ipa::Algorithm::prepare
> */
> -void ColorProcessing::prepare(IPAContext &context,
> +void ColorProcessing::prepare([[maybe_unused]] IPAContext &context,
> [[maybe_unused]] const uint32_t frame,
> - [[maybe_unused]] RkISP1FrameContext &frameContext,
> + RkISP1FrameContext &frameContext,
> rkisp1_params_cfg *params)
> {
> - auto &cproc = context.activeState.cproc;
> -
> /* Check if the algorithm configuration has been updated. */
> - if (!cproc.updateParams)
> + if (!frameContext.cproc.update)
> return;
Yes, this now responds to the frame context for the correct frame.
>
> - cproc.updateParams = false;
> -
> - params->others.cproc_config.brightness = cproc.brightness;
> - params->others.cproc_config.contrast = cproc.contrast;
> - params->others.cproc_config.sat = cproc.saturation;
> + params->others.cproc_config.brightness = frameContext.cproc.brightness;
> + params->others.cproc_config.contrast = frameContext.cproc.contrast;
> + params->others.cproc_config.sat = frameContext.cproc.saturation;
>
> params->module_en_update |= RKISP1_CIF_ISP_MODULE_CPROC;
> params->module_ens |= RKISP1_CIF_ISP_MODULE_CPROC;
> diff --git a/src/ipa/rkisp1/ipa_context.cpp b/src/ipa/rkisp1/ipa_context.cpp
> index cd1443e1ed46..936b77709417 100644
> --- a/src/ipa/rkisp1/ipa_context.cpp
> +++ b/src/ipa/rkisp1/ipa_context.cpp
> @@ -160,9 +160,6 @@ namespace libcamera::ipa::rkisp1 {
> *
> * \var IPAActiveState::cproc.saturation
> * \brief Saturation level
> - *
> - * \var IPAActiveState::cproc.updateParams
> - * \brief Indicates if ISP parameters need to be updated
> */
>
> /**
> @@ -236,6 +233,24 @@ namespace libcamera::ipa::rkisp1 {
> * \brief Whether the Auto White Balance algorithm is enabled
> */
>
> +/**
> + * \var RkISP1FrameContext::cproc
> + * \brief Color Processing parameters for this frame
> + *
> + * \struct RkISP1FrameContext::cproc.brightness
> + * \brief Brightness level
> + *
> + * \var RkISP1FrameContext::cproc.contrast
> + * \brief Contrast level
> + *
> + * \var RkISP1FrameContext::cproc.saturation
> + * \brief Saturation level
> + *
> + * \var RkISP1FrameContext::cproc.update
> + * \brief Indicates if the color processing parameters have been updated
> + * compared to the previous frame
> + */
> +
> /**
> * \var RkISP1FrameContext::sensor
> * \brief Sensor configuration that used been used for this frame
> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> index d97aae9a97b4..78edb607d039 100644
> --- a/src/ipa/rkisp1/ipa_context.h
> +++ b/src/ipa/rkisp1/ipa_context.h
> @@ -75,7 +75,6 @@ struct IPAActiveState {
> int8_t brightness;
> uint8_t contrast;
> uint8_t saturation;
> - bool updateParams;
> } cproc;
>
> struct {
> @@ -107,6 +106,13 @@ struct RkISP1FrameContext : public FrameContext {
> bool autoEnabled;
> } awb;
>
> + struct {
> + int8_t brightness;
> + uint8_t contrast;
> + uint8_t saturation;
> + bool update;
> + } cproc;
And I wonder if the common pattern will be lots of anonymous structures
that are in both the active state, and the frame context, while the
frame context has an 'updated' flag...
Maybe we can optimise this later - but I think this gets us moving
anyway to explore what it will become.
Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> +
> struct {
> uint32_t exposure;
> double gain;
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list