[libcamera-devel,v1,2/3] ipa: rpi: agc: Use std::string instead of char arrays

18717 diff mbox series

Replace the char array strings in struct AgcStatus with std::string
objects. This simplifies the string handling in the source code.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 src/ipa/rpi/controller/agc_status.h |  8 +++++---
 src/ipa/rpi/controller/rpi/agc.cpp  | 23 +++++++----------------
 2 files changed, 12 insertions(+), 19 deletions(-)

Hi Naush,

Quoting Naushir Patuck via libcamera-devel (2023-06-07 11:00:53)
> Replace the char array strings in struct AgcStatus with std::string
> objects. This simplifies the string handling in the source code.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

This seems better/cleaner indeed.

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

> ---
>  src/ipa/rpi/controller/agc_status.h |  8 +++++---
>  src/ipa/rpi/controller/rpi/agc.cpp  | 23 +++++++----------------
>  2 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h
> index 6abf09d9df57..6c112e76aa12 100644
> --- a/src/ipa/rpi/controller/agc_status.h
> +++ b/src/ipa/rpi/controller/agc_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>  
> +#include <string>
> +
>  #include <libcamera/base/utils.h>
>  
>  /*
> @@ -24,9 +26,9 @@ struct AgcStatus {
>         libcamera::utils::Duration targetExposureValue; /* (unfiltered) target total exposure AGC is aiming for */
>         libcamera::utils::Duration shutterTime;
>         double analogueGain;
> -       char exposureMode[32];
> -       char constraintMode[32];
> -       char meteringMode[32];
> +       std::string exposureMode;
> +       std::string constraintMode;
> +       std::string meteringMode;
>         double ev;
>         libcamera::utils::Duration flickerPeriod;
>         int floatingRegionEnable;
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e79c82e2e65b..b611157af1f0 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -226,7 +226,7 @@ Agc::Agc(Controller *controller)
>          * Setting status_.totalExposureValue_ to zero initially tells us
>          * it's not been calculated yet (i.e. Process hasn't yet run).
>          */
> -       memset(&status_, 0, sizeof(status_));
> +       status_ = {};
>         status_.ev = ev_;
>  }
>  
> @@ -524,12 +524,6 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)
>         status_.locked = lockCount_ == maxLockCount;
>  }
>  
> -static void copyString(std::string const &s, char *d, size_t size)
> -{
> -       size_t length = s.copy(d, size - 1);
> -       d[length] = '\0';
> -}
> -
>  void Agc::housekeepConfig()
>  {
>         /* First fetch all the up-to-date settings, so no one else has to do it. */
> @@ -544,30 +538,27 @@ void Agc::housekeepConfig()
>          * Make sure the "mode" pointers point to the up-to-date things, if
>          * they've changed.
>          */
> -       if (strcmp(meteringModeName_.c_str(), status_.meteringMode)) {
> +       if (meteringModeName_ != status_.meteringMode) {
>                 auto it = config_.meteringModes.find(meteringModeName_);
>                 if (it == config_.meteringModes.end())
>                         LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_;
>                 meteringMode_ = &it->second;
> -               copyString(meteringModeName_, status_.meteringMode,
> -                          sizeof(status_.meteringMode));
> +               status_.meteringMode = meteringModeName_;
>         }
> -       if (strcmp(exposureModeName_.c_str(), status_.exposureMode)) {
> +       if (exposureModeName_ != status_.exposureMode) {
>                 auto it = config_.exposureModes.find(exposureModeName_);
>                 if (it == config_.exposureModes.end())
>                         LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_;
>                 exposureMode_ = &it->second;
> -               copyString(exposureModeName_, status_.exposureMode,
> -                          sizeof(status_.exposureMode));
> +               status_.exposureMode = exposureModeName_;
>         }
> -       if (strcmp(constraintModeName_.c_str(), status_.constraintMode)) {
> +       if (constraintModeName_ != status_.constraintMode) {
>                 auto it =
>                         config_.constraintModes.find(constraintModeName_);
>                 if (it == config_.constraintModes.end())
>                         LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_;
>                 constraintMode_ = &it->second;
> -               copyString(constraintModeName_, status_.constraintMode,
> -                          sizeof(status_.constraintMode));
> +               status_.constraintMode = constraintModeName_;
>         }
>         LOG(RPiAgc, Debug) << "exposureMode "
>                            << exposureModeName_ << " constraintMode "
> -- 
> 2.34.1
>

Hi Naush,

Thank you for the patch.

On Wed, Jun 07, 2023 at 11:00:53AM +0100, Naushir Patuck via libcamera-devel wrote:
> Replace the char array strings in struct AgcStatus with std::string
> objects. This simplifies the string handling in the source code.
> 
> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

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

I wonder, however, if it wouldn't be even better to switch the Agc class
API to using enums instead of strings for the modes. ipa_base.cpp
converts from enums to strings, the conversion would be better placed in
agc.cpp I think. This would allow turning the cached mode names into
enums too, taking up less space, and making the comparison operations
more effective.

> ---
>  src/ipa/rpi/controller/agc_status.h |  8 +++++---
>  src/ipa/rpi/controller/rpi/agc.cpp  | 23 +++++++----------------
>  2 files changed, 12 insertions(+), 19 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/agc_status.h b/src/ipa/rpi/controller/agc_status.h
> index 6abf09d9df57..6c112e76aa12 100644
> --- a/src/ipa/rpi/controller/agc_status.h
> +++ b/src/ipa/rpi/controller/agc_status.h
> @@ -6,6 +6,8 @@
>   */
>  #pragma once
>  
> +#include <string>
> +
>  #include <libcamera/base/utils.h>
>  
>  /*
> @@ -24,9 +26,9 @@ struct AgcStatus {
>  	libcamera::utils::Duration targetExposureValue; /* (unfiltered) target total exposure AGC is aiming for */
>  	libcamera::utils::Duration shutterTime;
>  	double analogueGain;
> -	char exposureMode[32];
> -	char constraintMode[32];
> -	char meteringMode[32];
> +	std::string exposureMode;
> +	std::string constraintMode;
> +	std::string meteringMode;
>  	double ev;
>  	libcamera::utils::Duration flickerPeriod;
>  	int floatingRegionEnable;
> diff --git a/src/ipa/rpi/controller/rpi/agc.cpp b/src/ipa/rpi/controller/rpi/agc.cpp
> index e79c82e2e65b..b611157af1f0 100644
> --- a/src/ipa/rpi/controller/rpi/agc.cpp
> +++ b/src/ipa/rpi/controller/rpi/agc.cpp
> @@ -226,7 +226,7 @@ Agc::Agc(Controller *controller)
>  	 * Setting status_.totalExposureValue_ to zero initially tells us
>  	 * it's not been calculated yet (i.e. Process hasn't yet run).
>  	 */
> -	memset(&status_, 0, sizeof(status_));
> +	status_ = {};
>  	status_.ev = ev_;
>  }
>  
> @@ -524,12 +524,6 @@ void Agc::updateLockStatus(DeviceStatus const &deviceStatus)
>  	status_.locked = lockCount_ == maxLockCount;
>  }
>  
> -static void copyString(std::string const &s, char *d, size_t size)
> -{
> -	size_t length = s.copy(d, size - 1);
> -	d[length] = '\0';
> -}
> -
>  void Agc::housekeepConfig()
>  {
>  	/* First fetch all the up-to-date settings, so no one else has to do it. */
> @@ -544,30 +538,27 @@ void Agc::housekeepConfig()
>  	 * Make sure the "mode" pointers point to the up-to-date things, if
>  	 * they've changed.
>  	 */
> -	if (strcmp(meteringModeName_.c_str(), status_.meteringMode)) {
> +	if (meteringModeName_ != status_.meteringMode) {
>  		auto it = config_.meteringModes.find(meteringModeName_);
>  		if (it == config_.meteringModes.end())
>  			LOG(RPiAgc, Fatal) << "No metering mode " << meteringModeName_;
>  		meteringMode_ = &it->second;
> -		copyString(meteringModeName_, status_.meteringMode,
> -			   sizeof(status_.meteringMode));
> +		status_.meteringMode = meteringModeName_;
>  	}
> -	if (strcmp(exposureModeName_.c_str(), status_.exposureMode)) {
> +	if (exposureModeName_ != status_.exposureMode) {
>  		auto it = config_.exposureModes.find(exposureModeName_);
>  		if (it == config_.exposureModes.end())
>  			LOG(RPiAgc, Fatal) << "No exposure profile " << exposureModeName_;
>  		exposureMode_ = &it->second;
> -		copyString(exposureModeName_, status_.exposureMode,
> -			   sizeof(status_.exposureMode));
> +		status_.exposureMode = exposureModeName_;
>  	}
> -	if (strcmp(constraintModeName_.c_str(), status_.constraintMode)) {
> +	if (constraintModeName_ != status_.constraintMode) {
>  		auto it =
>  			config_.constraintModes.find(constraintModeName_);
>  		if (it == config_.constraintModes.end())
>  			LOG(RPiAgc, Fatal) << "No constraint list " << constraintModeName_;
>  		constraintMode_ = &it->second;
> -		copyString(constraintModeName_, status_.constraintMode,
> -			   sizeof(status_.constraintMode));
> +		status_.constraintMode = constraintModeName_;
>  	}
>  	LOG(RPiAgc, Debug) << "exposureMode "
>  			   << exposureModeName_ << " constraintMode "