[RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Barnabás Pőcze
pobrn at protonmail.com
Wed Jan 29 15:59:20 CET 2025
2025. január 29., szerda 15:09 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
> 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.
That is my thinking, but keeping the check does not hurt in my opinion.
>
> >
> >
> > >
> > > 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 ?
`size_t(x)` just casts `x` to `size_t`. Otherwise one encounters a `-Wsign-compare`
error because `x` is signed but `availableExposureModes_.size()` is not.
https://en.cppreference.com/w/cpp/language/explicit_cast
>
> >
> >
> > >
> > > > + 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