[PATCH v3 1/4] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setup

[Jacopo Mondi]

Hi Barnabás

On Fri, Mar 14, 2025 at 06:42:45PM +0100, Barnabás Pőcze wrote:
> `ControlInfo(Span<const int32_t>{...})` calls the incorrect constructor
> of `ControlInfo`. The intended constructor to be called is
> `ControlInfo(Span<const ControlValue>, ...)` however that is not called
> because a span of `const int32_t` is passed. Instead, the constructor
> `ControlInfo(const ControlValue &min, const ControlValue &max, ...)`
> will be called.
>
> Furthermore, since `values.back()` is used, only the last element of
> the array is actually set.
>
> To fix this, convert the array to contain `ControlValue` objects and use
> a separate variable to keep track of which element to set next.
>
> For each of `ExposureTimeMode{Auto,Manual}` save the V4L2 control value
> that is to be used when the libcamera control is set.
>
> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 89 +++++++++++++++-----
>  1 file changed, 70 insertions(+), 19 deletions(-)
>
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index dedcac89b..5c9025d9b 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -6,6 +6,7 @@
>   */
>
>  #include <algorithm>
> +#include <bitset>
>  #include <cmath>
>  #include <fstream>
>  #include <map>
> @@ -58,6 +59,9 @@ public:
>  	Stream stream_;
>  	std::map<PixelFormat, std::vector<SizeRange>> formats_;
>
> +	std::optional<v4l2_exposure_auto_type> autoExposureMode_;
> +	std::optional<v4l2_exposure_auto_type> manualExposureMode_;
> +
>  private:
>  	bool generateId();
>
> @@ -108,6 +112,26 @@ private:
>  	}
>  };
>
> +namespace {
> +
> +std::optional<controls::ExposureTimeModeEnum> v4l2ToExposureMode(int32_t x)
> +{
> +	using namespace controls;
> +
> +	switch (x) {
> +	case V4L2_EXPOSURE_AUTO:
> +	case V4L2_EXPOSURE_APERTURE_PRIORITY:
> +		return ExposureTimeModeAuto;
> +	case V4L2_EXPOSURE_MANUAL:
> +	case V4L2_EXPOSURE_SHUTTER_PRIORITY:
> +		return ExposureTimeModeManual;
> +	default:
> +		return {};
> +	}
> +}
> +
> +} /* namespace */
> +
>  UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)
>  	: CameraConfiguration(), data_(data)
>  {
> @@ -725,25 +749,52 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
>  		 * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
>  		 *			      V4L2_EXPOSURE_SHUTTER_PRIORITY }
>  		 */
> -		std::array<int32_t, 2> values{};
> -
> -		auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> -			[&](const ControlValue &val) {
> -				return (val.get<int32_t>() == V4L2_EXPOSURE_APERTURE_PRIORITY ||
> -					val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> -			});
> -		if (it != v4l2Values.end())
> -			values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> -
> -		it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> -			[&](const ControlValue &val) {
> -				return (val.get<int32_t>() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||
> -					val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> -			});
> -		if (it != v4l2Values.end())
> -			values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> -
> -		info = ControlInfo{Span<int32_t>{values}, values[0]};
> +
> +		std::bitset<
> +			std::max(V4L2_EXPOSURE_AUTO,
> +			std::max(V4L2_EXPOSURE_APERTURE_PRIORITY,
> +			std::max(V4L2_EXPOSURE_MANUAL,
> +				 V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1
> +		> exposureModes;
> +		std::optional<controls::ExposureTimeModeEnum> lcDef;
> +
> +		for (const ControlValue &value : v4l2Values) {
> +			const auto x = value.get<int32_t>();
> +
> +			if (0 <= x && static_cast<std::size_t>(x) < exposureModes.size()) {

I know you know I'm not a fan of checking for things that we know
can't happen as they part of the kernel/userspace ABI contract.

I know as well you prefer to check for < 0, so up to you

> +				exposureModes[x] = true;
> +
> +				if (x == def)
> +					lcDef = v4l2ToExposureMode(x);
> +			}
> +		}
> +
> +		if (exposureModes[V4L2_EXPOSURE_AUTO])
> +			autoExposureMode_ = V4L2_EXPOSURE_AUTO;
> +		else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])
> +			autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;
> +
> +		if (exposureModes[V4L2_EXPOSURE_MANUAL])
> +			manualExposureMode_ = V4L2_EXPOSURE_MANUAL;
> +		else if (exposureModes[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> +			manualExposureMode_ = V4L2_EXPOSURE_SHUTTER_PRIORITY;
> +
> +		std::array<ControlValue, 2> values;
> +		std::size_t count = 0;
> +
> +		if (autoExposureMode_)
> +			values[count++] = controls::ExposureTimeModeAuto;
> +
> +		if (manualExposureMode_)
> +			values[count++] = controls::ExposureTimeModeManual;
> +
> +		if (count == 0)
> +			return;

Here as well I don't think count can be 0. If the camera report
V4L2_CID_EXPOSURE_AUTO it should enumerate at least one value
associated with it, otherwise I presume registering the control in the
driver won't be possible.

> +
> +		info = ControlInfo{
> +			Span<const ControlValue>{ values.data(), count },
> +			!lcDef ? values.front() : *lcDef,

You could avoid this by doing

                        if (!lcDef || x == def)
                                lcDef = v4l2ToExposureMode(x);

in the above loop maybe

the rest looks good
Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>

Thanks
  j


> +		};
>  		break;
>  	}
>  	case V4L2_CID_EXPOSURE_ABSOLUTE:
> --
> 2.48.1
>