[{"id":25906,"web_url":"https://patchwork.libcamera.org/comment/25906/","msgid":"","date":"2022-11-25T01:16:32","subject":"Re: [libcamera-devel] [PATCH v5] 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 02:45:53PM +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\nReviewed-by: Laurent Pinchart \n\n> ---\n> \n> As I posted two v3 patches, I've gone straight to v5 on this.\n\n:-D\n\n> Now there's only the common path error report, and a couple of minor\n> text fixes.\n> \n> \n> include/libcamera/internal/v4l2_device.h | 4 ++--\n> src/libcamera/v4l2_device.cpp | 28 +++++++++++++++++++-----\n> 2 files changed, 25 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..57a88d96b12c 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,14 @@ 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 devices 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\treturn std::nullopt;\n> +\n> \treturn ControlInfo(indices,\n> \t\t\t ControlValue(static_cast(ctrl.default_value)));\n> }\n> @@ -631,7 +639,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> +\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 +688,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 860F8BE08B\n\tfor ;\n\tFri, 25 Nov 2022 01:16:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 46EF463311;\n\tFri, 25 Nov 2022 02:16:49 +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 6F8EC63311\n\tfor ;\n\tFri, 25 Nov 2022 02:16:48 +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 F202A501;\n\tFri, 25 Nov 2022 02:16:47 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1669339009;\n\tbh=eNK3FWa5bK+zWsDU98QUbAVzwiejCcWcmAWGjr1Hc/M=;\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=4bcM2MVtAirBuifBTHKFMVucsx7Swb0bH3UGBJiiuHdDyF3/mNcnWMD+PQsR8o3aa\n\tMe1mE9zhlzQ/rmy1ecWGjvr3zXv9rYmJsSkmoi+darTzIfdHjmhlH7nH4H24Tz6059\n\tqAcQVVoJo3tIC5FN7YHpSvECV0hUTy5lrKOjym3zRdQGSa4T/Mf4qYqb/Vqh5bQ+CE\n\tKniMyJtNnFm7dL1GP/wUO3NzQtbE3JEq4hbRFpm+YxWrs8CyLQa4i6D+2bnFffnkcv\n\tpaGZsAMalrh5hlZAGVTidnq31XgJPsVzr/OlYNZGs2ZD+l+pjKofFp4bLkjg7Gxbtq\n\tYtFwQaqvesCMQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1669339008;\n\tbh=eNK3FWa5bK+zWsDU98QUbAVzwiejCcWcmAWGjr1Hc/M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=WVED6s1N9nmet8TVkYnJq9Dy4Wv0nHQb2vAIGJ0cpV1nnOV/KW0lOjLRHCogUgixu\n\t5zhqiGOaqxqB7eRBZVlniVNu8yYG1JRV0VvMFM8FC/eIwFQ06aBSb6305m+y35Jmlm\n\tik1CYi70vBc2PBTmcr7toCNrV6hqjHYQDnSydQRo="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"WVED6s1N\"; dkim-atps=neutral","Date":"Fri, 25 Nov 2022 03:16:32 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20221124144553.3012369-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221124144553.3012369-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5] 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\" "}}]