[libcamera-devel] [PATCH v1 2/3] ipa: rpi: agc: Use std::string instead of char arrays
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jun 7 17:02:08 CEST 2023
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 at raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart at 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 "
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list