[v1] treewide: Do not use `*NameValueMap` for known values
diff mbox series

Message ID 20250522142921.1378425-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v1] treewide: Do not use `*NameValueMap` for known values
Related show

Commit Message

Barnabás Pőcze May 22, 2025, 2:29 p.m. UTC 
When the value is known, do not look it up via the control's `NameValueMap`,
instead, just refer to the value directly.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc_mean_luminance.cpp         |  5 ++---
 src/ipa/rkisp1/algorithms/agc.cpp             |  3 +--
 .../pipeline/virtual/config_parser.cpp        | 20 +++++++++++--------
 3 files changed, 15 insertions(+), 13 deletions(-)

Comments

Paul Elder May 23, 2025, 12:15 a.m. UTC | #1 
Quoting Barnabás Pőcze (2025-05-22 23:29:21)
> When the value is known, do not look it up via the control's `NameValueMap`,
> instead, just refer to the value directly.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>

> ---
>  src/ipa/libipa/agc_mean_luminance.cpp         |  5 ++---
>  src/ipa/rkisp1/algorithms/agc.cpp             |  3 +--
>  .../pipeline/virtual/config_parser.cpp        | 20 +++++++++++--------
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index f617fde81..d37a9b661 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -218,8 +218,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>                 constraintModes_[controls::ConstraintNormal].insert(
>                         constraintModes_[controls::ConstraintNormal].begin(),
>                         constraint);
> -               availableConstraintModes.push_back(
> -                       AeConstraintModeNameValueMap.at("ConstraintNormal"));
> +               availableConstraintModes.push_back(controls::ConstraintNormal);
>         }
>  
>         controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
> @@ -287,7 +286,7 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
>          * possible before touching gain.
>          */
>         if (availableExposureModes.empty()) {
> -               int32_t exposureModeId = AeExposureModeNameValueMap.at("ExposureNormal");
> +               int32_t exposureModeId = controls::ExposureNormal;
>                 std::vector<std::pair<utils::Duration, double>> stages = { };
>  
>                 std::shared_ptr<ExposureModeHelper> helper =
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index b3ac9400b..137a07500 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -68,10 +68,9 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
>         if (meteringModes_.empty()) {
>                 LOG(RkISP1Agc, Warning)
>                         << "No metering modes read from tuning file; defaulting to matrix";
> -               int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
>                 std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
>  
> -               meteringModes_[meteringModeId] = weights;
> +               meteringModes_[controls::MeteringMatrix] = weights;
>         }
>  
>         std::vector<ControlValue> meteringModes;
> diff --git a/src/libcamera/pipeline/virtual/config_parser.cpp b/src/libcamera/pipeline/virtual/config_parser.cpp
> index d9900add6..1d3d9ba87 100644
> --- a/src/libcamera/pipeline/virtual/config_parser.cpp
> +++ b/src/libcamera/pipeline/virtual/config_parser.cpp
> @@ -233,17 +233,21 @@ int ConfigParser::parseFrameGenerator(const YamlObject &cameraConfigData, Virtua
>  
>  int ConfigParser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)
>  {
> -       std::string location = cameraConfigData["location"].get<std::string>("CameraLocationFront");
> -
>         /* Default value is properties::CameraLocationFront */
> -       auto it = properties::LocationNameValueMap.find(location);
> -       if (it == properties::LocationNameValueMap.end()) {
> -               LOG(Virtual, Error)
> -                       << "location: " << location << " is not supported";
> -               return -EINVAL;
> +       int32_t location = properties::CameraLocationFront;
> +
> +       if (auto l = cameraConfigData["location"].get<std::string>()) {
> +               auto it = properties::LocationNameValueMap.find(*l);
> +               if (it == properties::LocationNameValueMap.end()) {
> +                       LOG(Virtual, Error)
> +                               << "location: " << *l << " is not supported";
> +                       return -EINVAL;
> +               }
> +
> +               location = it->second;
>         }
>  
> -       data->properties_.set(properties::Location, it->second);
> +       data->properties_.set(properties::Location, location);
>  
>         return 0;
>  }
> -- 
> 2.49.0
>
Laurent Pinchart May 26, 2025, 3:40 p.m. UTC | #2 
Hi Barnabás,

Thank you for the patch.

On Thu, May 22, 2025 at 04:29:21PM +0200, Barnabás Pőcze wrote:
> When the value is known, do not look it up via the control's `NameValueMap`,
> instead, just refer to the value directly.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp         |  5 ++---
>  src/ipa/rkisp1/algorithms/agc.cpp             |  3 +--
>  .../pipeline/virtual/config_parser.cpp        | 20 +++++++++++--------
>  3 files changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index f617fde81..d37a9b661 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -218,8 +218,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>  		constraintModes_[controls::ConstraintNormal].insert(
>  			constraintModes_[controls::ConstraintNormal].begin(),
>  			constraint);
> -		availableConstraintModes.push_back(
> -			AeConstraintModeNameValueMap.at("ConstraintNormal"));
> +		availableConstraintModes.push_back(controls::ConstraintNormal);

I'm tempted to use controls::AeConstraintModeEnum::ConstraintNormal (and
similarly below) already, but I suppose this should be a separate
tree-wide change.

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

>  	}
>  
>  	controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
> @@ -287,7 +286,7 @@ int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
>  	 * possible before touching gain.
>  	 */
>  	if (availableExposureModes.empty()) {
> -		int32_t exposureModeId = AeExposureModeNameValueMap.at("ExposureNormal");
> +		int32_t exposureModeId = controls::ExposureNormal;
>  		std::vector<std::pair<utils::Duration, double>> stages = { };
>  
>  		std::shared_ptr<ExposureModeHelper> helper =
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index b3ac9400b..137a07500 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -68,10 +68,9 @@ int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
>  	if (meteringModes_.empty()) {
>  		LOG(RkISP1Agc, Warning)
>  			<< "No metering modes read from tuning file; defaulting to matrix";
> -		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
>  		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
>  
> -		meteringModes_[meteringModeId] = weights;
> +		meteringModes_[controls::MeteringMatrix] = weights;
>  	}
>  
>  	std::vector<ControlValue> meteringModes;
> diff --git a/src/libcamera/pipeline/virtual/config_parser.cpp b/src/libcamera/pipeline/virtual/config_parser.cpp
> index d9900add6..1d3d9ba87 100644
> --- a/src/libcamera/pipeline/virtual/config_parser.cpp
> +++ b/src/libcamera/pipeline/virtual/config_parser.cpp
> @@ -233,17 +233,21 @@ int ConfigParser::parseFrameGenerator(const YamlObject &cameraConfigData, Virtua
>  
>  int ConfigParser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)
>  {
> -	std::string location = cameraConfigData["location"].get<std::string>("CameraLocationFront");
> -
>  	/* Default value is properties::CameraLocationFront */
> -	auto it = properties::LocationNameValueMap.find(location);
> -	if (it == properties::LocationNameValueMap.end()) {
> -		LOG(Virtual, Error)
> -			<< "location: " << location << " is not supported";
> -		return -EINVAL;
> +	int32_t location = properties::CameraLocationFront;
> +
> +	if (auto l = cameraConfigData["location"].get<std::string>()) {
> +		auto it = properties::LocationNameValueMap.find(*l);
> +		if (it == properties::LocationNameValueMap.end()) {
> +			LOG(Virtual, Error)
> +				<< "location: " << *l << " is not supported";
> +			return -EINVAL;
> +		}
> +
> +		location = it->second;
>  	}
>  
> -	data->properties_.set(properties::Location, it->second);
> +	data->properties_.set(properties::Location, location);
>  
>  	return 0;
>  }

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index f617fde81..d37a9b661 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -218,8 +218,7 @@  int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
 		constraintModes_[controls::ConstraintNormal].insert(
 			constraintModes_[controls::ConstraintNormal].begin(),
 			constraint);
-		availableConstraintModes.push_back(
-			AeConstraintModeNameValueMap.at("ConstraintNormal"));
+		availableConstraintModes.push_back(controls::ConstraintNormal);
 	}
 
 	controls_[&controls::AeConstraintMode] = ControlInfo(availableConstraintModes);
@@ -287,7 +286,7 @@  int AgcMeanLuminance::parseExposureModes(const YamlObject &tuningData)
 	 * possible before touching gain.
 	 */
 	if (availableExposureModes.empty()) {
-		int32_t exposureModeId = AeExposureModeNameValueMap.at("ExposureNormal");
+		int32_t exposureModeId = controls::ExposureNormal;
 		std::vector<std::pair<utils::Duration, double>> stages = { };
 
 		std::shared_ptr<ExposureModeHelper> helper =
diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index b3ac9400b..137a07500 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -68,10 +68,9 @@  int Agc::parseMeteringModes(IPAContext &context, const YamlObject &tuningData)
 	if (meteringModes_.empty()) {
 		LOG(RkISP1Agc, Warning)
 			<< "No metering modes read from tuning file; defaulting to matrix";
-		int32_t meteringModeId = controls::AeMeteringModeNameValueMap.at("MeteringMatrix");
 		std::vector<uint8_t> weights(context.hw->numHistogramWeights, 1);
 
-		meteringModes_[meteringModeId] = weights;
+		meteringModes_[controls::MeteringMatrix] = weights;
 	}
 
 	std::vector<ControlValue> meteringModes;
diff --git a/src/libcamera/pipeline/virtual/config_parser.cpp b/src/libcamera/pipeline/virtual/config_parser.cpp
index d9900add6..1d3d9ba87 100644
--- a/src/libcamera/pipeline/virtual/config_parser.cpp
+++ b/src/libcamera/pipeline/virtual/config_parser.cpp
@@ -233,17 +233,21 @@  int ConfigParser::parseFrameGenerator(const YamlObject &cameraConfigData, Virtua
 
 int ConfigParser::parseLocation(const YamlObject &cameraConfigData, VirtualCameraData *data)
 {
-	std::string location = cameraConfigData["location"].get<std::string>("CameraLocationFront");
-
 	/* Default value is properties::CameraLocationFront */
-	auto it = properties::LocationNameValueMap.find(location);
-	if (it == properties::LocationNameValueMap.end()) {
-		LOG(Virtual, Error)
-			<< "location: " << location << " is not supported";
-		return -EINVAL;
+	int32_t location = properties::CameraLocationFront;
+
+	if (auto l = cameraConfigData["location"].get<std::string>()) {
+		auto it = properties::LocationNameValueMap.find(*l);
+		if (it == properties::LocationNameValueMap.end()) {
+			LOG(Virtual, Error)
+				<< "location: " << *l << " is not supported";
+			return -EINVAL;
+		}
+
+		location = it->second;
 	}
 
-	data->properties_.set(properties::Location, it->second);
+	data->properties_.set(properties::Location, location);
 
 	return 0;
 }