| Message ID | 20210413061925.3054927-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ > 1 file changed, 21 insertions(+), 23 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index decd19ef..78c289c2 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_ }; > @@ -196,21 +195,26 @@ 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()); The v4l2ctrls array is created of size count which was ids.size(); Shouldn't we reserve this as ids.size() here instead of 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)); Can v4l2Ctrl initialisation be simplified to this?: v4l2_ext_control v4l2Ctrl = {}; > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > - ControlType type; > - > switch (info.type) { > - case V4L2_CTRL_TYPE_U8: > + case V4L2_CTRL_TYPE_U8: { > + ControlType type; > type = ControlTypeByte; > + ControlValue &value = ctrl.second; > + value.reserve(type, true, info.elems); Is this the only usage of type? Couldn't we just do value.reserve(ControlTypeByte, true, info.elems); Or at the least make it ControlType = ControlTypeByte; rather than use an extra line. > + Span<uint8_t> data = value.data(); > + v4l2Ctrl.p_u8 = data.data(); > + v4l2Ctrl.size = data.size(); > break; > + } > > default: > LOG(V4L2, Error) > @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > << info.type; > 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(); That switch statement must have been arranged like that to facilitate easily adding other types later. If we don't need to do that, can we fix this up to lower the indentation somehow? We're four levels in here in places :-S > } > > - 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 +247,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()); I was going to say can we pass v4l2Ctrls directly, but now I've seen your later patch ;=) > > return ctrls; > } >
Hi Kieran, On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ > > 1 file changed, 21 insertions(+), 23 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index decd19ef..78c289c2 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_ }; > > @@ -196,21 +195,26 @@ 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()); > > The v4l2ctrls array is created of size count which was ids.size(); > Shouldn't we reserve this as ids.size() here instead of ctrls.size()? > Yeah, I am also confused by the point upon creating this patch. The original implementation seems to contain a bug; count = id.size() and v4l2Ctrls's size is count, but the iteration is up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set. I expect this is out-of-bounds bug. This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and keeping the original return semantics. However, I still don't get the original return semantics is correct. The doc states "This method reads the value of all controls contained in \a ids, and returns their values as a ControlList." The implementation looks to return all the control values regardless of ids. +Laurent Pinchart do you have any idea? > > > 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)); > > Can v4l2Ctrl initialisation be simplified to this?: > v4l2_ext_control v4l2Ctrl = {}; > > > > > if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > - ControlType type; > > - > > switch (info.type) { > > - case V4L2_CTRL_TYPE_U8: > > + case V4L2_CTRL_TYPE_U8: { > > + ControlType type; > > type = ControlTypeByte; > > + ControlValue &value = ctrl.second; > > + value.reserve(type, true, info.elems); > > Is this the only usage of type? Couldn't we just do > value.reserve(ControlTypeByte, true, info.elems); > > Or at the least make it > > ControlType = ControlTypeByte; > > rather than use an extra line. > > > > + Span<uint8_t> data = value.data(); > > + v4l2Ctrl.p_u8 = data.data(); > > + v4l2Ctrl.size = data.size(); > > break; > > + } > > > > default: > > LOG(V4L2, Error) > > @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > << info.type; > > 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(); > > That switch statement must have been arranged like that to facilitate > easily adding other types later. > > If we don't need to do that, can we fix this up to lower the indentation > somehow? We're four levels in here in places :-S > > > > } > > > > - 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 +247,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()); > > I was going to say can we pass v4l2Ctrls directly, but now I've seen > your later patch ;=) > > > > > > > return ctrls; > > } > > > > -- > Regards > -- > Kieran
Hi Hiro, On 14/04/2021 02:45, Hirokazu Honda wrote: > Hi Kieran, > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: >> >> Hi Hiro, >> >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ >>> 1 file changed, 21 insertions(+), 23 deletions(-) >>> >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp >>> index decd19ef..78c289c2 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_ }; >>> @@ -196,21 +195,26 @@ 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()); >> >> The v4l2ctrls array is created of size count which was ids.size(); >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()? >> > > Yeah, I am also confused by the point upon creating this patch. > The original implementation seems to contain a bug; > count = id.size() and v4l2Ctrls's size is count, but the iteration is > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set. > I expect this is out-of-bounds bug. > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and > keeping the original return semantics. > However, I still don't get the original return semantics is correct. > The doc states "This method reads the value of all controls contained > in \a ids, and returns their values as a ControlList." > The implementation looks to return all the control values regardless of ids. > +Laurent Pinchart do you have any idea? I think this is where it's important to keep distinct changes in separate patches. Try to make this patch perform only the conversion to the vector, with like-for-like (bugz inclusive) functionality, and then it's easier to discuss and fix the bug if it is there directly in it's own patch. > >> >>> 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)); >> >> Can v4l2Ctrl initialisation be simplified to this?: >> v4l2_ext_control v4l2Ctrl = {}; >> >>> >>> if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { >>> - ControlType type; >>> - >>> switch (info.type) { >>> - case V4L2_CTRL_TYPE_U8: >>> + case V4L2_CTRL_TYPE_U8: { >>> + ControlType type; >>> type = ControlTypeByte; >>> + ControlValue &value = ctrl.second; >>> + value.reserve(type, true, info.elems); >> >> Is this the only usage of type? Couldn't we just do >> value.reserve(ControlTypeByte, true, info.elems); >> >> Or at the least make it >> >> ControlType = ControlTypeByte; >> >> rather than use an extra line. >> >> >>> + Span<uint8_t> data = value.data(); >>> + v4l2Ctrl.p_u8 = data.data(); >>> + v4l2Ctrl.size = data.size(); >>> break; >>> + } >>> >>> default: >>> LOG(V4L2, Error) >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) >>> << info.type; >>> 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(); >> >> That switch statement must have been arranged like that to facilitate >> easily adding other types later. >> >> If we don't need to do that, can we fix this up to lower the indentation >> somehow? We're four levels in here in places :-S >> >> >>> } >>> >>> - 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 +247,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()); >> >> I was going to say can we pass v4l2Ctrls directly, but now I've seen >> your later patch ;=) >> >> >> >>> >>> return ctrls; >>> } >>> >> >> -- >> Regards >> -- >> Kieran
On Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 14/04/2021 02:45, Hirokazu Honda wrote: > > Hi Kieran, > > > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > >> > >> Hi Hiro, > >> > >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ > >>> 1 file changed, 21 insertions(+), 23 deletions(-) > >>> > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > >>> index decd19ef..78c289c2 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_ }; > >>> @@ -196,21 +195,26 @@ 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()); > >> > >> The v4l2ctrls array is created of size count which was ids.size(); > >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()? > >> > > > > Yeah, I am also confused by the point upon creating this patch. > > The original implementation seems to contain a bug; > > count = id.size() and v4l2Ctrls's size is count, but the iteration is > > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set. > > I expect this is out-of-bounds bug. > > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and > > keeping the original return semantics. > > However, I still don't get the original return semantics is correct. > > The doc states "This method reads the value of all controls contained > > in \a ids, and returns their values as a ControlList." > > The implementation looks to return all the control values regardless of ids. > > +Laurent Pinchart do you have any idea? > > I think this is where it's important to keep distinct changes in > separate patches. > > Try to make this patch perform only the conversion to the vector, with > like-for-like (bugz inclusive) functionality, and then it's easier to > discuss and fix the bug if it is there directly in it's own patch. > I got it. The bug fix is much simpler than I thought. I submitted as an independent patch and will rebase this patch series on top of it. Thanks, -Hiro > > > >> > >>> 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)); > >> > >> Can v4l2Ctrl initialisation be simplified to this?: > >> v4l2_ext_control v4l2Ctrl = {}; > >> > >>> > >>> if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > >>> - ControlType type; > >>> - > >>> switch (info.type) { > >>> - case V4L2_CTRL_TYPE_U8: > >>> + case V4L2_CTRL_TYPE_U8: { > >>> + ControlType type; > >>> type = ControlTypeByte; > >>> + ControlValue &value = ctrl.second; > >>> + value.reserve(type, true, info.elems); > >> > >> Is this the only usage of type? Couldn't we just do > >> value.reserve(ControlTypeByte, true, info.elems); > >> > >> Or at the least make it > >> > >> ControlType = ControlTypeByte; > >> > >> rather than use an extra line. > >> > >> > >>> + Span<uint8_t> data = value.data(); > >>> + v4l2Ctrl.p_u8 = data.data(); > >>> + v4l2Ctrl.size = data.size(); > >>> break; > >>> + } > >>> > >>> default: > >>> LOG(V4L2, Error) > >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > >>> << info.type; > >>> 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(); > >> > >> That switch statement must have been arranged like that to facilitate > >> easily adding other types later. > >> > >> If we don't need to do that, can we fix this up to lower the indentation > >> somehow? We're four levels in here in places :-S > >> > >> > >>> } > >>> > >>> - 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 +247,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()); > >> > >> I was going to say can we pass v4l2Ctrls directly, but now I've seen > >> your later patch ;=) > >> > >> > >> > >>> > >>> return ctrls; > >>> } > >>> > >> > >> -- > >> Regards > >> -- > >> Kieran > > -- > Regards > -- > Kieran
On Thu, Apr 15, 2021 at 12:10 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham > <kieran.bingham@ideasonboard.com> wrote: > > > > Hi Hiro, > > > > On 14/04/2021 02:45, Hirokazu Honda wrote: > > > Hi Kieran, > > > > > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham > > > <kieran.bingham@ideasonboard.com> wrote: > > >> > > >> Hi Hiro, > > >> > > >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ > > >>> 1 file changed, 21 insertions(+), 23 deletions(-) > > >>> > > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > >>> index decd19ef..78c289c2 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_ }; > > >>> @@ -196,21 +195,26 @@ 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()); > > >> > > >> The v4l2ctrls array is created of size count which was ids.size(); > > >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()? > > >> > > > > > > Yeah, I am also confused by the point upon creating this patch. > > > The original implementation seems to contain a bug; > > > count = id.size() and v4l2Ctrls's size is count, but the iteration is > > > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set. > > > I expect this is out-of-bounds bug. > > > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and > > > keeping the original return semantics. > > > However, I still don't get the original return semantics is correct. > > > The doc states "This method reads the value of all controls contained > > > in \a ids, and returns their values as a ControlList." > > > The implementation looks to return all the control values regardless of ids. > > > +Laurent Pinchart do you have any idea? > > > > I think this is where it's important to keep distinct changes in > > separate patches. > > > > Try to make this patch perform only the conversion to the vector, with > > like-for-like (bugz inclusive) functionality, and then it's easier to > > discuss and fix the bug if it is there directly in it's own patch. > > > > I got it. The bug fix is much simpler than I thought. > I submitted as an independent patch and will rebase this patch series > on top of it. > > Thanks, > -Hiro > > > > > >> > > >>> 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)); > > >> > > >> Can v4l2Ctrl initialisation be simplified to this?: > > >> v4l2_ext_control v4l2Ctrl = {}; > > >> > > >>> > > >>> if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > >>> - ControlType type; > > >>> - > > >>> switch (info.type) { > > >>> - case V4L2_CTRL_TYPE_U8: > > >>> + case V4L2_CTRL_TYPE_U8: { > > >>> + ControlType type; > > >>> type = ControlTypeByte; > > >>> + ControlValue &value = ctrl.second; > > >>> + value.reserve(type, true, info.elems); > > >> > > >> Is this the only usage of type? Couldn't we just do > > >> value.reserve(ControlTypeByte, true, info.elems); > > >> > > >> Or at the least make it > > >> > > >> ControlType = ControlTypeByte; > > >> > > >> rather than use an extra line. > > >> > > >> > > >>> + Span<uint8_t> data = value.data(); > > >>> + v4l2Ctrl.p_u8 = data.data(); > > >>> + v4l2Ctrl.size = data.size(); > > >>> break; > > >>> + } > > >>> > > >>> default: > > >>> LOG(V4L2, Error) > > >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > >>> << info.type; > > >>> 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(); > > >> > > >> That switch statement must have been arranged like that to facilitate > > >> easily adding other types later. > > >> > > >> If we don't need to do that, can we fix this up to lower the indentation > > >> somehow? We're four levels in here in places :-S > > >> Sorry, I don't get your suggestion. Can you elaborate? > > >> > > >>> } > > >>> > > >>> - 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 +247,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()); > > >> > > >> I was going to say can we pass v4l2Ctrls directly, but now I've seen > > >> your later patch ;=) > > >> > > >> > > >> > > >>> > > >>> return ctrls; > > >>> } > > >>> > > >> > > >> -- > > >> Regards > > >> -- > > >> Kieran > > > > -- > > Regards > > -- > > Kieran
On Thu, Apr 15, 2021 at 12:42 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > On Thu, Apr 15, 2021 at 12:10 PM Hirokazu Honda <hiroh@chromium.org> wrote: > > > > On Wed, Apr 14, 2021 at 5:17 PM Kieran Bingham > > <kieran.bingham@ideasonboard.com> wrote: > > > > > > Hi Hiro, > > > > > > On 14/04/2021 02:45, Hirokazu Honda wrote: > > > > Hi Kieran, > > > > > > > > On Wed, Apr 14, 2021 at 6:58 AM Kieran Bingham > > > > <kieran.bingham@ideasonboard.com> wrote: > > > >> > > > >> Hi Hiro, > > > >> > > > >> On 13/04/2021 07:19, 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 | 44 +++++++++++++++++------------------ > > > >>> 1 file changed, 21 insertions(+), 23 deletions(-) > > > >>> > > > >>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > >>> index decd19ef..78c289c2 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_ }; > > > >>> @@ -196,21 +195,26 @@ 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()); > > > >> > > > >> The v4l2ctrls array is created of size count which was ids.size(); > > > >> Shouldn't we reserve this as ids.size() here instead of ctrls.size()? > > > >> > > > > > > > > Yeah, I am also confused by the point upon creating this patch. > > > > The original implementation seems to contain a bug; > > > > count = id.size() and v4l2Ctrls's size is count, but the iteration is > > > > up to ctrls.size() and thus v4l2Ctrls[ctrls.size() - 1] is set. > > > > I expect this is out-of-bounds bug. > > > > This patch fixes it by setting v4l2Ctrls.size() to ctrls.size() and > > > > keeping the original return semantics. > > > > However, I still don't get the original return semantics is correct. > > > > The doc states "This method reads the value of all controls contained > > > > in \a ids, and returns their values as a ControlList." > > > > The implementation looks to return all the control values regardless of ids. > > > > +Laurent Pinchart do you have any idea? > > > > > > I think this is where it's important to keep distinct changes in > > > separate patches. > > > > > > Try to make this patch perform only the conversion to the vector, with > > > like-for-like (bugz inclusive) functionality, and then it's easier to > > > discuss and fix the bug if it is there directly in it's own patch. > > > > > > > I got it. The bug fix is much simpler than I thought. > > I submitted as an independent patch and will rebase this patch series > > on top of it. > > > > Thanks, > > -Hiro > > > > > > > >> > > > >>> 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)); > > > >> > > > >> Can v4l2Ctrl initialisation be simplified to this?: > > > >> v4l2_ext_control v4l2Ctrl = {}; > > > >> Regarding this, we should not trust {} for zero initialization. The problem happens c structure has union. https://en.cppreference.com/w/cpp/language/zero_initialization > If T is a union type, the first non-static named data member is zero-initialized and all padding is initialized to zero bits. This means, some union value might not be zero if the first non static member in union is smaller than others. So I would basically do memset to v4l2 structure, which often has union. -Hiro > > > >>> > > > >>> if (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) { > > > >>> - ControlType type; > > > >>> - > > > >>> switch (info.type) { > > > >>> - case V4L2_CTRL_TYPE_U8: > > > >>> + case V4L2_CTRL_TYPE_U8: { > > > >>> + ControlType type; > > > >>> type = ControlTypeByte; > > > >>> + ControlValue &value = ctrl.second; > > > >>> + value.reserve(type, true, info.elems); > > > >> > > > >> Is this the only usage of type? Couldn't we just do > > > >> value.reserve(ControlTypeByte, true, info.elems); > > > >> > > > >> Or at the least make it > > > >> > > > >> ControlType = ControlTypeByte; > > > >> > > > >> rather than use an extra line. > > > >> > > > >> > > > >>> + Span<uint8_t> data = value.data(); > > > >>> + v4l2Ctrl.p_u8 = data.data(); > > > >>> + v4l2Ctrl.size = data.size(); > > > >>> break; > > > >>> + } > > > >>> > > > >>> default: > > > >>> LOG(V4L2, Error) > > > >>> @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > > >>> << info.type; > > > >>> 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(); > > > >> > > > >> That switch statement must have been arranged like that to facilitate > > > >> easily adding other types later. > > > >> > > > >> If we don't need to do that, can we fix this up to lower the indentation > > > >> somehow? We're four levels in here in places :-S > > > >> > > Sorry, I don't get your suggestion. Can you elaborate? > > > > >> > > > >>> } > > > >>> > > > >>> - 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 +247,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()); > > > >> > > > >> I was going to say can we pass v4l2Ctrls directly, but now I've seen > > > >> your later patch ;=) > > > >> > > > >> > > > >> > > > >>> > > > >>> return ctrls; > > > >>> } > > > >>> > > > >> > > > >> -- > > > >> Regards > > > >> -- > > > >> Kieran > > > > > > -- > > > Regards > > > -- > > > Kieran
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index decd19ef..78c289c2 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_ }; @@ -196,21 +195,26 @@ 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; - switch (info.type) { - case V4L2_CTRL_TYPE_U8: + case V4L2_CTRL_TYPE_U8: { + ControlType type; type = ControlTypeByte; + ControlValue &value = ctrl.second; + value.reserve(type, true, info.elems); + Span<uint8_t> data = value.data(); + v4l2Ctrl.p_u8 = data.data(); + v4l2Ctrl.size = data.size(); break; + } default: LOG(V4L2, Error) @@ -218,30 +222,23 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) << info.type; 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 +247,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 | 44 +++++++++++++++++------------------ 1 file changed, 21 insertions(+), 23 deletions(-)