[v5,20/36] ipa: rksip1: lsc: Make parseSizes() a class function
diff mbox series

Message ID 20260708-libipa-algorithms-v5-20-0759d0359f52@ideasonboard.com
State Superseded
Headers show
Series
  • ipa: libipa: Introduce libipa algorithms
Related show

Commit Message

Jacopo Mondi July 8, 2026, 3:51 p.m. UTC 
There are no reasons to have it as a static helper.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/lsc.cpp | 60 +++++++++++++++++++--------------------
 src/ipa/rkisp1/algorithms/lsc.h   |  2 ++
 2 files changed, 32 insertions(+), 30 deletions(-)

Comments

Stefan Klug July 13, 2026, 2:25 p.m. UTC | #1 
Hi Jacopo,

Quoting Jacopo Mondi (2026-07-08 17:51:02)
> There are no reasons to have it as a static helper.

Out of curiosity, what is the reason to make it a class member? Isn't it
generally preferable to have functions that do not touch class members
external to the class?

Best regards
Stefan

> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/lsc.cpp | 60 +++++++++++++++++++--------------------
>  src/ipa/rkisp1/algorithms/lsc.h   |  2 ++
>  2 files changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index f32c46084dd1..e03bf361c4b9 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -34,36 +34,6 @@ namespace {
>  
>  constexpr int kColourTemperatureQuantization = 10;
>  
> -std::vector<double> parseSizes(const ValueNode &tuningData,
> -                              const char *prop)
> -{
> -       std::vector<double> sizes =
> -               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
> -       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
> -               LOG(RkISP1Lsc, Error)
> -                       << "Invalid '" << prop << "' values: expected "
> -                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
> -                       << " elements, got " << sizes.size();
> -               return {};
> -       }
> -
> -       /*
> -        * The sum of all elements must be 0.5 to satisfy hardware constraints.
> -        * Validate it here, allowing a 1% tolerance as rounding errors may
> -        * prevent an exact match (further adjustments will be performed in
> -        * LensShadingCorrection::prepare()).
> -        */
> -       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
> -       if (sum < 0.495 || sum > 0.505) {
> -               LOG(RkISP1Lsc, Error)
> -                       << "Invalid '" << prop << "' values: sum of the elements"
> -                       << " should be 0.5, got " << sum;
> -               return {};
> -       }
> -
> -       return sizes;
> -}
> -
>  unsigned int quantize(unsigned int value, unsigned int step)
>  {
>         return std::lround(value / static_cast<double>(step)) * step;
> @@ -138,6 +108,36 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>         return 0;
>  }
>  
> +std::vector<double> LensShadingCorrection::parseSizes(const ValueNode &tuningData,
> +                                                     const char *prop)
> +{
> +       std::vector<double> sizes =
> +               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
> +       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
> +               LOG(RkISP1Lsc, Error)
> +                       << "Invalid '" << prop << "' values: expected "
> +                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
> +                       << " elements, got " << sizes.size();
> +               return {};
> +       }
> +
> +       /*
> +        * The sum of all elements must be 0.5 to satisfy hardware constraints.
> +        * Validate it here, allowing a 1% tolerance as rounding errors may
> +        * prevent an exact match (further adjustments will be performed in
> +        * LensShadingCorrection::prepare()).
> +        */
> +       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
> +       if (sum < 0.495 || sum > 0.505) {
> +               LOG(RkISP1Lsc, Error)
> +                       << "Invalid '" << prop << "' values: sum of the elements"
> +                       << " should be 0.5, got " << sum;
> +               return {};
> +       }
> +
> +       return sizes;
> +}
> +
>  /**
>   * \copydoc libcamera::ipa::Algorithm::configure
>   */
> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> index 2a428293cb2e..ee037353bbe4 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.h
> +++ b/src/ipa/rkisp1/algorithms/lsc.h
> @@ -39,6 +39,8 @@ public:
>  private:
>         void setParameters(rkisp1_cif_isp_lsc_config &config);
>         void copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set0);
> +       std::vector<double> parseSizes(const ValueNode &tuningData,
> +                                      const char *prop);
>  
>         std::vector<double> xSize_;
>         std::vector<double> ySize_;
> 
> -- 
> 2.54.0
>
Jacopo Mondi July 14, 2026, 7:30 a.m. UTC | #2 
On Mon, Jul 13, 2026 at 04:25:54PM +0200, Stefan Klug wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2026-07-08 17:51:02)
> > There are no reasons to have it as a static helper.
>
> Out of curiosity, what is the reason to make it a class member? Isn't it
> generally preferable to have functions that do not touch class members
> external to the class?

