[libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove the controls order assumption in updateControls()

[Hirokazu Honda]

On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi <jacopo at 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 at 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 at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel