[RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Wed Jan 29 10:13:10 CET 2025
Hi Barnabás
On Wed, Jan 29, 2025 at 08:37:30AM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2025. január 29., szerda 9:09 keltezéssel, Jacopo Mondi <jacopo.mondi at ideasonboard.com> írta:
>
> > Hi Barnabas
> >
> > On Tue, Jan 28, 2025 at 07:09:26PM +0000, Barnabás Pőcze wrote:
> > > Hi
> > >
> > >
> > > 2025. január 28., kedd 19:17 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.
> > > >
> > > > So the two constructors your point our are
> > > >
> > > > ControlInfo::ControlInfo(const ControlValue &min,
> > > > const ControlValue &max,
> > > > const ControlValue &def)
> > > > : min_(min), max_(max), def_(def)
> > > > {
> > > > }
> > > >
> > > > ControlInfo::ControlInfo(const ControlValue &min,
> > > > const ControlValue &max,
> > > > const ControlValue &def)
> > > > : min_(min), max_(max), def_(def)
> > > > {
> > > > }
> > >
> > > I think you meant the following instead?
> > >
> > > ControlInfo::ControlInfo(Span<const ControlValue> values,
> > > const ControlValue &def)
> > > { ... }
> > >
> > >
> >
> > Indeed, bad copy and paste, sorry
> >
> > > >
> > > > right ?
> > > >
> > > >
> > > > And we call them with
> > > >
> > > > info = ControlInfo{Span<int32_t>{values}, values[0]};
> > > >
> > > > which, if I got you right gets expanded to
> > > >
> > > > ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max,
> > > > const ControlValue &def)
> > > >
> > > >
> > > > as int32_t are implicitly converted to ControlValue instances.
> > > >
> > > > As 'values' is declared as
> > > >
> > > > std::array<int32_t, 2> values{};
> > > >
> > > > I presume the intermediate call would look like
> > > >
> > > > ControlInfo(int32_t, int32_t, int32_t)
> > >
> > > ControlInfo(Span<const int32_t>, int32_t, int32_t)
> >
> > How do we get from 2 parameters
> >
> > ControlInfo{Span<int32_t>{values}, values[0]};
> >
> > to three parameters ?
> >
> > ControlInfo(Span<const int32_t>, int32_t, int32_t)
> >
>
> Oops, sorry, I was thinking of the other case for some reason. In the case of
>
> info = ControlInfo{Span<int32_t>{values}, values[0]};
>
> it will be
>
> ControlInfo(Span<int32_t>, int32_t)
>
> and the third argument `def` will be a default initialized `ControlValue`
> as per the the default arguments:
Ah! That default initialization is what I was missing
>
> explicit ControlInfo(const ControlValue &min = {},
> const ControlValue &max = {},
> const ControlValue &def = {});
>
> So with the implicit conversions and default arguments it is essentially the same as:
>
> ControlInfo(/* min = */ ControlValue(Span<int32_t>),
> /* max = */ ControlValue(int32_t),
> /* def = */ ControlValue())
Awful indeed
That's how min and max looks like when run on my laptop
min = [ 0, 1 ]
max = 0
Something fishy indeed
>
>
> > >
> > >
> > > >
> > > > and because of implicit conversion we get to
> > > >
> > > > ControlInfo(const ControlValue &min, const ControlValue &max,
> > > > const ControlValue &def)
> > > >
> > > > How come that, if I do (before this patch)
> > > >
> > > > - info = ControlInfo{Span<int32_t>{values}, values[0]};
> > > > + info = ControlInfo{Span<int32_t>{values}, values[0], values[0]};
> > > >
> > > > the code still compiles even if values is of size 2 ?
> > >
> > > As far as I can tell:
> > >
> > > * the second and third arguments (`values[0]`) will be converted to `ControlValue`
> > > using the following constructor:
> > >
> > > template<typename T, std::enable_if_t<!details::is_span<T>::value &&
> > > details::control_type<T>::value &&
> > > !std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > ControlValue(const T &value)
> > > : type_(ControlTypeNone), numElements_(0)
> > > {
> > > set(details::control_type<std::remove_cv_t<T>>::value, false,
> > > &value, 1, sizeof(T));
> > > }
> > >
> > >
> > > * the first argument, the span of `const int32_t` will be converted to `ControlValue`
> > > using the following constructor:
> > >
> > > template<typename T, std::enable_if_t<details::is_span<T>::value ||
> > > std::is_same<std::string, std::remove_cv_t<T>>::value,
> > > std::nullptr_t> = nullptr>
> > > ControlValue(const T &value)
> > > : type_(ControlTypeNone), numElements_(0)
> > > {
> > > set(details::control_type<std::remove_cv_t<T>>::value, true,
> > > value.data(), value.size(), sizeof(typename T::value_type));
> > > }
> > >
> > > The resulting `ControlInfo` is nonetheless of questionable usefulness as the
> > > types of min/max/def will not be the same, but this is probably expected by
> > > users.
> > >
> >
> > Agreed, just trying to get to the details and see how we can avoid
> > this from happening generically
>
> At compile time one way could be add a templated `ControlValue` type. At runtime,
> see e.g. https://lists.libcamera.org/pipermail/libcamera-devel/2025-January/048475.html
>
>
> >
> > >
> > > >
> > > > >
> > > > > 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.
> > > >
> > > > Please split this to a different patch. And in any case, but I have to
> > > > re-check, I think the ControlValidator makes sure the control set by
> > > > the applications on a camera are supported.
> > >
> > > Sorry, what I meant is that it was not taken into account which of the
> > > `V4L2_EXPOSURE_*` modes are supported. The code unconditionally used
> > > `V4L2_EXPOSURE_{MANUAL,APERTURE_PRIORITY}`. Is there something I am missing?
> >
> > No, I thought CameraControlValidator::validate() was making sure the
> > ControlValue provided along with the ControlId to ControlList::set()
> > was valid against the ControlInfo values, but it only makes sure the
> > id is valid
> >
> > bool CameraControlValidator::validate(unsigned int id) const
> > {
> > const ControlInfoMap &controls = camera_->controls();
> > return controls.find(id) != controls.end();
> > }
> >
> > Maybe this is the right level at which this kind of check should be
> > performed instead of doing so in each single pipeline handler ?
> >
> > Have you considered that ?
>
> But how can v4l2 controls be checked there? E.g. `ExposureTimeModeAuto` is added to
> the list of options if either `V4L2_EXPOSURE_AUTO` or `V4L2_EXPOSURE_APERTURE_PRIORITY`
> is available. But if `V4L2_EXPOSURE_APERTURE_PRIORITY` is not available, then
> `V4L2_EXPOSURE_AUTO` should be used, no? As far as I can see this is one layer
> below the controls that are set by the user on the request. I am a bit lost as
> to which check you think should be moved somewhere else.
You're right, I might have mixed up two issues.
What you're looking at is what V4L2 control to use when a libcamera
control maps to multiple of them as in
ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,
V4L2_EXPOSURE_APERTURE_PRIORITY }
ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
V4L2_EXPOSURE_SHUTTER_PRIORITY }
and this indeed needs to be fixed.
The issue I was looking at is instead relative to what applications
can set in the Request::controls ControlList, and I thought the
ControlValidator verified that the value passed to
void ControlList::set(const Control<T> &ctrl, const V &value)
is in the range/list of valid values stored in the ControlInfoMap. But
as far as I can see
bool CameraControlValidator::validate(unsigned int id) const
only verifies the id validity. A proper validation might indeed be
very complex, that might be why we never got to implement it.
I'll review the patch separately as this thread is already long
enough.
Thanks
j
>
>
> >
> > >
> > >
> > > >
> > > > >
> > > > > 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_;
> > > > > +
> > > > > 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())
> > > > > + 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]};
> > > >
> > > > isn't this simpler ?
> > > >
> > > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > > @@ -723,7 +723,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > > > * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,
> > > > * V4L2_EXPOSURE_SHUTTER_PRIORITY }
> > > > */
> > > > - std::array<int32_t, 2> values{};
> > > > + std::array<ControlValue, 2> values{};
> > > >
> > > > auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > > > [&](const ControlValue &val) {
> > > > @@ -731,7 +731,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > > > val.get<int32_t>() == V4L2_EXPOSURE_AUTO) ? true : false;
> > > > });
> > > > if (it != v4l2Values.end())
> > > > - values.back() = static_cast<int32_t>(controls::ExposureTimeModeAuto);
> > > > + values.back() = controls::ExposureTimeModeAuto;
> > > >
> > > > it = std::find_if(v4l2Values.begin(), v4l2Values.end(),
> > > > [&](const ControlValue &val) {
> > > > @@ -739,9 +739,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,
> > > > val.get<int32_t>() == V4L2_EXPOSURE_MANUAL) ? true : false;
> > > > });
> > > > if (it != v4l2Values.end())
> > > > - values.back() = static_cast<int32_t>(controls::ExposureTimeModeManual);
> > > > + values.back() = controls::ExposureTimeModeManual;
> > > >
> > > > - info = ControlInfo{Span<int32_t>{values}, values[0]};
> > > > + info = ControlInfo{Span<const ControlValue>{values}, values[0]};
> > > > break;
> > > > }
> > > > case V4L2_CID_EXPOSURE_ABSOLUTE:
> > > >
> > >
> > > See my comment above, the `availableExposureModes_` is needed later in `processControl()`
> >
> > Yes, later :) I'm still leaning towards "two issues, two patches" even
> > if the second one builds on the former.
> >
> > > to determine which of the `V4L2_EXPOSURE_*` value to use. So this approach
> > > seemed simplest since the `availableExposureModes_` bitset has to be calculated
> > > either way. Also, the above change always sets `values[1]`.
> >
> > Right, that's another issue, even more so if the camera does not
> > report any exposure related controls, in this case even values[0]
> > won't be valid.
> >
> > How is
> > Span<const ControlValue>{values}
> >
> > constructed ?
> >
> > If I read
> >
> > template<std::size_t N>
> > constexpr Span(const std::array<value_type, N> &arr,
> > std::enable_if_t<std::is_convertible<std::remove_pointer_t<decltype(utils::data(arr))> (*)[],
> > element_type (*)[]>::value &&
> > N == Extent,
> > std::nullptr_t> = nullptr) noexcept
> > : data_(arr.data())
> > {
> > }
> >
> > right the span<> will have the same size as the array fixed size (2 in
> > this case) regardless of how many elements have actually been
> > populated.
>
> Yes, indeed, but that is not the desired outcome. That is why the proposed patch
> has
>
> Span<const ControlValue>{ values.data(), count }
>
> so only the first `count` elements are taken into account.
>
>
> > >
> > >
> > > > > break;
> > > > > }
> > > > > case V4L2_CID_EXPOSURE_ABSOLUTE:
> > > > > --
> > > > > 2.48.1
> > > > >
> > > > >
> > > >
> > >
> > >
>
>
> Regards,
> Barnabás Pőcze
More information about the libcamera-devel
mailing list