I haven't found any indication in
https://google.github.io/styleguide/cppguide.html
wether functions like this should be given internal linkage or made
class members.

To me, this function it's an helper that serves the tuning file
parsing logic. It doesn't touch class members, that's true, but I
don't see any advantage in having it external to the class.

>
> Best regards
> Stefan
>
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  src/ipa/rkisp1/algorithms/lsc.cpp | 60 +++++++++++++++++++--------------------
> >  src/ipa/rkisp1/algorithms/lsc.h   |  2 ++
> >  2 files changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> > index f32c46084dd1..e03bf361c4b9 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> > @@ -34,36 +34,6 @@ namespace {
> >
> >  constexpr int kColourTemperatureQuantization = 10;
> >
> > -std::vector<double> parseSizes(const ValueNode &tuningData,
> > -                              const char *prop)
> > -{
> > -       std::vector<double> sizes =
> > -               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
> > -       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
> > -               LOG(RkISP1Lsc, Error)
> > -                       << "Invalid '" << prop << "' values: expected "
> > -                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
> > -                       << " elements, got " << sizes.size();
> > -               return {};
> > -       }
> > -
> > -       /*
> > -        * The sum of all elements must be 0.5 to satisfy hardware constraints.
> > -        * Validate it here, allowing a 1% tolerance as rounding errors may
> > -        * prevent an exact match (further adjustments will be performed in
> > -        * LensShadingCorrection::prepare()).
> > -        */
> > -       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
> > -       if (sum < 0.495 || sum > 0.505) {
> > -               LOG(RkISP1Lsc, Error)
> > -                       << "Invalid '" << prop << "' values: sum of the elements"
> > -                       << " should be 0.5, got " << sum;
> > -               return {};
> > -       }
> > -
> > -       return sizes;
> > -}
> > -
> >  unsigned int quantize(unsigned int value, unsigned int step)
> >  {
> >         return std::lround(value / static_cast<double>(step)) * step;
> > @@ -138,6 +108,36 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
> >         return 0;
> >  }
> >
> > +std::vector<double> LensShadingCorrection::parseSizes(const ValueNode &tuningData,
> > +                                                     const char *prop)
> > +{
> > +       std::vector<double> sizes =
> > +               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
> > +       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
> > +               LOG(RkISP1Lsc, Error)
> > +                       << "Invalid '" << prop << "' values: expected "
> > +                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
> > +                       << " elements, got " << sizes.size();
> > +               return {};
> > +       }
> > +
> > +       /*
> > +        * The sum of all elements must be 0.5 to satisfy hardware constraints.
> > +        * Validate it here, allowing a 1% tolerance as rounding errors may
> > +        * prevent an exact match (further adjustments will be performed in
> > +        * LensShadingCorrection::prepare()).
> > +        */
> > +       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
> > +       if (sum < 0.495 || sum > 0.505) {
> > +               LOG(RkISP1Lsc, Error)
> > +                       << "Invalid '" << prop << "' values: sum of the elements"
> > +                       << " should be 0.5, got " << sum;
> > +               return {};
> > +       }
> > +
> > +       return sizes;
> > +}
> > +
> >  /**
> >   * \copydoc libcamera::ipa::Algorithm::configure
> >   */
> > diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
> > index 2a428293cb2e..ee037353bbe4 100644
> > --- a/src/ipa/rkisp1/algorithms/lsc.h
> > +++ b/src/ipa/rkisp1/algorithms/lsc.h
> > @@ -39,6 +39,8 @@ public:
> >  private:
> >         void setParameters(rkisp1_cif_isp_lsc_config &config);
> >         void copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set0);
> > +       std::vector<double> parseSizes(const ValueNode &tuningData,
> > +                                      const char *prop);
> >
> >         std::vector<double> xSize_;
> >         std::vector<double> ySize_;
> >
> > --
> > 2.54.0
> >
Barnabás Pőcze July 15, 2026, 12:53 p.m. UTC | #3 
2026. 07. 14. 9:30 keltezéssel, Jacopo Mondi írta:
> On Mon, Jul 13, 2026 at 04:25:54PM +0200, Stefan Klug wrote:
>> Hi Jacopo,
>>
>> Quoting Jacopo Mondi (2026-07-08 17:51:02)
>>> There are no reasons to have it as a static helper.
>>
>> Out of curiosity, what is the reason to make it a class member? Isn't it
>> generally preferable to have functions that do not touch class members
>> external to the class?
> 
> I haven't found any indication in
> https://google.github.io/styleguide/cppguide.html
> wether functions like this should be given internal linkage or made
> class members.
> 
> To me, this function it's an helper that serves the tuning file
> parsing logic. It doesn't touch class members, that's true, but I
> don't see any advantage in having it external to the class.

