[RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix `ExposureTimeMode` control
Barnabás Pőcze
pobrn at protonmail.com
Tue Jan 28 20:09:26 CET 2025
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)
{ ... }
>
> 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)
>
> 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.
>
> >
> > 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?
>
> >
> > 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()`
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]`.
> > break;
> > }
> > case V4L2_CID_EXPOSURE_ABSOLUTE:
> > --
> > 2.48.1
> >
> >
>
Regards,
Barnabás Pőcze
More information about the libcamera-devel
mailing list