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

[Jacopo Mondi]

Hi Barnabás

On Fri, Feb 28, 2025 at 11:45:38AM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. február 28., péntek 12:06 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Mon, Feb 17, 2025 at 06:53:34PM +0000, 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 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
> >
> > The enum v4l2_exposure_auto_type definition is part of the linux
> > kernel ABI, and I don't expect them to change. Maybe new modes might
> > be added, but in that case you would have to change the above anyway.
>
> I think the intent here is very clear. Alternatives I can think of: `std::bitset<4>`,
> or `std::bitset<V4L2_EXPOSURE_APERTURE_PRIORITY + 1>` are all less clear in my opinion.
>
> If there was a `V4L2_EXPOSURE_LAST` or `V4L2_EXPOSURE_COUNT` or whatever, I would
> have used that, but it doesn't appear to be a thing.

That's true as well. I don't know how bitsets are actually
represented, but if they're stored in an integer anyway I would have
just bitset<32>

Anyway, it's a minor, up to you

>
> `std::set<v4l2_exposure_auto_type>` is an option, but I would avoid it if possible,
> however maybe that will be it...
>
> Any suggestions?
>
>
> >
> >
> > > +	> 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())
> >
> > v4l2Values comes from kernel and can only be a value from enum
> > v4l2_exposure_auto_type right ? The above check shouldn't be needed,
> > at least the < 0 part..
>
> I suppose you're right, but I still prefer to have bounds checking.
>

I understand.

When I see checks for things that are apparently not needed I always
end up asking what am I missing and in what case, using this example,
x might be less than 0 (because it's clear it can't).

However, defensive programming is surely welcome at the expense of some
future me asking "why the heck can x be < 0 ?"
>
> >
> > > +				availableExposureModes_[x] = true;
> > > +		}
> > > +
> > > +		std::array<ControlValue, 2> values;
> > > +		std::size_t count = 0;
> > > +
> > > +		if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> >
> > Break this lines as suggested by Kieran
>
> In my understanding the suggestion was not to break the line. But regardless, I think

Wasn't it ?

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

> I would like to send a slightly different new version (which will not have these
> long lines). Since sending this I have come to the conclusion that the
> libcamera-control -> v4l2-control mapping should be in a single place.

Yup, no problems

>
>
> Regards,
> Barnabás Pőcze
>
> >
> > Thanks
> >   j
> >
> > > +			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
> > >
> > >
> >