[RFC] ipa: rkisp1: agc: Initialize enum controls with a list of values
diff mbox series

Message ID 20250123120918.86780-1-stefan.klug@ideasonboard.com
State Accepted
Commit ee918b370a08b362de75ee590cfc46fe8e38a0f0
Headers show
Series
  • [RFC] ipa: rkisp1: agc: Initialize enum controls with a list of values
Related show

Commit Message

Stefan Klug Jan. 23, 2025, 12:09 p.m. UTC
The controls ExposureTimeMode and AnalogueGainMode are shown in camshark
as normal int entries instead of enum popups. The reason is that
ControlInfos for these controls are constructed using min/max instead
of a list of valid ControlValues. Camshark uses the values() vector to
deduce if the control is an enum or not. It might be debatable if this
is the correct check, but all other ControlInfos for enum controls in
libcamera are initialized using a list.

Modify the construction of the ControlInfos to use the Span based
constructor to fix that issue.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Comments

Laurent Pinchart Jan. 23, 2025, 10:56 p.m. UTC | #1
Hi Stefan,

Thank you for the patch.

On Thu, Jan 23, 2025 at 01:09:14PM +0100, Stefan Klug wrote:
> The controls ExposureTimeMode and AnalogueGainMode are shown in camshark
> as normal int entries instead of enum popups. The reason is that
> ControlInfos for these controls are constructed using min/max instead
> of a list of valid ControlValues. Camshark uses the values() vector to
> deduce if the control is an enum or not. It might be debatable if this
> is the correct check, but all other ControlInfos for enum controls in
> libcamera are initialized using a list.

Maybe the Control (or ControlId) class should report if a control is an
enum. We don't have that now though, and even if we did, this patch
looks good as it makes the two controls consistent with the other
enumerated controls.

> Modify the construction of the ControlInfos to use the Span based
> constructor to fix that issue.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>

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

Could you please do the same for the RPi IPA module ?

> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1680669c6875..e7c6de757593 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -149,13 +149,13 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>  		return ret;
>  
>  	context.ctrlMap[&controls::ExposureTimeMode] =
> -		ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> -			    static_cast<int32_t>(controls::ExposureTimeModeManual),
> -			    static_cast<int32_t>(controls::ExposureTimeModeAuto));
> +		ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
> +				ControlValue(controls::ExposureTimeModeManual) } },
> +			    controls::ExposureTimeModeAuto);
>  	context.ctrlMap[&controls::AnalogueGainMode] =
> -		ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> -			    static_cast<int32_t>(controls::AnalogueGainModeManual),
> -			    static_cast<int32_t>(controls::AnalogueGainModeAuto));
> +		ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
> +				ControlValue(controls::AnalogueGainModeManual) } },
> +			    controls::AnalogueGainModeAuto);
>  	/* \todo Move this to the Camera class */
>  	context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
>  	context.ctrlMap.merge(controls());
Paul Elder Jan. 27, 2025, 9:16 a.m. UTC | #2
On Thu, Jan 23, 2025 at 01:09:14PM +0100, Stefan Klug wrote:
> The controls ExposureTimeMode and AnalogueGainMode are shown in camshark
> as normal int entries instead of enum popups. The reason is that
> ControlInfos for these controls are constructed using min/max instead
> of a list of valid ControlValues. Camshark uses the values() vector to
> deduce if the control is an enum or not. It might be debatable if this
> is the correct check, but all other ControlInfos for enum controls in

I think that is the correct way. (I guess technically you could also
check ControlId::enumerators(), but in cam we check
info.values().empty() first before iterating over enumerators() anyway)

> libcamera are initialized using a list.
> 
> Modify the construction of the ControlInfos to use the Span based
> constructor to fix that issue.
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/agc.cpp | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index 1680669c6875..e7c6de757593 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -149,13 +149,13 @@ int Agc::init(IPAContext &context, const YamlObject &tuningData)
>  		return ret;
>  
>  	context.ctrlMap[&controls::ExposureTimeMode] =
> -		ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> -			    static_cast<int32_t>(controls::ExposureTimeModeManual),
> -			    static_cast<int32_t>(controls::ExposureTimeModeAuto));
> +		ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
> +				ControlValue(controls::ExposureTimeModeManual) } },
> +			    controls::ExposureTimeModeAuto);
>  	context.ctrlMap[&controls::AnalogueGainMode] =
> -		ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> -			    static_cast<int32_t>(controls::AnalogueGainModeManual),
> -			    static_cast<int32_t>(controls::AnalogueGainModeAuto));
> +		ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
> +				ControlValue(controls::AnalogueGainModeManual) } },
> +			    controls::AnalogueGainModeAuto);

Looks good to me.


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

>  	/* \todo Move this to the Camera class */
>  	context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
>  	context.ctrlMap.merge(controls());
> -- 
> 2.43.0
>

Patch
diff mbox series

diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
index 1680669c6875..e7c6de757593 100644
--- a/src/ipa/rkisp1/algorithms/agc.cpp
+++ b/src/ipa/rkisp1/algorithms/agc.cpp
@@ -149,13 +149,13 @@  int Agc::init(IPAContext &context, const YamlObject &tuningData)
 		return ret;
 
 	context.ctrlMap[&controls::ExposureTimeMode] =
-		ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
-			    static_cast<int32_t>(controls::ExposureTimeModeManual),
-			    static_cast<int32_t>(controls::ExposureTimeModeAuto));
+		ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
+				ControlValue(controls::ExposureTimeModeManual) } },
+			    controls::ExposureTimeModeAuto);
 	context.ctrlMap[&controls::AnalogueGainMode] =
-		ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
-			    static_cast<int32_t>(controls::AnalogueGainModeManual),
-			    static_cast<int32_t>(controls::AnalogueGainModeAuto));
+		ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
+				ControlValue(controls::AnalogueGainModeManual) } },
+			    controls::AnalogueGainModeAuto);
 	/* \todo Move this to the Camera class */
 	context.ctrlMap[&controls::AeEnable] = ControlInfo(false, true, true);
 	context.ctrlMap.merge(controls());