| Message ID | 20210423093653.1726488-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Fri, Apr 23, 2021 at 06:36:51PM +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. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------ > 1 file changed, 20 insertions(+), 21 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index ce2860c4..bbe8f154 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -173,13 +173,18 @@ 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_ }; > + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > + > + for (size_t i = 0, j = 0; j < ids.size(); ++j) { > + const unsigned int id = ids[j]; > + if (ctrls.contains(id)) > + continue; > > - for (uint32_t id : ids) { > const auto iter = controls_.find(id); > if (iter == controls_.end()) { > LOG(V4L2, Error) > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > return {}; > } > > - ctrls.set(id, {}); > - } > - > - struct v4l2_ext_control v4l2Ctrls[count]; > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > - > - unsigned int i = 0; > - for (auto &ctrl : ctrls) { > - unsigned int id = ctrl.first; > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; You increase i here, ... > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > + ControlValue value{}; ControlValue has a default constructor, no need for {}. > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > ControlType type; > - > switch (info.type) { > case V4L2_CTRL_TYPE_U8: > type = ControlTypeByte; > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > return {}; > } > > - ControlValue &value = ctrl.second; > value.reserve(type, true, info.elems); > Span<uint8_t> data = value.data(); > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > v4l2Ctrls[i].size = data.size(); ... and use it here. I don't think that's correct. > } > > - v4l2Ctrls[i].id = id; > - i++; > + v4l2Ctrl.id = id; Should we move this just after the declaration of v4l2Ctrl above ? > + ctrls.set(id, std::move(value)); v4l2Ctrls contains pointers to data stored in the ControlValue instances. As far as I can tell the pointers will remain valid, but that's very dependent on the internals of ControlList. To be honest, I'm not very fond of patches 1/4 and 2/4 in this series. They make the code more fragile and possibly a bit less efficient (although that's likely not significant, as there shouldn't be thousands of controls in the list). The VLA removal is fine, but the rest doesn't bring much value in my opinion. Let's split this in two parts, with the fixes first, and the rework on top, so both can be discussed separately. I've posted the former as a v6. > } > > + v4l2Ctrls.resize(ctrls.size()); > + > 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 +242,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; > }
Hi Laurent, On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Fri, Apr 23, 2021 at 06:36:51PM +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. > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------ > > 1 file changed, 20 insertions(+), 21 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index ce2860c4..bbe8f154 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -173,13 +173,18 @@ 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_ }; > > + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > + > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) { > > + const unsigned int id = ids[j]; > > + if (ctrls.contains(id)) > > + continue; > > > > - for (uint32_t id : ids) { > > const auto iter = controls_.find(id); > > if (iter == controls_.end()) { > > LOG(V4L2, Error) > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > return {}; > > } > > > > - ctrls.set(id, {}); > > - } > > - > > - struct v4l2_ext_control v4l2Ctrls[count]; > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > - > > - unsigned int i = 0; > > - for (auto &ctrl : ctrls) { > > - unsigned int id = ctrl.first; > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; > > You increase i here, ... > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > + ControlValue value{}; > > ControlValue has a default constructor, no need for {}. > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > ControlType type; > > - > > switch (info.type) { > > case V4L2_CTRL_TYPE_U8: > > type = ControlTypeByte; > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > return {}; > > } > > > > - ControlValue &value = ctrl.second; > > value.reserve(type, true, info.elems); > > Span<uint8_t> data = value.data(); > > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > v4l2Ctrls[i].size = data.size(); > > ... and use it here. I don't think that's correct. > > > } > > > > - v4l2Ctrls[i].id = id; > > - i++; > > + v4l2Ctrl.id = id; > > Should we move this just after the declaration of v4l2Ctrl above ? > > > + ctrls.set(id, std::move(value)); > > v4l2Ctrls contains pointers to data stored in the ControlValue > instances. As far as I can tell the pointers will remain valid, but > that's very dependent on the internals of ControlList. > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series. > They make the code more fragile and possibly a bit less efficient > (although that's likely not significant, as there shouldn't be thousands > of controls in the list). The VLA removal is fine, but the rest doesn't > bring much value in my opinion. > Can you clarify more how the new code is fragile and less efficient? You're right, I am sorry that this implementation has some problems. But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs? -Hiro > Let's split this in two parts, with the fixes first, and the rework on > top, so both can be discussed separately. I've posted the former as a > v6. > > > } > > > > + v4l2Ctrls.resize(ctrls.size()); > > + > > 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 +242,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
Hi Hiro, On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote: > On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote: > > On Fri, Apr 23, 2021 at 06:36:51PM +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. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > --- > > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------ > > > 1 file changed, 20 insertions(+), 21 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index ce2860c4..bbe8f154 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -173,13 +173,18 @@ 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_ }; > > > + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > > + > > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) { > > > + const unsigned int id = ids[j]; > > > + if (ctrls.contains(id)) > > > + continue; > > > > > > - for (uint32_t id : ids) { > > > const auto iter = controls_.find(id); > > > if (iter == controls_.end()) { > > > LOG(V4L2, Error) > > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > return {}; > > > } > > > > > > - ctrls.set(id, {}); > > > - } > > > - > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > - > > > - unsigned int i = 0; > > > - for (auto &ctrl : ctrls) { > > > - unsigned int id = ctrl.first; > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; > > > > You increase i here, ... > > > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > + ControlValue value{}; > > > > ControlValue has a default constructor, no need for {}. > > > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > ControlType type; > > > - > > > switch (info.type) { > > > case V4L2_CTRL_TYPE_U8: > > > type = ControlTypeByte; > > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > return {}; > > > } > > > > > > - ControlValue &value = ctrl.second; > > > value.reserve(type, true, info.elems); > > > Span<uint8_t> data = value.data(); > > > > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > v4l2Ctrls[i].size = data.size(); > > > > ... and use it here. I don't think that's correct. > > > > > } > > > > > > - v4l2Ctrls[i].id = id; > > > - i++; > > > + v4l2Ctrl.id = id; > > > > Should we move this just after the declaration of v4l2Ctrl above ? > > > > > + ctrls.set(id, std::move(value)); > > > > v4l2Ctrls contains pointers to data stored in the ControlValue > > instances. As far as I can tell the pointers will remain valid, but > > that's very dependent on the internals of ControlList. > > > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series. > > They make the code more fragile and possibly a bit less efficient > > (although that's likely not significant, as there shouldn't be thousands > > of controls in the list). The VLA removal is fine, but the rest doesn't > > bring much value in my opinion. > > Can you clarify more how the new code is fragile and less efficient? > You're right, I am sorry that this implementation has some problems. > But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs? The efficiency is related to the additional lookup in updateControls() (the O(N*log(N)) we've discussed previously). I think it's a theoretical problem only, given the expected size of the control list, I expect the difference to be completely negligible in practice. As for the fragility, I was referring to the fact that we store ub v4l2Ctrls pointers to data from the ControlValue, stored in the ControlList. Any reallocation in the underlying container would make those pointers invalid. It's an existing issue, but with the current implementation, we don't touch the ControlList after adding controls to it at the beginning of getControls(), while with this patch we rely on ctrls.set(id, std::move(value)); to not cause a reallocation. I think that's guaranteed by the current implementation of ControlList, but if that changes later, we'll possibly have hard to debug bugs. These two points are not blocker by themselves, but the gains should outweight the risks. It will be easier to check that after rebasing patches 1/4 and 2/4 on top of the VLA removal. > > Let's split this in two parts, with the fixes first, and the rework on > > top, so both can be discussed separately. I've posted the former as a > > v6. > > > > > } > > > > > > + v4l2Ctrls.resize(ctrls.size()); > > > + > > > 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 +242,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 Mon, Apr 26, 2021 at 11:46 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote: > > On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote: > > > On Fri, Apr 23, 2021 at 06:36:51PM +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. > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > --- > > > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------ > > > > 1 file changed, 20 insertions(+), 21 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index ce2860c4..bbe8f154 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -173,13 +173,18 @@ 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_ }; > > > > + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); > > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > > > + > > > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) { > > > > + const unsigned int id = ids[j]; > > > > + if (ctrls.contains(id)) > > > > + continue; > > > > > > > > - for (uint32_t id : ids) { > > > > const auto iter = controls_.find(id); > > > > if (iter == controls_.end()) { > > > > LOG(V4L2, Error) > > > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > return {}; > > > > } > > > > > > > > - ctrls.set(id, {}); > > > > - } > > > > - > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > > - > > > > - unsigned int i = 0; > > > > - for (auto &ctrl : ctrls) { > > > > - unsigned int id = ctrl.first; > > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; > > > > > > You increase i here, ... > > > > > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > > + ControlValue value{}; > > > > > > ControlValue has a default constructor, no need for {}. > > > > > > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > > ControlType type; > > > > - > > > > switch (info.type) { > > > > case V4L2_CTRL_TYPE_U8: > > > > type = ControlTypeByte; > > > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > return {}; > > > > } > > > > > > > > - ControlValue &value = ctrl.second; > > > > value.reserve(type, true, info.elems); > > > > Span<uint8_t> data = value.data(); > > > > > > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > v4l2Ctrls[i].size = data.size(); > > > > > > ... and use it here. I don't think that's correct. > > > > > > > } > > > > > > > > - v4l2Ctrls[i].id = id; > > > > - i++; > > > > + v4l2Ctrl.id = id; > > > > > > Should we move this just after the declaration of v4l2Ctrl above ? > > > > > > > + ctrls.set(id, std::move(value)); > > > > > > v4l2Ctrls contains pointers to data stored in the ControlValue > > > instances. As far as I can tell the pointers will remain valid, but > > > that's very dependent on the internals of ControlList. > > > > > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series. > > > They make the code more fragile and possibly a bit less efficient > > > (although that's likely not significant, as there shouldn't be thousands > > > of controls in the list). The VLA removal is fine, but the rest doesn't > > > bring much value in my opinion. > > > > Can you clarify more how the new code is fragile and less efficient? > > You're right, I am sorry that this implementation has some problems. > > But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs? > > The efficiency is related to the additional lookup in updateControls() > (the O(N*log(N)) we've discussed previously). I think it's a theoretical > problem only, given the expected size of the control list, I expect the > difference to be completely negligible in practice. As for the > fragility, I was referring to the fact that we store ub v4l2Ctrls > pointers to data from the ControlValue, stored in the ControlList. Any > reallocation in the underlying container would make those pointers > invalid. It's an existing issue, but with the current implementation, we > don't touch the ControlList after adding controls to it at the beginning > of getControls(), while with this patch we rely on > > ctrls.set(id, std::move(value)); > Yes, this should be ctrls.set(id, {}) or ctrls.set(id, value). Sorry this std::move() is just confusing, which is actually equivalent to ctrls.set(id, value). > to not cause a reallocation. I think that's guaranteed by the current > implementation of ControlList, but if that changes later, we'll possibly > have hard to debug bugs. > > These two points are not blocker by themselves, but the gains should > outweight the risks. It will be easier to check that after rebasing > patches 1/4 and 2/4 on top of the VLA removal. > Okay, I would like to merge patch 1/4 at least. I think our life will be easier if we have utils::enumerate(). > > > Let's split this in two parts, with the fixes first, and the rework on > > > top, so both can be discussed separately. I've posted the former as a > > > v6. > > > > > > > } > > > > > > > > + v4l2Ctrls.resize(ctrls.size()); > > > > + > > > > 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 +242,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
Hi Hiro, On Mon, Apr 26, 2021 at 11:55:53AM +0900, Hirokazu Honda wrote: > On Mon, Apr 26, 2021 at 11:46 AM Laurent Pinchart wrote: > > On Mon, Apr 26, 2021 at 11:01:11AM +0900, Hirokazu Honda wrote: > > > On Mon, Apr 26, 2021 at 9:12 AM Laurent Pinchart wrote: > > > > On Fri, Apr 23, 2021 at 06:36:51PM +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. > > > > > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > > > > --- > > > > > src/libcamera/v4l2_device.cpp | 41 +++++++++++++++++------------------ > > > > > 1 file changed, 20 insertions(+), 21 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > > index ce2860c4..bbe8f154 100644 > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > @@ -173,13 +173,18 @@ 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_ }; > > > > > + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); > > > > > + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); > > > > > + > > > > > + for (size_t i = 0, j = 0; j < ids.size(); ++j) { > > > > > + const unsigned int id = ids[j]; > > > > > + if (ctrls.contains(id)) > > > > > + continue; > > > > > > > > > > - for (uint32_t id : ids) { > > > > > const auto iter = controls_.find(id); > > > > > if (iter == controls_.end()) { > > > > > LOG(V4L2, Error) > > > > > @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > > return {}; > > > > > } > > > > > > > > > > - ctrls.set(id, {}); > > > > > - } > > > > > - > > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > > > - > > > > > - unsigned int i = 0; > > > > > - for (auto &ctrl : ctrls) { > > > > > - unsigned int id = ctrl.first; > > > > > + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; > > > > > > > > You increase i here, ... > > > > > > > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > > > + ControlValue value{}; > > > > > > > > ControlValue has a default constructor, no need for {}. > > > > > > > > > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > > > ControlType type; > > > > > - > > > > > switch (info.type) { > > > > > case V4L2_CTRL_TYPE_U8: > > > > > type = ControlTypeByte; > > > > > @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > > return {}; > > > > > } > > > > > > > > > > - ControlValue &value = ctrl.second; > > > > > value.reserve(type, true, info.elems); > > > > > Span<uint8_t> data = value.data(); > > > > > > > > > > @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > > v4l2Ctrls[i].size = data.size(); > > > > > > > > ... and use it here. I don't think that's correct. > > > > > > > > > } > > > > > > > > > > - v4l2Ctrls[i].id = id; > > > > > - i++; > > > > > + v4l2Ctrl.id = id; > > > > > > > > Should we move this just after the declaration of v4l2Ctrl above ? > > > > > > > > > + ctrls.set(id, std::move(value)); > > > > > > > > v4l2Ctrls contains pointers to data stored in the ControlValue > > > > instances. As far as I can tell the pointers will remain valid, but > > > > that's very dependent on the internals of ControlList. > > > > > > > > To be honest, I'm not very fond of patches 1/4 and 2/4 in this series. > > > > They make the code more fragile and possibly a bit less efficient > > > > (although that's likely not significant, as there shouldn't be thousands > > > > of controls in the list). The VLA removal is fine, but the rest doesn't > > > > bring much value in my opinion. > > > > > > Can you clarify more how the new code is fragile and less efficient? > > > You're right, I am sorry that this implementation has some problems. > > > But I think 1/4 is worth doing and correct, and 2/4 is good if I fix the bugs? > > > > The efficiency is related to the additional lookup in updateControls() > > (the O(N*log(N)) we've discussed previously). I think it's a theoretical > > problem only, given the expected size of the control list, I expect the > > difference to be completely negligible in practice. As for the > > fragility, I was referring to the fact that we store ub v4l2Ctrls > > pointers to data from the ControlValue, stored in the ControlList. Any > > reallocation in the underlying container would make those pointers > > invalid. It's an existing issue, but with the current implementation, we > > don't touch the ControlList after adding controls to it at the beginning > > of getControls(), while with this patch we rely on > > > > ctrls.set(id, std::move(value)); > > Yes, this should be ctrls.set(id, {}) or ctrls.set(id, value). > Sorry this std::move() is just confusing, which is actually equivalent > to ctrls.set(id, value). > > > to not cause a reallocation. I think that's guaranteed by the current > > implementation of ControlList, but if that changes later, we'll possibly > > have hard to debug bugs. > > > > These two points are not blocker by themselves, but the gains should > > outweight the risks. It will be easier to check that after rebasing > > patches 1/4 and 2/4 on top of the VLA removal. > > Okay, I would like to merge patch 1/4 at least. > I think our life will be easier if we have utils::enumerate(). If you can review that patch, I'll merge it :-) > > > > Let's split this in two parts, with the fixes first, and the rework on > > > > top, so both can be discussed separately. I've posted the former as a > > > > v6. > > > > > > > > > } > > > > > > > > > > + v4l2Ctrls.resize(ctrls.size()); > > > > > + > > > > > 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 +242,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; > > > > > }
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index ce2860c4..bbe8f154 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -173,13 +173,18 @@ 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_ }; + std::vector<v4l2_ext_control> v4l2Ctrls(ids.size()); + memset(v4l2Ctrls.data(), 0, sizeof(v4l2_ext_control) * ctrls.size()); + + for (size_t i = 0, j = 0; j < ids.size(); ++j) { + const unsigned int id = ids[j]; + if (ctrls.contains(id)) + continue; - for (uint32_t id : ids) { const auto iter = controls_.find(id); if (iter == controls_.end()) { LOG(V4L2, Error) @@ -187,20 +192,12 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) return {}; } - ctrls.set(id, {}); - } - - struct v4l2_ext_control v4l2Ctrls[count]; - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); - - unsigned int i = 0; - for (auto &ctrl : ctrls) { - unsigned int id = ctrl.first; + v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i++]; const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; + ControlValue value{}; if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { ControlType type; - switch (info.type) { case V4L2_CTRL_TYPE_U8: type = ControlTypeByte; @@ -213,7 +210,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) return {}; } - ControlValue &value = ctrl.second; value.reserve(type, true, info.elems); Span<uint8_t> data = value.data(); @@ -221,21 +217,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) v4l2Ctrls[i].size = data.size(); } - v4l2Ctrls[i].id = id; - i++; + v4l2Ctrl.id = id; + ctrls.set(id, std::move(value)); } + v4l2Ctrls.resize(ctrls.size()); + 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 +242,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 | 41 +++++++++++++++++------------------ 1 file changed, 20 insertions(+), 21 deletions(-)