| Message ID | 20210415044854.3348529-1-hiroh@chromium.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 8fd79934..ee8c3fed 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) Unrelated to this patch, it occurred to me that the function should take a Span<const uint32_t> instead of a vector. Would you like to submit an additional patch ? > { > - unsigned int count = ids.size(); > - if (count == 0) > + if (ids.empty()) > return {}; > > ControlList ctrls; > @@ -196,22 +195,25 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > ctrls.set(id, {}); > } > > - struct v4l2_ext_control v4l2Ctrls[count]; > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > - > - unsigned int i = 0; > + std::vector<v4l2_ext_control> v4l2Ctrls; > + v4l2Ctrls.reserve(ctrls.size()); This changes the behaviour slightly, in that if the ids vector contains the same ID multiple times, you'll allocate less memory here. That's actually better :-) > for (auto &ctrl : ctrls) { > unsigned int id = ctrl.first; > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > + v4l2_ext_control v4l2Ctrl; > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl)); > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > - ControlType type; > + ControlValue &value = ctrl.second; > > switch (info.type) { > - case V4L2_CTRL_TYPE_U8: > - type = ControlTypeByte; > + case V4L2_CTRL_TYPE_U8: { > + value.reserve(ControlTypeByte, true, info.elems); > + Span<uint8_t> data = value.data(); > + v4l2Ctrl.p_u8 = data.data(); > + v4l2Ctrl.size = data.size(); > break; > - > + } > default: > LOG(V4L2, Error) > << "Unsupported payload control type " > @@ -219,29 +221,23 @@ 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(); > - > - v4l2Ctrls[i].p_u8 = data.data(); > - v4l2Ctrls[i].size = data.size(); Is there a need to move this code to the switch ? It looks fairly generic, it would be nice not to duplicate it when (if) we latter add support for other control types (such as V4L2_CTRL_TYPE_U16). > } > > - v4l2Ctrls[i].id = id; > - i++; > + v4l2Ctrl.id = id; > + v4l2Ctrls.push_back(std::move(v4l2Ctrl)); The move won't help much, it will copy the v4l2_ext_control instance. Should we resize() instead of reserve(), and operate directly on the vector entry in this loop ? > } > > 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 {}; > @@ -250,10 +246,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()); updateControls should also take a Span argument, that could be done in the same patch that updates the argument to getControls(). Overall this patch is good, the comments above are small issues. > > return ctrls; > }
On Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > Thank you for the patch. > > On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++------------------- > > 1 file changed, 20 insertions(+), 23 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 8fd79934..ee8c3fed 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) > > Unrelated to this patch, it occurred to me that the function should take > a Span<const uint32_t> instead of a vector. Would you like to submit an > additional patch ? > I tried this today. Span has an issue in a call like the following? ../src/libcamera/camera_sensor.cpp:804:43: error: no matching constructor for initialization of 'Span<const uint32_t>' (aka 'Span<const unsigned int>') ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, > > { > > - unsigned int count = ids.size(); > > - if (count == 0) > > + if (ids.empty()) > > return {}; > > > > ControlList ctrls; > > @@ -196,22 +195,25 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > ctrls.set(id, {}); > > } > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > - > > - unsigned int i = 0; > > + std::vector<v4l2_ext_control> v4l2Ctrls; > > + v4l2Ctrls.reserve(ctrls.size()); > > This changes the behaviour slightly, in that if the ids vector contains > the same ID multiple times, you'll allocate less memory here. That's > actually better :-) > > > for (auto &ctrl : ctrls) { > > unsigned int id = ctrl.first; > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > + v4l2_ext_control v4l2Ctrl; > > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl)); > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > - ControlType type; > > + ControlValue &value = ctrl.second; > > > > switch (info.type) { > > - case V4L2_CTRL_TYPE_U8: > > - type = ControlTypeByte; > > + case V4L2_CTRL_TYPE_U8: { > > + value.reserve(ControlTypeByte, true, info.elems); > > + Span<uint8_t> data = value.data(); > > + v4l2Ctrl.p_u8 = data.data(); > > + v4l2Ctrl.size = data.size(); > > break; > > - > > + } > > default: > > LOG(V4L2, Error) > > << "Unsupported payload control type " > > @@ -219,29 +221,23 @@ 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(); > > - > > - v4l2Ctrls[i].p_u8 = data.data(); > > - v4l2Ctrls[i].size = data.size(); > > Is there a need to move this code to the switch ? It looks fairly > generic, it would be nice not to duplicate it when (if) we latter add > support for other control types (such as V4L2_CTRL_TYPE_U16). > > > } > > > > - v4l2Ctrls[i].id = id; > > - i++; > > + v4l2Ctrl.id = id; > > + v4l2Ctrls.push_back(std::move(v4l2Ctrl)); > > The move won't help much, it will copy the v4l2_ext_control instance. > Should we resize() instead of reserve(), and operate directly on the > vector entry in this loop ? > > > } > > > > 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 {}; > > @@ -250,10 +246,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()); > > updateControls should also take a Span argument, that could be done in > the same patch that updates the argument to getControls(). > > Overall this patch is good, the comments above are small issues. > > > > > return ctrls; > > } > -- > Regards, > > Laurent Pinchart
Hi Hiro, On Thu, Apr 22, 2021 at 11:09:06AM +0900, Hirokazu Honda wrote: > On Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart wrote: > > On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++------------------- > > > 1 file changed, 20 insertions(+), 23 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 8fd79934..ee8c3fed 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) > > > > Unrelated to this patch, it occurred to me that the function should take > > a Span<const uint32_t> instead of a vector. Would you like to submit an > > additional patch ? > > I tried this today. Span has an issue in a call like the following? > > ../src/libcamera/camera_sensor.cpp:804:43: error: no matching > constructor for initialization of 'Span<const uint32_t>' (aka > 'Span<const unsigned int>') > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, Indeed, the span class has no constructor that takes an std::initializer_list. This could be solved by constructing the initializer list explicitly in the caller (but would still require the following patch). diff --git a/include/libcamera/span.h b/include/libcamera/span.h index 7a35806b1710..33e059aac102 100644 --- a/include/libcamera/span.h +++ b/include/libcamera/span.h @@ -67,6 +67,12 @@ constexpr T *data(T (&array)[N]) noexcept return array; } +template<typename T> +constexpr const T *data(std::initializer_list<T> c) noexcept +{ + return c.begin(); +} + template<std::size_t I, typename T> struct tuple_element; Alternatively, we could add a consructor take takes an initializer list. Let's postpone this for now. > > > { > > > - unsigned int count = ids.size(); > > > - if (count == 0) > > > + if (ids.empty()) > > > return {}; > > > > > > ControlList ctrls; > > > @@ -196,22 +195,25 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > ctrls.set(id, {}); > > > } > > > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > - > > > - unsigned int i = 0; > > > + std::vector<v4l2_ext_control> v4l2Ctrls; > > > + v4l2Ctrls.reserve(ctrls.size()); > > > > This changes the behaviour slightly, in that if the ids vector contains > > the same ID multiple times, you'll allocate less memory here. That's > > actually better :-) > > > > > for (auto &ctrl : ctrls) { > > > unsigned int id = ctrl.first; > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > + v4l2_ext_control v4l2Ctrl; > > > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl)); > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > - ControlType type; > > > + ControlValue &value = ctrl.second; > > > > > > switch (info.type) { > > > - case V4L2_CTRL_TYPE_U8: > > > - type = ControlTypeByte; > > > + case V4L2_CTRL_TYPE_U8: { > > > + value.reserve(ControlTypeByte, true, info.elems); > > > + Span<uint8_t> data = value.data(); > > > + v4l2Ctrl.p_u8 = data.data(); > > > + v4l2Ctrl.size = data.size(); > > > break; > > > - > > > + } > > > default: > > > LOG(V4L2, Error) > > > << "Unsupported payload control type " > > > @@ -219,29 +221,23 @@ 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(); > > > - > > > - v4l2Ctrls[i].p_u8 = data.data(); > > > - v4l2Ctrls[i].size = data.size(); > > > > Is there a need to move this code to the switch ? It looks fairly > > generic, it would be nice not to duplicate it when (if) we latter add > > support for other control types (such as V4L2_CTRL_TYPE_U16). > > > > > } > > > > > > - v4l2Ctrls[i].id = id; > > > - i++; > > > + v4l2Ctrl.id = id; > > > + v4l2Ctrls.push_back(std::move(v4l2Ctrl)); > > > > The move won't help much, it will copy the v4l2_ext_control instance. > > Should we resize() instead of reserve(), and operate directly on the > > vector entry in this loop ? > > > > > } > > > > > > 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 {}; > > > @@ -250,10 +246,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()); > > > > updateControls should also take a Span argument, that could be done in > > the same patch that updates the argument to getControls(). > > > > Overall this patch is good, the comments above are small issues. > > > > > > > > return ctrls; > > > }
Hi Laurent, On Tue, Apr 27, 2021 at 10:25 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Hiro, > > On Thu, Apr 22, 2021 at 11:09:06AM +0900, Hirokazu Honda wrote: > > On Tue, Apr 20, 2021 at 9:59 AM Laurent Pinchart wrote: > > > On Thu, Apr 15, 2021 at 01:48:52PM +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 | 43 ++++++++++++++++------------------- > > > > 1 file changed, 20 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > > index 8fd79934..ee8c3fed 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) > > > > > > Unrelated to this patch, it occurred to me that the function should take > > > a Span<const uint32_t> instead of a vector. Would you like to submit an > > > additional patch ? > > > > I tried this today. Span has an issue in a call like the following? > > > > ../src/libcamera/camera_sensor.cpp:804:43: error: no matching > > constructor for initialization of 'Span<const uint32_t>' (aka > > 'Span<const unsigned int>') > > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE, > > Indeed, the span class has no constructor that takes an > std::initializer_list. This could be solved by constructing the > initializer list explicitly in the caller (but would still require the > following patch). > Thanks. I note the task to my task sets which I deal with when I have quite a bit time. -Hiro > diff --git a/include/libcamera/span.h b/include/libcamera/span.h > index 7a35806b1710..33e059aac102 100644 > --- a/include/libcamera/span.h > +++ b/include/libcamera/span.h > @@ -67,6 +67,12 @@ constexpr T *data(T (&array)[N]) noexcept > return array; > } > > +template<typename T> > +constexpr const T *data(std::initializer_list<T> c) noexcept > +{ > + return c.begin(); > +} > + > template<std::size_t I, typename T> > struct tuple_element; > > Alternatively, we could add a consructor take takes an initializer list. > Let's postpone this for now. > > > > > { > > > > - unsigned int count = ids.size(); > > > > - if (count == 0) > > > > + if (ids.empty()) > > > > return {}; > > > > > > > > ControlList ctrls; > > > > @@ -196,22 +195,25 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > > ctrls.set(id, {}); > > > > } > > > > > > > > - struct v4l2_ext_control v4l2Ctrls[count]; > > > > - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); > > > > - > > > > - unsigned int i = 0; > > > > + std::vector<v4l2_ext_control> v4l2Ctrls; > > > > + v4l2Ctrls.reserve(ctrls.size()); > > > > > > This changes the behaviour slightly, in that if the ids vector contains > > > the same ID multiple times, you'll allocate less memory here. That's > > > actually better :-) > > > > > > > for (auto &ctrl : ctrls) { > > > > unsigned int id = ctrl.first; > > > > const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; > > > > + v4l2_ext_control v4l2Ctrl; > > > > + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl)); > > > > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > > - ControlType type; > > > > + ControlValue &value = ctrl.second; > > > > > > > > switch (info.type) { > > > > - case V4L2_CTRL_TYPE_U8: > > > > - type = ControlTypeByte; > > > > + case V4L2_CTRL_TYPE_U8: { > > > > + value.reserve(ControlTypeByte, true, info.elems); > > > > + Span<uint8_t> data = value.data(); > > > > + v4l2Ctrl.p_u8 = data.data(); > > > > + v4l2Ctrl.size = data.size(); > > > > break; > > > > - > > > > + } > > > > default: > > > > LOG(V4L2, Error) > > > > << "Unsupported payload control type " > > > > @@ -219,29 +221,23 @@ 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(); > > > > - > > > > - v4l2Ctrls[i].p_u8 = data.data(); > > > > - v4l2Ctrls[i].size = data.size(); > > > > > > Is there a need to move this code to the switch ? It looks fairly > > > generic, it would be nice not to duplicate it when (if) we latter add > > > support for other control types (such as V4L2_CTRL_TYPE_U16). > > > > > > > } > > > > > > > > - v4l2Ctrls[i].id = id; > > > > - i++; > > > > + v4l2Ctrl.id = id; > > > > + v4l2Ctrls.push_back(std::move(v4l2Ctrl)); > > > > > > The move won't help much, it will copy the v4l2_ext_control instance. > > > Should we resize() instead of reserve(), and operate directly on the > > > vector entry in this loop ? > > > > > > > } > > > > > > > > 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 {}; > > > > @@ -250,10 +246,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()); > > > > > > updateControls should also take a Span argument, that could be done in > > > the same patch that updates the argument to getControls(). > > > > > > Overall this patch is good, the comments above are small issues. > > > > > > > > > > > return ctrls; > > > > } > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 8fd79934..ee8c3fed 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; @@ -196,22 +195,25 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) ctrls.set(id, {}); } - struct v4l2_ext_control v4l2Ctrls[count]; - memset(v4l2Ctrls, 0, sizeof(v4l2Ctrls)); - - unsigned int i = 0; + std::vector<v4l2_ext_control> v4l2Ctrls; + v4l2Ctrls.reserve(ctrls.size()); for (auto &ctrl : ctrls) { unsigned int id = ctrl.first; const struct v4l2_query_ext_ctrl &info = controlInfo_[id]; + v4l2_ext_control v4l2Ctrl; + memset(&v4l2Ctrl, 0, sizeof(v4l2Ctrl)); if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { - ControlType type; + ControlValue &value = ctrl.second; switch (info.type) { - case V4L2_CTRL_TYPE_U8: - type = ControlTypeByte; + case V4L2_CTRL_TYPE_U8: { + value.reserve(ControlTypeByte, true, info.elems); + Span<uint8_t> data = value.data(); + v4l2Ctrl.p_u8 = data.data(); + v4l2Ctrl.size = data.size(); break; - + } default: LOG(V4L2, Error) << "Unsupported payload control type " @@ -219,29 +221,23 @@ 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(); - - v4l2Ctrls[i].p_u8 = data.data(); - v4l2Ctrls[i].size = data.size(); } - v4l2Ctrls[i].id = id; - i++; + v4l2Ctrl.id = id; + v4l2Ctrls.push_back(std::move(v4l2Ctrl)); } 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 {}; @@ -250,10 +246,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 | 43 ++++++++++++++++------------------- 1 file changed, 20 insertions(+), 23 deletions(-)