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

[Barnabás Pőcze]

Hi


2025. február 27., csütörtök 22:30 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:

> 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 ?

The C++ standard library already covers this. std::min/max/minmax all support
std::initializer_list arguments. But unfortunately the libstdc++ implementation
before GCC10 is not actually constexpr if `_GLIBCXX_DEBUG` is defined. I would
be more in favor of dropping GCC9 than introducing a bespoke implementation just
for this GCC bug.


Regards,
Barnabás Pőcze

> 
> 
> > +       > 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
> >
> >
>