[libcamera-devel] [PATCH v3 1/5] libcamera: controls: Updates to gain and exposure controls

[Laurent Pinchart]

Hi Naush,

Thank you for the patch.

On Fri, Apr 03, 2020 at 03:53:01PM +0100, Naushir Patuck wrote:
> Rename:
> ManualExposure -> ExposureTime
> ManualGain -> AnalogueGain
> 
> Use micro-seconds units for ExposureTime. This is changed from milli-
> seconds. The latter would not allow very low exposure times.
> 
> AnalogueGain switch to use a float to allow fractional gain adjustments.
> 
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
>  src/libcamera/control_ids.yaml               | 20 ++++++++++++++------
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp |  8 ++++----
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml
> index 4befec74..839eea76 100644
> --- a/src/libcamera/control_ids.yaml
> +++ b/src/libcamera/control_ids.yaml
> @@ -10,7 +10,7 @@ controls:
>        description: |
>          Enable or disable the AE.
>  
> -        \sa ManualExposure
> +        \sa ExposureTime AnalogueGain
>  
>    - AeLocked:
>        type: bool
> @@ -42,12 +42,20 @@ controls:
>        type: int32_t
>        description: Specify a fixed saturation parameter
>  
> -  - ManualExposure:
> +  - ExposureTime:
>        type: int32_t
> -      description: Specify a fixed exposure time in milli-seconds
> +      description: |
> +        Exposure time (shutter speed) for the frame applied in the sensor
> +        device. This value is specified in micro-seconds.
>  
> -  - ManualGain:
> -      type: int32_t
> -      description: Specify a fixed gain parameter
> +        \sa AnalogueGain AeEnable
> +
> +  - AnalogueGain:
> +      type: float
> +      description: |
> +        Analogue gain value applied in the sensor device.
> +        The value of the control specifies the gain multiplier applied to all
> +        colour channels. This value cannot be lower than 1.0.

I wonder if there could be use cases for a value lower than 1.0, but
that can be done later if/when needed, so no issue for now.

>  
> +        \sa ExposureTime AeEnable
>  ...
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index ffbddf27..d7df95e4 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -251,10 +251,10 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
>  			controls.set(V4L2_CID_CONTRAST, value);
>  		} else if (id == controls::Saturation) {
>  			controls.set(V4L2_CID_SATURATION, value);
> -		} else if (id == controls::ManualExposure) {
> +		} else if (id == controls::ExposureTime) {
>  			controls.set(V4L2_CID_EXPOSURE_AUTO, static_cast<int32_t>(1));
>  			controls.set(V4L2_CID_EXPOSURE_ABSOLUTE, value);
> -		} else if (id == controls::ManualGain) {
> +		} else if (id == controls::AnalogueGain) {
>  			controls.set(V4L2_CID_GAIN, value);
>  		}
>  	}

The unit change breaks UVC support, which you then fixed in the next
patch in the series. We try to avoid bisection breakages when possible,
so I would like to squash 1/5 and 2/5 together unless there's an
objection.

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

> @@ -364,10 +364,10 @@ int UVCCameraData::init(MediaEntity *entity)
>  			id = &controls::Saturation;
>  			break;
>  		case V4L2_CID_EXPOSURE_ABSOLUTE:
> -			id = &controls::ManualExposure;
> +			id = &controls::ExposureTime;
>  			break;
>  		case V4L2_CID_GAIN:
> -			id = &controls::ManualGain;
> +			id = &controls::AnalogueGain;
>  			break;
>  		default:
>  			continue;

-- 
Regards,

Laurent Pinchart