[{"id":16499,"web_url":"https://patchwork.libcamera.org/comment/16499/","msgid":"<20210422072953.t24gwxcswoj27c4u@uno.localdomain>","date":"2021-04-22T07:29:53","subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Hiro,\n\nOn Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote:\n> The original updateControls() has the assumption that ctrls and\n> v4l2Ctrls lists in the same order. It is dependent on the caller\n\ns/in the same order/are in the same order/\n\n> implementation though. This changes updateControls()\n> implementation so that it works without the assumption.\n>\n> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> ---\n>  src/libcamera/v4l2_device.cpp | 32 +++++++++++++-------------------\n>  1 file changed, 13 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index decd19ef..3c32d682 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n>\n>  \tControlList ctrls{ controls_ };\n>\n> -\t/*\n> -\t * Start by filling the ControlList. This can't be combined with filling\n> -\t * v4l2Ctrls, as updateControls() relies on both containers having the\n> -\t * same order, and the control list is based on a map, which is not\n> -\t * sorted by insertion order.\n> -\t */\n>  \tfor (uint32_t id : ids) {\n>  \t\tconst auto iter = controls_.find(id);\n>  \t\tif (iter == controls_.end()) {\n> @@ -525,19 +519,19 @@ void V4L2Device::updateControls(ControlList *ctrls,\n>  \t\t\t\tconst struct v4l2_ext_control *v4l2Ctrls,\n>  \t\t\t\tunsigned int count)\n>  {\n> -\tunsigned int i = 0;\n> -\tfor (auto &ctrl : *ctrls) {\n> -\t\tif (i == count)\n> -\t\t\tbreak;\n> -\n> -\t\tconst struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> -\t\tunsigned int id = ctrl.first;\n> -\t\tControlValue &value = ctrl.second;\n> +\tfor (unsigned int i = 0; i < count; i++) {\n> +\t\tconst struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> +\t\tconst unsigned int id = v4l2Ctrl.id;\n> +\t\tif (!ctrls->contains(id)) {\n> +\t\t\tLOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n> +\t\t\t\t\t << id;\n> +\t\t\treturn;\n> +\t\t}\n\nCan this happen ?\n\n>\n> -\t\tconst auto iter = controls_.find(id);\n> -\t\tswitch (iter->first->type()) {\n> +\t\tControlValue value = ctrls->get(id);\n\nThis should be &value (or better *value, if you want to modify it) ? see [1]\n\n> +\t\tswitch (value.type()) {\n\nThe type information recorded in controls_ (which is a\nControlInfoMap) and in the ControlValue should be the same, hence\nthere should be no functional changes, right ?\n\n>  \t\tcase ControlTypeInteger64:\n> -\t\t\tvalue.set<int64_t>(v4l2Ctrl->value64);\n> +\t\t\tvalue.set<int64_t>(v4l2Ctrl.value64);\n>  \t\t\tbreak;\n>\n>  \t\tcase ControlTypeByte:\n> @@ -552,11 +546,11 @@ void V4L2Device::updateControls(ControlList *ctrls,\n>  \t\t\t * \\todo To be changed when support for string controls\n>  \t\t\t * will be added.\n>  \t\t\t */\n> -\t\t\tvalue.set<int32_t>(v4l2Ctrl->value);\n> +\t\t\tvalue.set<int32_t>(v4l2Ctrl.value);\n>  \t\t\tbreak;\n>  \t\t}\n>\n> -\t\ti++;\n> +\t\tctrls->set(id, value);\n\n[1] so you can avoid this ?\n\nI still think we pay a little performance loss, for the additional\ncrls->get() but the code is nicer to ead indeed\n\nCan you specify you've run all tests and they're all good in the cover\nletter of the next iteration for out peace of mind ? :)\n\nThanks\n   j\n>  \t}\n>  }\n>\n> --\n> 2.31.1.368.gbe11c130af-goog\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BD6A5BDB15\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 22 Apr 2021 07:29:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8DA6468848;\n\tThu, 22 Apr 2021 09:29:14 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[217.70.183.199])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3872A68847\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 09:29:13 +0200 (CEST)","from uno.localdomain (host-82-57-193-192.retail.telecomitalia.it\n\t[82.57.193.192]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay9-d.mail.gandi.net (Postfix) with ESMTPSA id 77F9BFF804;\n\tThu, 22 Apr 2021 07:29:12 +0000 (UTC)"],"X-Originating-IP":"82.57.193.192","Date":"Thu, 22 Apr 2021 09:29:53 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<20210422072953.t24gwxcswoj27c4u@uno.localdomain>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-2-hiroh@chromium.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210422021809.520675-2-hiroh@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16501,"web_url":"https://patchwork.libcamera.org/comment/16501/","msgid":"<CAO5uPHMCmec-GWLdjFKdv4ZuYA9KKTQK-EF4vg1UKswX+PJU+w@mail.gmail.com>","date":"2021-04-22T07:40:44","subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi <jacopo@jmondi.org> wrote:\n>\n> Hi Hiro,\n>\n> On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote:\n> > The original updateControls() has the assumption that ctrls and\n> > v4l2Ctrls lists in the same order. It is dependent on the caller\n>\n> s/in the same order/are in the same order/\n>\n> > implementation though. This changes updateControls()\n> > implementation so that it works without the assumption.\n> >\n> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > ---\n> >  src/libcamera/v4l2_device.cpp | 32 +++++++++++++-------------------\n> >  1 file changed, 13 insertions(+), 19 deletions(-)\n> >\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index decd19ef..3c32d682 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> >\n> >       ControlList ctrls{ controls_ };\n> >\n> > -     /*\n> > -      * Start by filling the ControlList. This can't be combined with filling\n> > -      * v4l2Ctrls, as updateControls() relies on both containers having the\n> > -      * same order, and the control list is based on a map, which is not\n> > -      * sorted by insertion order.\n> > -      */\n> >       for (uint32_t id : ids) {\n> >               const auto iter = controls_.find(id);\n> >               if (iter == controls_.end()) {\n> > @@ -525,19 +519,19 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> >                               const struct v4l2_ext_control *v4l2Ctrls,\n> >                               unsigned int count)\n> >  {\n> > -     unsigned int i = 0;\n> > -     for (auto &ctrl : *ctrls) {\n> > -             if (i == count)\n> > -                     break;\n> > -\n> > -             const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > -             unsigned int id = ctrl.first;\n> > -             ControlValue &value = ctrl.second;\n> > +     for (unsigned int i = 0; i < count; i++) {\n> > +             const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > +             const unsigned int id = v4l2Ctrl.id;\n> > +             if (!ctrls->contains(id)) {\n> > +                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n> > +                                      << id;\n> > +                     return;\n> > +             }\n>\n> Can this happen ?\n>\n\nNope, I will delete it in the next patch series.\n\n> >\n> > -             const auto iter = controls_.find(id);\n> > -             switch (iter->first->type()) {\n> > +             ControlValue value = ctrls->get(id);\n>\n> This should be &value (or better *value, if you want to modify it) ? see [1]\n>\n> > +             switch (value.type()) {\n>\n> The type information recorded in controls_ (which is a\n> ControlInfoMap) and in the ControlValue should be the same, hence\n> there should be no functional changes, right ?\n>\n\nYes, I understand so.\n\n> >               case ControlTypeInteger64:\n> > -                     value.set<int64_t>(v4l2Ctrl->value64);\n> > +                     value.set<int64_t>(v4l2Ctrl.value64);\n> >                       break;\n> >\n> >               case ControlTypeByte:\n> > @@ -552,11 +546,11 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> >                        * \\todo To be changed when support for string controls\n> >                        * will be added.\n> >                        */\n> > -                     value.set<int32_t>(v4l2Ctrl->value);\n> > +                     value.set<int32_t>(v4l2Ctrl.value);\n> >                       break;\n> >               }\n> >\n> > -             i++;\n> > +             ctrls->set(id, value);\n>\n> [1] so you can avoid this ?\n>\n> I still think we pay a little performance loss, for the additional\n> crls->get() but the code is nicer to ead indeed\n>\n\nSurprisingly, ControlList doesn't allow getting a mutable reference\nwhile they allow a mutable iterator through begin()-end().\nI would add ControlList::get(id) that returns a mutable reference or\npointer, if we all agree.\n\n\n> Can you specify you've run all tests and they're all good in the cover\n> letter of the next iteration for out peace of mind ? :)\n>\n\nSure. Now I am working for a way of running the test on test chromebook device.\n\n-Hiro\n> Thanks\n>    j\n> >       }\n> >  }\n> >\n> > --\n> > 2.31.1.368.gbe11c130af-goog\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 92453BDB17\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 22 Apr 2021 07:40:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19C4D68850;\n\tThu, 22 Apr 2021 09:40:56 +0200 (CEST)","from mail-ed1-x532.google.com (mail-ed1-x532.google.com\n\t[IPv6:2a00:1450:4864:20::532])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 47ACF68847\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 09:40:55 +0200 (CEST)","by mail-ed1-x532.google.com with SMTP id bx20so51106597edb.12\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 22 Apr 2021 00:40:55 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"TeuJqiRQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=iZD5vpF5yuTUVFodajRtZ5Hnj87B93Ow9v/75egxmbY=;\n\tb=TeuJqiRQqCLBUzanmdsfLm1CeKq3Z2Tug6iOG7bkUTCSHNO9PC48c0+uP+sGQgxb5B\n\tqFw40GJwfttkt3FSEMLDUVs7OCfE8KH0toWado/HMJgtkoM33fipW7SMHhxbNVux4Fre\n\tL1GnTZ8RdE9BvreJT/lXTPHNQzbRmLHRnT32c=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=iZD5vpF5yuTUVFodajRtZ5Hnj87B93Ow9v/75egxmbY=;\n\tb=M4nKaBL0cimEbEwH4jscO0BPuElaYI+9PF54wSkC1NAWtivLX+/UU3kezgnRWvC9fw\n\typJbVktLNzydb/+wMPMFV+onDYq+ek2cOabqWJZSL/buzQnUonO7y0PMiP8Ab55IbI5U\n\tdXzj1Ir+rTEhSJcwenExmMFcB+P3E/Ut+3urJZIy827sIDZXONm97b9U+bBvfScT4+Pi\n\tiM0eWNrRoiVQiy4f8PQ5RFuzjBrg50N555ei7neCQ3Eg+/aBHOofrq0QMluPM0M4GhOV\n\tio0yUs9xQId4gzKz9JMW97A4WLyBh1U7pegjKHtvigjKgBGfS/Fw2JnDq1oUJ8Eqdrw5\n\ttwLA==","X-Gm-Message-State":"AOAM531Q1f97ZZ8q5MSO6GYLVNHvvy6a5/pTwQRyjwUx7aWM4VWbAT72\n\tx0mSbGj2LNDcbbfP/lWPLu4MEUkzysKNXumoFj8zbw==","X-Google-Smtp-Source":"ABdhPJxVUaYC+dOGSJFw7f3KqKkhpGqeLW5h1DkM1vPZyRP3sB85KWflOCtlSQj3g4YO/T/oLjR99MUQsDuSDdeUCF0=","X-Received":"by 2002:a05:6402:488:: with SMTP id\n\tk8mr2073984edv.233.1619077254960; \n\tThu, 22 Apr 2021 00:40:54 -0700 (PDT)","MIME-Version":"1.0","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-2-hiroh@chromium.org>\n\t<20210422072953.t24gwxcswoj27c4u@uno.localdomain>","In-Reply-To":"<20210422072953.t24gwxcswoj27c4u@uno.localdomain>","From":"Hirokazu Honda <hiroh@chromium.org>","Date":"Thu, 22 Apr 2021 16:40:44 +0900","Message-ID":"<CAO5uPHMCmec-GWLdjFKdv4ZuYA9KKTQK-EF4vg1UKswX+PJU+w@mail.gmail.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":16520,"web_url":"https://patchwork.libcamera.org/comment/16520/","msgid":"<YII2th2A4sqWknFH@pendragon.ideasonboard.com>","date":"2021-04-23T02:53:42","subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nThank you for the patch.\n\nOn Thu, Apr 22, 2021 at 04:40:44PM +0900, Hirokazu Honda wrote:\n> On Thu, Apr 22, 2021 at 4:29 PM Jacopo Mondi wrote:\n> > On Thu, Apr 22, 2021 at 11:18:06AM +0900, Hirokazu Honda wrote:\n> > > The original updateControls() has the assumption that ctrls and\n> > > v4l2Ctrls lists in the same order. It is dependent on the caller\n> >\n> > s/in the same order/are in the same order/\n> >\n> > > implementation though. This changes updateControls()\n> > > implementation so that it works without the assumption.\n> > >\n> > > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>\n> > > ---\n> > >  src/libcamera/v4l2_device.cpp | 32 +++++++++++++-------------------\n> > >  1 file changed, 13 insertions(+), 19 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index decd19ef..3c32d682 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -179,12 +179,6 @@ ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> > >\n> > >       ControlList ctrls{ controls_ };\n> > >\n> > > -     /*\n> > > -      * Start by filling the ControlList. This can't be combined with filling\n> > > -      * v4l2Ctrls, as updateControls() relies on both containers having the\n> > > -      * same order, and the control list is based on a map, which is not\n> > > -      * sorted by insertion order.\n> > > -      */\n\nNow that this limitation is removed, should we also combine the\ngeneration of ctrls and v4l2Ctrls in the same loop ?\n\n> > >       for (uint32_t id : ids) {\n> > >               const auto iter = controls_.find(id);\n> > >               if (iter == controls_.end()) {\n> > > @@ -525,19 +519,19 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> > >                               const struct v4l2_ext_control *v4l2Ctrls,\n> > >                               unsigned int count)\n> > >  {\n> > > -     unsigned int i = 0;\n> > > -     for (auto &ctrl : *ctrls) {\n> > > -             if (i == count)\n> > > -                     break;\n> > > -\n> > > -             const struct v4l2_ext_control *v4l2Ctrl = &v4l2Ctrls[i];\n> > > -             unsigned int id = ctrl.first;\n> > > -             ControlValue &value = ctrl.second;\n> > > +     for (unsigned int i = 0; i < count; i++) {\n> > > +             const struct v4l2_ext_control &v4l2Ctrl = v4l2Ctrls[i];\n> > > +             const unsigned int id = v4l2Ctrl.id;\n> > > +             if (!ctrls->contains(id)) {\n> > > +                     LOG(V4L2, Error) << \"ControlList doesn't contain id: \"\n> > > +                                      << id;\n> > > +                     return;\n> > > +             }\n> >\n> > Can this happen ?\n> \n> Nope, I will delete it in the next patch series.\n> \n> > >\n> > > -             const auto iter = controls_.find(id);\n> > > -             switch (iter->first->type()) {\n> > > +             ControlValue value = ctrls->get(id);\n> >\n> > This should be &value (or better *value, if you want to modify it) ? see [1]\n> >\n> > > +             switch (value.type()) {\n> >\n> > The type information recorded in controls_ (which is a\n> > ControlInfoMap) and in the ControlValue should be the same, hence\n> > there should be no functional changes, right ?\n> \n> Yes, I understand so.\n> \n> > >               case ControlTypeInteger64:\n> > > -                     value.set<int64_t>(v4l2Ctrl->value64);\n> > > +                     value.set<int64_t>(v4l2Ctrl.value64);\n> > >                       break;\n> > >\n> > >               case ControlTypeByte:\n> > > @@ -552,11 +546,11 @@ void V4L2Device::updateControls(ControlList *ctrls,\n> > >                        * \\todo To be changed when support for string controls\n> > >                        * will be added.\n> > >                        */\n> > > -                     value.set<int32_t>(v4l2Ctrl->value);\n> > > +                     value.set<int32_t>(v4l2Ctrl.value);\n> > >                       break;\n> > >               }\n> > >\n> > > -             i++;\n> > > +             ctrls->set(id, value);\n> >\n> > [1] so you can avoid this ?\n> >\n> > I still think we pay a little performance loss, for the additional\n> > crls->get() but the code is nicer to ead indeed\n> \n> Surprisingly, ControlList doesn't allow getting a mutable reference\n> while they allow a mutable iterator through begin()-end().\n> I would add ControlList::get(id) that returns a mutable reference or\n> pointer, if we all agree.\n\nGood question. I think the reason why there's no mutable accessor (a\nnon-const version of ControlList::get()) was that it was envisioned that\nsetting a control value would perform sanity checks (for instance\nchecking the value against the minimum/maximum). Accessing the\nunderlying storage directly wouldn't allow us to do so. This is not\nimplemented, and the iterators allow us to bypass that, so we could as\nwell be consistent. I'm just a bit worried that making use of mutable\naccess through the code base will make it more difficult to add checks\nlater. Given that the code in this function only deals with single u32\nand u64 values, the impact of a copy is probably negligible.\n\n> > Can you specify you've run all tests and they're all good in the cover\n> > letter of the next iteration for out peace of mind ? :)\n> \n> Sure. Now I am working for a way of running the test on test chromebook device.\n> \n> > >       }\n> > >  }","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E7A16BDB1A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tFri, 23 Apr 2021 02:53:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A51168872;\n\tFri, 23 Apr 2021 04:53:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6266860513\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri, 23 Apr 2021 04:53:47 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C1A39EE;\n\tFri, 23 Apr 2021 04:53:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Z+QzvXCy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1619146426;\n\tbh=+fmr6BzUeOkfnYK5gNLlszg5r1KDTpC2C5z3ZfLFOPM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Z+QzvXCy/KPbjWJ0Ds55m6cAZOZn3KKZy1qd/3oFpdbhOuzPuAGEIv18hVfdsHjdb\n\t3r/RHzLLjYDHJaz2ZLCNxgOnaYNcRnzChkQOdoL4nT2tkQwo3MxlR7/jMT6ddetvxP\n\tuWiJT3KwEBsSoEo/BPtD6jGCy2DeazddinxY779c=","Date":"Fri, 23 Apr 2021 05:53:42 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Hirokazu Honda <hiroh@chromium.org>","Message-ID":"<YII2th2A4sqWknFH@pendragon.ideasonboard.com>","References":"<20210422021809.520675-1-hiroh@chromium.org>\n\t<20210422021809.520675-2-hiroh@chromium.org>\n\t<20210422072953.t24gwxcswoj27c4u@uno.localdomain>\n\t<CAO5uPHMCmec-GWLdjFKdv4ZuYA9KKTQK-EF4vg1UKswX+PJU+w@mail.gmail.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<CAO5uPHMCmec-GWLdjFKdv4ZuYA9KKTQK-EF4vg1UKswX+PJU+w@mail.gmail.com>","Subject":"Re: [libcamera-devel] [PATCH v4 1/4] libcamera: V4L2Device: Remove\n\tthe controls order assumption in updateControls()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Cc":"libcamera devel <libcamera-devel@lists.libcamera.org>","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]