| Message ID | 20210422021809.520675-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Interesting https://stackoverflow.com/a/21519063 I initially thougth replacing VLA in C++ code was mostly a matter of style... On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote: > The original code uses Variable-Length-Array, which is not > officially supported in C++. This replaces the array with > std::vector. One thing that always bothered me of this class is std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_; std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; everytime I see these identifiers I forget they refer to v4l2 stuff. Should they be renamed ? > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++----------------- > 1 file changed, 14 insertions(+), 17 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 3c32d682..617e6ec9 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -173,8 +173,7 @@ void V4L2Device::close() > */ > ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > { > - unsigned int count = ids.size(); > - if (count == 0) > + if (ids.empty()) > return {}; > > ControlList ctrls{ controls_ }; > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > ctrls.set(id, {}); > } > > - struct v4l2_ext_control v4l2Ctrls[count]; > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > + std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size()); > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); Do you need the memset ? I read: explicit vector( size_type count ); 4) Constructs the container with count default-inserted instances of T. No copies are made. "default inserted instances of T" makes me wonder if a default-inserted v4l2_ext_control is actually zeroed I wonder if using reserve() would be better > > - unsigned int i = 0; > - for (auto &ctrl : ctrls) { > - unsigned int id = ctrl.first; > + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) { C++ is the language of the beast. Neat though > + const unsigned int id = ctrl->first; > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > ControlType type; > - > switch (info.type) { > case V4L2_CTRL_TYPE_U8: > type = ControlTypeByte; > break; > - > default: > LOG(V4L2, Error) > << "Unsupported payload control type " > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > return {}; > } > > - ControlValue &value = ctrl.second; > + ControlValue &value = ctrl->second; > value.reserve(type, true, info.elems); > Span<uint8_t> data = value.data(); > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > v4l2Ctrls[i].size = data.size(); > } > > - v4l2Ctrls[i].id = id; > - i++; > + v4l2Ctrl.id = id; However, shouldn't we try to squeeze the two loops (the one that initialize ctrls and this one) in a single one now that the ordering restrictions is gone ? > } > > struct v4l2_ext_controls v4l2ExtCtrls = {}; > v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL; > - v4l2ExtCtrls.controls = v4l2Ctrls; > - v4l2ExtCtrls.count = count; > + v4l2ExtCtrls.controls = v4l2Ctrls.data(); > + v4l2ExtCtrls.count = v4l2Ctrls.size(); > > int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls); > if (ret) { > unsigned int errorIdx = v4l2ExtCtrls.error_idx; > > /* Generic validation error. */ > - if (errorIdx == 0 || errorIdx >= count) { > + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) { > LOG(V4L2, Error) << "Unable to read controls: " > << strerror(-ret); > return {}; > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > /* A specific control failed. */ > LOG(V4L2, Error) << "Unable to read control " << errorIdx > << ": " << strerror(-ret); > - count = errorIdx - 1; > + > + v4l2Ctrls.resize(errorIdx); > } > > - updateControls(&ctrls, v4l2Ctrls, count); > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > return ctrls; > } > -- > 2.31.1.368.gbe11c130af-goog > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote: > Hi Hiro, > > Interesting https://stackoverflow.com/a/21519063 "An aesthetic or correct way of invoking an action that requires DOM manipulation as well as controller related tasks in EmberJS" Please don't tell me you plan to rewrite libcamera in JS :-) > I initially thougth replacing VLA in C++ code was mostly a matter of > style... > > On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote: > > The original code uses Variable-Length-Array, which is not > > officially supported in C++. This replaces the array with > > std::vector. > > One thing that always bothered me of this class is > > std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_; > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > everytime I see these identifiers I forget they refer to v4l2 stuff. > Should they be renamed ? > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++----------------- > > 1 file changed, 14 insertions(+), 17 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 3c32d682..617e6ec9 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -173,8 +173,7 @@ void V4L2Device::close() > > */ > > ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > { > > - unsigned int count = ids.size(); > > - if (count == 0) > > + if (ids.empty()) > > return {}; > > > > ControlList ctrls{ controls_ }; > > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > ctrls.set(id, {}); > > } > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > + std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size()); > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > Do you need the memset ? > > I read: > > explicit vector( size_type count ); > > 4) Constructs the container with count default-inserted instances of T. No copies are made. > > "default inserted instances of T" makes me wonder if a > default-inserted v4l2_ext_control is actually zeroed > > I wonder if using reserve() would be better In a previous reply, Hiro mentioned that the zero-initialization will only initialize the first member of unions. memset() is possibly safer. > > > > - unsigned int i = 0; > > - for (auto &ctrl : ctrls) { > > - unsigned int id = ctrl.first; > > + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) { > > C++ is the language of the beast. > Neat though I've posted "[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for range-based for loops" which would allow writing this as for (auto &[i, ctrl] = utils::enumerate(ctrls)) { It's not a blocker though, we can merge this series and rework it later. I'd however appreciate feedback on the above patch (and the rest of the series). > > + const unsigned int id = ctrl->first; > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > ControlType type; > > - > > switch (info.type) { > > case V4L2_CTRL_TYPE_U8: > > type = ControlTypeByte; > > break; > > - I really think those blank lines help readability. > > default: > > LOG(V4L2, Error) > > << "Unsupported payload control type " > > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > return {}; > > } > > > > - ControlValue &value = ctrl.second; > > + ControlValue &value = ctrl->second; > > value.reserve(type, true, info.elems); > > Span<uint8_t> data = value.data(); > > > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > v4l2Ctrls[i].size = data.size(); > > } > > > > - v4l2Ctrls[i].id = id; > > - i++; > > + v4l2Ctrl.id = id; > > However, shouldn't we try to squeeze the two loops (the one that > initialize ctrls and this one) in a single one now that the ordering > restrictions is gone ? That's what I replied to 1/4 :-) I'd have a small preference for doing it in 1/4 as that's where the restriction is removed, or at least in a patch inserted before 1/4 and 2/4. > > } > > > > struct v4l2_ext_controls v4l2ExtCtrls = {}; > > v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL; > > - v4l2ExtCtrls.controls = v4l2Ctrls; > > - v4l2ExtCtrls.count = count; > > + v4l2ExtCtrls.controls = v4l2Ctrls.data(); > > + v4l2ExtCtrls.count = v4l2Ctrls.size(); > > > > int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls); > > if (ret) { > > unsigned int errorIdx = v4l2ExtCtrls.error_idx; > > > > /* Generic validation error. */ > > - if (errorIdx == 0 || errorIdx >= count) { > > + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) { > > LOG(V4L2, Error) << "Unable to read controls: " > > << strerror(-ret); > > return {}; > > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > /* A specific control failed. */ > > LOG(V4L2, Error) << "Unable to read control " << errorIdx > > << ": " << strerror(-ret); > > - count = errorIdx - 1; > > + > > + v4l2Ctrls.resize(errorIdx); > > } > > > > - updateControls(&ctrls, v4l2Ctrls, count); > > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > > > return ctrls; > > }
On Fri, Apr 23, 2021 at 12:05 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote: > > Hi Hiro, > > > > Interesting https://stackoverflow.com/a/21519063 > > "An aesthetic or correct way of invoking an action that requires DOM > manipulation as well as controller related tasks in EmberJS" > > Please don't tell me you plan to rewrite libcamera in JS :-) > > > I initially thougth replacing VLA in C++ code was mostly a matter of > > style... > > > > On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote: > > > The original code uses Variable-Length-Array, which is not > > > officially supported in C++. This replaces the array with > > > std::vector. > > > > One thing that always bothered me of this class is > > > > std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_; > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > > everytime I see these identifiers I forget they refer to v4l2 stuff. > > Should they be renamed ? > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++----------------- > > > 1 file changed, 14 insertions(+), 17 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 3c32d682..617e6ec9 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -173,8 +173,7 @@ void V4L2Device::close() > > > */ > > > ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > { > > > - unsigned int count = ids.size(); > > > - if (count == 0) > > > + if (ids.empty()) > > > return {}; > > > > > > ControlList ctrls{ controls_ }; > > > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > ctrls.set(id, {}); > > > } > > > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > + std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size()); > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > > > Do you need the memset ? > > > > I read: > > > > explicit vector( size_type count ); > > > > 4) Constructs the container with count default-inserted instances of T. No copies are made. > > > > "default inserted instances of T" makes me wonder if a > > default-inserted v4l2_ext_control is actually zeroed > > > > I wonder if using reserve() would be better > > In a previous reply, Hiro mentioned that the zero-initialization will > only initialize the first member of unions. memset() is possibly safer. > > > > > > > - unsigned int i = 0; > > > - for (auto &ctrl : ctrls) { > > > - unsigned int id = ctrl.first; > > > + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) { > > > > C++ is the language of the beast. > > Neat though > > I've posted "[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for > range-based for loops" which would allow writing this as > > for (auto &[i, ctrl] = utils::enumerate(ctrls)) { > > It's not a blocker though, we can merge this series and rework it later. > I'd however appreciate feedback on the above patch (and the rest of the > series). Thanks for informing me. I would love zip and enumerator iterators. :) I will review them later. > > > > + const unsigned int id = ctrl->first; > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > ControlType type; > > > - > > > switch (info.type) { > > > case V4L2_CTRL_TYPE_U8: > > > type = ControlTypeByte; > > > break; > > > - > > I really think those blank lines help readability. > > > > default: > > > LOG(V4L2, Error) > > > << "Unsupported payload control type " > > > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > return {}; > > > } > > > > > > - ControlValue &value = ctrl.second; > > > + ControlValue &value = ctrl->second; > > > value.reserve(type, true, info.elems); > > > Span<uint8_t> data = value.data(); > > > > > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > v4l2Ctrls[i].size = data.size(); > > > } > > > > > > - v4l2Ctrls[i].id = id; > > > - i++; > > > + v4l2Ctrl.id = id; > > > > However, shouldn't we try to squeeze the two loops (the one that > > initialize ctrls and this one) in a single one now that the ordering > > restrictions is gone ? > > That's what I replied to 1/4 :-) I'd have a small preference for doing > it in 1/4 as that's where the restriction is removed, or at least in a > patch inserted before 1/4 and 2/4. > > > > } > > > > > > struct v4l2_ext_controls v4l2ExtCtrls = {}; > > > v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL; > > > - v4l2ExtCtrls.controls = v4l2Ctrls; > > > - v4l2ExtCtrls.count = count; > > > + v4l2ExtCtrls.controls = v4l2Ctrls.data(); > > > + v4l2ExtCtrls.count = v4l2Ctrls.size(); > > > > > > int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls); > > > if (ret) { > > > unsigned int errorIdx = v4l2ExtCtrls.error_idx; > > > > > > /* Generic validation error. */ > > > - if (errorIdx == 0 || errorIdx >= count) { > > > + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) { > > > LOG(V4L2, Error) << "Unable to read controls: " > > > << strerror(-ret); > > > return {}; > > > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > /* A specific control failed. */ > > > LOG(V4L2, Error) << "Unable to read control " << errorIdx > > > << ": " << strerror(-ret); > > > - count = errorIdx - 1; > > > + > > > + v4l2Ctrls.resize(errorIdx); > > > } > > > > > > - updateControls(&ctrls, v4l2Ctrls, count); > > > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > > > > > return ctrls; > > > } > > -- > Regards, > > Laurent Pinchart
On Fri, Apr 23, 2021 at 06:05:50AM +0300, Laurent Pinchart wrote: > On Thu, Apr 22, 2021 at 09:50:46AM +0200, Jacopo Mondi wrote: > > Hi Hiro, > > > > Interesting https://stackoverflow.com/a/21519063 > > "An aesthetic or correct way of invoking an action that requires DOM > manipulation as well as controller related tasks in EmberJS" What the heck! I don't even know what EmberJS is! I clicked on the 'share' button of stackoverflow post about VLA in C++ and that's the link I get. Didn't know "share" actually meant "share a random stackoverflow thread" :) > > Please don't tell me you plan to rewrite libcamera in JS :-) > > > I initially thougth replacing VLA in C++ code was mostly a matter of > > style... > > > > On Thu, Apr 22, 2021 at 11:18:07AM +0900, Hirokazu Honda wrote: > > > The original code uses Variable-Length-Array, which is not > > > officially supported in C++. This replaces the array with > > > std::vector. > > > > One thing that always bothered me of this class is > > > > std::map<unsigned int, struct v4l2_query_ext_ctrl> controlInfo_; > > std::vector<std::unique_ptr<V4L2ControlId>> controlIds_; > > > > everytime I see these identifiers I forget they refer to v4l2 stuff. > > Should they be renamed ? > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++----------------- > > > 1 file changed, 14 insertions(+), 17 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 3c32d682..617e6ec9 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -173,8 +173,7 @@ void V4L2Device::close() > > > */ > > > ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > { > > > - unsigned int count = ids.size(); > > > - if (count == 0) > > > + if (ids.empty()) > > > return {}; > > > > > > ControlList ctrls{ controls_ }; > > > @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > ctrls.set(id, {}); > > > } > > > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > + std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size()); > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > > > Do you need the memset ? > > > > I read: > > > > explicit vector( size_type count ); > > > > 4) Constructs the container with count default-inserted instances of T. No copies are made. > > > > "default inserted instances of T" makes me wonder if a > > default-inserted v4l2_ext_control is actually zeroed > > > > I wonder if using reserve() would be better > > In a previous reply, Hiro mentioned that the zero-initialization will > only initialize the first member of unions. memset() is possibly safer. > > > > > > > - unsigned int i = 0; > > > - for (auto &ctrl : ctrls) { > > > - unsigned int id = ctrl.first; > > > + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) { > > > > C++ is the language of the beast. > > Neat though > > I've posted "[PATCH/RFC 1/3] libcamera: utils: Add enumerate view for > range-based for loops" which would allow writing this as > > for (auto &[i, ctrl] = utils::enumerate(ctrls)) { > > It's not a blocker though, we can merge this series and rework it later. > I'd however appreciate feedback on the above patch (and the rest of the > series). > > > > + const unsigned int id = ctrl->first; > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > ControlType type; > > > - > > > switch (info.type) { > > > case V4L2_CTRL_TYPE_U8: > > > type = ControlTypeByte; > > > break; > > > - > > I really think those blank lines help readability. > > > > default: > > > LOG(V4L2, Error) > > > << "Unsupported payload control type " > > > @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > return {}; > > > } > > > > > > - ControlValue &value = ctrl.second; > > > + ControlValue &value = ctrl->second; > > > value.reserve(type, true, info.elems); > > > Span<uint8_t> data = value.data(); > > > > > > @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > v4l2Ctrls[i].size = data.size(); > > > } > > > > > > - v4l2Ctrls[i].id = id; > > > - i++; > > > + v4l2Ctrl.id = id; > > > > However, shouldn't we try to squeeze the two loops (the one that > > initialize ctrls and this one) in a single one now that the ordering > > restrictions is gone ? > > That's what I replied to 1/4 :-) I'd have a small preference for doing > it in 1/4 as that's where the restriction is removed, or at least in a > patch inserted before 1/4 and 2/4. > Wherever works ! Thanks j > > > } > > > > > > struct v4l2_ext_controls v4l2ExtCtrls = {}; > > > v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL; > > > - v4l2ExtCtrls.controls = v4l2Ctrls; > > > - v4l2ExtCtrls.count = count; > > > + v4l2ExtCtrls.controls = v4l2Ctrls.data(); > > > + v4l2ExtCtrls.count = v4l2Ctrls.size(); > > > > > > int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls); > > > if (ret) { > > > unsigned int errorIdx = v4l2ExtCtrls.error_idx; > > > > > > /* Generic validation error. */ > > > - if (errorIdx == 0 || errorIdx >= count) { > > > + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) { > > > LOG(V4L2, Error) << "Unable to read controls: " > > > << strerror(-ret); > > > return {}; > > > @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > /* A specific control failed. */ > > > LOG(V4L2, Error) << "Unable to read control " << errorIdx > > > << ": " << strerror(-ret); > > > - count = errorIdx - 1; > > > + > > > + v4l2Ctrls.resize(errorIdx); > > > } > > > > > > - updateControls(&ctrls, v4l2Ctrls, count); > > > + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > > > > > return ctrls; > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 3c32d682..617e6ec9 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -173,8 +173,7 @@ void V4L2Device::close() */ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) { - unsigned int count = ids.size(); - if (count == 0) + if (ids.empty()) return {}; ControlList ctrls{ controls_ }; @@ -190,22 +189,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) ctrls.set(id, {}); } - struct v4l2_ext_control v4l2Ctrls[count]; - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); + std::vector<v4l2_ext_control> v4l2Ctrls(ctrls.size()); + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); - unsigned int i = 0; - for (auto &ctrl : ctrls) { - unsigned int id = ctrl.first; + for (auto [ctrl, i] = std::pair(ctrls.begin(), 0u); i < ctrls.size(); ctrl++, i++) { + const unsigned int id = ctrl->first; const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { ControlType type; - switch (info.type) { case V4L2_CTRL_TYPE_U8: type = ControlTypeByte; break; - default: LOG(V4L2, Error) << "Unsupported payload control type " @@ -213,7 +210,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) return {}; } - ControlValue &value = ctrl.second; + ControlValue &value = ctrl->second; value.reserve(type, true, info.elems); Span<uint8_t> data = value.data(); @@ -221,21 +218,20 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) v4l2Ctrls[i].size = data.size(); } - v4l2Ctrls[i].id = id; - i++; + v4l2Ctrl.id = id; } struct v4l2_ext_controls v4l2ExtCtrls = {}; v4l2ExtCtrls.which = V4L2_CTRL_WHICH_CUR_VAL; - v4l2ExtCtrls.controls = v4l2Ctrls; - v4l2ExtCtrls.count = count; + v4l2ExtCtrls.controls = v4l2Ctrls.data(); + v4l2ExtCtrls.count = v4l2Ctrls.size(); int ret = ioctl(VIDIOC_G_EXT_CTRLS, &v4l2ExtCtrls); if (ret) { unsigned int errorIdx = v4l2ExtCtrls.error_idx; /* Generic validation error. */ - if (errorIdx == 0 || errorIdx >= count) { + if (errorIdx == 0 || errorIdx >= v4l2Ctrls.size()) { LOG(V4L2, Error) << "Unable to read controls: " << strerror(-ret); return {}; @@ -244,10 +240,11 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) /* A specific control failed. */ LOG(V4L2, Error) << "Unable to read control " << errorIdx << ": " << strerror(-ret); - count = errorIdx - 1; + + v4l2Ctrls.resize(errorIdx); } - updateControls(&ctrls, v4l2Ctrls, count); + updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); return ctrls; }
The original code uses Variable-Length-Array, which is not officially supported in C++. This replaces the array with std::vector. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/v4l2_device.cpp | 31 ++++++++++++++----------------- 1 file changed, 14 insertions(+), 17 deletions(-)