[libcamera-devel] [PATCH] libcamera: V4L2Device: Fix a type mismatch of ControlValue

[Hirokazu Honda]

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>