| Message ID | 20210423093653.1726488-1-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:50PM +0900, Hirokazu Honda wrote: > The original updateControls() has the assumption that ctrls and > v4l2Ctrls lists are in the same order. It is dependent on the > caller implementation though. This changes updateControls() > implementation so that it works without the assumption. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/v4l2_device.cpp | 27 ++++++++------------------- > 1 file changed, 8 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index decd19ef..ce2860c4 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > ControlList ctrls{ controls_ }; > > - /* > - * Start by filling the ControlList. This can't be combined with filling > - * v4l2Ctrls, as updateControls() relies on both containers having the > - * same order, and the control list is based on a map, which is not > - * sorted by insertion order. > - */ > for (uint32_t id : ids) { > const auto iter = controls_.find(id); > if (iter == controls_.end()) { > @@ -525,19 +519,14 @@ void V4L2Device::updateControls(ControlList *ctrls, > const struct v4l2_ext_control *v4l2Ctrls, > unsigned int count) > { > - unsigned int i = 0; > - for (auto &ctrl : *ctrls) { > - if (i == count) > - break; > + for (unsigned int i = 0; i < count; i++) { > + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > + const unsigned int id = v4l2Ctrl.id; The main reason why we had a local id variable is because using ctrl.first is not very readable. With this patch, we could use v4l2Ctrl.id to replace id below. No need to change this though, a local variable is fine. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > - unsigned int id = ctrl.first; > - ControlValue &value = ctrl.second; > - > - const auto iter = controls_.find(id); > - switch (iter->first->type()) { > + ControlValue value = ctrls->get(id); > + switch (value.type()) { > case ControlTypeInteger64: > - value.set<int64_t>(v4l2Ctrl->value64); > + value.set<int64_t>(v4l2Ctrl.value64); > break; > > case ControlTypeByte: > @@ -552,11 +541,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > * \todo To be changed when support for string controls > * will be added. > */ > - value.set<int32_t>(v4l2Ctrl->value); > + value.set<int32_t>(v4l2Ctrl.value); > break; > } > > - i++; > + ctrls->set(id, value); > } > } >
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index decd19ef..ce2860c4 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) ControlList ctrls{ controls_ }; - /* - * Start by filling the ControlList. This can't be combined with filling - * v4l2Ctrls, as updateControls() relies on both containers having the - * same order, and the control list is based on a map, which is not - * sorted by insertion order. - */ for (uint32_t id : ids) { const auto iter = controls_.find(id); if (iter == controls_.end()) { @@ -525,19 +519,14 @@ void V4L2Device::updateControls(ControlList *ctrls, const struct v4l2_ext_control *v4l2Ctrls, unsigned int count) { - unsigned int i = 0; - for (auto &ctrl : *ctrls) { - if (i == count) - break; + for (unsigned int i = 0; i < count; i++) { + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; + const unsigned int id = v4l2Ctrl.id; - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; - unsigned int id = ctrl.first; - ControlValue &value = ctrl.second; - - const auto iter = controls_.find(id); - switch (iter->first->type()) { + ControlValue value = ctrls->get(id); + switch (value.type()) { case ControlTypeInteger64: - value.set<int64_t>(v4l2Ctrl->value64); + value.set<int64_t>(v4l2Ctrl.value64); break; case ControlTypeByte: @@ -552,11 +541,11 @@ void V4L2Device::updateControls(ControlList *ctrls, * \todo To be changed when support for string controls * will be added. */ - value.set<int32_t>(v4l2Ctrl->value); + value.set<int32_t>(v4l2Ctrl.value); break; } - i++; + ctrls->set(id, value); } }
The original updateControls() has the assumption that ctrls and v4l2Ctrls lists are in the same order. It is dependent on the caller implementation though. This changes updateControls() implementation so that it works without the assumption. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/v4l2_device.cpp | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-)