If it is moved into the class, I would suggest marking it `static`,
making it clear that it touches no non-static members.


> 
>>
>> Best regards
>> Stefan
>>
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
>>> ---
>>>   src/ipa/rkisp1/algorithms/lsc.cpp | 60 +++++++++++++++++++--------------------
>>>   src/ipa/rkisp1/algorithms/lsc.h   |  2 ++
>>>   2 files changed, 32 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
>>> index f32c46084dd1..e03bf361c4b9 100644
>>> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
>>> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
>>> @@ -34,36 +34,6 @@ namespace {
>>>
>>>   constexpr int kColourTemperatureQuantization = 10;
>>>
>>> -std::vector<double> parseSizes(const ValueNode &tuningData,
>>> -                              const char *prop)
>>> -{
>>> -       std::vector<double> sizes =
>>> -               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
>>> -       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
>>> -               LOG(RkISP1Lsc, Error)
>>> -                       << "Invalid '" << prop << "' values: expected "
>>> -                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
>>> -                       << " elements, got " << sizes.size();
>>> -               return {};
>>> -       }
>>> -
>>> -       /*
>>> -        * The sum of all elements must be 0.5 to satisfy hardware constraints.
>>> -        * Validate it here, allowing a 1% tolerance as rounding errors may
>>> -        * prevent an exact match (further adjustments will be performed in
>>> -        * LensShadingCorrection::prepare()).
>>> -        */
>>> -       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
>>> -       if (sum < 0.495 || sum > 0.505) {
>>> -               LOG(RkISP1Lsc, Error)
>>> -                       << "Invalid '" << prop << "' values: sum of the elements"
>>> -                       << " should be 0.5, got " << sum;
>>> -               return {};
>>> -       }
>>> -
>>> -       return sizes;
>>> -}
>>> -
>>>   unsigned int quantize(unsigned int value, unsigned int step)
>>>   {
>>>          return std::lround(value / static_cast<double>(step)) * step;
>>> @@ -138,6 +108,36 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>>>          return 0;
>>>   }
>>>
>>> +std::vector<double> LensShadingCorrection::parseSizes(const ValueNode &tuningData,
>>> +                                                     const char *prop)
>>> +{
>>> +       std::vector<double> sizes =
>>> +               tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
>>> +       if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
>>> +               LOG(RkISP1Lsc, Error)
>>> +                       << "Invalid '" << prop << "' values: expected "
>>> +                       << RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
>>> +                       << " elements, got " << sizes.size();
>>> +               return {};
>>> +       }
>>> +
>>> +       /*
>>> +        * The sum of all elements must be 0.5 to satisfy hardware constraints.
>>> +        * Validate it here, allowing a 1% tolerance as rounding errors may
>>> +        * prevent an exact match (further adjustments will be performed in
>>> +        * LensShadingCorrection::prepare()).
>>> +        */
>>> +       double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
>>> +       if (sum < 0.495 || sum > 0.505) {
>>> +               LOG(RkISP1Lsc, Error)
>>> +                       << "Invalid '" << prop << "' values: sum of the elements"
>>> +                       << " should be 0.5, got " << sum;
>>> +               return {};
>>> +       }
>>> +
>>> +       return sizes;
>>> +}
>>> +
>>>   /**
>>>    * \copydoc libcamera::ipa::Algorithm::configure
>>>    */
>>> diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
>>> index 2a428293cb2e..ee037353bbe4 100644
>>> --- a/src/ipa/rkisp1/algorithms/lsc.h
>>> +++ b/src/ipa/rkisp1/algorithms/lsc.h
>>> @@ -39,6 +39,8 @@ public:
>>>   private:
>>>          void setParameters(rkisp1_cif_isp_lsc_config &config);
>>>          void copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set0);
>>> +       std::vector<double> parseSizes(const ValueNode &tuningData,
>>> +                                      const char *prop);
>>>
>>>          std::vector<double> xSize_;
>>>          std::vector<double> ySize_;
>>>
>>> --
>>> 2.54.0
>>>

