[{"id":4529,"web_url":"https://patchwork.libcamera.org/comment/4529/","msgid":"<20200426144358.qyakklzm2hrskxpi@uno.localdomain>","date":"2020-04-26T14:43:58","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify\n\tusage of getControls()","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Apr 26, 2020 at 02:16:23AM +0300, Laurent Pinchart wrote:\n> The V4L2Device::getControls() function takes a ControlList that needs to\n> be pre-populated with dummy entries for the controls that need to be\n> read. This is a cumbersome API, especially when reading a single\n> control. Make it nicer by passing the list of V4L2 controls as a vector\n> of control IDs, and returning a ControlList.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> ---\n>  src/libcamera/camera_sensor.cpp       | 23 ++++++---------\n>  src/libcamera/include/camera_sensor.h |  2 +-\n\nWhile I welcome this for the v4l2 device part, I would leave\nCameraSensor out, as before defining what kind of controls the class\nshould accepta (v4l2 control IDs or libcamera Control<> instances) we\nshould probably have a bit more use cases...\n\n>  src/libcamera/include/v4l2_device.h   |  2 +-\n>  src/libcamera/v4l2_device.cpp         | 42 ++++++++++++---------------\n>  test/v4l2_videodevice/controls.cpp    | 20 +++++++------\n>  5 files changed, 41 insertions(+), 48 deletions(-)\n>\n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index 2219a4307436..ce585cab1a4f 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -299,30 +299,25 @@ const ControlInfoMap &CameraSensor::controls() const\n>\n>  /**\n>   * \\brief Read controls from the sensor\n> - * \\param[inout] ctrls The list of controls to read\n> + * \\param[in] ids The list of control to read, specified by their ID\n>   *\n> - * This method reads the value of all controls contained in \\a ctrls, and stores\n> - * their values in the corresponding \\a ctrls entry.\n> + * This method reads the value of all controls contained in \\a ids, and returns\n> + * their values as a ControlList.\n>   *\n> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n>   * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any\n>   * other error occurs during validation of the requested controls, no control is\n> - * read and this method returns -EINVAL.\n> + * read and this method returns an empty control list.\n>   *\n> - * If an error occurs while reading the controls, the index of the first control\n> - * that couldn't be read is returned. The value of all controls below that index\n> - * are updated in \\a ctrls, while the value of all the other controls are not\n> - * changed.\n> + * If an error occurs while reading the controls, an empty list is returned.\n>   *\n>   * \\sa V4L2Device::getControls()\n>   *\n> - * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> - * \\retval i The index of the control that failed\n> + * \\return The control values in a ControlList\n>   */\n> -int CameraSensor::getControls(ControlList *ctrls)\n> +ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)\n>  {\n> -\treturn subdev_->getControls(ctrls);\n> +\treturn subdev_->getControls(ids);\n>  }\n>\n>  /**\n> diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> index 99cff98128dc..5277f7f7fe87 100644\n> --- a/src/libcamera/include/camera_sensor.h\n> +++ b/src/libcamera/include/camera_sensor.h\n> @@ -43,7 +43,7 @@ public:\n>  \tint setFormat(V4L2SubdeviceFormat *format);\n>\n>  \tconst ControlInfoMap &controls() const;\n> -\tint getControls(ControlList *ctrls);\n> +\tControlList getControls(const std::vector<uint32_t> &ids);\n>  \tint setControls(ControlList *ctrls);\n>\n>  \tconst ControlList &properties() const { return properties_; }\n> diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> index ce8edd98a01d..e604a40df4c9 100644\n> --- a/src/libcamera/include/v4l2_device.h\n> +++ b/src/libcamera/include/v4l2_device.h\n> @@ -26,7 +26,7 @@ public:\n>\n>  \tconst ControlInfoMap &controls() const { return controls_; }\n>\n> -\tint getControls(ControlList *ctrls);\n> +\tControlList getControls(const std::vector<uint32_t> &ids);\n>  \tint setControls(ControlList *ctrls);\n>\n>  \tconst std::string &deviceNode() const { return deviceNode_; }\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index 03e305165096..989ed05c9692 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -147,46 +147,42 @@ void V4L2Device::close()\n>\n>  /**\n>   * \\brief Read controls from the device\n> - * \\param[inout] ctrls The list of controls to read\n> + * \\param[in] ids The list of control to read, specified by their ID\n>   *\n> - * This method reads the value of all controls contained in \\a ctrls, and stores\n> - * their values in the corresponding \\a ctrls entry.\n> + * This method reads the value of all controls contained in \\a ids, and returns\n> + * their values as a ControlList.\n>   *\n> - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n>   * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any\n>   * other error occurs during validation of the requested controls, no control is\n\nI'm about to merge the documentation update that removes compound\ncontrols from the list of unsupported controls, so please rebase the\nnext version.\n\n> - * read and this method returns -EINVAL.\n> + * read and this method returns an empty control list.\n>   *\n> - * If an error occurs while reading the controls, the index of the first control\n> - * that couldn't be read is returned. The value of all controls below that index\n> - * are updated in \\a ctrls, while the value of all the other controls are not\n> - * changed.\n> + * If an error occurs while reading the controls, an empty list is returned.\n>   *\n> - * \\return 0 on success or an error code otherwise\n> - * \\retval -EINVAL One of the control is not supported or not accessible\n> - * \\retval i The index of the control that failed\n> + * \\return The control values in a ControlList\n>   */\n> -int V4L2Device::getControls(ControlList *ctrls)\n> +ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n\nSo we're copying back a ControlList now, thoughts on efficiency ?\n\n>  {\n> -\tunsigned int count = ctrls->size();\n> +\tunsigned int count = ids.size();\n>  \tif (count == 0)\n> -\t\treturn 0;\n> +\t\treturn {};\n> +\n> +\tControlList ctrls{ controls_ };\n>\n>  \tstruct v4l2_ext_control v4l2Ctrls[count];\n>  \tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n>\n>  \tunsigned int i = 0;\n> -\tfor (auto &ctrl : *ctrls) {\n> -\t\tunsigned int id = ctrl.first;\n> +\tfor (uint32_t id : ids) {\n>  \t\tconst auto iter = controls_.find(id);\n>  \t\tif (iter == controls_.end()) {\n>  \t\t\tLOG(V4L2, Error)\n>  \t\t\t\t<< \"Control \" << utils::hex(id) << \" not found\";\n> -\t\t\treturn -EINVAL;\n> +\t\t\treturn {};\n>  \t\t}\n>\n>  \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> -\t\tControlValue &value = ctrl.second;\n> +\t\tControlValue &value = ctrls.set(id, {});\n>\n>  \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n>  \t\t\tControlType type;\n> @@ -200,7 +196,7 @@ int V4L2Device::getControls(ControlList *ctrls)\n>  \t\t\t\tLOG(V4L2, Error)\n>  \t\t\t\t\t<< \"Unsupported payload control type \"\n>  \t\t\t\t\t<< info.type;\n> -\t\t\t\treturn -EINVAL;\n> +\t\t\t\treturn {};\n>  \t\t\t}\n>\n>  \t\t\tvalue.reserve(type, true, info.elems);\n> @@ -227,7 +223,7 @@ int V4L2Device::getControls(ControlList *ctrls)\n>  \t\tif (errorIdx == 0 || errorIdx >= count) {\n>  \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n>  \t\t\t\t\t << strerror(-ret);\n> -\t\t\treturn -EINVAL;\n> +\t\t\treturn {};\n>  \t\t}\n>\n>  \t\t/* A specific control failed. */\n> @@ -237,9 +233,9 @@ int V4L2Device::getControls(ControlList *ctrls)\n>  \t\tret = errorIdx;\n>  \t}\n>\n> -\tupdateControls(ctrls, v4l2Ctrls, count);\n> +\tupdateControls(&ctrls, v4l2Ctrls, count);\n>\n> -\treturn ret;\n> +\treturn ctrls;\n>  }\n>\n>  /**\n> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp\n> index da9e0111e221..347af2112f1a 100644\n> --- a/test/v4l2_videodevice/controls.cpp\n> +++ b/test/v4l2_videodevice/controls.cpp\n> @@ -57,18 +57,20 @@ protected:\n>  \t\tconst ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;\n>\n>  \t\t/* Test getting controls. */\n> -\t\tControlList ctrls(infoMap);\n> -\t\tctrls.set(V4L2_CID_BRIGHTNESS, -1);\n> -\t\tctrls.set(V4L2_CID_CONTRAST, -1);\n> -\t\tctrls.set(V4L2_CID_SATURATION, -1);\n> -\t\tctrls.set(VIVID_CID_U8_4D_ARRAY, 0);\n> -\n> -\t\tint ret = capture_->getControls(&ctrls);\n> -\t\tif (ret) {\n> +\t\tControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,\n> +\t\t\t\t\t\t\t    V4L2_CID_CONTRAST,\n> +\t\t\t\t\t\t\t    V4L2_CID_SATURATION,\n> +\t\t\t\t\t\t\t    VIVID_CID_U8_4D_ARRAY });\n> +\t\tif (ctrls.empty()) {\n>  \t\t\tcerr << \"Failed to get controls\" << endl;\n>  \t\t\treturn TestFail;\n>  \t\t}\n>\n> +\t\tif (ctrls.infoMap() != &infoMap) {\n> +\t\t\tcerr << \"Incorrect infoMap for retrieved controls\" << endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n>  \t\tif (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 ||\n>  \t\t    ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 ||\n>  \t\t    ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) {\n> @@ -97,7 +99,7 @@ protected:\n>  \t\tstd::fill(u8Values.begin(), u8Values.end(), u8.min().get<uint8_t>());\n>  \t\tctrls.set(VIVID_CID_U8_4D_ARRAY, Span<const uint8_t>(u8Values));\n>\n> -\t\tret = capture_->setControls(&ctrls);\n> +\t\tint ret = capture_->setControls(&ctrls);\n>  \t\tif (ret) {\n>  \t\t\tcerr << \"Failed to set controls\" << endl;\n>  \t\t\treturn TestFail;\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"<jacopo@jmondi.org>","Received":["from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C6E5A603FA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 26 Apr 2020 16:40:57 +0200 (CEST)","from uno.localdomain\n\t(host170-48-dynamic.3-87-r.retail.telecomitalia.it [87.3.48.170])\n\t(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id DA6621C0003;\n\tSun, 26 Apr 2020 14:40:55 +0000 (UTC)"],"X-Originating-IP":"87.3.48.170","Date":"Sun, 26 Apr 2020 16:43:58 +0200","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426144358.qyakklzm2hrskxpi@uno.localdomain>","References":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>\n\t<20200425231623.6505-2-laurent.pinchart@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200425231623.6505-2-laurent.pinchart@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify\n\tusage of getControls()","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>","X-List-Received-Date":"Sun, 26 Apr 2020 14:40:57 -0000"}},{"id":4537,"web_url":"https://patchwork.libcamera.org/comment/4537/","msgid":"<20200426172656.GE5950@pendragon.ideasonboard.com>","date":"2020-04-26T17:26:56","subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify\n\tusage of getControls()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Apr 26, 2020 at 04:43:58PM +0200, Jacopo Mondi wrote:\n> On Sun, Apr 26, 2020 at 02:16:23AM +0300, Laurent Pinchart wrote:\n> > The V4L2Device::getControls() function takes a ControlList that needs to\n> > be pre-populated with dummy entries for the controls that need to be\n> > read. This is a cumbersome API, especially when reading a single\n> > control. Make it nicer by passing the list of V4L2 controls as a vector\n> > of control IDs, and returning a ControlList.\n> >\n> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> > ---\n> >  src/libcamera/camera_sensor.cpp       | 23 ++++++---------\n> >  src/libcamera/include/camera_sensor.h |  2 +-\n> \n> While I welcome this for the v4l2 device part, I would leave\n> CameraSensor out, as before defining what kind of controls the class\n> should accepta (v4l2 control IDs or libcamera Control<> instances) we\n> should probably have a bit more use cases...\n\nSure, but the CameraSensor class uses V4L2Device::getControls(), so it\nhas to be modified :-) I could keep the existing\nCameraSensor::getControls() API and reimplement it on top of the new\nV4L2Device::getControls(), but that's unnecessary churn in my opinion as\nthere's no user at the moment. The other option is do drop\nCameraSensor::getControls() completely for now. I can do that, but isn't\nit simpler to keep it until we decide what to do with it ?\n\n> >  src/libcamera/include/v4l2_device.h   |  2 +-\n> >  src/libcamera/v4l2_device.cpp         | 42 ++++++++++++---------------\n> >  test/v4l2_videodevice/controls.cpp    | 20 +++++++------\n> >  5 files changed, 41 insertions(+), 48 deletions(-)\n> >\n> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> > index 2219a4307436..ce585cab1a4f 100644\n> > --- a/src/libcamera/camera_sensor.cpp\n> > +++ b/src/libcamera/camera_sensor.cpp\n> > @@ -299,30 +299,25 @@ const ControlInfoMap &CameraSensor::controls() const\n> >\n> >  /**\n> >   * \\brief Read controls from the sensor\n> > - * \\param[inout] ctrls The list of controls to read\n> > + * \\param[in] ids The list of control to read, specified by their ID\n> >   *\n> > - * This method reads the value of all controls contained in \\a ctrls, and stores\n> > - * their values in the corresponding \\a ctrls entry.\n> > + * This method reads the value of all controls contained in \\a ids, and returns\n> > + * their values as a ControlList.\n> >   *\n> > - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> > + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> >   * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any\n> >   * other error occurs during validation of the requested controls, no control is\n> > - * read and this method returns -EINVAL.\n> > + * read and this method returns an empty control list.\n> >   *\n> > - * If an error occurs while reading the controls, the index of the first control\n> > - * that couldn't be read is returned. The value of all controls below that index\n> > - * are updated in \\a ctrls, while the value of all the other controls are not\n> > - * changed.\n> > + * If an error occurs while reading the controls, an empty list is returned.\n> >   *\n> >   * \\sa V4L2Device::getControls()\n> >   *\n> > - * \\return 0 on success or an error code otherwise\n> > - * \\retval -EINVAL One of the control is not supported or not accessible\n> > - * \\retval i The index of the control that failed\n> > + * \\return The control values in a ControlList\n> >   */\n> > -int CameraSensor::getControls(ControlList *ctrls)\n> > +ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)\n> >  {\n> > -\treturn subdev_->getControls(ctrls);\n> > +\treturn subdev_->getControls(ids);\n> >  }\n> >\n> >  /**\n> > diff --git a/src/libcamera/include/camera_sensor.h b/src/libcamera/include/camera_sensor.h\n> > index 99cff98128dc..5277f7f7fe87 100644\n> > --- a/src/libcamera/include/camera_sensor.h\n> > +++ b/src/libcamera/include/camera_sensor.h\n> > @@ -43,7 +43,7 @@ public:\n> >  \tint setFormat(V4L2SubdeviceFormat *format);\n> >\n> >  \tconst ControlInfoMap &controls() const;\n> > -\tint getControls(ControlList *ctrls);\n> > +\tControlList getControls(const std::vector<uint32_t> &ids);\n> >  \tint setControls(ControlList *ctrls);\n> >\n> >  \tconst ControlList &properties() const { return properties_; }\n> > diff --git a/src/libcamera/include/v4l2_device.h b/src/libcamera/include/v4l2_device.h\n> > index ce8edd98a01d..e604a40df4c9 100644\n> > --- a/src/libcamera/include/v4l2_device.h\n> > +++ b/src/libcamera/include/v4l2_device.h\n> > @@ -26,7 +26,7 @@ public:\n> >\n> >  \tconst ControlInfoMap &controls() const { return controls_; }\n> >\n> > -\tint getControls(ControlList *ctrls);\n> > +\tControlList getControls(const std::vector<uint32_t> &ids);\n> >  \tint setControls(ControlList *ctrls);\n> >\n> >  \tconst std::string &deviceNode() const { return deviceNode_; }\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index 03e305165096..989ed05c9692 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -147,46 +147,42 @@ void V4L2Device::close()\n> >\n> >  /**\n> >   * \\brief Read controls from the device\n> > - * \\param[inout] ctrls The list of controls to read\n> > + * \\param[in] ids The list of control to read, specified by their ID\n> >   *\n> > - * This method reads the value of all controls contained in \\a ctrls, and stores\n> > - * their values in the corresponding \\a ctrls entry.\n> > + * This method reads the value of all controls contained in \\a ids, and returns\n> > + * their values as a ControlList.\n> >   *\n> > - * If any control in \\a ctrls is not supported by the device, is disabled (i.e.\n> > + * If any control in \\a ids is not supported by the device, is disabled (i.e.\n> >   * has the V4L2_CTRL_FLAG_DISABLED flag set), is a compound control, or if any\n> >   * other error occurs during validation of the requested controls, no control is\n> \n> I'm about to merge the documentation update that removes compound\n> controls from the list of unsupported controls, so please rebase the\n> next version.\n\nSure, no problem.\n\n> > - * read and this method returns -EINVAL.\n> > + * read and this method returns an empty control list.\n> >   *\n> > - * If an error occurs while reading the controls, the index of the first control\n> > - * that couldn't be read is returned. The value of all controls below that index\n> > - * are updated in \\a ctrls, while the value of all the other controls are not\n> > - * changed.\n> > + * If an error occurs while reading the controls, an empty list is returned.\n> >   *\n> > - * \\return 0 on success or an error code otherwise\n> > - * \\retval -EINVAL One of the control is not supported or not accessible\n> > - * \\retval i The index of the control that failed\n> > + * \\return The control values in a ControlList\n> >   */\n> > -int V4L2Device::getControls(ControlList *ctrls)\n> > +ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)\n> \n> So we're copying back a ControlList now, thoughts on efficiency ?\n\nWith return valued optimization we get copy ellision, so it should be\nfine. RVO is mandatory since C++-17 and optional before, and in practice\nboth g++ and clang++ implement it for C++-14.\n\n> >  {\n> > -\tunsigned int count = ctrls->size();\n> > +\tunsigned int count = ids.size();\n> >  \tif (count == 0)\n> > -\t\treturn 0;\n> > +\t\treturn {};\n> > +\n> > +\tControlList ctrls{ controls_ };\n> >\n> >  \tstruct v4l2_ext_control v4l2Ctrls[count];\n> >  \tmemset(v4l2Ctrls, 0, sizeof(v4l2Ctrls));\n> >\n> >  \tunsigned int i = 0;\n> > -\tfor (auto &ctrl : *ctrls) {\n> > -\t\tunsigned int id = ctrl.first;\n> > +\tfor (uint32_t id : ids) {\n> >  \t\tconst auto iter = controls_.find(id);\n> >  \t\tif (iter == controls_.end()) {\n> >  \t\t\tLOG(V4L2, Error)\n> >  \t\t\t\t<< \"Control \" << utils::hex(id) << \" not found\";\n> > -\t\t\treturn -EINVAL;\n> > +\t\t\treturn {};\n> >  \t\t}\n> >\n> >  \t\tconst struct v4l2_query_ext_ctrl &info = controlInfo_[id];\n> > -\t\tControlValue &value = ctrl.second;\n> > +\t\tControlValue &value = ctrls.set(id, {});\n> >\n> >  \t\tif (info.flags & V4L2_CTRL_FLAG_HAS_PAYLOAD) {\n> >  \t\t\tControlType type;\n> > @@ -200,7 +196,7 @@ int V4L2Device::getControls(ControlList *ctrls)\n> >  \t\t\t\tLOG(V4L2, Error)\n> >  \t\t\t\t\t<< \"Unsupported payload control type \"\n> >  \t\t\t\t\t<< info.type;\n> > -\t\t\t\treturn -EINVAL;\n> > +\t\t\t\treturn {};\n> >  \t\t\t}\n> >\n> >  \t\t\tvalue.reserve(type, true, info.elems);\n> > @@ -227,7 +223,7 @@ int V4L2Device::getControls(ControlList *ctrls)\n> >  \t\tif (errorIdx == 0 || errorIdx >= count) {\n> >  \t\t\tLOG(V4L2, Error) << \"Unable to read controls: \"\n> >  \t\t\t\t\t << strerror(-ret);\n> > -\t\t\treturn -EINVAL;\n> > +\t\t\treturn {};\n> >  \t\t}\n> >\n> >  \t\t/* A specific control failed. */\n> > @@ -237,9 +233,9 @@ int V4L2Device::getControls(ControlList *ctrls)\n> >  \t\tret = errorIdx;\n> >  \t}\n> >\n> > -\tupdateControls(ctrls, v4l2Ctrls, count);\n> > +\tupdateControls(&ctrls, v4l2Ctrls, count);\n> >\n> > -\treturn ret;\n> > +\treturn ctrls;\n> >  }\n> >\n> >  /**\n> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp\n> > index da9e0111e221..347af2112f1a 100644\n> > --- a/test/v4l2_videodevice/controls.cpp\n> > +++ b/test/v4l2_videodevice/controls.cpp\n> > @@ -57,18 +57,20 @@ protected:\n> >  \t\tconst ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;\n> >\n> >  \t\t/* Test getting controls. */\n> > -\t\tControlList ctrls(infoMap);\n> > -\t\tctrls.set(V4L2_CID_BRIGHTNESS, -1);\n> > -\t\tctrls.set(V4L2_CID_CONTRAST, -1);\n> > -\t\tctrls.set(V4L2_CID_SATURATION, -1);\n> > -\t\tctrls.set(VIVID_CID_U8_4D_ARRAY, 0);\n> > -\n> > -\t\tint ret = capture_->getControls(&ctrls);\n> > -\t\tif (ret) {\n> > +\t\tControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,\n> > +\t\t\t\t\t\t\t    V4L2_CID_CONTRAST,\n> > +\t\t\t\t\t\t\t    V4L2_CID_SATURATION,\n> > +\t\t\t\t\t\t\t    VIVID_CID_U8_4D_ARRAY });\n> > +\t\tif (ctrls.empty()) {\n> >  \t\t\tcerr << \"Failed to get controls\" << endl;\n> >  \t\t\treturn TestFail;\n> >  \t\t}\n> >\n> > +\t\tif (ctrls.infoMap() != &infoMap) {\n> > +\t\t\tcerr << \"Incorrect infoMap for retrieved controls\" << endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> >  \t\tif (ctrls.get(V4L2_CID_BRIGHTNESS).get<int32_t>() == -1 ||\n> >  \t\t    ctrls.get(V4L2_CID_CONTRAST).get<int32_t>() == -1 ||\n> >  \t\t    ctrls.get(V4L2_CID_SATURATION).get<int32_t>() == -1) {\n> > @@ -97,7 +99,7 @@ protected:\n> >  \t\tstd::fill(u8Values.begin(), u8Values.end(), u8.min().get<uint8_t>());\n> >  \t\tctrls.set(VIVID_CID_U8_4D_ARRAY, Span<const uint8_t>(u8Values));\n> >\n> > -\t\tret = capture_->setControls(&ctrls);\n> > +\t\tint ret = capture_->setControls(&ctrls);\n> >  \t\tif (ret) {\n> >  \t\t\tcerr << \"Failed to set controls\" << endl;\n> >  \t\t\treturn TestFail;","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 05DC362E55\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 26 Apr 2020 19:27:12 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5AAB74F7;\n\tSun, 26 Apr 2020 19:27:11 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"bfavJoEW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587922031;\n\tbh=lxGBEBfjUOrKEDpYlTfuSEC/qe20h/0S/v6BTWWTh6E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=bfavJoEWGl1Zcn+AXupJR4HqT8iag8F963bpS5ZleKweovJZGUodswirtHE4MeoMn\n\tJR197/+OvX1qzw1bux4nKJNI980hnlZDzuEMrSO94KvEuL9rXEoVeEN0C4Ym12zS3H\n\tfSyQ9b/aJSnFSSmQnUfGVs3MusmOnttPQNBpdGFY=","Date":"Sun, 26 Apr 2020 20:26:56 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200426172656.GE5950@pendragon.ideasonboard.com>","References":"<20200425231623.6505-1-laurent.pinchart@ideasonboard.com>\n\t<20200425231623.6505-2-laurent.pinchart@ideasonboard.com>\n\t<20200426144358.qyakklzm2hrskxpi@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200426144358.qyakklzm2hrskxpi@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 2/2] libcamera: v4l2_device: Simplify\n\tusage of getControls()","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>","X-List-Received-Date":"Sun, 26 Apr 2020 17:27:12 -0000"}}]