| Message ID | 20210525053341.2574042-1-hiroh@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Hiro, Thank you for the patch. On Tue, May 25, 2021 at 02:33:41PM +0900, Hirokazu Honda wrote: > The commit (34bee5e8) of removing the controls order assumption > doesn't change the update ControlValue type in > V4L2Device::updateControls(). However, this is wrong because the > ControlValue is set to a placeholder ControlValue in > V4L2Device::getControls(), so the type is ControlTypeNone. > We should rather look up the type in V4L2Device::controls_ with a > specified id. As we've merged a revert to quickly fix the master branch, could you repost this squashed with the original patch ? > Fixes: 34bee5e8 (libcamera: V4L2Device: Remove the controls order assumption in updateControls()) > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/v4l2_device.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 7f7e5b8f..cc4f52bf 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -621,7 +621,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > const unsigned int id = v4l2Ctrl.id; > > ControlValue value = ctrls->get(id); > - switch (value.type()) { > + > + auto iter = controls_.find(id); Maybe const auto ? > + ASSERT(iter != controls_.end()); > + const ControlType type = iter->first->type(); > + switch (type) { > case ControlTypeInteger64: > value.set<int64_t>(v4l2Ctrl.value64); > break;
Hi Hiro, On Tue, May 25, 2021 at 02:33:41PM +0900, Hirokazu Honda wrote: > The commit (34bee5e8) of removing the controls order assumption We usually spell the commit message out as: Commit 34bee5e84ecb ("libcamera: V4L2Device: Remove the controls order assumption in updateControls()") removed the control odering assumption without changing the updateControls() function accordingly. > doesn't change the update ControlValue type in > V4L2Device::updateControls(). However, this is wrong because the > ControlValue is set to a placeholder ControlValue in > V4L2Device::getControls(), so the type is ControlTypeNone. > We should rather look up the type in V4L2Device::controls_ with a > specified id. > > Fixes: 34bee5e8 (libcamera: V4L2Device: Remove the controls order assumption in updateControls()) > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > --- > src/libcamera/v4l2_device.cpp | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp > index 7f7e5b8f..cc4f52bf 100644 > --- a/src/libcamera/v4l2_device.cpp > +++ b/src/libcamera/v4l2_device.cpp > @@ -621,7 +621,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > const unsigned int id = v4l2Ctrl.id; > > ControlValue value = ctrls->get(id); I've just noticed we do ControlValue value = ctrls.get() switch() { value.set(); } ctrls.set(id, value); Can't we just take a reference and modify value ? (not related to this patch though). > - switch (value.type()) { > + > + auto iter = controls_.find(id); const auto & ? > + ASSERT(iter != controls_.end()); > + const ControlType type = iter->first->type(); > + switch (type) { You could save the local variable if you want to > case ControlTypeInteger64: > value.set<int64_t>(v4l2Ctrl.value64); > break; > -- > 2.31.1.818.g46aad6cb9e-goog >
Hi Laurent, On Tue, May 25, 2021 at 6:50 PM Laurent Pinchart < laurent.pinchart@ideasonboard.com> wrote: > Hi Hiro, > > Thank you for the patch. > > On Tue, May 25, 2021 at 02:33:41PM +0900, Hirokazu Honda wrote: > > The commit (34bee5e8) of removing the controls order assumption > > doesn't change the update ControlValue type in > > V4L2Device::updateControls(). However, this is wrong because the > > ControlValue is set to a placeholder ControlValue in > > V4L2Device::getControls(), so the type is ControlTypeNone. > > We should rather look up the type in V4L2Device::controls_ with a > > specified id. > > As we've merged a revert to quickly fix the master branch, could you > repost this squashed with the original patch ? > > Thanks. I will do and test with your submit patch. > > Fixes: 34bee5e8 (libcamera: V4L2Device: Remove the controls order > assumption in updateControls()) > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/v4l2_device.cpp | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > index 7f7e5b8f..cc4f52bf 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -621,7 +621,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > > const unsigned int id = v4l2Ctrl.id; > > > > ControlValue value = ctrls->get(id); > > - switch (value.type()) { > > + > > + auto iter = controls_.find(id); > > Maybe const auto ? > > > + ASSERT(iter != controls_.end()); > > + const ControlType type = iter->first->type(); > > + switch (type) { > > case ControlTypeInteger64: > > value.set<int64_t>(v4l2Ctrl.value64); > > break; > > -- > Regards, > > Laurent Pinchart >
Hi Jacopo, thanks for reviewing, On Tue, May 25, 2021 at 7:35 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hi Hiro, > > On Tue, May 25, 2021 at 02:33:41PM +0900, Hirokazu Honda wrote: > > The commit (34bee5e8) of removing the controls order assumption > > We usually spell the commit message out as: > > Commit 34bee5e84ecb ("libcamera: V4L2Device: Remove the controls order > assumption in updateControls()") removed the control odering > assumption without changing the updateControls() function accordingly. > > > doesn't change the update ControlValue type in > > V4L2Device::updateControls(). However, this is wrong because the > > ControlValue is set to a placeholder ControlValue in > > V4L2Device::getControls(), so the type is ControlTypeNone. > > We should rather look up the type in V4L2Device::controls_ with a > > specified id. > > > > > Fixes: 34bee5e8 (libcamera: V4L2Device: Remove the controls order > assumption in updateControls()) > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org> > > --- > > src/libcamera/v4l2_device.cpp | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/v4l2_device.cpp > b/src/libcamera/v4l2_device.cpp > > index 7f7e5b8f..cc4f52bf 100644 > > --- a/src/libcamera/v4l2_device.cpp > > +++ b/src/libcamera/v4l2_device.cpp > > @@ -621,7 +621,11 @@ void V4L2Device::updateControls(ControlList *ctrls, > > const unsigned int id = v4l2Ctrl.id; > > > > ControlValue value = ctrls->get(id); > > I've just noticed we do > > ControlValue value = ctrls.get() > switch() { > value.set(); > } > ctrls.set(id, value); > > Can't we just take a reference and modify value ? > (not related to this patch though). > > Yeah, that looks strange that we couldn't get a reference. I asked Laurent in the previous review (I couldn't dig up for a moment though). With this set/get, we enforce to validate the new value in set() by ControlList::find(). Thanks, -Hiro > > - switch (value.type()) { > > + > > + auto iter = controls_.find(id); > > const auto & ? > > > + ASSERT(iter != controls_.end()); > > + const ControlType type = iter->first->type(); > > + switch (type) { > > You could save the local variable if you want to > > > case ControlTypeInteger64: > > value.set<int64_t>(v4l2Ctrl.value64); > > break; > > -- > > 2.31.1.818.g46aad6cb9e-goog > > >
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp index 7f7e5b8f..cc4f52bf 100644 --- a/src/libcamera/v4l2_device.cpp +++ b/src/libcamera/v4l2_device.cpp @@ -621,7 +621,11 @@ void V4L2Device::updateControls(ControlList *ctrls, const unsigned int id = v4l2Ctrl.id; ControlValue value = ctrls->get(id); - switch (value.type()) { + + auto iter = controls_.find(id); + ASSERT(iter != controls_.end()); + const ControlType type = iter->first->type(); + switch (type) { case ControlTypeInteger64: value.set<int64_t>(v4l2Ctrl.value64); break;
The commit (34bee5e8) of removing the controls order assumption doesn't change the update ControlValue type in V4L2Device::updateControls(). However, this is wrong because the ControlValue is set to a placeholder ControlValue in V4L2Device::getControls(), so the type is ControlTypeNone. We should rather look up the type in V4L2Device::controls_ with a specified id. Fixes: 34bee5e8 (libcamera: V4L2Device: Remove the controls order assumption in updateControls()) Signed-off-by: Hirokazu Honda <hiroh@chromium.org> --- src/libcamera/v4l2_device.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)