[PATCH v6 10/12] libcamera: camera: Pre-process AeEnable control

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Wed, Jan 08, 2025 at 06:09:40PM -0600, Paul Elder wrote:
> Handle the AeEnable under-the-hood in the Camera class, such that

s/under-the-hood/under the hood/

> AeEnable activates ExposureTimeMode and AnalogueGain together. This
> allows applications the convenience of setting auto/manual mode of all
> of the AE-related controls, as well as protecting applications against a
> nasty behavior change if an aperture control is added in the future.
> This also moves common handling code out of the IPA.
> 
> While we also want to inject AeEnable in Camera::controls() so that IPAs
> don't have to report it, it is technically difficult at the moment as
> ControlInfoMaps are not easily modifiable.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> New in v6
> ---
>  src/libcamera/camera.cpp | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 69a7ee535..4e0f63930 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -19,6 +19,7 @@
>  #include <libcamera/base/thread.h>
>  
>  #include <libcamera/color_space.h>
> +#include <libcamera/control_ids.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -1325,6 +1326,23 @@ int Camera::queueRequest(Request *request)
>  		}
>  	}
>  
> +	/* Pre-process AeEnable */

s/AeEnable/AeEnable./

> +	ControlList &controls = request->controls();
> +	const auto &aeEnable = controls.get(controls::AeEnable);
> +	if (aeEnable) {
> +		if (!controls.contains(controls::AnalogueGainMode.id())) {

I think we need to first check that the camera supports the
AnalogueGainMode control. Do we also need to check what values the
control support, or can we assume that if the camera exposes AeEnable
and AnalogueGainMode than both auto and manual modes will be available ?

> +			controls.set(controls::AnalogueGainMode,
> +				     *aeEnable ? controls::AnalogueGainModeAuto
> +					       : controls::AnalogueGainModeManual);
> +		}

You can drop the curly braces here, and below too.

> +
> +		if (!controls.contains(controls::ExposureTimeMode.id())) {

Seem here, checking it supports the ExposureTimeMode control.

Otherwise this looks fine to me.

> +			controls.set(controls::ExposureTimeMode,
> +				     *aeEnable ? controls::ExposureTimeModeAuto
> +					       : controls::ExposureTimeModeManual);
> +		}
> +	}
> +
>  	d->pipe_->invokeMethod(&PipelineHandler::queueRequest,
>  			       ConnectionTypeQueued, request);
>  

-- 
Regards,

Laurent Pinchart