| Message ID | 20210413061925.3054927-3-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, On 13/04/2021 07:19, Hirokazu Honda wrote: > V4L2Device::updateControls() takes two arguments, raw array and > its size, for the v4l2_ext_control values. This replaces it with > std::vector. This patch does more than just replace the raw array for the vector like-for-like. It also changes the processing of how the controls are searched and iterated. That should be documented here, if it's still appropriate - though to ease things - it might be clearer/easier to have the change to a vector directly, and then fix up the parsing if that's a specific improvement. > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > include/libcamera/internal/v4l2_device.h | 3 +- > src/libcamera/v4l2_device.cpp | 36 +++++++++++++----------- > 2 files changed, 20 insertions(+), 19 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > index d006bf68..4cce3840 100644 > --- a/include/libcamera/internal/v4l2_device.h > +++ b/include/libcamera/internal/v4l2_device.h > @@ -55,8 +55,7 @@ protected: > private: > void listControls(); > void updateControls(ControlList *ctrls, > - const struct v4l2_ext_control *v4l2Ctrls, > - unsigned int count); > + const std::vector<v4l2_ext_control> &v4l2Ctrls); > > void eventAvailable(EventNotifier *notifier); > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 8625dde8..8f29cd7f 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -251,7 +251,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > v4l2Ctrls.resize(errorIdx); > } > > - updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > + updateControls(&ctrls, v4l2Ctrls); > > return ctrls; > } > @@ -353,7 +353,7 @@ int V4L2Device::setControls(ControlList *ctrls) > ret = errorIdx; > } > > - updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > + updateControls(ctrls, v4l2Ctrls); > > return ret; > } > @@ -517,25 +517,27 @@ void V4L2Device::listControls() > * values in \a v4l2Ctrls > * \param[inout] ctrls List of V4L2 controls to update > * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver > - * \param[in] count The number of controls to update > */ > void V4L2Device::updateControls(ControlList *ctrls, > - const struct v4l2_ext_control *v4l2Ctrls, > - unsigned int count) > + const std::vector<v4l2_ext_control> &v4l2Ctrls) > { > - unsigned int i = 0; > - for (auto &ctrl : *ctrls) { > - if (i == count) > - break; > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > + if (!ctrls->contains(v4l2Ctrl.id)) { > + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; > + continue; > + } Hrm, are these extra validation checks? or is it because we've inverted the search now? > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > - unsigned int id = ctrl.first; > - ControlValue &value = ctrl.second; > + const auto it = controls_.find(v4l2Ctrl.id); > + if (it == controls_.end()) { > + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; > + continue; > + } > > - const auto iter = controls_.find(id); > - switch (iter->first->type()) { > + const ControlValue &value = ctrls->get(v4l2Ctrl.id); > + ControlValue newValue = value; > + switch (it->first->type()) { > case ControlTypeInteger64: > - value.set<int64_t>(v4l2Ctrl->value64); > + newValue.set<int64_t>(v4l2Ctrl.value64); > break; > > case ControlTypeByte: > @@ -550,11 +552,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > * \todo To be changed when support for string controls > * will be added. > */ > - value.set<int32_t>(v4l2Ctrl->value); > + newValue.set<int32_t>(v4l2Ctrl.value); > break; > } > > - i++; > + ctrls->set(v4l2Ctrl.id, newValue); It's quite hard to distinguish what's going on in there. Does this do more than just use a vector instead of an array ? For instance, was ctrls.set() called previously in some other way which isn't jumping out at me in the diff above? Ah, I suspect previously, the 'value' was the ControlValue and it was already obtained because the search was through the ctrls list... > } > } > >
Hi Kieran, On Wed, Apr 14, 2021 at 7:32 AM Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > Hi Hiro, > > On 13/04/2021 07:19, Hirokazu Honda wrote: > > V4L2Device::updateControls() takes two arguments, raw array and > > its size, for the v4l2_ext_control values. This replaces it with > > std::vector. > > This patch does more than just replace the raw array for the vector > like-for-like. > > It also changes the processing of how the controls are searched and > iterated. > > That should be documented here, if it's still appropriate - though to > ease things - it might be clearer/easier to have the change to a vector > directly, and then fix up the parsing if that's a specific improvement. > > > > > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > include/libcamera/internal/v4l2_device.h | 3 +- > > src/libcamera/v4l2_device.cpp | 36 +++++++++++++----------- > > 2 files changed, 20 insertions(+), 19 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h > > index d006bf68..4cce3840 100644 > > --- a/include/libcamera/internal/v4l2_device.h > > +++ b/include/libcamera/internal/v4l2_device.h > > @@ -55,8 +55,7 @@ protected: > > private: > > void listControls(); > > void updateControls(ControlList *ctrls, > > - const struct v4l2_ext_control *v4l2Ctrls, > > - unsigned int count); > > + const std::vector<v4l2_ext_control> &v4l2Ctrls); > > > > void eventAvailable(EventNotifier *notifier); > > > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > > index 8625dde8..8f29cd7f 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -251,7 +251,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) > > v4l2Ctrls.resize(errorIdx); > > } > > > > - updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > + updateControls(&ctrls, v4l2Ctrls); > > > > return ctrls; > > } > > @@ -353,7 +353,7 @@ int V4L2Device::setControls(ControlList *ctrls) > > ret = errorIdx; > > } > > > > - updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); > > + updateControls(ctrls, v4l2Ctrls); > > > > return ret; > > } > > @@ -517,25 +517,27 @@ void V4L2Device::listControls() > > * values in \a v4l2Ctrls > > * \param[inout] ctrls List of V4L2 controls to update > > * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver > > - * \param[in] count The number of controls to update > > */ > > void V4L2Device::updateControls(ControlList *ctrls, > > - const struct v4l2_ext_control *v4l2Ctrls, > > - unsigned int count) > > + const std::vector<v4l2_ext_control> &v4l2Ctrls) > > { > > - unsigned int i = 0; > > - for (auto &ctrl : *ctrls) { > > - if (i == count) > > - break; > > + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { > > + if (!ctrls->contains(v4l2Ctrl.id)) { > > + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; > > + continue; > > + } > > Hrm, are these extra validation checks? > or is it because we've inverted the search now? > > > - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; > > - unsigned int id = ctrl.first; > > - ControlValue &value = ctrl.second; > > + const auto it = controls_.find(v4l2Ctrl.id); > > + if (it == controls_.end()) { > > + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; > > + continue; > > + } > > > > - const auto iter = controls_.find(id); > > - switch (iter->first->type()) { > > + const ControlValue &value = ctrls->get(v4l2Ctrl.id); > > + ControlValue newValue = value; > > + switch (it->first->type()) { > > case ControlTypeInteger64: > > - value.set<int64_t>(v4l2Ctrl->value64); > > + newValue.set<int64_t>(v4l2Ctrl.value64); > > break; > > > > case ControlTypeByte: > > @@ -550,11 +552,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > > * \todo To be changed when support for string controls > > * will be added. > > */ > > - value.set<int32_t>(v4l2Ctrl->value); > > + newValue.set<int32_t>(v4l2Ctrl.value); > > break; > > } > > > > - i++; > > + ctrls->set(v4l2Ctrl.id, newValue); > > It's quite hard to distinguish what's going on in there. > Does this do more than just use a vector instead of an array ? > > For instance, was ctrls.set() called previously in some other way which > isn't jumping out at me in the diff above? > > Ah, I suspect previously, the 'value' was the ControlValue and it was > already obtained because the search was through the ctrls list... > The problem is ControlList() somehow allows to return a reference or pointer via begin-end, but somehow doesn't via find or get. It is very strange that I can only change it by get & set if without begin-end. I separate the patch for the change except vector-array change. PTAL https://patchwork.libcamera.org/patch/11934/ Best Regards, -Hiro > > > > } > > } > > > > > > -- > Regards > -- > Kieran
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h index d006bf68..4cce3840 100644 --- a/include/libcamera/internal/v4l2_device.h +++ b/include/libcamera/internal/v4l2_device.h @@ -55,8 +55,7 @@ protected: private: void listControls(); void updateControls(ControlList *ctrls, - const struct v4l2_ext_control *v4l2Ctrls, - unsigned int count); + const std::vector<v4l2_ext_control> &v4l2Ctrls); void eventAvailable(EventNotifier *notifier); diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 8625dde8..8f29cd7f 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -251,7 +251,7 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids) v4l2Ctrls.resize(errorIdx); } - updateControls(&ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); + updateControls(&ctrls, v4l2Ctrls); return ctrls; } @@ -353,7 +353,7 @@ int V4L2Device::setControls(ControlList *ctrls) ret = errorIdx; } - updateControls(ctrls, v4l2Ctrls.data(), v4l2Ctrls.size()); + updateControls(ctrls, v4l2Ctrls); return ret; } @@ -517,25 +517,27 @@ void V4L2Device::listControls() * values in \a v4l2Ctrls * \param[inout] ctrls List of V4L2 controls to update * \param[in] v4l2Ctrls List of V4L2 extended controls as returned by the driver - * \param[in] count The number of controls to update */ void V4L2Device::updateControls(ControlList *ctrls, - const struct v4l2_ext_control *v4l2Ctrls, - unsigned int count) + const std::vector<v4l2_ext_control> &v4l2Ctrls) { - unsigned int i = 0; - for (auto &ctrl : *ctrls) { - if (i == count) - break; + for (const v4l2_ext_control &v4l2Ctrl : v4l2Ctrls) { + if (!ctrls->contains(v4l2Ctrl.id)) { + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; + continue; + } - const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i]; - unsigned int id = ctrl.first; - ControlValue &value = ctrl.second; + const auto it = controls_.find(v4l2Ctrl.id); + if (it == controls_.end()) { + LOG(V4L2, Error) << "Unknown id: " << v4l2Ctrl.id; + continue; + } - const auto iter = controls_.find(id); - switch (iter->first->type()) { + const ControlValue &value = ctrls->get(v4l2Ctrl.id); + ControlValue newValue = value; + switch (it->first->type()) { case ControlTypeInteger64: - value.set<int64_t>(v4l2Ctrl->value64); + newValue.set<int64_t>(v4l2Ctrl.value64); break; case ControlTypeByte: @@ -550,11 +552,11 @@ void V4L2Device::updateControls(ControlList *ctrls, * \todo To be changed when support for string controls * will be added. */ - value.set<int32_t>(v4l2Ctrl->value); + newValue.set<int32_t>(v4l2Ctrl.value); break; } - i++; + ctrls->set(v4l2Ctrl.id, newValue); } }
V4L2Device::updateControls() takes two arguments, raw array and its size, for the v4l2_ext_control values. This replaces it with std::vector. Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- include/libcamera/internal/v4l2_device.h | 3 +- src/libcamera/v4l2_device.cpp | 36 +++++++++++++----------- 2 files changed, 20 insertions(+), 19 deletions(-)