[{"id":33676,"web_url":"https://patchwork.libcamera.org/comment/33676/","msgid":"<2z3vqvzvw2kd62kn2saigpkcu7fhgrjftigckq6woymlom2rgh@mxg5kn46pu6z>","date":"2025-03-21T13:55:29","subject":"Re: [PATCH v3 1/4] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setup","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Mar 14, 2025 at 06:42:45PM +0100, Barnabás Pőcze wrote:\n> `ControlInfo(Span{...})` calls the incorrect constructor\n> of `ControlInfo`. The intended constructor to be called is\n> `ControlInfo(Span, ...)` however that is not called\n> because a span of `const int32_t` is passed. Instead, the constructor\n> `ControlInfo(const ControlValue &min, const ControlValue &max, ...)`\n> will be called.\n>\n> Furthermore, since `values.back()` is used, only the last element of\n> the array is actually set.\n>\n> To fix this, convert the array to contain `ControlValue` objects and use\n> a separate variable to keep track of which element to set next.\n>\n> For each of `ExposureTimeMode{Auto,Manual}` save the V4L2 control value\n> that is to be used when the libcamera control is set.\n>\n> Fixes: bad8d591f8acfa (\"libcamera: uvcvideo: Register ExposureTimeMode control\")\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 89 +++++++++++++++-----\n> 1 file changed, 70 insertions(+), 19 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index dedcac89b..5c9025d9b 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -58,6 +59,9 @@ public:\n> \tStream stream_;\n> \tstd::map> formats_;\n>\n> +\tstd::optional autoExposureMode_;\n> +\tstd::optional manualExposureMode_;\n> +\n> private:\n> \tbool generateId();\n>\n> @@ -108,6 +112,26 @@ private:\n> \t}\n> };\n>\n> +namespace {\n> +\n> +std::optional v4l2ToExposureMode(int32_t x)\n> +{\n> +\tusing namespace controls;\n> +\n> +\tswitch (x) {\n> +\tcase V4L2_EXPOSURE_AUTO:\n> +\tcase V4L2_EXPOSURE_APERTURE_PRIORITY:\n> +\t\treturn ExposureTimeModeAuto;\n> +\tcase V4L2_EXPOSURE_MANUAL:\n> +\tcase V4L2_EXPOSURE_SHUTTER_PRIORITY:\n> +\t\treturn ExposureTimeModeManual;\n> +\tdefault:\n> +\t\treturn {};\n> +\t}\n> +}\n> +\n> +} /* namespace */\n> +\n> UVCCameraConfiguration::UVCCameraConfiguration(UVCCameraData *data)\n> \t: CameraConfiguration(), data_(data)\n> {\n> @@ -725,25 +749,52 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\t * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,\n> \t\t *\t\t\t V4L2_EXPOSURE_SHUTTER_PRIORITY }\n> \t\t */\n> -\t\tstd::array values{};\n> -\n> -\t\tauto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n> -\t\t\t[&](const ControlValue &val) {\n> -\t\t\t\treturn (val.get() == V4L2_EXPOSURE_APERTURE_PRIORITY ||\n> -\t\t\t\t\tval.get() == V4L2_EXPOSURE_AUTO) ? true : false;\n> -\t\t\t});\n> -\t\tif (it != v4l2Values.end())\n> -\t\t\tvalues.back() = static_cast(controls::ExposureTimeModeAuto);\n> -\n> -\t\tit = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n> -\t\t\t[&](const ControlValue &val) {\n> -\t\t\t\treturn (val.get() == V4L2_EXPOSURE_SHUTTER_PRIORITY ||\n> -\t\t\t\t\tval.get() == V4L2_EXPOSURE_MANUAL) ? true : false;\n> -\t\t\t});\n> -\t\tif (it != v4l2Values.end())\n> -\t\t\tvalues.back() = static_cast(controls::ExposureTimeModeManual);\n> -\n> -\t\tinfo = ControlInfo{Span{values}, values[0]};\n> +\n> +\t\tstd::bitset<\n> +\t\t\tstd::max(V4L2_EXPOSURE_AUTO,\n> +\t\t\tstd::max(V4L2_EXPOSURE_APERTURE_PRIORITY,\n> +\t\t\tstd::max(V4L2_EXPOSURE_MANUAL,\n> +\t\t\t\t V4L2_EXPOSURE_SHUTTER_PRIORITY))) + 1\n> +\t\t> exposureModes;\n> +\t\tstd::optional lcDef;\n> +\n> +\t\tfor (const ControlValue &value : v4l2Values) {\n> +\t\t\tconst auto x = value.get();\n> +\n> +\t\t\tif (0 <= x && static_cast(x) < exposureModes.size()) {\n\nI know you know I'm not a fan of checking for things that we know\ncan't happen as they part of the kernel/userspace ABI contract.\n\nI know as well you prefer to check for < 0, so up to you\n\n> +\t\t\t\texposureModes[x] = true;\n> +\n> +\t\t\t\tif (x == def)\n> +\t\t\t\t\tlcDef = v4l2ToExposureMode(x);\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (exposureModes[V4L2_EXPOSURE_AUTO])\n> +\t\t\tautoExposureMode_ = V4L2_EXPOSURE_AUTO;\n> +\t\telse if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> +\t\t\tautoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n> +\n> +\t\tif (exposureModes[V4L2_EXPOSURE_MANUAL])\n> +\t\t\tmanualExposureMode_ = V4L2_EXPOSURE_MANUAL;\n> +\t\telse if (exposureModes[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> +\t\t\tmanualExposureMode_ = V4L2_EXPOSURE_SHUTTER_PRIORITY;\n> +\n> +\t\tstd::array values;\n> +\t\tstd::size_t count = 0;\n> +\n> +\t\tif (autoExposureMode_)\n> +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> +\n> +\t\tif (manualExposureMode_)\n> +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> +\n> +\t\tif (count == 0)\n> +\t\t\treturn;\n\nHere as well I don't think count can be 0. If the camera report\nV4L2_CID_EXPOSURE_AUTO it should enumerate at least one value\nassociated with it, otherwise I presume registering the control in the\ndriver won't be possible.\n\n> +\n> +\t\tinfo = ControlInfo{\n> +\t\t\tSpan{ values.data(), count },\n> +\t\t\t!lcDef ? values.front() : *lcDef,\n\nYou could avoid this by doing\n\n if (!lcDef || x == def)\n lcDef = v4l2ToExposureMode(x);\n\nin the above loop maybe\n\nthe rest looks good\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n\n> +\t\t};\n> \t\tbreak;\n> \t}\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> --\n> 2.48.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 C99DFC3274\n\tfor ;\n\tFri, 21 Mar 2025 13:55:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1B4C468947;\n\tFri, 21 Mar 2025 14:55:50 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 211AD68947\n\tfor ;\n\tFri, 21 Mar 2025 14:55:48 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CFC15220;\n\tFri, 21 Mar 2025 14:53:55 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DJX3O31C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742565243;\n\tbh=9iGG4RRzFFHnH4MaqQHNCrHX4k0Gn4NIzpUubKz5hbk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DJX3O31C+b8pa7d68qFPC2IXGKOIow/9br1RCMBXkgt74hx1jWHQHJGNXMlQdSjet\n\tn3+Cm5d/wAu+rT4pnB/IsbTiTBxvqT7+DUug92eHRsEUkSbPqRo2rNTzJasIBX/kZ+\n\tIJnNhuC/QEEj3ND+6f+zgvHZQrA5Ntfcv6yq4zxg=","Date":"Fri, 21 Mar 2025 14:55:29 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 1/4] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setup","Message-ID":"<2z3vqvzvw2kd62kn2saigpkcu7fhgrjftigckq6woymlom2rgh@mxg5kn46pu6z>","References":"<20250314174248.1015718-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174248.1015718-2-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250314174248.1015718-2-barnabas.pocze@ideasonboard.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]