| Message ID | 20250217185327.306509-2-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabas, Thanks for the fix ups - I see the compilers are all now happy with this series, so the gcc-9 issue is gone. Quoting Barnabás Pőcze (2025-02-17 18:53:34) > `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. > > Furthermore, since `values.back()` is used, only the last element of > the array is actually populated. > > To fix this, convert the array to contain `ControlValue` objects > and use a separate variable to keep track of which element is to > be populated next. > > The available modes are saved for later so that the appropriate mode > can be selected when the control is set. > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- > 1 file changed, 27 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index dedcac89b..1f604b91e 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, > + std::max(V4L2_EXPOSURE_MANUAL, > + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, > + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 I assume std::max becomes constexpr here (checked, and it does). clangformat wants to indent these, but it does make sense as a list. I wonder if putting them it a local const array and using some constexpr iteration ... but then we may as well put a constexpr helper in to include/libcamera/base/utils.h .... Maybe if we find ourselves doing this again ? > + > availableExposureModes_; > + > private: > bool generateId(); > > @@ -725,25 +733,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 && static_cast<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]) This hits some long lines. Might be tempting to format this as: if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) values[count++] = controls::ExposureTimeModeAuto; But I like this patch for more succinct code. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + 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 Barnabás On Mon, Feb 17, 2025 at 06:53:34PM +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. > > Furthermore, since `values.back()` is used, only the last element of > the array is actually populated. > > To fix this, convert the array to contain `ControlValue` objects > and use a separate variable to keep track of which element is to > be populated next. > > The available modes are saved for later so that the appropriate mode > can be selected when the control is set. > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- > 1 file changed, 27 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > index dedcac89b..1f604b91e 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, > + std::max(V4L2_EXPOSURE_MANUAL, > + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, > + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 The enum v4l2_exposure_auto_type definition is part of the linux kernel ABI, and I don't expect them to change. Maybe new modes might be added, but in that case you would have to change the above anyway. > + > availableExposureModes_; > + > private: > bool generateId(); > > @@ -725,25 +733,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 && static_cast<size_t>(x) < availableExposureModes_.size()) v4l2Values comes from kernel and can only be a value from enum v4l2_exposure_auto_type right ? The above check shouldn't be needed, at least the < 0 part.. > + availableExposureModes_[x] = true; > + } > + > + std::array<ControlValue, 2> values; > + std::size_t count = 0; > + > + if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) Break this lines as suggested by Kieran Thanks j > + 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 2025. február 28., péntek 12:06 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > Hi Barnabás > > On Mon, Feb 17, 2025 at 06:53:34PM +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. > > > > Furthermore, since `values.back()` is used, only the last element of > > the array is actually populated. > > > > To fix this, convert the array to contain `ControlValue` objects > > and use a separate variable to keep track of which element is to > > be populated next. > > > > The available modes are saved for later so that the appropriate mode > > can be selected when the control is set. > > > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- > > 1 file changed, 27 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index dedcac89b..1f604b91e 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, > > + std::max(V4L2_EXPOSURE_MANUAL, > > + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, > > + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 > > The enum v4l2_exposure_auto_type definition is part of the linux > kernel ABI, and I don't expect them to change. Maybe new modes might > be added, but in that case you would have to change the above anyway. I think the intent here is very clear. Alternatives I can think of: `std::bitset<4>`, or `std::bitset<V4L2_EXPOSURE_APERTURE_PRIORITY + 1>` are all less clear in my opinion. If there was a `V4L2_EXPOSURE_LAST` or `V4L2_EXPOSURE_COUNT` or whatever, I would have used that, but it doesn't appear to be a thing. `std::set<v4l2_exposure_auto_type>` is an option, but I would avoid it if possible, however maybe that will be it... Any suggestions? > > > > + > availableExposureModes_; > > + > > private: > > bool generateId(); > > > > @@ -725,25 +733,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 && static_cast<size_t>(x) < availableExposureModes_.size()) > > v4l2Values comes from kernel and can only be a value from enum > v4l2_exposure_auto_type right ? The above check shouldn't be needed, > at least the < 0 part.. I suppose you're right, but I still prefer to have bounds checking. > > > + availableExposureModes_[x] = true; > > + } > > + > > + std::array<ControlValue, 2> values; > > + std::size_t count = 0; > > + > > + if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) > > Break this lines as suggested by Kieran In my understanding the suggestion was not to break the line. But regardless, I think I would like to send a slightly different new version (which will not have these long lines). Since sending this I have come to the conclusion that the libcamera-control -> v4l2-control mapping should be in a single place. Regards, Barnabás Pőcze > > Thanks > j > > > + 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 Barnabás On Fri, Feb 28, 2025 at 11:45:38AM +0000, Barnabás Pőcze wrote: > Hi > > > 2025. február 28., péntek 12:06 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > > > Hi Barnabás > > > > On Mon, Feb 17, 2025 at 06:53:34PM +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. > > > > > > Furthermore, since `values.back()` is used, only the last element of > > > the array is actually populated. > > > > > > To fix this, convert the array to contain `ControlValue` objects > > > and use a separate variable to keep track of which element is to > > > be populated next. > > > > > > The available modes are saved for later so that the appropriate mode > > > can be selected when the control is set. > > > > > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > > --- > > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- > > > 1 file changed, 27 insertions(+), 19 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > > index dedcac89b..1f604b91e 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, > > > + std::max(V4L2_EXPOSURE_MANUAL, > > > + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, > > > + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 > > > > The enum v4l2_exposure_auto_type definition is part of the linux > > kernel ABI, and I don't expect them to change. Maybe new modes might > > be added, but in that case you would have to change the above anyway. > > I think the intent here is very clear. Alternatives I can think of: `std::bitset<4>`, > or `std::bitset<V4L2_EXPOSURE_APERTURE_PRIORITY + 1>` are all less clear in my opinion. > > If there was a `V4L2_EXPOSURE_LAST` or `V4L2_EXPOSURE_COUNT` or whatever, I would > have used that, but it doesn't appear to be a thing. That's true as well. I don't know how bitsets are actually represented, but if they're stored in an integer anyway I would have just bitset<32> Anyway, it's a minor, up to you > > `std::set<v4l2_exposure_auto_type>` is an option, but I would avoid it if possible, > however maybe that will be it... > > Any suggestions? > > > > > > > > > + > availableExposureModes_; > > > + > > > private: > > > bool generateId(); > > > > > > @@ -725,25 +733,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 && static_cast<size_t>(x) < availableExposureModes_.size()) > > > > v4l2Values comes from kernel and can only be a value from enum > > v4l2_exposure_auto_type right ? The above check shouldn't be needed, > > at least the < 0 part.. > > I suppose you're right, but I still prefer to have bounds checking. > I understand. When I see checks for things that are apparently not needed I always end up asking what am I missing and in what case, using this example, x might be less than 0 (because it's clear it can't). However, defensive programming is surely welcome at the expense of some future me asking "why the heck can x be < 0 ?" > > > > > > + availableExposureModes_[x] = true; > > > + } > > > + > > > + std::array<ControlValue, 2> values; > > > + std::size_t count = 0; > > > + > > > + if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) > > > > Break this lines as suggested by Kieran > > In my understanding the suggestion was not to break the line. But regardless, I think Wasn't it ? ------------------------------------------------------------------------------- This hits some long lines. Might be tempting to format this as: if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) values[count++] = controls::ExposureTimeModeAuto; ------------------------------------------------------------------------------- > I would like to send a slightly different new version (which will not have these > long lines). Since sending this I have come to the conclusion that the > libcamera-control -> v4l2-control mapping should be in a single place. Yup, no problems > > > Regards, > Barnabás Pőcze > > > > > Thanks > > j > > > > > + 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 2025. február 27., csütörtök 22:30 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta: > Hi Barnabas, > > Thanks for the fix ups - I see the compilers are all now happy with this > series, so the gcc-9 issue is gone. > > > Quoting Barnabás Pőcze (2025-02-17 18:53:34) > > `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. > > > > Furthermore, since `values.back()` is used, only the last element of > > the array is actually populated. > > > > To fix this, convert the array to contain `ControlValue` objects > > and use a separate variable to keep track of which element is to > > be populated next. > > > > The available modes are saved for later so that the appropriate mode > > can be selected when the control is set. > > > > Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > > --- > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- > > 1 file changed, 27 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp > > index dedcac89b..1f604b91e 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, > > + std::max(V4L2_EXPOSURE_MANUAL, > > + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, > > + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 > > I assume std::max becomes constexpr here (checked, and it does). > clangformat wants to indent these, but it does make sense as a list. > > I wonder if putting them it a local const array and using some constexpr > iteration ... but then we may as well put a constexpr helper in to > include/libcamera/base/utils.h .... > > Maybe if we find ourselves doing this again ? The C++ standard library already covers this. std::min/max/minmax all support std::initializer_list arguments. But unfortunately the libstdc++ implementation before GCC10 is not actually constexpr if `_GLIBCXX_DEBUG` is defined. I would be more in favor of dropping GCC9 than introducing a bespoke implementation just for this GCC bug. Regards, Barnabás Pőcze > > > > + > availableExposureModes_; > > + > > private: > > bool generateId(); > > > > @@ -725,25 +733,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 && static_cast<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]) > > This hits some long lines. Might be tempting to format this as: > > > if (availableExposureModes_[V4L2_EXPOSURE_AUTO] || > availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY]) > values[count++] = controls::ExposureTimeModeAuto; > > But I like this patch for more succinct code. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > + 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..1f604b91e 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, + std::max(V4L2_EXPOSURE_MANUAL, + std::max(V4L2_EXPOSURE_APERTURE_PRIORITY, + V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1 + > availableExposureModes_; + private: bool generateId(); @@ -725,25 +733,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 && static_cast<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. Furthermore, since `values.back()` is used, only the last element of the array is actually populated. To fix this, convert the array to contain `ControlValue` objects and use a separate variable to keep track of which element is to be populated next. The available modes are saved for later so that the appropriate mode can be selected when the control is set. Fixes: bad8d591f8acfa ("libcamera: uvcvideo: Register ExposureTimeMode control") Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 46 ++++++++++++-------- 1 file changed, 27 insertions(+), 19 deletions(-)