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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Thu, Apr 22, 2021 at 04:40:44PM +0900, Hirokazu Honda wrote:
> On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi wrote:
> > 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.
> > > -      */

Now that this limitation is removed, should we also combine the
generation of ctrls and v4l2Ctrls in the same loop ?

> > >       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.

Good question. I think the reason why there's no mutable accessor (a
non-const version of ControlList::get()) was that it was envisioned that
setting a control value would perform sanity checks (for instance
checking the value against the minimum/maximum). Accessing the
underlying storage directly wouldn't allow us to do so. This is not
implemented, and the iterators allow us to bypass that, so we could as
well be consistent. I'm just a bit worried that making use of mutable
access through the code base will make it more difficult to add checks
later. Given that the code in this function only deals with single u32
and u64 values, the impact of a copy is probably negligible.

> > 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.
> 
> > >       }
> > >  }

-- 
Regards,

Laurent Pinchart