[{"id":25891,"web_url":"https://patchwork.libcamera.org/comment/25891/","msgid":"<20221124123136.lwjtlpdkvlhk7pei@uno.localdomain>","date":"2022-11-24T12:31:36","subject":"Re: [libcamera-devel] [PATCH v3] libcamera: v4l2_device: Workaround\n\tfaulty control menus","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\n this seems a better fix indeed: not registering the\ncontrol at all if faulty. Users might be wonder why the control is not\nexposed, but the warning message should warn them.. speaking of which\n\n\nOn Wed, Nov 23, 2022 at 01:42:32PM +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> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/v4l2_device.h | 4 ++--\n> src/libcamera/v4l2_device.cpp | 23 ++++++++++++++++++-----\n> 2 files changed, 20 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..c2e21bdf33b7 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> +\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\nI would explicitely say \"Cannot register control...\"\n\nApart from that\nReviewed-by: Jacopo Mondi \n\nThanks\n j\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,9 @@ 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> +\t\tif (info)\n> +\t\t\tctrls.emplace(controlIds_.back().get(), *info);\n> \t}\n>\n> \tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n> @@ -670,7 +683,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>\n> --\n> 2.34.1\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 C918EBE08B\n\tfor ;\n\tThu, 24 Nov 2022 12:31:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4342063313;\n\tThu, 24 Nov 2022 13:31:40 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 085FE632EA\n\tfor ;\n\tThu, 24 Nov 2022 13:31:38 +0100 (CET)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 3FEB31BF207;\n\tThu, 24 Nov 2022 12:31:36 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669293100;\n\tbh=Ri6BJjrq99ddnJ14Kf/pgc1cemG88tuljaqlp1jorpA=;\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=uiXq9C9QQ5SJE8tMJ4ZaYQXFwib/6aBkOVTsGaJT4UTkWrrAyS91XR5xeZ9AfxnGG\n\tu5G/F3F8G66o2IEV5JX3zSc32P7wjrpEdkmzbCdTMdSsWhTR0VHizLWvUin0ix/Loe\n\typKNu9pTia/ZIpFKLAV3BKDaI+QixBI07a2KjkD47ZmykimVj9OS2ggELN2SSiyUO4\n\tooJO69qtf2Fm2SIPjp9VubkMZ2tOuypEWU1llXX+Rdgn/MV/Yuu4+n6wI5w41w0Bbq\n\t8jL8+8DR4kVPrvSt0Gpzcool8upKxZm5WyxqjxnFlGH2rIpSo8fvENnSUpbIM1m3pU\n\tyaKB5fopma7Xg==","Date":"Thu, 24 Nov 2022 13:31:36 +0100","To":"Kieran Bingham ","Message-ID":"<20221124123136.lwjtlpdkvlhk7pei@uno.localdomain>","References":"<20221123134232.1937002-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221123134232.1937002-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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera devel ,\n\tMarian Buschsieweke ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":25892,"web_url":"https://patchwork.libcamera.org/comment/25892/","msgid":"<6807d528-8190-fae5-2bb7-61633fae0de4@ideasonboard.com>","date":"2022-11-24T12:41:38","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":"On 24/11/2022 12:31, Jacopo Mondi wrote:\n> Hi Kieran\n> \n> this seems a better fix indeed: not registering the\n> control at all if faulty. Users might be wonder why the control is not\n> exposed, but the warning message should warn them.. speaking of which\n> \n> \n> On Wed, Nov 23, 2022 at 01:42:32PM +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>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/internal/v4l2_device.h | 4 ++--\n>> src/libcamera/v4l2_device.cpp | 23 ++++++++++++++++++-----\n>> 2 files changed, 20 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..c2e21bdf33b7 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>> +\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> \n> I would explicitely say \"Cannot register control...\"\n> \n\nAs there are two error paths here that could fail to add a control, I'll \nupdate the higher layer as follows:\n\n\n std::optional info = v4l2ControlInfo(ctrl);\n\n if (!info) {\n LOG(V4L2, Error)\n << \"Control \" << ctrl.name\n << \" cannot be registered.\"\n\n continue;\n }\n\n ctrls.emplace(controlIds_.back().get(), *info);\n\n\n> Apart from that\n> Reviewed-by: Jacopo Mondi \n\nThanks\n\n> \n> Thanks\n> j\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,9 @@ 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>> +\t\tif (info)\n>> +\t\t\tctrls.emplace(controlIds_.back().get(), *info);\n>> \t}\n>>\n>> \tcontrols_ = ControlInfoMap(std::move(ctrls), controlIdMap_);\n>> @@ -670,7 +683,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>>\n>> --\n>> 2.34.1\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 330ABBDE6B\n\tfor ;\n\tThu, 24 Nov 2022 12:41:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 685FC6331A;\n\tThu, 24 Nov 2022 13:41:40 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 77D26632EA\n\tfor ;\n\tThu, 24 Nov 2022 13:41:39 +0100 (CET)","from [192.168.0.32]\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 DE1F425B;\n\tThu, 24 Nov 2022 13:41:38 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669293700;\n\tbh=nW85llkeq0lvzHQVm7AMEK0pMCHKc17Sq27z6fnuKtE=;\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=HuHw4mYFSIfLqnEJRTXBJOPhBdqad2aKIZhEj7SOIj0i6NxEiEaAjs5cmRFsa6iSA\n\trIwDdTUwsOECOFEqyVoCRH1C1Kq0XIyVGI2++q+4Ka4FFJ8azYE19tZSh621n5Slab\n\t8qxGd8Y1L2gmVisz5ifVmhqkFRXeJ/+6Tyb/m/4stNt54mDZgMYg6Qcl7hu7PFugQz\n\tBKrHLTvvFhc2u+g0SlNj0hQGUG7Pmnrl764MHx5l+Bh7hIH/+u7JtSEHckKZAuZ90X\n\teWlkk4k5IrmEjM1RW9TwYZk4TmIzhil3C2Xs+hLvg5iLRuB3L6AIfVo7CWo5zYnVbl\n\t/jpwRYZNCusxA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669293699;\n\tbh=nW85llkeq0lvzHQVm7AMEK0pMCHKc17Sq27z6fnuKtE=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=VeqHogsrNzW5txXnq3RHcPOh6pXmCFk0JF3G2voFvsytFYU6UPe++MYLXzQ+ufdCl\n\tC62wW+H6YIVoClMT9ypekPI4xoFMD6THg12g+FP75JWsU/EioVzJKVWoNBhXih8P8s\n\tlWGqvb8kdCtMayJw3+UohU8+n8/oZ7ab/T1OjZpU="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"VeqHogsr\"; dkim-atps=neutral","Message-ID":"<6807d528-8190-fae5-2bb7-61633fae0de4@ideasonboard.com>","Date":"Thu, 24 Nov 2022 12:41:38 +0000","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101\n\tThunderbird/102.4.2","Content-Language":"en-US","To":"Jacopo Mondi ","References":"<20221123134232.1937002-1-kieran.bingham@ideasonboard.com>\n\t<20221124123136.lwjtlpdkvlhk7pei@uno.localdomain>","In-Reply-To":"<20221124123136.lwjtlpdkvlhk7pei@uno.localdomain>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}}]