[libcamera-devel,v2,1/2] ipa: rkisp1: Compute LSC algorithm parameter during configure
diff mbox series

Message ID 20220929091245.2159838-2-fsylvestre@baylibre.com
State Superseded
Headers show
Series
  • Take into account color temperature during LSC algorithm for rkisp1
Related show

Commit Message

Florian Sylvestre Sept. 29, 2022, 9:12 a.m. UTC 
LSC gradient parameters where initially computed during prepare() phase.
Because these parameters can be computed only one time and stays constant for
each frame after: move the computation to the configure() function.

Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
---
 src/ipa/rkisp1/algorithms/lsc.cpp | 66 ++++++++++++++++++-------------
 src/ipa/rkisp1/algorithms/lsc.h   |  4 ++
 2 files changed, 42 insertions(+), 28 deletions(-)

Comments

Laurent Pinchart Sept. 30, 2022, 1:22 a.m. UTC | #1 
Hi Florian,

Thank you for the patch.

On Thu, Sep 29, 2022 at 11:12:44AM +0200, Florian Sylvestre via libcamera-devel wrote:
> LSC gradient parameters where initially computed during prepare() phase.

s/where/were/

But actually, as the commit message describes the current situation,

LSC gradient parameters are currently computed during the prepare() phase.

> Because these parameters can be computed only one time and stays constant for

s/stays/stay/

> each frame after: move the computation to the configure() function.

s/:/,/

> Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> ---
>  src/ipa/rkisp1/algorithms/lsc.cpp | 66 ++++++++++++++++++-------------
>  src/ipa/rkisp1/algorithms/lsc.h   |  4 ++
>  2 files changed, 42 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index 44245caa..b1f39dfd 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -125,30 +125,15 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>  int LensShadingCorrection::configure(IPAContext &context,
>  				     [[maybe_unused]] const IPACameraSensorInfo &configInfo)
>  {
> -	context.configuration.lsc.enabled = initialized_;
> -	return 0;
> -}
> -
> -/**
> - * \copydoc libcamera::ipa::Algorithm::prepare
> - */
> -void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
> -				    [[maybe_unused]] IPAFrameContext &frameContext,
> -				    rkisp1_params_cfg *params)
> -{
> -	if (frame > 0)
> -		return;
> -
>  	if (!initialized_)
> -		return;
> +		return EINVAL;

This is a change of behaviour, as configure() previously returned 0 in
that case, and the return value of configure() is checked by the caller.

This being said, it looks like we could drop the initialized_ flag, here
and in the other algorithms. It can only be false if init() returns an
error, and in that case the IPA module initialization fails. I recall we
introduced the initialized_ flag for a reason, but I can't locate any
point in the history where it was actually useful. I'll submit a patch.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
> -	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
>  	const Size &size = context.configuration.sensor.size;
>  	Size totalSize{};
>  
>  	for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) {
> -		config.x_size_tbl[i] = xSize_[i] * size.width;
> -		config.y_size_tbl[i] = ySize_[i] * size.height;
> +		xSizes_[i] = xSize_[i] * size.width;
> +		ySizes_[i] = ySize_[i] * size.height;
>  
>  		/*
>  		 * To prevent unexpected behavior of the ISP, the sum of x_size_tbl and
> @@ -157,25 +142,50 @@ void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
>  		 * rounding-induced errors.
>  		 */
>  		if (i == RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE - 1) {
> -			config.x_size_tbl[i] = size.width / 2 - totalSize.width;
> -			config.y_size_tbl[i] = size.height / 2 - totalSize.height;
> +			xSizes_[i] = size.width / 2 - totalSize.width;
> +			ySizes_[i] = size.height / 2 - totalSize.height;
>  		}
>  
> -		totalSize.width += config.x_size_tbl[i];
> -		totalSize.height += config.y_size_tbl[i];
> +		totalSize.width += xSizes_[i];
> +		totalSize.height += ySizes_[i];
>  
> -		config.x_grad_tbl[i] = std::round(32768 / config.x_size_tbl[i]);
> -		config.y_grad_tbl[i] = std::round(32768 / config.y_size_tbl[i]);
> +		xGrad_[i] = std::round(32768 / xSizes_[i]);
> +		yGrad_[i] = std::round(32768 / ySizes_[i]);
>  	}
>  
> +	context.configuration.lsc.enabled = true;
> +	return 0;
> +}
> +
> +/**
> + * \copydoc libcamera::ipa::Algorithm::prepare
> + */
> +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
> +				    const uint32_t frame,
> +				    [[maybe_unused]] IPAFrameContext &frameContext,
> +				    rkisp1_params_cfg *params)
> +{
> +	if (frame > 0)
> +		return;
> +
> +	if (!initialized_)
> +		return;
> +
> +	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> +
> +	memcpy(config.x_grad_tbl, xGrad_, sizeof(config.x_grad_tbl));
> +	memcpy(config.y_grad_tbl, yGrad_, sizeof(config.y_grad_tbl));
> +	memcpy(config.x_size_tbl, xSizes_, sizeof(config.x_size_tbl));
> +	memcpy(config.y_size_tbl, ySizes_, sizeof(config.y_size_tbl));
> +
>  	std::copy(rData_.begin(), rData_.end(),
> -		  &params->others.lsc_config.r_data_tbl[0][0]);
> +		  &config.r_data_tbl[0][0]);
>  	std::copy(grData_.begin(), grData_.end(),
> -		  &params->others.lsc_config.gr_data_tbl[0][0]);
> +		  &config.gr_data_tbl[0][0]);
>  	std::copy(gbData_.begin(), gbData_.end(),
> -		  &params->others.lsc_config.gb_data_tbl[0][0]);
> +		  &config.gb_data_tbl[0][0]);
>  	std::copy(bData_.begin(), bData_.end(),
> -		  &params->others.lsc_config.b_data_tbl[0][0]);
> +		  &config.b_data_tbl[0][0]);
>  
>  	params->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;
>  	params->module_ens |= RKISP1_CIF_ISP_MODULE_LSC;
> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> index da957d3e..b5cd5047 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.h
> +++ b/src/ipa/rkisp1/algorithms/lsc.h
> @@ -35,6 +35,10 @@ private:
>  
>  	std::vector<double> xSize_;
>  	std::vector<double> ySize_;
> +	uint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	uint16_t yGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	uint16_t xSizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> +	uint16_t ySizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
>  };
>  
>  } /* namespace ipa::rkisp1::algorithms */
Jacopo Mondi Sept. 30, 2022, 9:23 a.m. UTC | #2 
Can this be rebased on "ipa: rkisp1: Remove initialized_ flags from
algorithms" ?

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

