[{"id":25899,"web_url":"https://patchwork.libcamera.org/comment/25899/","msgid":"","date":"2022-11-24T14:07:57","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Thu, Nov 24, 2022 at 01:03:08PM +0000, Kieran Bingham via libcamera-devel wrote:\n> Some UVC cameras have been identified that can provide V4L2 menu\n> controls without any menu items.\n> \n> This leads to a segfault where we try to construct a\n> ControlInfo(Span<>,default) with an empty span.\n> \n> Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to\n> return std::optional to be able to account in the caller if\n> the control is valid, and only add acceptable controls to the supported\n> control list.\n> \n> Menu controls without a list of menu items are no longer added as a\n> valid control and a warning is logged.\n> \n> This also fixes a potential crash that would have occured in the\n> unlikely event that a ctrl.minimum was set to less than 0.\n> \n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=167\n> Reported-by: Marian Buschsieweke \n> Reviewed-by: Jacopo Mondi \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/v4l2_device.h | 4 +--\n> src/libcamera/v4l2_device.cpp | 31 ++++++++++++++++++++----\n> 2 files changed, 28 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> index 75304be11f77..50d4adbc5f2b 100644\n> --- a/include/libcamera/internal/v4l2_device.h\n> +++ b/include/libcamera/internal/v4l2_device.h\n> @@ -70,8 +70,8 @@ protected:\n> private:\n> \tstatic ControlType v4l2CtrlType(uint32_t ctrlType);\n> \tstatic std::unique_ptr v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);\n> -\tControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> -\tControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> +\tstd::optional v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> +\tstd::optional v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> \n> \tvoid listControls();\n> \tvoid updateControls(ControlList *ctrls,\n> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> index c17b323f8af0..5dd51037960d 100644\n> --- a/src/libcamera/v4l2_device.cpp\n> +++ b/src/libcamera/v4l2_device.cpp\n> @@ -529,7 +529,7 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> * \\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control\n> * \\return A ControlInfo that represents \\a ctrl\n> */\n> -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> +std::optional V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> {\n> \tswitch (ctrl.type) {\n> \tcase V4L2_CTRL_TYPE_U8:\n> @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> *\n> * \\return A ControlInfo that represents \\a ctrl\n> */\n> -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> +std::optional V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> {\n> \tstd::vector indices;\n> \tstruct v4l2_querymenu menu = {};\n> \tmenu.id = ctrl.id;\n> \n> \tif (ctrl.minimum < 0)\n> -\t\treturn ControlInfo();\n> +\t\treturn std::nullopt;\n> \n> \tfor (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {\n> \t\tmenu.index = index;\n> @@ -583,6 +583,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct\n> \t\tindices.push_back(index);\n> \t}\n> \n> +\t/*\n> +\t * Some faulty UVC drivers are known to return an empty menu control.\n\ns/drivers/devices/ ?\n\n> +\t * Controls without a menu option can not be set, or read, so they are\n> +\t * not exposed.\n> +\t */\n> +\tif (indices.size() == 0) {\n> +\t\tLOG(V4L2, Warning)\n> +\t\t\t<< \"Menu control '\" << ctrl.name << \"' has no entries\";\n\nWould you consider this a driver bug ? If so a warning sounds fine, and\nwe should also fix the driver. If not, maybe we should demote the\nwarning message to a debug message ?\n\n> +\t\treturn std::nullopt;\n> +\t}\n> +\n> \treturn ControlInfo(indices,\n> \t\t\t ControlValue(static_cast(ctrl.default_value)));\n> }\n> @@ -631,7 +642,17 @@ void V4L2Device::listControls()\n> \t\tcontrolIdMap_[ctrl.id] = controlIds_.back().get();\n> \t\tcontrolInfo_.emplace(ctrl.id, ctrl);\n> \n> -\t\tctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n> +\t\tstd::optional info = v4l2ControlInfo(ctrl);\n> +\n> +\t\tif (!info) {\n> +\t\t\tLOG(V4L2, Error)\n> +\t\t\t\t<< \"Control \" << ctrl.name\n> +\t\t\t\t<< \" cannot be registered.\";\n\ns/registered./registered/\n\nBut I would drop this as we already log a message in\nv4l2MenuControlInfo() (or you could drop the message there instead).\n\n> +\n> +\t\t\tcontinue;\n> +\t\t}\n> +\n> +\t\tctrls.emplace(controlIds_.back().get(), *info);\n> \t}\n> \n> \tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()\n> \t\t\tcontinue;\n> \t\t}\n> \n> -\t\tinfo = v4l2ControlInfo(ctrl);\n> +\t\tinfo = *v4l2ControlInfo(ctrl);\n> \t}\n> }\n>","headers":{"Return-Path":"","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 4BFD4BE08B\n\tfor ;\n\tThu, 24 Nov 2022 14:08:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B3776331F;\n\tThu, 24 Nov 2022 15:08:14 +0100 (CET)","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 47E696330D\n\tfor ;\n\tThu, 24 Nov 2022 15:08:13 +0100 (CET)","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 9FBD8496;\n\tThu, 24 Nov 2022 15:08:12 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669298894;\n\tbh=9KZ0TBJoJuM7UG8qujXxcJ8FiFH+FyIby4M1k4R9pwU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=nqWgC2f3KzlRkn7e3eUYK1nEbmmw+MTFc7ll4mvdrV2+I8xA4CDDGic4yqV66HQKg\n\tTRrocalmDFgK8Nbjs69aq6t04GzHC1S7hUG1V9QOcX6+2JTsX7+w1N0KNtvPTuScjB\n\tMr7nbsZvLdqtFqNh8IxM/BkgxnTlKa3BDt+DScQzVgK61qzhoKxXL+x3W6nh3c5UOA\n\tjhwHmlnePCjm3apQu1aYnCr0oZF58+YyWcSl+aHjhUHusbzNWDBQkdjTmWAX1kohZG\n\tjAeSnbSo4deZCzzZaQ6d/IZKTdFf60eC80REoX+GEBtvZwYbecGDn/SalywDoeyBgQ\n\tEFb+86kJJDYAA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669298892;\n\tbh=9KZ0TBJoJuM7UG8qujXxcJ8FiFH+FyIby4M1k4R9pwU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Tq+efAr55DezFIrA4syOgOCdmY5zylYAqcnwOWhpANS5BOoWARZ6wXylch2c7rrOB\n\t/MEPmTVbxKFTd36sz0W0m0qwaAgQx4qyo8g8SUui8Awg3+qveCxQac0vdBBoc1b4ew\n\t4BzSXCwWF9TFzDnrFOXzCNVrn/5jo8TzS43/aJmY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Tq+efAr5\"; dkim-atps=neutral","Date":"Thu, 24 Nov 2022 16:07:57 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221124130308.2928570-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221124130308.2928570-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ,\n\tMarian Buschsieweke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25900,"web_url":"https://patchwork.libcamera.org/comment/25900/","msgid":"<166929931780.2936560.18041148778792025112@Monstersaurus>","date":"2022-11-24T14:15:17","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Laurent Pinchart (2022-11-24 14:07:57)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Thu, Nov 24, 2022 at 01:03:08PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > Some UVC cameras have been identified that can provide V4L2 menu\n> > controls without any menu items.\n> > \n> > This leads to a segfault where we try to construct a\n> > ControlInfo(Span<>,default) with an empty span.\n> > \n> > Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to\n> > return std::optional to be able to account in the caller if\n> > the control is valid, and only add acceptable controls to the supported\n> > control list.\n> > \n> > Menu controls without a list of menu items are no longer added as a\n> > valid control and a warning is logged.\n> > \n> > This also fixes a potential crash that would have occured in the\n> > unlikely event that a ctrl.minimum was set to less than 0.\n> > \n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=167\n> > Reported-by: Marian Buschsieweke \n> > Reviewed-by: Jacopo Mondi \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/internal/v4l2_device.h | 4 +--\n> > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++++++++----\n> > 2 files changed, 28 insertions(+), 7 deletions(-)\n> > \n> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > index 75304be11f77..50d4adbc5f2b 100644\n> > --- a/include/libcamera/internal/v4l2_device.h\n> > +++ b/include/libcamera/internal/v4l2_device.h\n> > @@ -70,8 +70,8 @@ protected:\n> > private:\n> > static ControlType v4l2CtrlType(uint32_t ctrlType);\n> > static std::unique_ptr v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);\n> > - ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > - ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > + std::optional v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > + std::optional v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > \n> > void listControls();\n> > void updateControls(ControlList *ctrls,\n> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > index c17b323f8af0..5dd51037960d 100644\n> > --- a/src/libcamera/v4l2_device.cpp\n> > +++ b/src/libcamera/v4l2_device.cpp\n> > @@ -529,7 +529,7 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> > * \\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control\n> > * \\return A ControlInfo that represents \\a ctrl\n> > */\n> > -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > +std::optional V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > {\n> > switch (ctrl.type) {\n> > case V4L2_CTRL_TYPE_U8:\n> > @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > *\n> > * \\return A ControlInfo that represents \\a ctrl\n> > */\n> > -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > +std::optional V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > {\n> > std::vector indices;\n> > struct v4l2_querymenu menu = {};\n> > menu.id = ctrl.id;\n> > \n> > if (ctrl.minimum < 0)\n> > - return ControlInfo();\n> > + return std::nullopt;\n> > \n> > for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {\n> > menu.index = index;\n> > @@ -583,6 +583,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct\n> > indices.push_back(index);\n> > }\n> > \n> > + /*\n> > + * Some faulty UVC drivers are known to return an empty menu control.\n> \n> s/drivers/devices/ ?\n\nAck.\n\n> \n> > + * Controls without a menu option can not be set, or read, so they are\n> > + * not exposed.\n> > + */\n> > + if (indices.size() == 0) {\n> > + LOG(V4L2, Warning)\n> > + << \"Menu control '\" << ctrl.name << \"' has no entries\";\n> \n> Would you consider this a driver bug ? If so a warning sounds fine, and\n> we should also fix the driver. If not, maybe we should demote the\n> warning message to a debug message ?\n\nNo - I understand that it's a firmware bug on the UVC device. The UVC\ndriver exposes a control, and then generates the menu options available\nfrom a bitmask. My *assumption* is that this device did not set the\nbitmask, so there were no menu options available.\n\nI don't think that can be fixed in uvcvideo driver - if there's no\nsupported option for the control, it's ... just not a useful control.\n\nEven if we 'fix' the uvcvideo driver to not expose menu controls that\nhave no menu - they are already out there in the wild - and we thus need\nto support this issue here.\n\nIf this happens on other occasions, then it could be a driver bug, so\nI'd like to kee the error as an explicit warning.\n\n\n> > + return std::nullopt;\n> > + }\n> > +\n> > return ControlInfo(indices,\n> > ControlValue(static_cast(ctrl.default_value)));\n> > }\n> > @@ -631,7 +642,17 @@ void V4L2Device::listControls()\n> > controlIdMap_[ctrl.id] = controlIds_.back().get();\n> > controlInfo_.emplace(ctrl.id, ctrl);\n> > \n> > - ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n> > + std::optional info = v4l2ControlInfo(ctrl);\n> > +\n> > + if (!info) {\n> > + LOG(V4L2, Error)\n> > + << \"Control \" << ctrl.name\n> > + << \" cannot be registered.\";\n> \n> s/registered./registered/\n> \n> But I would drop this as we already log a message in\n> v4l2MenuControlInfo() (or you could drop the message there instead).\n\nI added this at the request of Jacopo on the previous version, and I\nagree, because at this point it's an Error - and it catches more error\npaths than just the one highlighted above which would otherwise not\nreport a message.\n\n\n> \n> > +\n> > + continue;\n> > + }\n> > +\n> > + ctrls.emplace(controlIds_.back().get(), *info);\n> > }\n> > \n> > controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> > @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()\n> > continue;\n> > }\n> > \n> > - info = v4l2ControlInfo(ctrl);\n> > + info = *v4l2ControlInfo(ctrl);\n> > }\n> > }\n> > \n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","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 625A9BDE6B\n\tfor ;\n\tThu, 24 Nov 2022 14:15:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D86C86331F;\n\tThu, 24 Nov 2022 15:15:21 +0100 (CET)","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 807936330D\n\tfor ;\n\tThu, 24 Nov 2022 15:15:20 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 08188496;\n\tThu, 24 Nov 2022 15:15:19 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669299321;\n\tbh=VZcBMf2ihxLM7oGo6Av4kO6ycIWjbPeunlA+ofkt/mQ=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=Sa0aUoStjSL7wUd+cUt8KyB3ooz6tsb036ILsl00Ve2UaclLpNIGIbYgurNZLnK+w\n\t5N1Pa12gFNnJROznVsFrCESFe4bV+qiZMpY5O9PA+7b2F+LUBSSz0x9BHuX4I8NXO6\n\teFQkm51YImdsw7tWXjkwjdo8ebHCAqQdhKYTGFweesrNxuzhUGqgZvzXZBbhLwF+b6\n\tvUNFjUDzQ8TWQiEvCWP9dTda94VhGVcB8RQNrlF5UVw2xl2OlL7Xx+xzz54el6EV40\n\tzhwSX1Jgzgmpu5UVy2bvwpmrV6ceE3IKOge90FljtCIu4uCwlZOLWU7e8ulFgkHGTd\n\telIgayhSZ2KgQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669299320;\n\tbh=VZcBMf2ihxLM7oGo6Av4kO6ycIWjbPeunlA+ofkt/mQ=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=e0WcDkyXglsxqA9WhrW7vzePOWY48PS8oY+GFpay8sNh4fnkd70RoMS1n9ozIWeqE\n\tZI4HFDxJiNfVhYfkvUuKi0R0yaOSehoMP6pVV4xWby8DHVvaWhrPQmt38ofhxII//w\n\tAyrsSqlcCkj/o5FjKdJxdKKPh2dl9CkNan4TBJIc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"e0WcDkyX\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20221124130308.2928570-1-kieran.bingham@ideasonboard.com>\n\t","To":"Laurent Pinchart ","Date":"Thu, 24 Nov 2022 14:15:17 +0000","Message-ID":"<166929931780.2936560.18041148778792025112@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera devel ,\n\tMarian Buschsieweke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25902,"web_url":"https://patchwork.libcamera.org/comment/25902/","msgid":"","date":"2022-11-24T14:19:48","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Nov 24, 2022 at 02:15:17PM +0000, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2022-11-24 14:07:57)\n> > On Thu, Nov 24, 2022 at 01:03:08PM +0000, Kieran Bingham via libcamera-devel wrote:\n> > > Some UVC cameras have been identified that can provide V4L2 menu\n> > > controls without any menu items.\n> > > \n> > > This leads to a segfault where we try to construct a\n> > > ControlInfo(Span<>,default) with an empty span.\n> > > \n> > > Convert the v4l2ControlInfo and v4l2MenuControlInfo helper functions to\n> > > return std::optional to be able to account in the caller if\n> > > the control is valid, and only add acceptable controls to the supported\n> > > control list.\n> > > \n> > > Menu controls without a list of menu items are no longer added as a\n> > > valid control and a warning is logged.\n> > > \n> > > This also fixes a potential crash that would have occured in the\n> > > unlikely event that a ctrl.minimum was set to less than 0.\n> > > \n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=167\n> > > Reported-by: Marian Buschsieweke \n> > > Reviewed-by: Jacopo Mondi \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > include/libcamera/internal/v4l2_device.h | 4 +--\n> > > src/libcamera/v4l2_device.cpp | 31 ++++++++++++++++++++----\n> > > 2 files changed, 28 insertions(+), 7 deletions(-)\n> > > \n> > > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h\n> > > index 75304be11f77..50d4adbc5f2b 100644\n> > > --- a/include/libcamera/internal/v4l2_device.h\n> > > +++ b/include/libcamera/internal/v4l2_device.h\n> > > @@ -70,8 +70,8 @@ protected:\n> > > private:\n> > > static ControlType v4l2CtrlType(uint32_t ctrlType);\n> > > static std::unique_ptr v4l2ControlId(const v4l2_query_ext_ctrl &ctrl);\n> > > - ControlInfo v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > > - ControlInfo v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > > + std::optional v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > > + std::optional v4l2MenuControlInfo(const v4l2_query_ext_ctrl &ctrl);\n> > > \n> > > void listControls();\n> > > void updateControls(ControlList *ctrls,\n> > > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp\n> > > index c17b323f8af0..5dd51037960d 100644\n> > > --- a/src/libcamera/v4l2_device.cpp\n> > > +++ b/src/libcamera/v4l2_device.cpp\n> > > @@ -529,7 +529,7 @@ std::unique_ptr V4L2Device::v4l2ControlId(const v4l2_query_ext_ctrl &\n> > > * \\param[in] ctrl The v4l2_query_ext_ctrl that represents a V4L2 control\n> > > * \\return A ControlInfo that represents \\a ctrl\n> > > */\n> > > -ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > > +std::optional V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > > {\n> > > switch (ctrl.type) {\n> > > case V4L2_CTRL_TYPE_U8:\n> > > @@ -566,14 +566,14 @@ ControlInfo V4L2Device::v4l2ControlInfo(const v4l2_query_ext_ctrl &ctrl)\n> > > *\n> > > * \\return A ControlInfo that represents \\a ctrl\n> > > */\n> > > -ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > +std::optional V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ctrl)\n> > > {\n> > > std::vector indices;\n> > > struct v4l2_querymenu menu = {};\n> > > menu.id = ctrl.id;\n> > > \n> > > if (ctrl.minimum < 0)\n> > > - return ControlInfo();\n> > > + return std::nullopt;\n> > > \n> > > for (int32_t index = ctrl.minimum; index <= ctrl.maximum; ++index) {\n> > > menu.index = index;\n> > > @@ -583,6 +583,17 @@ ControlInfo V4L2Device::v4l2MenuControlInfo(const struct v4l2_query_ext_ctrl &ct\n> > > indices.push_back(index);\n> > > }\n> > > \n> > > + /*\n> > > + * Some faulty UVC drivers are known to return an empty menu control.\n> > \n> > s/drivers/devices/ ?\n> \n> Ack.\n> \n> > \n> > > + * Controls without a menu option can not be set, or read, so they are\n> > > + * not exposed.\n> > > + */\n> > > + if (indices.size() == 0) {\n> > > + LOG(V4L2, Warning)\n> > > + << \"Menu control '\" << ctrl.name << \"' has no entries\";\n> > \n> > Would you consider this a driver bug ? If so a warning sounds fine, and\n> > we should also fix the driver. If not, maybe we should demote the\n> > warning message to a debug message ?\n> \n> No - I understand that it's a firmware bug on the UVC device. The UVC\n> driver exposes a control, and then generates the menu options available\n> from a bitmask. My *assumption* is that this device did not set the\n> bitmask, so there were no menu options available.\n> \n> I don't think that can be fixed in uvcvideo driver - if there's no\n> supported option for the control, it's ... just not a useful control.\n\nThe uvcvideo driver could skip the control in that case. It would be\nless confusing for applications in general.\n\n> Even if we 'fix' the uvcvideo driver to not expose menu controls that\n> have no menu - they are already out there in the wild - and we thus need\n> to support this issue here.\n\nAbsolutely, I'm fine with this patch.\n\n> If this happens on other occasions, then it could be a driver bug, so\n> I'd like to kee the error as an explicit warning.\n\nA warning printed every time such devices are used bothers me a bit. If\nwe update the uvcvideo driver to skip the control in that case, I'd be\nfine with a warning message.\n\n> > > + return std::nullopt;\n> > > + }\n> > > +\n> > > return ControlInfo(indices,\n> > > ControlValue(static_cast(ctrl.default_value)));\n> > > }\n> > > @@ -631,7 +642,17 @@ void V4L2Device::listControls()\n> > > controlIdMap_[ctrl.id] = controlIds_.back().get();\n> > > controlInfo_.emplace(ctrl.id, ctrl);\n> > > \n> > > - ctrls.emplace(controlIds_.back().get(), v4l2ControlInfo(ctrl));\n> > > + std::optional info = v4l2ControlInfo(ctrl);\n> > > +\n> > > + if (!info) {\n> > > + LOG(V4L2, Error)\n> > > + << \"Control \" << ctrl.name\n> > > + << \" cannot be registered.\";\n> > \n> > s/registered./registered/\n> > \n> > But I would drop this as we already log a message in\n> > v4l2MenuControlInfo() (or you could drop the message there instead).\n> \n> I added this at the request of Jacopo on the previous version, and I\n> agree, because at this point it's an Error - and it catches more error\n> paths than just the one highlighted above which would otherwise not\n> report a message.\n\nThen I'd drop the first one. If one message bothers me already, two will\nbe worse :-)\n\n> > > +\n> > > + continue;\n> > > + }\n> > > +\n> > > + ctrls.emplace(controlIds_.back().get(), *info);\n> > > }\n> > > \n> > > controls_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> > > @@ -670,7 +691,7 @@ void V4L2Device::updateControlInfo()\n> > > continue;\n> > > }\n> > > \n> > > - info = v4l2ControlInfo(ctrl);\n> > > + info = *v4l2ControlInfo(ctrl);\n> > > }\n> > > }\n> > >","headers":{"Return-Path":"","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 34229BE08B\n\tfor ;\n\tThu, 24 Nov 2022 14:20:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9BFC26331F;\n\tThu, 24 Nov 2022 15:20:05 +0100 (CET)","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 6CF766330D\n\tfor ;\n\tThu, 24 Nov 2022 15:20:04 +0100 (CET)","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 BA074496;\n\tThu, 24 Nov 2022 15:20:03 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669299605;\n\tbh=X5Q2bhmFCVOsfu7L0xbPO6gTcODXMXxjvfsLmtqu9WY=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vIyOgJ5n6fPh5/l/c+7sVV5TESRwJKWKRRS2NxuyJwzbZIaQIEe0HaRRkFVRv2tJi\n\ttHVztviX+VgYBfJxolmLHbJf3SNxXstsp8NZgA542/BhAQMdscLtcmNVk+iv6B8AWB\n\tbLsrhnhkGrfSgKLC+YDa3I4xCbpn7rUvutiK3jd6yWBSrz2hAwExNsJb5cND/uCxiY\n\tCg5J9PtHVRaM8fZvKnwWGbSldLXJCPH87VvbXOmOInbObeXLHEjuSwjXFiNPkphY2U\n\tYGE3Y1acvYgcpHz6buDF1SpToSFScPtZGLd/h+zexTKO+7IDgnZIWhvhOGiyhrD3ft\n\tKASvIShzeXv8g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669299604;\n\tbh=X5Q2bhmFCVOsfu7L0xbPO6gTcODXMXxjvfsLmtqu9WY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dXDVFWmEant+M0SiDG+Sft4ArY73dSG3+hW3stCmO2yf0oitWHWWZUH+2wliL/pMH\n\t0dd5o8EHOzXpAra5+klmrf2BxSJxwP+6sTwfE1yKBjGEDAG2ociT0JvjQ6kfMcV1MF\n\t8Hx7Ej95HhwOd2fUZ6A6PjVTQVGdiIoVJnFuv8Qs="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"dXDVFWmE\"; dkim-atps=neutral","Date":"Thu, 24 Nov 2022 16:19:48 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221124130308.2928570-1-kieran.bingham@ideasonboard.com>\n\t\n\t<166929931780.2936560.18041148778792025112@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166929931780.2936560.18041148778792025112@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera devel ,\n\tMarian Buschsieweke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]