[RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control

Jacopo Mondi jacopo.mondi at ideasonboard.com
Wed Jan 29 15:09:36 CET 2025 

Hi Barnabás

On Wed, Jan 29, 2025 at 12:13:12PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > Hi Barnabás
> >
> > On Tue, Jan 28, 2025 at 12:14:01PM +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.
> > >
> > > To fix this, simply pass a span of `ControlValue` instead.
> > >
> > > Furthermore, the mapping in `UVCCameraData::processControl()` is also not
> > > entirely correct because the control value is retrieved as a `bool`
> > > instead of - the correct type - `int32_t`. Additionally, the available
> > > modes are not taken into account.
> > >
> > > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`
> > > and select the appropriate mode based on the mapping established
> > > in the comment.
> > >
> > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control")
> > > Signed-off-by: Barnabás Pőcze <pobrn at protonmail.com>
> > > ---
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 81 +++++++++++++-------
> > >  1 file changed, 53 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index dedcac89b..7821cceb0 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,
> > > +		V4L2_EXPOSURE_MANUAL,
> > > +		V4L2_EXPOSURE_APERTURE_PRIORITY,
> > > +		V4L2_EXPOSURE_SHUTTER_PRIORITY,
> > > +	}) + 1> availableExposureModes_;
> > > +
> >
> > I presume a bitset is used for efficiency ?
>
> I suppose you could say that. I thought an `std::set` was a bit of an overkill.
>
>
> >
> >
> > >  private:
> > >  	bool generateId();
> > >
> > > @@ -95,8 +103,8 @@ public:
> > >  	bool match(DeviceEnumerator *enumerator) override;
> > >
> > >  private:
> > > -	int processControl(ControlList *controls, unsigned int id,
> > > -			   const ControlValue &value);
> > > +	int processControl(UVCCameraData *data, ControlList *controls,
> > > +			   unsigned int id, const ControlValue &value);
> > >  	int processControls(UVCCameraData *data, Request *request);
> > >
> > >  	bool acquireDevice(Camera *camera) override;
> > > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)
> > >  	data->video_->releaseBuffers();
> > >  }
> > >
> > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > > -				       const ControlValue &value)
> > > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,
> > > +				       unsigned int id, const ControlValue &value)
> > >  {
> > >  	uint32_t cid;
> > >
> > > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,
> > >  	}
> > >
> > >  	case V4L2_CID_EXPOSURE_AUTO: {
> > > -		int32_t ivalue = value.get<bool>()
> > > -			       ? V4L2_EXPOSURE_APERTURE_PRIORITY
> > > -			       : V4L2_EXPOSURE_MANUAL;
> > > -		controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue);
> > > +		switch (value.get<int32_t>()) {
> > > +		case controls::ExposureTimeModeAuto:
> > > +			if (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])
> > > +				controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);
> > > +			else if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> > > +				controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);
> > > +			else
> > > +				ASSERT(false);
> > > +			break;
> > > +		case controls::ExposureTimeModeManual:
> > > +			if (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])
> > > +				controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);
> > > +			else if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])
> > > +				controls->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);
> > > +			else
> > > +				ASSERT(false);
> > > +			break;
> > > +		default:
> > > +			ASSERT(false);
> > > +			break;
> > > +		}
> > >  		break;
> > >  	}
> > >
> > > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)
> > >  	ControlList controls(data->video_->controls());
> > >
> > >  	for (const auto &[id, value] : request->controls())
> > > -		processControl(&controls, id, value);
> > > +		processControl(data, &controls, id, value);
> > >
> > >  	for (const auto &ctrl : controls)
> > >  		LOG(UVC, Debug)
> > > @@ -725,25 +750,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 && size_t(x) < availableExposureModes_.size())
> >
> > Can x be negative ? It should come from enum v4l2_exposure_auto_type,
> > doesn't it ?
>
> Yes, it should, but I don't know if it will ever be extended or similar, so
> doing the bounds checking seemed the most logical decision.

Well, one of the assumptions about Linux is that we don't break
userspace, so even if new values can be added to the possible control
values, I don't expect the existing ones to be changed or any new
negative value to be added there.

>
>
> >
> > Also, isn't size_t(x) == 4, why do you need to check it against the
> > bitset size ?
>
> I can't see why `x` would be 4. But `availableExposureModes_.size()` is 4.

Because I don't know what std::size_t() does.

To me it's the same size_t type as in the one in C (but defined in the
std namespace by <cstddef> (which you should probably include)).
https://en.cppreference.com/w/cpp/types/size_t

and I was expecting be size_t(int32_t) == sizeof(int32_t).

When I instead apply it to a variable it always returns me the
value of the variable.

	int32_t a = atoi(argv[1]);
	std::cout << "size_t(int32_t) = " << std::size_t(a) << std::endl;

        $ ./a.out 5
        size_t(int32_t) = 5
        $ ./a.out 10
        size_t(int32_t) = 10
        $ ./a.out 166
        size_t(int32_t) = 166

What the heck is this for ?

>
>
> >
> > > +				availableExposureModes_[x] = true;
> > > +		}
> > > +
> > > +		std::array<ControlValue, 2> values;
> > > +		std::size_t count = 0;
> > > +
> > > +		if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])
> > > +			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
> > >
> > >
> >

More information about the libcamera-devel mailing list