Thanks
   j

On Fri, Sep 30, 2022 at 04:22:23AM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Florian,
>
> Thank you for the patch.
>
> On Thu, Sep 29, 2022 at 11:12:44AM +0200, Florian Sylvestre via libcamera-devel wrote:
> > LSC gradient parameters where initially computed during prepare() phase.
>
> s/where/were/
>
> But actually, as the commit message describes the current situation,
>
> LSC gradient parameters are currently computed during the prepare() phase.
>
> > Because these parameters can be computed only one time and stays constant for
>
> s/stays/stay/
>
> > each frame after: move the computation to the configure() function.
>
> s/:/,/
>
> > Signed-off-by: Florian Sylvestre <fsylvestre@baylibre.com>
> > ---
> >  src/ipa/rkisp1/algorithms/lsc.cpp | 66 ++++++++++++++++++-------------
> >  src/ipa/rkisp1/algorithms/lsc.h   |  4 ++
> >  2 files changed, 42 insertions(+), 28 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index 44245caa..b1f39dfd 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -125,30 +125,15 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> >  int LensShadingCorrection::configure(IPAContext &context,
> >  				     [[maybe_unused]] const IPACameraSensorInfo &configInfo)
> >  {
> > -	context.configuration.lsc.enabled = initialized_;
> > -	return 0;
> > -}
> > -
> > -/**
> > - * \copydoc libcamera::ipa::Algorithm::prepare
> > - */
> > -void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
> > -				    [[maybe_unused]] IPAFrameContext &frameContext,
> > -				    rkisp1_params_cfg *params)
> > -{
> > -	if (frame > 0)
> > -		return;
> > -
> >  	if (!initialized_)
> > -		return;
> > +		return EINVAL;
>
> This is a change of behaviour, as configure() previously returned 0 in
> that case, and the return value of configure() is checked by the caller.
>
> This being said, it looks like we could drop the initialized_ flag, here
> and in the other algorithms. It can only be false if init() returns an
> error, and in that case the IPA module initialization fails. I recall we
> introduced the initialized_ flag for a reason, but I can't locate any
> point in the history where it was actually useful. I'll submit a patch.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> >
> > -	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> >  	const Size &size = context.configuration.sensor.size;
> >  	Size totalSize{};
> >
> >  	for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) {
> > -		config.x_size_tbl[i] = xSize_[i] * size.width;
> > -		config.y_size_tbl[i] = ySize_[i] * size.height;
> > +		xSizes_[i] = xSize_[i] * size.width;
> > +		ySizes_[i] = ySize_[i] * size.height;
> >
> >  		/*
> >  		 * To prevent unexpected behavior of the ISP, the sum of x_size_tbl and
> > @@ -157,25 +142,50 @@ void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
> >  		 * rounding-induced errors.
> >  		 */
> >  		if (i == RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE - 1) {
> > -			config.x_size_tbl[i] = size.width / 2 - totalSize.width;
> > -			config.y_size_tbl[i] = size.height / 2 - totalSize.height;
> > +			xSizes_[i] = size.width / 2 - totalSize.width;
> > +			ySizes_[i] = size.height / 2 - totalSize.height;
> >  		}
> >
> > -		totalSize.width += config.x_size_tbl[i];
> > -		totalSize.height += config.y_size_tbl[i];
> > +		totalSize.width += xSizes_[i];
> > +		totalSize.height += ySizes_[i];
> >
> > -		config.x_grad_tbl[i] = std::round(32768 / config.x_size_tbl[i]);
> > -		config.y_grad_tbl[i] = std::round(32768 / config.y_size_tbl[i]);
> > +		xGrad_[i] = std::round(32768 / xSizes_[i]);
> > +		yGrad_[i] = std::round(32768 / ySizes_[i]);
> >  	}
> >
> > +	context.configuration.lsc.enabled = true;
> > +	return 0;
> > +}
> > +
> > +/**
> > + * \copydoc libcamera::ipa::Algorithm::prepare
> > + */
> > +void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
> > +				    const uint32_t frame,
> > +				    [[maybe_unused]] IPAFrameContext &frameContext,
> > +				    rkisp1_params_cfg *params)
> > +{
> > +	if (frame > 0)
> > +		return;
> > +
> > +	if (!initialized_)
> > +		return;
> > +
> > +	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
> > +
> > +	memcpy(config.x_grad_tbl, xGrad_, sizeof(config.x_grad_tbl));
> > +	memcpy(config.y_grad_tbl, yGrad_, sizeof(config.y_grad_tbl));
> > +	memcpy(config.x_size_tbl, xSizes_, sizeof(config.x_size_tbl));
> > +	memcpy(config.y_size_tbl, ySizes_, sizeof(config.y_size_tbl));
> > +
> >  	std::copy(rData_.begin(), rData_.end(),
> > -		  &params->others.lsc_config.r_data_tbl[0][0]);
> > +		  &config.r_data_tbl[0][0]);
> >  	std::copy(grData_.begin(), grData_.end(),
> > -		  &params->others.lsc_config.gr_data_tbl[0][0]);
> > +		  &config.gr_data_tbl[0][0]);
> >  	std::copy(gbData_.begin(), gbData_.end(),
> > -		  &params->others.lsc_config.gb_data_tbl[0][0]);
> > +		  &config.gb_data_tbl[0][0]);
> >  	std::copy(bData_.begin(), bData_.end(),
> > -		  &params->others.lsc_config.b_data_tbl[0][0]);
> > +		  &config.b_data_tbl[0][0]);
> >
> >  	params->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;
> >  	params->module_ens |= RKISP1_CIF_ISP_MODULE_LSC;
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> > index da957d3e..b5cd5047 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.h
> > +++ b/src/ipa/rkisp1/algorithms/lsc.h
> > @@ -35,6 +35,10 @@ private:
> >
> >  	std::vector<double> xSize_;
> >  	std::vector<double> ySize_;
> > +	uint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > +	uint16_t yGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > +	uint16_t xSizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> > +	uint16_t ySizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
> >  };
> >
> >  } /* namespace ipa::rkisp1::algorithms */
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
index 44245caa..b1f39dfd 100644
--- a/src/ipa/rkisp1/algorithms/lsc.cpp
+++ b/src/ipa/rkisp1/algorithms/lsc.cpp
@@ -125,30 +125,15 @@  int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
 int LensShadingCorrection::configure(IPAContext &context,
 				     [[maybe_unused]] const IPACameraSensorInfo &configInfo)
 {
-	context.configuration.lsc.enabled = initialized_;
-	return 0;
-}
-
-/**
- * \copydoc libcamera::ipa::Algorithm::prepare
- */
-void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
-				    [[maybe_unused]] IPAFrameContext &frameContext,
-				    rkisp1_params_cfg *params)
-{
-	if (frame > 0)
-		return;
-
 	if (!initialized_)
-		return;
+		return EINVAL;
 
-	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
 	const Size &size = context.configuration.sensor.size;
 	Size totalSize{};
 
 	for (unsigned int i = 0; i < RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE; ++i) {
-		config.x_size_tbl[i] = xSize_[i] * size.width;
-		config.y_size_tbl[i] = ySize_[i] * size.height;
+		xSizes_[i] = xSize_[i] * size.width;
+		ySizes_[i] = ySize_[i] * size.height;
 
 		/*
 		 * To prevent unexpected behavior of the ISP, the sum of x_size_tbl and
@@ -157,25 +142,50 @@  void LensShadingCorrection::prepare(IPAContext &context, const uint32_t frame,
 		 * rounding-induced errors.
 		 */
 		if (i == RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE - 1) {
-			config.x_size_tbl[i] = size.width / 2 - totalSize.width;
-			config.y_size_tbl[i] = size.height / 2 - totalSize.height;
+			xSizes_[i] = size.width / 2 - totalSize.width;
+			ySizes_[i] = size.height / 2 - totalSize.height;
 		}
 
-		totalSize.width += config.x_size_tbl[i];
-		totalSize.height += config.y_size_tbl[i];
+		totalSize.width += xSizes_[i];
+		totalSize.height += ySizes_[i];
 
-		config.x_grad_tbl[i] = std::round(32768 / config.x_size_tbl[i]);
-		config.y_grad_tbl[i] = std::round(32768 / config.y_size_tbl[i]);
+		xGrad_[i] = std::round(32768 / xSizes_[i]);
+		yGrad_[i] = std::round(32768 / ySizes_[i]);
 	}
 
+	context.configuration.lsc.enabled = true;
+	return 0;
+}
+
+/**
+ * \copydoc libcamera::ipa::Algorithm::prepare
+ */
+void LensShadingCorrection::prepare([[maybe_unused]] IPAContext &context,
+				    const uint32_t frame,
+				    [[maybe_unused]] IPAFrameContext &frameContext,
+				    rkisp1_params_cfg *params)
+{
+	if (frame > 0)
+		return;
+
+	if (!initialized_)
+		return;
+
+	struct rkisp1_cif_isp_lsc_config &config = params->others.lsc_config;
+
+	memcpy(config.x_grad_tbl, xGrad_, sizeof(config.x_grad_tbl));
+	memcpy(config.y_grad_tbl, yGrad_, sizeof(config.y_grad_tbl));
+	memcpy(config.x_size_tbl, xSizes_, sizeof(config.x_size_tbl));
+	memcpy(config.y_size_tbl, ySizes_, sizeof(config.y_size_tbl));
+
 	std::copy(rData_.begin(), rData_.end(),
-		  &params->others.lsc_config.r_data_tbl[0][0]);
+		  &config.r_data_tbl[0][0]);
 	std::copy(grData_.begin(), grData_.end(),
-		  &params->others.lsc_config.gr_data_tbl[0][0]);
+		  &config.gr_data_tbl[0][0]);
 	std::copy(gbData_.begin(), gbData_.end(),
-		  &params->others.lsc_config.gb_data_tbl[0][0]);
+		  &config.gb_data_tbl[0][0]);
 	std::copy(bData_.begin(), bData_.end(),
-		  &params->others.lsc_config.b_data_tbl[0][0]);
+		  &config.b_data_tbl[0][0]);
 
 	params->module_en_update |= RKISP1_CIF_ISP_MODULE_LSC;
 	params->module_ens |= RKISP1_CIF_ISP_MODULE_LSC;
diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
index da957d3e..b5cd5047 100644
--- a/src/ipa/rkisp1/algorithms/lsc.h
+++ b/src/ipa/rkisp1/algorithms/lsc.h
@@ -35,6 +35,10 @@  private:
 
 	std::vector<double> xSize_;
 	std::vector<double> ySize_;
+	uint16_t xGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	uint16_t yGrad_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	uint16_t xSizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
+	uint16_t ySizes_[RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE];
 };
 
 } /* namespace ipa::rkisp1::algorithms */