| Message ID | 20250128121352.494582-2-pobrn@protonmail.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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) { } 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) 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 ? > > 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. > > 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@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: > break; > } > case V4L2_CID_EXPOSURE_ABSOLUTE: > -- > 2.48.1 > >
Hi 2025. január 28., kedd 19:17 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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
Hi 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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. > > 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. > > > + 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 > > > > >
Quoting Barnabás Pőcze (2025-01-29 12:13:12) > Hi > > > 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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. It looks like GCC-9 has some issues around here: https://gitlab.freedesktop.org/camera/libcamera/-/jobs/70104933 -- Kieran
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@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@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 > > > > > > > >
2025. január 29., szerda 15:09 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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 > > > > > > > > > > >
Hi On Wed, Jan 29, 2025 at 02:59:20PM +0000, Barnabás Pőcze wrote: > 2025. január 29., szerda 15:09 keltezéssel, Jacopo Mondi <jacopo.mondi@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@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@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 > ahah, ok, this was rather stupid :) I'll take the occasion to remind you we try to avoid C-style casts in the code base ;) > > > > > > > > > > > > > > > > > > + 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 > > > > > > > > > > > > > >
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]}; break; } case V4L2_CID_EXPOSURE_ABSOLUTE:
`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@protonmail.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 81 +++++++++++++------- 1 file changed, 53 insertions(+), 28 deletions(-)