[libcamera-devel] ipa: rpi: cac: Minor code improvements and tidying

19241 diff mbox series

We make a few small improvements to the code:

* The arrayToSet method is prevented from overwriting the end of the
  array if there are too many values in the input table. If you supply
  a table, it will force you to put the correct number of elements in
  it.

* The arrayToSet and setStrength member functions are turned into
  static functions. (There may be a different public setStrength
  member function in future.)

* When no tables at all are given, the configuration is flagged as
  being disabled, so that we can avoid copying tables full of zeroes
  around. As a consequence, the pipeline handler too will disable this
  hardware block rather than run it needlessly. (Note that the tuning
  tool will put in a completely empty "rpi.cac" block if no CAC tuning
  images are supplied, benefiting from this behaviour.)

* The initialise member function is removed as it does nothing.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/ipa/rpi/controller/rpi/cac.cpp | 82 ++++++++++++++++++++----------
 src/ipa/rpi/controller/rpi/cac.h   |  5 +-
 2 files changed, 55 insertions(+), 32 deletions(-)

Hi David,

Thank you for this patch.

On Fri, 24 Nov 2023 at 16:37, David Plowman via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> We make a few small improvements to the code:
>
> * The arrayToSet method is prevented from overwriting the end of the
>   array if there are too many values in the input table. If you supply
>   a table, it will force you to put the correct number of elements in
>   it.
>
> * The arrayToSet and setStrength member functions are turned into
>   static functions. (There may be a different public setStrength
>   member function in future.)
>
> * When no tables at all are given, the configuration is flagged as
>   being disabled, so that we can avoid copying tables full of zeroes
>   around. As a consequence, the pipeline handler too will disable this
>   hardware block rather than run it needlessly. (Note that the tuning
>   tool will put in a completely empty "rpi.cac" block if no CAC tuning
>   images are supplied, benefiting from this behaviour.)
>
> * The initialise member function is removed as it does nothing.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
>  src/ipa/rpi/controller/rpi/cac.cpp | 82 ++++++++++++++++++++----------
>  src/ipa/rpi/controller/rpi/cac.h   |  5 +-
>  2 files changed, 55 insertions(+), 32 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/cac.cpp b/src/ipa/rpi/controller/rpi/cac.cpp
> index 7c123da1..f2c8d282 100644
> --- a/src/ipa/rpi/controller/rpi/cac.cpp
> +++ b/src/ipa/rpi/controller/rpi/cac.cpp
> @@ -27,40 +27,23 @@ char const *Cac::name() const
>         return NAME;
>  }
>
> -int Cac::read(const libcamera::YamlObject &params)
> -{
> -       arrayToSet(params["lut_rx"], config_.lutRx);
> -       arrayToSet(params["lut_ry"], config_.lutRy);
> -       arrayToSet(params["lut_bx"], config_.lutBx);
> -       arrayToSet(params["lut_by"], config_.lutBy);
> -       cacStatus_.lutRx = config_.lutRx;
> -       cacStatus_.lutRy = config_.lutRy;
> -       cacStatus_.lutBx = config_.lutBx;
> -       cacStatus_.lutBy = config_.lutBy;
> -       double strength = params["strength"].get<double>(1);
> -       setStrength(config_.lutRx, cacStatus_.lutRx, strength);
> -       setStrength(config_.lutBx, cacStatus_.lutBx, strength);
> -       setStrength(config_.lutRy, cacStatus_.lutRy, strength);
> -       setStrength(config_.lutBy, cacStatus_.lutBy, strength);
> -       return 0;
> -}
> -
> -void Cac::initialise()
> -{
> -}
> -
> -void Cac::arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray)
> +static bool arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray, const Size &size)
>  {
>         int num = 0;
> -       const Size &size = getHardwareConfig().cacRegions;
> -       inputArray.resize((size.width + 1) * (size.height + 1));
> +       int max_num = (size.width + 1) * (size.height + 1);
> +       inputArray.resize(max_num);
> +
>         for (const auto &p : params.asList()) {
> +               if (num == max_num)
> +                       return false;
>                 inputArray[num++] = p.get<double>(0);
>         }
> +
> +       return num == max_num;
>  }
>
> -void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> -                     double strengthFactor)
> +static void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> +                       double strengthFactor)
>  {
>         int num = 0;
>         for (const auto &p : inputArray) {
> @@ -68,9 +51,52 @@ void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outp
>         }
>  }
>
> +int Cac::read(const libcamera::YamlObject &params)
> +{
> +       config_.enabled = params.contains("lut_rx") && params.contains("lut_ry") &&
> +                         params.contains("lut_bx") && params.contains("lut_by");
> +       if (!config_.enabled)
> +               return 0;
> +
> +       const Size &size = getHardwareConfig().cacRegions;
> +
> +       if (!arrayToSet(params["lut_rx"], config_.lutRx, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_rx table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_ry"], config_.lutRy, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_ry table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_bx"], config_.lutBx, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_bx table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_by"], config_.lutBy, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_by table";
> +               return -EINVAL;
> +       }
> +
> +       double strength = params["strength"].get<double>(1);
> +       cacStatus_.lutRx = config_.lutRx;
> +       cacStatus_.lutRy = config_.lutRy;
> +       cacStatus_.lutBx = config_.lutBx;
> +       cacStatus_.lutBy = config_.lutBy;
> +       setStrength(config_.lutRx, cacStatus_.lutRx, strength);
> +       setStrength(config_.lutBx, cacStatus_.lutBx, strength);
> +       setStrength(config_.lutRy, cacStatus_.lutRy, strength);
> +       setStrength(config_.lutBy, cacStatus_.lutBy, strength);
> +
> +       return 0;
> +}
> +
>  void Cac::prepare(Metadata *imageMetadata)
>  {
> -       imageMetadata->set("cac.status", cacStatus_);
> +       if (config_.enabled)
> +               imageMetadata->set("cac.status", cacStatus_);
>  }
>
>  // Register algorithm with the system.
> diff --git a/src/ipa/rpi/controller/rpi/cac.h b/src/ipa/rpi/controller/rpi/cac.h
> index 419180ab..a7b14c00 100644
> --- a/src/ipa/rpi/controller/rpi/cac.h
> +++ b/src/ipa/rpi/controller/rpi/cac.h
> @@ -12,6 +12,7 @@
>  namespace RPiController {
>
>  struct CacConfig {
> +       bool enabled;
>         std::vector<double> lutRx;
>         std::vector<double> lutRy;
>         std::vector<double> lutBx;
> @@ -24,15 +25,11 @@ public:
>         Cac(Controller *controller = NULL);
>         char const *name() const override;
>         int read(const libcamera::YamlObject &params) override;
> -       void initialise() override;
>         void prepare(Metadata *imageMetadata) override;
> -       void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> -                        double strengthFactor);
>
>  private:
>         CacConfig config_;
>         CacStatus cacStatus_;
> -       void arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray);
>  };
>
>  } // namespace RPiController
> --
> 2.39.2
>

