| Message ID | 20210422021809.520675-2-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote: > The original updateControls() has the assumption that ctrls and > v4l2Ctrls lists in the same order. It is dependent on the caller s/in the same order/are in the same order/ > 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 | 32 +++++++++++++------------------- > 1 file changed, 13 insertions(+), 19 deletions(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index decd19ef..3c32d682 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,19 @@ 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; > - > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > - unsigned int id = ctrl.first; > - ControlValue &value = ctrl.second; > + for (unsigned int i = 0; i < count; i++) { > + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > + const unsigned int id = v4l2Ctrl.id; > + if (!ctrls->contains(id)) { > + LOG(V4L2, Error) << "ControlList doesn't contain id: " > + << id; > + return; > + } Can this happen ? > > - const auto iter = controls_.find(id); > - switch (iter->first->type()) { > + ControlValue value = ctrls->get(id); This should be &value (or better *value, if you want to modify it) ? see [1] > + switch (value.type()) { The type information recorded in controls_ (which is a ControlInfoMap) and in the ControlValue should be the same, hence there should be no functional changes, right ? > case ControlTypeInteger64: > - value.set<int64_t>(v4l2Ctrl->value64); > + value.set<int64_t>(v4l2Ctrl.value64); > break; > > case ControlTypeByte: > @@ -552,11 +546,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); [1] so you can avoid this ? I still think we pay a little performance loss, for the additional crls->get() but the code is nicer to ead indeed Can you specify you've run all tests and they're all good in the cover letter of the next iteration for out peace of mind ? :) Thanks j > } > } > > -- > 2.31.1.368.gbe11c130af-goog > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Hiro, > > On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote: > > The original updateControls() has the assumption that ctrls and > > v4l2Ctrls lists in the same order. It is dependent on the caller > > s/in the same order/are in the same order/ > > > 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 | 32 +++++++++++++------------------- > > 1 file changed, 13 insertions(+), 19 deletions(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index decd19ef..3c32d682 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,19 @@ 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; > > - > > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > > - unsigned int id = ctrl.first; > > - ControlValue &value = ctrl.second; > > + for (unsigned int i = 0; i < count; i++) { > > + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > + const unsigned int id = v4l2Ctrl.id; > > + if (!ctrls->contains(id)) { > > + LOG(V4L2, Error) << "ControlList doesn't contain id: " > > + << id; > > + return; > > + } > > Can this happen ? > Nope, I will delete it in the next patch series. > > > > - const auto iter = controls_.find(id); > > - switch (iter->first->type()) { > > + ControlValue value = ctrls->get(id); > > This should be &value (or better *value, if you want to modify it) ? see [1] > > > + switch (value.type()) { > > The type information recorded in controls_ (which is a > ControlInfoMap) and in the ControlValue should be the same, hence > there should be no functional changes, right ? > Yes, I understand so. > > case ControlTypeInteger64: > > - value.set<int64_t>(v4l2Ctrl->value64); > > + value.set<int64_t>(v4l2Ctrl.value64); > > break; > > > > case ControlTypeByte: > > @@ -552,11 +546,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); > > [1] so you can avoid this ? > > I still think we pay a little performance loss, for the additional > crls->get() but the code is nicer to ead indeed > Surprisingly, ControlList doesn't allow getting a mutable reference while they allow a mutable iterator through begin()-end(). I would add ControlList::get(id) that returns a mutable reference or pointer, if we all agree. > Can you specify you've run all tests and they're all good in the cover > letter of the next iteration for out peace of mind ? :) > Sure. Now I am working for a way of running the test on test chromebook device. -Hiro > Thanks > j > > } > > } > > > > -- > > 2.31.1.368.gbe11c130af-goog > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Hiro, Thank you for the patch. On Thu, Apr 22, 2021 at 04:40:44PM +0900, Hirokazu Honda wrote: > On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi wrote: > > On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote: > > > The original updateControls() has the assumption that ctrls and > > > v4l2Ctrls lists in the same order. It is dependent on the caller > > > > s/in the same order/are in the same order/ > > > > > 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 | 32 +++++++++++++------------------- > > > 1 file changed, 13 insertions(+), 19 deletions(-) > > > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > > index decd19ef..3c32d682 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. > > > - */ Now that this limitation is removed, should we also combine the generation of ctrls and v4l2Ctrls in the same loop ? > > > for (uint32_t id : ids) { > > > const auto iter = controls_.find(id); > > > if (iter == controls_.end()) { > > > @@ -525,19 +519,19 @@ 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; > > > - > > > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > > > - unsigned int id = ctrl.first; > > > - ControlValue &value = ctrl.second; > > > + for (unsigned int i = 0; i < count; i++) { > > > + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; > > > + const unsigned int id = v4l2Ctrl.id; > > > + if (!ctrls->contains(id)) { > > > + LOG(V4L2, Error) << "ControlList doesn't contain id: " > > > + << id; > > > + return; > > > + } > > > > Can this happen ? > > Nope, I will delete it in the next patch series. > > > > > > > - const auto iter = controls_.find(id); > > > - switch (iter->first->type()) { > > > + ControlValue value = ctrls->get(id); > > > > This should be &value (or better *value, if you want to modify it) ? see [1] > > > > > + switch (value.type()) { > > > > The type information recorded in controls_ (which is a > > ControlInfoMap) and in the ControlValue should be the same, hence > > there should be no functional changes, right ? > > Yes, I understand so. > > > > case ControlTypeInteger64: > > > - value.set<int64_t>(v4l2Ctrl->value64); > > > + value.set<int64_t>(v4l2Ctrl.value64); > > > break; > > > > > > case ControlTypeByte: > > > @@ -552,11 +546,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); > > > > [1] so you can avoid this ? > > > > I still think we pay a little performance loss, for the additional > > crls->get() but the code is nicer to ead indeed > > Surprisingly, ControlList doesn't allow getting a mutable reference > while they allow a mutable iterator through begin()-end(). > I would add ControlList::get(id) that returns a mutable reference or > pointer, if we all agree. Good question. I think the reason why there's no mutable accessor (a non-const version of ControlList::get()) was that it was envisioned that setting a control value would perform sanity checks (for instance checking the value against the minimum/maximum). Accessing the underlying storage directly wouldn't allow us to do so. This is not implemented, and the iterators allow us to bypass that, so we could as well be consistent. I'm just a bit worried that making use of mutable access through the code base will make it more difficult to add checks later. Given that the code in this function only deals with single u32 and u64 values, the impact of a copy is probably negligible. > > Can you specify you've run all tests and they're all good in the cover > > letter of the next iteration for out peace of mind ? :) > > Sure. Now I am working for a way of running the test on test chromebook device. > > > > } > > > }
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index decd19ef..3c32d682 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,19 @@ 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; - - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; - unsigned int id = ctrl.first; - ControlValue &value = ctrl.second; + for (unsigned int i = 0; i < count; i++) { + const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i]; + const unsigned int id = v4l2Ctrl.id; + if (!ctrls->contains(id)) { + LOG(V4L2, Error) << "ControlList doesn't contain id: " + << id; + return; + } - 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 +546,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 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 | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-)