[libcamera-devel,v4,21/32] ipa: rkisp1: cproc: Store per-frame information in frame context
diff mbox series

Message ID 20220908014200.28728-22-laurent.pinchart@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: Frame context queue, IPU3 & RkISP consolidation, and RkISP1 improvements
Related show

Commit Message

Laurent Pinchart Sept. 8, 2022, 1:41 a.m. UTC 
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@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(-)

Comments

Kieran Bingham Sept. 20, 2022, 11:02 p.m. UTC | #1 
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@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@ideasonboard.com>

> +
>         struct {
>                 uint32_t exposure;
>                 double gain;
> -- 
> Regards,
> 
> Laurent Pinchart
>
Jacopo Mondi Sept. 21, 2022, 8:36 p.m. UTC | #2 
Hi Laurent,

On Thu, Sep 08, 2022 at 04:41:49AM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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@ideasonboard.com>

This one is easier indeed

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
  j

> ---
>  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;
>  }
>
>  /**
>   * \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;
>
> -	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;
> +
>  	struct {
>  		uint32_t exposure;
>  		double gain;
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart Sept. 22, 2022, 8:32 p.m. UTC | #3 
Hi Kieran,

On Wed, Sep 21, 2022 at 12:02:32AM +0100, Kieran Bingham wrote:
> 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@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...

It seems so. In v5 there will be a few code blocks along the lines of

	unsigned int value = std::round(std::clamp(*sharpness, 0.0f, 10.0f));

	if (filter.sharpness != value) {
		filter.sharpness = value;
		update = true;
	}

or

	if (filter.denoise != 1) {
		filter.denoise = 1;
		update = true;
	}

It would be nice to simplify that, but I'm not sure how yet. It think we
need more algorithms converted, in different IPA modules, before seeing
how to best refactor the code.

> 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@ideasonboard.com>
> 
> > +
> >         struct {
> >                 uint32_t exposure;
> >                 double gain;

Patch
diff mbox series 

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;
 }
 
 /**
  * \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;
 
-	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;
+
 	struct {
 		uint32_t exposure;
 		double gain;