[libcamera-devel] [PATCH] libcamera: V4L2Device: Fix a type mismatch of ControlValue
Hirokazu Honda
hiroh at chromium.org
Wed May 26 08:33:52 CEST 2021
Hi Jacopo, thanks for reviewing,
On Tue, May 25, 2021 at 7:35 PM Jacopo Mondi <jacopo at 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 at 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
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210526/441ce1df/attachment-0001.htm>
More information about the libcamera-devel
mailing list