[PATCH v2 1/2] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control setup

[Kieran Bingham]

Hi Barnabas,

Thanks for the fix ups - I see the compilers are all now happy with this
series, so the gcc-9 issue is gone.


Quoting Barnabás Pőcze (2025-02-17 18:53:34)
> `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 populated.
> 
> To fix this, convert the array to contain `ControlValue` objects
> and use a separate variable to keep track of which element is to
> be populated next.
> 
> The available modes are saved for later so that the appropriate mode
> can be selected when the control is set.
> 
> Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> ---
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++--------
>  1 file changed, 27 insertions(+), 19 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index dedcac89b..1f604b91e 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,13 @@ public:
>         Stream stream_;
>         std::map<PixelFormat, std::vector<SizeRange>> formats_;
>  
> +       std::bitset<
> +               std::max(V4L2_EXPOSURE_AUTO,
> +               std::max(V4L2_EXPOSURE_MANUAL,
> +               std::max(V4L2_EXPOSURE_APERTURE_PRIORITY,
> +                        V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1

I assume std::max becomes constexpr here (checked, and it does).
clangformat wants to indent these, but it does make sense as a list.

I wonder if putting them it a local const array and using some constexpr
iteration ... but then we may as well put a constexpr helper in to
include/libcamera/base/utils.h ....

Maybe if we find ourselves doing this again ?


> +       > availableExposureModes_;
> +
>  private:
>         bool generateId();
>  
> @@ -725,25 +733,25 @@ 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]};
> +               for (const ControlValue &value : v4l2Values) {
> +                       auto x = value.get<int32_t>();
> +                       if (0 <= x && static_cast<size_t>(x) < availableExposureModes_.size())
> +                               availableExposureModes_[x] = true;
> +               }
> +
> +               std::array<ControlValue, 2> values;
> +               std::size_t count = 0;
> +
> +               if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])

This hits some long lines. Might be tempting to format this as:


		if (availableExposureModes_[V4L2_EXPOSURE_AUTO] ||
		    availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
			values[count++] = controls::ExposureTimeModeAuto;

But I like this patch for more succinct code.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> +                       values[count++] = controls::ExposureTimeModeAuto;
> +
> +               if (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> +                       values[count++] = controls::ExposureTimeModeManual;
> +
> +               if (count == 0)
> +                       return;
> +
> +               info = ControlInfo{ Span<const ControlValue>{ values.data(), count }, values[0] };
>                 break;
>         }
>         case V4L2_CID_EXPOSURE_ABSOLUTE:
> -- 
> 2.48.1
> 
>