Quoting David Plowman via libcamera-devel (2023-11-24 16:37:42)
> We make a few small improvements to the code:

Four bullet points in a commit message is usually a sign that the patch
should be split ... but this is fairly small and all contained under
src/ipa/rpi ... so I'll not dwell on that here. Just harder to review
...

> 
> * The arrayToSet method is prevented from overwriting the end of the
>   array if there are too many values in the input table. If you supply
>   a table, it will force you to put the correct number of elements in
>   it.

Ack.

> 
> * The arrayToSet and setStrength member functions are turned into
>   static functions. (There may be a different public setStrength
>   member function in future.)

Ack,

> 
> * When no tables at all are given, the configuration is flagged as
>   being disabled, so that we can avoid copying tables full of zeroes
>   around. As a consequence, the pipeline handler too will disable this
>   hardware block rather than run it needlessly. (Note that the tuning
>   tool will put in a completely empty "rpi.cac" block if no CAC tuning
>   images are supplied, benefiting from this behaviour.)

Ack,

> 
> * The initialise member function is removed as it does nothing.

Ack,


Ok - they check out so 


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  src/ipa/rpi/controller/rpi/cac.cpp | 82 ++++++++++++++++++++----------
>  src/ipa/rpi/controller/rpi/cac.h   |  5 +-
>  2 files changed, 55 insertions(+), 32 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/cac.cpp b/src/ipa/rpi/controller/rpi/cac.cpp
> index 7c123da1..f2c8d282 100644
> --- a/src/ipa/rpi/controller/rpi/cac.cpp
> +++ b/src/ipa/rpi/controller/rpi/cac.cpp
> @@ -27,40 +27,23 @@ char const *Cac::name() const
>         return NAME;
>  }
>  
> -int Cac::read(const libcamera::YamlObject &params)
> -{
> -       arrayToSet(params["lut_rx"], config_.lutRx);
> -       arrayToSet(params["lut_ry"], config_.lutRy);
> -       arrayToSet(params["lut_bx"], config_.lutBx);
> -       arrayToSet(params["lut_by"], config_.lutBy);
> -       cacStatus_.lutRx = config_.lutRx;
> -       cacStatus_.lutRy = config_.lutRy;
> -       cacStatus_.lutBx = config_.lutBx;
> -       cacStatus_.lutBy = config_.lutBy;
> -       double strength = params["strength"].get<double>(1);
> -       setStrength(config_.lutRx, cacStatus_.lutRx, strength);
> -       setStrength(config_.lutBx, cacStatus_.lutBx, strength);
> -       setStrength(config_.lutRy, cacStatus_.lutRy, strength);
> -       setStrength(config_.lutBy, cacStatus_.lutBy, strength);
> -       return 0;
> -}
> -
> -void Cac::initialise()
> -{
> -}
> -
> -void Cac::arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray)
> +static bool arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray, const Size &size)
>  {
>         int num = 0;
> -       const Size &size = getHardwareConfig().cacRegions;
> -       inputArray.resize((size.width + 1) * (size.height + 1));
> +       int max_num = (size.width + 1) * (size.height + 1);
> +       inputArray.resize(max_num);
> +
>         for (const auto &p : params.asList()) {
> +               if (num == max_num)
> +                       return false;
>                 inputArray[num++] = p.get<double>(0);
>         }
> +
> +       return num == max_num;
>  }
>  
> -void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> -                     double strengthFactor)
> +static void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> +                       double strengthFactor)
>  {
>         int num = 0;
>         for (const auto &p : inputArray) {
> @@ -68,9 +51,52 @@ void Cac::setStrength(std::vector<double> &inputArray, std::vector<double> &outp
>         }
>  }
>  
> +int Cac::read(const libcamera::YamlObject &params)
> +{
> +       config_.enabled = params.contains("lut_rx") && params.contains("lut_ry") &&
> +                         params.contains("lut_bx") && params.contains("lut_by");
> +       if (!config_.enabled)
> +               return 0;
> +
> +       const Size &size = getHardwareConfig().cacRegions;
> +
> +       if (!arrayToSet(params["lut_rx"], config_.lutRx, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_rx table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_ry"], config_.lutRy, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_ry table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_bx"], config_.lutBx, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_bx table";
> +               return -EINVAL;
> +       }
> +
> +       if (!arrayToSet(params["lut_by"], config_.lutBy, size)) {
> +               LOG(RPiCac, Error) << "Bad CAC lut_by table";
> +               return -EINVAL;
> +       }
> +
> +       double strength = params["strength"].get<double>(1);
> +       cacStatus_.lutRx = config_.lutRx;
> +       cacStatus_.lutRy = config_.lutRy;
> +       cacStatus_.lutBx = config_.lutBx;
> +       cacStatus_.lutBy = config_.lutBy;
> +       setStrength(config_.lutRx, cacStatus_.lutRx, strength);
> +       setStrength(config_.lutBx, cacStatus_.lutBx, strength);
> +       setStrength(config_.lutRy, cacStatus_.lutRy, strength);
> +       setStrength(config_.lutBy, cacStatus_.lutBy, strength);
> +
> +       return 0;
> +}
> +
>  void Cac::prepare(Metadata *imageMetadata)
>  {
> -       imageMetadata->set("cac.status", cacStatus_);
> +       if (config_.enabled)
> +               imageMetadata->set("cac.status", cacStatus_);
>  }
>  
>  // Register algorithm with the system.
> diff --git a/src/ipa/rpi/controller/rpi/cac.h b/src/ipa/rpi/controller/rpi/cac.h
> index 419180ab..a7b14c00 100644
> --- a/src/ipa/rpi/controller/rpi/cac.h
> +++ b/src/ipa/rpi/controller/rpi/cac.h
> @@ -12,6 +12,7 @@
>  namespace RPiController {
>  
>  struct CacConfig {
> +       bool enabled;
>         std::vector<double> lutRx;
>         std::vector<double> lutRy;
>         std::vector<double> lutBx;
> @@ -24,15 +25,11 @@ public:
>         Cac(Controller *controller = NULL);
>         char const *name() const override;
>         int read(const libcamera::YamlObject &params) override;
> -       void initialise() override;
>         void prepare(Metadata *imageMetadata) override;
> -       void setStrength(std::vector<double> &inputArray, std::vector<double> &outputArray,
> -                        double strengthFactor);
>  
>  private:
>         CacConfig config_;
>         CacStatus cacStatus_;
> -       void arrayToSet(const libcamera::YamlObject &params, std::vector<double> &inputArray);
>  };
>  
>  } // namespace RPiController
> -- 
> 2.39.2
>