| Message ID | 20210427012150.5338-1-hiroh@chromium.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. I'm going through the patchwork backlog and realized I had forgotten about this one. Sorry about that. On Tue, Apr 27, 2021 at 10:21:50AM +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> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Will push shortly once the tests pass. > --- > src/libcamera/v4l2_device.cpp | 26 +++++++------------------- > 1 file changed, 7 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 397029ac..64501523 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -178,12 +178,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()) { > @@ -520,19 +514,13 @@ void V4L2Device::listControls() > void V4L2Device::updateControls(ControlList *ctrls, > Span<const v4l2_ext_control> v4l2Ctrls) > { > - unsigned int i = 0; > - for (auto &ctrl : *ctrls) { > - if (i == v4l2Ctrls.size()) > - break; > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > + 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: > @@ -547,11 +535,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); > } > } >
Hi Laurent, On Tue, May 25, 2021 at 8:52 AM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > Thank you for the patch. I'm going through the patchwork backlog and > realized I had forgotten about this one. Sorry about that. > > On Tue, Apr 27, 2021 at 10:21:50AM +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> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Will push shortly once the tests pass. > > I am sorry I noticed the patch has a bug. Somehow I haven't caught it. I submit a fix patch for review. https://patchwork.libcamera.org/patch/12396/ -Hiro > > --- > > src/libcamera/v4l2_device.cpp | 26 +++++++------------------- > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > index 397029ac..64501523 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -178,12 +178,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()) { > > @@ -520,19 +514,13 @@ void V4L2Device::listControls() > > void V4L2Device::updateControls(ControlList *ctrls, > > Span<const v4l2_ext_control> v4l2Ctrls) > > { > > - unsigned int i = 0; > > - for (auto &ctrl : *ctrls) { > > - if (i == v4l2Ctrls.size()) > > - break; > > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > > + 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: > > @@ -547,11 +535,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); > > } > > } > > > > -- > Regards, > > Laurent Pinchart >
Hi Hiro, On Tue, May 25, 2021 at 02:35:17PM +0900, Hirokazu Honda wrote: > On Tue, May 25, 2021 at 8:52 AM Laurent Pinchart wrote: > > Hi Hiro, > > > > Thank you for the patch. I'm going through the patchwork backlog and > > realized I had forgotten about this one. Sorry about that. > > > > On Tue, Apr 27, 2021 at 10:21:50AM +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> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Will push shortly once the tests pass. > > I am sorry I noticed the patch has a bug. Somehow I haven't caught it. > I submit a fix patch for review. > https://patchwork.libcamera.org/patch/12396/ Oops :-S We've received reports from users who are affected by this, so I've sent a revert that I'll merge now, to be able to discuss the fix without pressure. > > > --- > > > src/libcamera/v4l2_device.cpp | 26 +++++++------------------- > > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index 397029ac..64501523 100644 > > > --- a/src/libcamera/v4l2_device.cpp > > > +++ b/src/libcamera/v4l2_device.cpp > > > @@ -178,12 +178,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()) { > > > @@ -520,19 +514,13 @@ void V4L2Device::listControls() > > > void V4L2Device::updateControls(ControlList *ctrls, > > > Span<const v4l2_ext_control> v4l2Ctrls) > > > { > > > - unsigned int i = 0; > > > - for (auto &ctrl : *ctrls) { > > > - if (i == v4l2Ctrls.size()) > > > - break; > > > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > > > + 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: > > > @@ -547,11 +535,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); > > > } > > > } > > >
Hi Laurent, On Tue, May 25, 2021 at 5:58 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > On Tue, May 25, 2021 at 02:35:17PM +0900, Hirokazu Honda wrote: > > On Tue, May 25, 2021 at 8:52 AM Laurent Pinchart wrote: > > > Hi Hiro, > > > > > > Thank you for the patch. I'm going through the patchwork backlog and > > > realized I had forgotten about this one. Sorry about that. > > > > > > On Tue, Apr 27, 2021 at 10:21:50AM +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> > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > Will push shortly once the tests pass. > > > > I am sorry I noticed the patch has a bug. Somehow I haven't caught it. > > I submit a fix patch for review. > > https://patchwork.libcamera.org/patch/12396/ > > Oops :-S We've received reports from users who are affected by this, so > I've sent a revert that I'll merge now, to be able to discuss the fix > without pressure. > > Sounds good. sorry for breakage. -Hiro > > > > --- > > > > src/libcamera/v4l2_device.cpp | 26 +++++++------------------- > > > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > > > > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > > > index 397029ac..64501523 100644 > > > > --- a/src/libcamera/v4l2_device.cpp > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > @@ -178,12 +178,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()) { > > > > @@ -520,19 +514,13 @@ void V4L2Device::listControls() > > > > void V4L2Device::updateControls(ControlList *ctrls, > > > > Span<const v4l2_ext_control> v4l2Ctrls) > > > > { > > > > - unsigned int i = 0; > > > > - for (auto &ctrl : *ctrls) { > > > > - if (i == v4l2Ctrls.size()) > > > > - break; > > > > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > > > > + 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: > > > > @@ -547,11 +535,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); > > > > } > > > > } > > > > > > -- > Regards, > > Laurent Pinchart >
Hi Hiro, On Tue, May 25, 2021 at 06:05:33PM +0900, Hirokazu Honda wrote: > On Tue, May 25, 2021 at 5:58 PM Laurent Pinchart wrote: > > On Tue, May 25, 2021 at 02:35:17PM +0900, Hirokazu Honda wrote: > > > On Tue, May 25, 2021 at 8:52 AM Laurent Pinchart wrote: > > > > Hi Hiro, > > > > > > > > Thank you for the patch. I'm going through the patchwork backlog and > > > > realized I had forgotten about this one. Sorry about that. > > > > > > > > On Tue, Apr 27, 2021 at 10:21:50AM +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> > > > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > Will push shortly once the tests pass. > > > > > > I am sorry I noticed the patch has a bug. Somehow I haven't caught it. > > > I submit a fix patch for review. > > > https://patchwork.libcamera.org/patch/12396/ > > > > Oops :-S We've received reports from users who are affected by this, so > > I've sent a revert that I'll merge now, to be able to discuss the fix > > without pressure. > > Sounds good. sorry for breakage. No worries, it happens. It means we're missing a unit test case though. > > > > > --- > > > > > src/libcamera/v4l2_device.cpp | 26 +++++++------------------- > > > > > 1 file changed, 7 insertions(+), 19 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/v4l2_device.cpp > > b/src/libcamera/v4l2_device.cpp > > > > > index 397029ac..64501523 100644 > > > > > --- a/src/libcamera/v4l2_device.cpp > > > > > +++ b/src/libcamera/v4l2_device.cpp > > > > > @@ -178,12 +178,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()) { > > > > > @@ -520,19 +514,13 @@ void V4L2Device::listControls() > > > > > void V4L2Device::updateControls(ControlList *ctrls, > > > > > Span<const v4l2_ext_control> v4l2Ctrls) > > > > > { > > > > > - unsigned int i = 0; > > > > > - for (auto &ctrl : *ctrls) { > > > > > - if (i == v4l2Ctrls.size()) > > > > > - break; > > > > > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > > > > > + 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: > > > > > @@ -547,11 +535,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 397029ac..64501523 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -178,12 +178,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()) { @@ -520,19 +514,13 @@ void V4L2Device::listControls() void V4L2Device::updateControls(ControlList *ctrls, Span<const v4l2_ext_control> v4l2Ctrls) { - unsigned int i = 0; - for (auto &ctrl : *ctrls) { - if (i == v4l2Ctrls.size()) - break; + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { + 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: @@ -547,11 +535,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 | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-)