Patch
diff mbox series 

diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
index f32c46084dd1..e03bf361c4b9 100644
--- a/src/ipa/rkisp1/algorithms/lsc.cpp
+++ b/src/ipa/rkisp1/algorithms/lsc.cpp
@@ -34,36 +34,6 @@  namespace {
 
 constexpr int kColourTemperatureQuantization = 10;
 
-std::vector<double> parseSizes(const ValueNode &tuningData,
-			       const char *prop)
-{
-	std::vector<double> sizes =
-		tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
-	if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
-		LOG(RkISP1Lsc, Error)
-			<< "Invalid '" << prop << "' values: expected "
-			<< RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
-			<< " elements, got " << sizes.size();
-		return {};
-	}
-
-	/*
-	 * The sum of all elements must be 0.5 to satisfy hardware constraints.
-	 * Validate it here, allowing a 1% tolerance as rounding errors may
-	 * prevent an exact match (further adjustments will be performed in
-	 * LensShadingCorrection::prepare()).
-	 */
-	double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
-	if (sum < 0.495 || sum > 0.505) {
-		LOG(RkISP1Lsc, Error)
-			<< "Invalid '" << prop << "' values: sum of the elements"
-			<< " should be 0.5, got " << sum;
-		return {};
-	}
-
-	return sizes;
-}
-
 unsigned int quantize(unsigned int value, unsigned int step)
 {
 	return std::lround(value / static_cast<double>(step)) * step;
@@ -138,6 +108,36 @@  int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
 	return 0;
 }
 
+std::vector<double> LensShadingCorrection::parseSizes(const ValueNode &tuningData,
+						      const char *prop)
+{
+	std::vector<double> sizes =
+		tuningData[prop].get<std::vector<double>>().value_or(utils::defopt);
+	if (sizes.size() != RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE) {
+		LOG(RkISP1Lsc, Error)
+			<< "Invalid '" << prop << "' values: expected "
+			<< RKISP1_CIF_ISP_LSC_SECTORS_TBL_SIZE
+			<< " elements, got " << sizes.size();
+		return {};
+	}
+
+	/*
+	 * The sum of all elements must be 0.5 to satisfy hardware constraints.
+	 * Validate it here, allowing a 1% tolerance as rounding errors may
+	 * prevent an exact match (further adjustments will be performed in
+	 * LensShadingCorrection::prepare()).
+	 */
+	double sum = std::accumulate(sizes.begin(), sizes.end(), 0.0);
+	if (sum < 0.495 || sum > 0.505) {
+		LOG(RkISP1Lsc, Error)
+			<< "Invalid '" << prop << "' values: sum of the elements"
+			<< " should be 0.5, got " << sum;
+		return {};
+	}
+
+	return sizes;
+}
+
 /**
  * \copydoc libcamera::ipa::Algorithm::configure
  */
diff --git a/src/ipa/rkisp1/algorithms/lsc.h b/src/ipa/rkisp1/algorithms/lsc.h
index 2a428293cb2e..ee037353bbe4 100644
--- a/src/ipa/rkisp1/algorithms/lsc.h
+++ b/src/ipa/rkisp1/algorithms/lsc.h
@@ -39,6 +39,8 @@  public:
 private:
 	void setParameters(rkisp1_cif_isp_lsc_config &config);
 	void copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set0);
+	std::vector<double> parseSizes(const ValueNode &tuningData,
+				       const char *prop);
 
 	std::vector<double> xSize_;
 	std::vector<double> ySize_;