[{"id":33222,"web_url":"https://patchwork.libcamera.org/comment/33222/","msgid":"<5aiawnczlvpvn72ehqj4gotyw5czmn7mmhmewlufryxfab6t4p@ubo25nvrftr4>","date":"2025-01-28T18:17:03","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","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 Tue, Jan 28, 2025 at 12:14:01PM +0000, 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\nSo the two constructors your point our are\n\nControlInfo::ControlInfo(const ControlValue &min,\n\t\t\t const ControlValue &max,\n\t\t\t const ControlValue &def)\n\t: min_(min), max_(max), def_(def)\n{\n}\n\nControlInfo::ControlInfo(const ControlValue &min,\n\t\t\t const ControlValue &max,\n\t\t\t const ControlValue &def)\n\t: min_(min), max_(max), def_(def)\n{\n}\n\nright ?\n\n\nAnd we call them with\n\n\t\tinfo = ControlInfo{Span{values}, values[0]};\n\nwhich, if I got you right gets expanded to\n\nControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max,\n\t\t\t const ControlValue &def)\n\n\nas int32_t are implicitly converted to ControlValue instances.\n\nAs 'values' is declared as\n\n\t\tstd::array values{};\n\nI presume the intermediate call would look like\n\n ControlInfo(int32_t, int32_t, int32_t)\n\nand because of implicit conversion we get to\n\n ControlInfo(const ControlValue &min, const ControlValue &max,\n\t const ControlValue &def)\n\nHow come that, if I do (before this patch)\n\n- info = ControlInfo{Span{values}, values[0]};\n+ info = ControlInfo{Span{values}, values[0], values[0]};\n\nthe code still compiles even if values is of size 2 ?\n\n>\n> To fix this, simply pass a span of `ControlValue` instead.\n>\n> Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> entirely correct because the control value is retrieved as a `bool`\n> instead of - the correct type - `int32_t`. Additionally, the available\n> modes are not taken into account.\n\nPlease split this to a different patch. And in any case, but I have to\nre-check, I think the ControlValidator makes sure the control set by\nthe applications on a camera are supported.\n\n>\n> To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> and select the appropriate mode based on the mapping established\n> in the comment.\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 | 81 +++++++++++++-------\n> 1 file changed, 53 insertions(+), 28 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index dedcac89b..7821cceb0 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,13 @@ public:\n> \tStream stream_;\n> \tstd::map> formats_;\n>\n> +\tstd::bitset +\t\tV4L2_EXPOSURE_AUTO,\n> +\t\tV4L2_EXPOSURE_MANUAL,\n> +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> +\t}) + 1> availableExposureModes_;\n> +\n> private:\n> \tbool generateId();\n>\n> @@ -95,8 +103,8 @@ public:\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> private:\n> -\tint processControl(ControlList *controls, unsigned int id,\n> -\t\t\t const ControlValue &value);\n> +\tint processControl(UVCCameraData *data, ControlList *controls,\n> +\t\t\t unsigned int id, const ControlValue &value);\n> \tint processControls(UVCCameraData *data, Request *request);\n>\n> \tbool acquireDevice(Camera *camera) override;\n> @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> \tdata->video_->releaseBuffers();\n> }\n>\n> -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> -\t\t\t\t const ControlValue &value)\n> +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> +\t\t\t\t unsigned int id, const ControlValue &value)\n> {\n> \tuint32_t cid;\n>\n> @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t}\n>\n> \tcase V4L2_CID_EXPOSURE_AUTO: {\n> -\t\tint32_t ivalue = value.get()\n> -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> +\t\tswitch (value.get()) {\n> +\t\tcase controls::ExposureTimeModeAuto:\n> +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> +\t\t\telse\n> +\t\t\t\tASSERT(false);\n> +\t\t\tbreak;\n> +\t\tcase controls::ExposureTimeModeManual:\n> +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> +\t\t\telse\n> +\t\t\t\tASSERT(false);\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\tASSERT(false);\n> +\t\t\tbreak;\n> +\t\t}\n> \t\tbreak;\n> \t}\n>\n> @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> \tControlList controls(data->video_->controls());\n>\n> \tfor (const auto &[id, value] : request->controls())\n> -\t\tprocessControl(&controls, id, value);\n> +\t\tprocessControl(data, &controls, id, value);\n>\n> \tfor (const auto &ctrl : controls)\n> \t\tLOG(UVC, Debug)\n> @@ -725,25 +750,25 @@ 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> +\t\tfor (const ControlValue &value : v4l2Values) {\n> +\t\t\tauto x = value.get();\n> +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> +\t\t\t\tavailableExposureModes_[x] = true;\n> +\t\t}\n> +\n> +\t\tstd::array values;\n> +\t\tstd::size_t count = 0;\n> +\n> +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> +\n> +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> +\n> +\t\tif (count == 0)\n> +\t\t\treturn;\n> +\n> +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n\nisn't this simpler ?\n\n--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n@@ -723,7 +723,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,\n * V4L2_EXPOSURE_SHUTTER_PRIORITY }\n */\n- std::array values{};\n+ std::array values{};\n\n auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n [&](const ControlValue &val) {\n@@ -731,7 +731,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n val.get() == V4L2_EXPOSURE_AUTO) ? true : false;\n });\n if (it != v4l2Values.end())\n- values.back() = static_cast(controls::ExposureTimeModeAuto);\n+ values.back() = controls::ExposureTimeModeAuto;\n\n it = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n [&](const ControlValue &val) {\n@@ -739,9 +739,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n val.get() == V4L2_EXPOSURE_MANUAL) ? true : false;\n });\n if (it != v4l2Values.end())\n- values.back() = static_cast(controls::ExposureTimeModeManual);\n+ values.back() = controls::ExposureTimeModeManual;\n\n- info = ControlInfo{Span{values}, values[0]};\n+ info = ControlInfo{Span{values}, values[0]};\n break;\n }\n case V4L2_CID_EXPOSURE_ABSOLUTE:\n\n> \t\tbreak;\n> \t}\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> --\n> 2.48.1\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 29F56BDB13\n\tfor ;\n\tTue, 28 Jan 2025 18:17:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6FDC168546;\n\tTue, 28 Jan 2025 19:17:08 +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 87B7668546\n\tfor ;\n\tTue, 28 Jan 2025 19:17:06 +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 2AFFA2A5;\n\tTue, 28 Jan 2025 19:15:59 +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=\"pPmBGLAm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738088159;\n\tbh=XRFr+oIB3Q/37sLQXR8RiYsOAkO6muY4ZFuIYD31mbo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pPmBGLAmhKXBgCgWnYU9j+uwwhKUCdHicb4MggjJx2EscmKA4Fkt66dLPgKzF+Ssd\n\tzzOFmYYSE9Hn68xx5dUWcfnCEGI/hWRlvfwFJh7yHEOSCJMMmmWYAKjRf1jEMpZ8MT\n\t1p4UzDCf3xStZ3DSJj5ADZz5ibPjEz4Hs5zTC+4M=","Date":"Tue, 28 Jan 2025 19:17:03 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"<5aiawnczlvpvn72ehqj4gotyw5czmn7mmhmewlufryxfab6t4p@ubo25nvrftr4>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250128121352.494582-2-pobrn@protonmail.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\" "}},{"id":33223,"web_url":"https://patchwork.libcamera.org/comment/33223/","msgid":"<1jgqGqjlCFjPaMzq4CN-Ne9y8s_dY0_CscVN3dyQB_aQh-ZNbvv5czgbe2_8bVhAiYseWnZPd_XvvAFAJAhQRByyDLqz2tz-T3nyu63PN1g=@protonmail.com>","date":"2025-01-28T19:09:26","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 28., kedd 19:17 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> So the two constructors your point our are\n> \n> ControlInfo::ControlInfo(const ControlValue &min,\n> \t\t\t const ControlValue &max,\n> \t\t\t const ControlValue &def)\n> \t: min_(min), max_(max), def_(def)\n> {\n> }\n> \n> ControlInfo::ControlInfo(const ControlValue &min,\n> \t\t\t const ControlValue &max,\n> \t\t\t const ControlValue &def)\n> \t: min_(min), max_(max), def_(def)\n> {\n> }\n\nI think you meant the following instead?\n\n\tControlInfo::ControlInfo(Span values,\n\t\t\t\t const ControlValue &def)\n\t{ ... }\n\n\n> \n> right ?\n> \n> \n> And we call them with\n> \n> \t\tinfo = ControlInfo{Span{values}, values[0]};\n> \n> which, if I got you right gets expanded to\n> \n> ControlInfo::ControlInfo(const ControlValue &min, const ControlValue &max,\n> \t\t\t const ControlValue &def)\n> \n> \n> as int32_t are implicitly converted to ControlValue instances.\n> \n> As 'values' is declared as\n> \n> \t\tstd::array values{};\n> \n> I presume the intermediate call would look like\n> \n> ControlInfo(int32_t, int32_t, int32_t)\n\nControlInfo(Span, int32_t, int32_t)\n\n\n> \n> and because of implicit conversion we get to\n> \n> ControlInfo(const ControlValue &min, const ControlValue &max,\n> \t const ControlValue &def)\n> \n> How come that, if I do (before this patch)\n> \n> - info = ControlInfo{Span{values}, values[0]};\n> + info = ControlInfo{Span{values}, values[0], values[0]};\n> \n> the code still compiles even if values is of size 2 ?\n\nAs far as I can tell:\n\n * the second and third arguments (`values[0]`) will be converted to `ControlValue`\n using the following constructor:\n\n\ttemplate::value &&\n\t\t\t\t\t details::control_type::value &&\n\t\t\t\t\t !std::is_same>::value,\n\t\t\t\t\t std::nullptr_t> = nullptr>\n\tControlValue(const T &value)\n\t\t: type_(ControlTypeNone), numElements_(0)\n\t{\n\t\tset(details::control_type>::value, false,\n\t\t &value, 1, sizeof(T));\n\t}\n\n\n * the first argument, the span of `const int32_t` will be converted to `ControlValue`\n using the following constructor:\n\n\ttemplate::value ||\n\t\t\t\t\t std::is_same>::value,\n\t\t\t\t\t std::nullptr_t> = nullptr>\n\tControlValue(const T &value)\n\t\t: type_(ControlTypeNone), numElements_(0)\n\t{\n\t\tset(details::control_type>::value, true,\n\t\t value.data(), value.size(), sizeof(typename T::value_type));\n\t}\n\nThe resulting `ControlInfo` is nonetheless of questionable usefulness as the\ntypes of min/max/def will not be the same, but this is probably expected by\nusers.\n\n\n> \n> >\n> > To fix this, simply pass a span of `ControlValue` instead.\n> >\n> > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > entirely correct because the control value is retrieved as a `bool`\n> > instead of - the correct type - `int32_t`. Additionally, the available\n> > modes are not taken into account.\n> \n> Please split this to a different patch. And in any case, but I have to\n> re-check, I think the ControlValidator makes sure the control set by\n> the applications on a camera are supported.\n\nSorry, what I meant is that it was not taken into account which of the\n`V4L2_EXPOSURE_*` modes are supported. The code unconditionally used\n`V4L2_EXPOSURE_{MANUAL,APERTURE_PRIORITY}`. Is there something I am missing?\n\n\n> \n> >\n> > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > and select the appropriate mode based on the mapping established\n> > in the comment.\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 | 81 +++++++++++++-------\n> > 1 file changed, 53 insertions(+), 28 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index dedcac89b..7821cceb0 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,13 @@ public:\n> > \tStream stream_;\n> > \tstd::map> formats_;\n> >\n> > +\tstd::bitset > +\t\tV4L2_EXPOSURE_AUTO,\n> > +\t\tV4L2_EXPOSURE_MANUAL,\n> > +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> > +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > +\t}) + 1> availableExposureModes_;\n> > +\n> > private:\n> > \tbool generateId();\n> >\n> > @@ -95,8 +103,8 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> > private:\n> > -\tint processControl(ControlList *controls, unsigned int id,\n> > -\t\t\t const ControlValue &value);\n> > +\tint processControl(UVCCameraData *data, ControlList *controls,\n> > +\t\t\t unsigned int id, const ControlValue &value);\n> > \tint processControls(UVCCameraData *data, Request *request);\n> >\n> > \tbool acquireDevice(Camera *camera) override;\n> > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > \tdata->video_->releaseBuffers();\n> > }\n> >\n> > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > -\t\t\t\t const ControlValue &value)\n> > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> > +\t\t\t\t unsigned int id, const ControlValue &value)\n> > {\n> > \tuint32_t cid;\n> >\n> > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > \t}\n> >\n> > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > -\t\tint32_t ivalue = value.get()\n> > -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > +\t\tswitch (value.get()) {\n> > +\t\tcase controls::ExposureTimeModeAuto:\n> > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> > +\t\t\telse\n> > +\t\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\tcase controls::ExposureTimeModeManual:\n> > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> > +\t\t\telse\n> > +\t\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > \t\tbreak;\n> > \t}\n> >\n> > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \tControlList controls(data->video_->controls());\n> >\n> > \tfor (const auto &[id, value] : request->controls())\n> > -\t\tprocessControl(&controls, id, value);\n> > +\t\tprocessControl(data, &controls, id, value);\n> >\n> > \tfor (const auto &ctrl : controls)\n> > \t\tLOG(UVC, Debug)\n> > @@ -725,25 +750,25 @@ 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> > +\t\tfor (const ControlValue &value : v4l2Values) {\n> > +\t\t\tauto x = value.get();\n> > +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> > +\t\t\t\tavailableExposureModes_[x] = true;\n> > +\t\t}\n> > +\n> > +\t\tstd::array values;\n> > +\t\tstd::size_t count = 0;\n> > +\n> > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> > +\n> > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> > +\n> > +\t\tif (count == 0)\n> > +\t\t\treturn;\n> > +\n> > +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n> \n> isn't this simpler ?\n> \n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -723,7 +723,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,\n> * V4L2_EXPOSURE_SHUTTER_PRIORITY }\n> */\n> - std::array values{};\n> + std::array values{};\n> \n> auto it = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n> [&](const ControlValue &val) {\n> @@ -731,7 +731,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> val.get() == V4L2_EXPOSURE_AUTO) ? true : false;\n> });\n> if (it != v4l2Values.end())\n> - values.back() = static_cast(controls::ExposureTimeModeAuto);\n> + values.back() = controls::ExposureTimeModeAuto;\n> \n> it = std::find_if(v4l2Values.begin(), v4l2Values.end(),\n> [&](const ControlValue &val) {\n> @@ -739,9 +739,9 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> val.get() == V4L2_EXPOSURE_MANUAL) ? true : false;\n> });\n> if (it != v4l2Values.end())\n> - values.back() = static_cast(controls::ExposureTimeModeManual);\n> + values.back() = controls::ExposureTimeModeManual;\n> \n> - info = ControlInfo{Span{values}, values[0]};\n> + info = ControlInfo{Span{values}, values[0]};\n> break;\n> }\n> case V4L2_CID_EXPOSURE_ABSOLUTE:\n> \n\nSee my comment above, the `availableExposureModes_` is needed later in `processControl()`\nto determine which of the `V4L2_EXPOSURE_*` value to use. So this approach\nseemed simplest since the `availableExposureModes_` bitset has to be calculated\neither way. Also, the above change always sets `values[1]`.\n\n\n> > \t\tbreak;\n> > \t}\n> > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > --\n> > 2.48.1\n> >\n> >\n> \n\n\nRegards,\nBarnabás Pőcze","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 607B3BD87C\n\tfor ;\n\tTue, 28 Jan 2025 19:09:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 44A3C6855D;\n\tTue, 28 Jan 2025 20:09:34 +0100 (CET)","from mail-10629.protonmail.ch (mail-10629.protonmail.ch\n\t[79.135.106.29])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5526568546\n\tfor ;\n\tTue, 28 Jan 2025 20:09:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"d1HWF3tp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1738091371; x=1738350571;\n\tbh=w53knga3SuG6Y6N7DP307yWFZh+6mLJSQl6bvKl9yOU=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=d1HWF3tpaVbE6GncbP/BeTBh1HGHPlw8esqQgMLqXcoi/if6vXX/Qrb2vtx3f7mS4\n\tZmt6e+fTb5NpW2B9zan8nos7q0SLClf3v8rDkuJudhTDPS1mbUj3tY+kSOmoz+LDwX\n\tVJ2Xhz6DCOpCbZcygqfjX+R6jh9m8MdS7P6SQIn0I6BETwJqi2CbSrneQdOz89yrtd\n\tCzk4E+W6Ji4Jkp+7EuafnICK53/jC21hbRIVlEt8l8Y0kR3O0Jo5XFAaxlUdXZkZ2Z\n\tkQ3F7T9i+sm5VImSUp5NynkTAZTUL5YHXprBdx/B2lbbFCRMTsx5yYqCejwxOXzSpW\n\tZOvNyv0z5GQ4w==","Date":"Tue, 28 Jan 2025 19:09:26 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"<1jgqGqjlCFjPaMzq4CN-Ne9y8s_dY0_CscVN3dyQB_aQh-ZNbvv5czgbe2_8bVhAiYseWnZPd_XvvAFAJAhQRByyDLqz2tz-T3nyu63PN1g=@protonmail.com>","In-Reply-To":"<5aiawnczlvpvn72ehqj4gotyw5czmn7mmhmewlufryxfab6t4p@ubo25nvrftr4>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t<5aiawnczlvpvn72ehqj4gotyw5czmn7mmhmewlufryxfab6t4p@ubo25nvrftr4>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"73504a5a42ce153d4457bdf84274520a8729911a","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}},{"id":33226,"web_url":"https://patchwork.libcamera.org/comment/33226/","msgid":"<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>","date":"2025-01-29T12:13:12","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> > To fix this, simply pass a span of `ControlValue` instead.\n> >\n> > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > entirely correct because the control value is retrieved as a `bool`\n> > instead of - the correct type - `int32_t`. Additionally, the available\n> > modes are not taken into account.\n> >\n> > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > and select the appropriate mode based on the mapping established\n> > in the comment.\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 | 81 +++++++++++++-------\n> > 1 file changed, 53 insertions(+), 28 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index dedcac89b..7821cceb0 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,13 @@ public:\n> > \tStream stream_;\n> > \tstd::map> formats_;\n> >\n> > +\tstd::bitset > +\t\tV4L2_EXPOSURE_AUTO,\n> > +\t\tV4L2_EXPOSURE_MANUAL,\n> > +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> > +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > +\t}) + 1> availableExposureModes_;\n> > +\n> \n> I presume a bitset is used for efficiency ?\n\nI suppose you could say that. I thought an `std::set` was a bit of an overkill.\n\n\n> \n> \n> > private:\n> > \tbool generateId();\n> >\n> > @@ -95,8 +103,8 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> > private:\n> > -\tint processControl(ControlList *controls, unsigned int id,\n> > -\t\t\t const ControlValue &value);\n> > +\tint processControl(UVCCameraData *data, ControlList *controls,\n> > +\t\t\t unsigned int id, const ControlValue &value);\n> > \tint processControls(UVCCameraData *data, Request *request);\n> >\n> > \tbool acquireDevice(Camera *camera) override;\n> > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > \tdata->video_->releaseBuffers();\n> > }\n> >\n> > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > -\t\t\t\t const ControlValue &value)\n> > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> > +\t\t\t\t unsigned int id, const ControlValue &value)\n> > {\n> > \tuint32_t cid;\n> >\n> > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > \t}\n> >\n> > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > -\t\tint32_t ivalue = value.get()\n> > -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > +\t\tswitch (value.get()) {\n> > +\t\tcase controls::ExposureTimeModeAuto:\n> > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> > +\t\t\telse\n> > +\t\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\tcase controls::ExposureTimeModeManual:\n> > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> > +\t\t\telse\n> > +\t\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\tdefault:\n> > +\t\t\tASSERT(false);\n> > +\t\t\tbreak;\n> > +\t\t}\n> > \t\tbreak;\n> > \t}\n> >\n> > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > \tControlList controls(data->video_->controls());\n> >\n> > \tfor (const auto &[id, value] : request->controls())\n> > -\t\tprocessControl(&controls, id, value);\n> > +\t\tprocessControl(data, &controls, id, value);\n> >\n> > \tfor (const auto &ctrl : controls)\n> > \t\tLOG(UVC, Debug)\n> > @@ -725,25 +750,25 @@ 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> > +\t\tfor (const ControlValue &value : v4l2Values) {\n> > +\t\t\tauto x = value.get();\n> > +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> \n> Can x be negative ? It should come from enum v4l2_exposure_auto_type,\n> doesn't it ?\n\nYes, it should, but I don't know if it will ever be extended or similar, so\ndoing the bounds checking seemed the most logical decision.\n\n\n> \n> Also, isn't size_t(x) == 4, why do you need to check it against the\n> bitset size ?\n\nI can't see why `x` would be 4. But `availableExposureModes_.size()` is 4.\n\n\n> \n> > +\t\t\t\tavailableExposureModes_[x] = true;\n> > +\t\t}\n> > +\n> > +\t\tstd::array values;\n> > +\t\tstd::size_t count = 0;\n> > +\n> > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> > +\n> > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> > +\n> > +\t\tif (count == 0)\n> > +\t\t\treturn;\n> > +\n> > +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n> > \t\tbreak;\n> > \t}\n> > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > --\n> > 2.48.1\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 8643DC32C2\n\tfor ;\n\tWed, 29 Jan 2025 12:13:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D9DEB6855C;\n\tWed, 29 Jan 2025 13:13:19 +0100 (CET)","from mail-10630.protonmail.ch (mail-10630.protonmail.ch\n\t[79.135.106.30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 19A486851B\n\tfor ;\n\tWed, 29 Jan 2025 13:13:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"T4C9euWF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1738152798; x=1738411998;\n\tbh=+/XV6bkfqMV2PEXcMddy+hijyLhMs9u7QnMe2laMUl8=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=T4C9euWFB154r0tm3QC9DsZ9roVjPAfvX01CcPLIzp0NfX7Q9rYz+vjD8i/mtbgpO\n\tHecWR5HbVD9cvzbUQDwH55pF0DjoD4yW8bJOTuxE3picJF/oy+CpX+KLNIXPoKc0QX\n\twf31nZz/89tnajOHysiYge8mWFj1LqR9zHbH2BhQ992PI1hXo68BigtpLB2sp9mIBB\n\thqukbqpGSvvh5Hx9QpXATos95NUNK36Al1+o5gR5IdM0L4Geda0RObzB/He0PDdo7j\n\tMCNLpuMTR0v/NKlhnbdNrnZ8YidoBzPV0f2nyw+KEcC2CKMYmeJPoYKv6gXgzIdzc9\n\t/dPrQBEc92qOw==","Date":"Wed, 29 Jan 2025 12:13:12 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>","In-Reply-To":"","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"31024298aec9a6753eaa180243c4cf88f0c45eeb","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}},{"id":33227,"web_url":"https://patchwork.libcamera.org/comment/33227/","msgid":"<173815349370.1594000.13684913872421558631@ping.linuxembedded.co.uk>","date":"2025-01-29T12:24:53","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Barnabás Pőcze (2025-01-29 12:13:12)\n> Hi\n> \n> \n> 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi írta:\n> \n> > Hi Barnabás\n> > \n> > On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> > > To fix this, simply pass a span of `ControlValue` instead.\n> > >\n> > > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > > entirely correct because the control value is retrieved as a `bool`\n> > > instead of - the correct type - `int32_t`. Additionally, the available\n> > > modes are not taken into account.\n> > >\n> > > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > > and select the appropriate mode based on the mapping established\n> > > in the comment.\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 | 81 +++++++++++++-------\n> > > 1 file changed, 53 insertions(+), 28 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index dedcac89b..7821cceb0 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,13 @@ public:\n> > > Stream stream_;\n> > > std::map> formats_;\n> > >\n> > > + std::bitset > > + V4L2_EXPOSURE_AUTO,\n> > > + V4L2_EXPOSURE_MANUAL,\n> > > + V4L2_EXPOSURE_APERTURE_PRIORITY,\n> > > + V4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > > + }) + 1> availableExposureModes_;\n> > > +\n> > \n> > I presume a bitset is used for efficiency ?\n> \n> I suppose you could say that. I thought an `std::set` was a bit of an overkill.\n\nIt looks like GCC-9 has some issues around here:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/70104933\n\n--\nKieran","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 C9285BD808\n\tfor ;\n\tWed, 29 Jan 2025 12:24:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4F4FE68557;\n\tWed, 29 Jan 2025 13:24:58 +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 A13736851B\n\tfor ;\n\tWed, 29 Jan 2025 13:24:56 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B7C7655;\n\tWed, 29 Jan 2025 13:23:48 +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=\"mkFwHk4t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738153428;\n\tbh=i7K0fumvWDOEIK0Kff127PVDVTlWsmCehP0CcPp5UIM=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=mkFwHk4tlngbjBHEuZ7CytuxHKvxcyPXGtsSJGAsoJVlV4TdfSW1wJWWS4bpbz3dV\n\to7pspYUaAhlT70IZKepJUUsjrdc730q89DUVAQj3WTFJvaHjiKyqcUeHZlG25k2AuH\n\t86pSV0Z7ocCTXTzCRfjdPbQxFHi1r6qCwD/r79h8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t\n\t<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ,\n\tJacopo Mondi ","Date":"Wed, 29 Jan 2025 12:24:53 +0000","Message-ID":"<173815349370.1594000.13684913872421558631@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}},{"id":33229,"web_url":"https://patchwork.libcamera.org/comment/33229/","msgid":"","date":"2025-01-29T14:09:36","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","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 Wed, Jan 29, 2025 at 12:13:12PM +0000, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> > > To fix this, simply pass a span of `ControlValue` instead.\n> > >\n> > > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > > entirely correct because the control value is retrieved as a `bool`\n> > > instead of - the correct type - `int32_t`. Additionally, the available\n> > > modes are not taken into account.\n> > >\n> > > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > > and select the appropriate mode based on the mapping established\n> > > in the comment.\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 | 81 +++++++++++++-------\n> > > 1 file changed, 53 insertions(+), 28 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index dedcac89b..7821cceb0 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,13 @@ public:\n> > > \tStream stream_;\n> > > \tstd::map> formats_;\n> > >\n> > > +\tstd::bitset > > +\t\tV4L2_EXPOSURE_AUTO,\n> > > +\t\tV4L2_EXPOSURE_MANUAL,\n> > > +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> > > +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > > +\t}) + 1> availableExposureModes_;\n> > > +\n> >\n> > I presume a bitset is used for efficiency ?\n>\n> I suppose you could say that. I thought an `std::set` was a bit of an overkill.\n>\n>\n> >\n> >\n> > > private:\n> > > \tbool generateId();\n> > >\n> > > @@ -95,8 +103,8 @@ public:\n> > > \tbool match(DeviceEnumerator *enumerator) override;\n> > >\n> > > private:\n> > > -\tint processControl(ControlList *controls, unsigned int id,\n> > > -\t\t\t const ControlValue &value);\n> > > +\tint processControl(UVCCameraData *data, ControlList *controls,\n> > > +\t\t\t unsigned int id, const ControlValue &value);\n> > > \tint processControls(UVCCameraData *data, Request *request);\n> > >\n> > > \tbool acquireDevice(Camera *camera) override;\n> > > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > > \tdata->video_->releaseBuffers();\n> > > }\n> > >\n> > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > -\t\t\t\t const ControlValue &value)\n> > > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> > > +\t\t\t\t unsigned int id, const ControlValue &value)\n> > > {\n> > > \tuint32_t cid;\n> > >\n> > > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > \t}\n> > >\n> > > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > > -\t\tint32_t ivalue = value.get()\n> > > -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > > -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > > -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > > +\t\tswitch (value.get()) {\n> > > +\t\tcase controls::ExposureTimeModeAuto:\n> > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> > > +\t\t\telse\n> > > +\t\t\t\tASSERT(false);\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::ExposureTimeModeManual:\n> > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> > > +\t\t\telse\n> > > +\t\t\t\tASSERT(false);\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\tASSERT(false);\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > \t\tbreak;\n> > > \t}\n> > >\n> > > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > > \tControlList controls(data->video_->controls());\n> > >\n> > > \tfor (const auto &[id, value] : request->controls())\n> > > -\t\tprocessControl(&controls, id, value);\n> > > +\t\tprocessControl(data, &controls, id, value);\n> > >\n> > > \tfor (const auto &ctrl : controls)\n> > > \t\tLOG(UVC, Debug)\n> > > @@ -725,25 +750,25 @@ 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> > > +\t\tfor (const ControlValue &value : v4l2Values) {\n> > > +\t\t\tauto x = value.get();\n> > > +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> >\n> > Can x be negative ? It should come from enum v4l2_exposure_auto_type,\n> > doesn't it ?\n>\n> Yes, it should, but I don't know if it will ever be extended or similar, so\n> doing the bounds checking seemed the most logical decision.\n\nWell, one of the assumptions about Linux is that we don't break\nuserspace, so even if new values can be added to the possible control\nvalues, I don't expect the existing ones to be changed or any new\nnegative value to be added there.\n\n>\n>\n> >\n> > Also, isn't size_t(x) == 4, why do you need to check it against the\n> > bitset size ?\n>\n> I can't see why `x` would be 4. But `availableExposureModes_.size()` is 4.\n\nBecause I don't know what std::size_t() does.\n\nTo me it's the same size_t type as in the one in C (but defined in the\nstd namespace by (which you should probably include)).\nhttps://en.cppreference.com/w/cpp/types/size_t\n\nand I was expecting be size_t(int32_t) == sizeof(int32_t).\n\nWhen I instead apply it to a variable it always returns me the\nvalue of the variable.\n\n\tint32_t a = atoi(argv[1]);\n\tstd::cout << \"size_t(int32_t) = \" << std::size_t(a) << std::endl;\n\n $ ./a.out 5\n size_t(int32_t) = 5\n $ ./a.out 10\n size_t(int32_t) = 10\n $ ./a.out 166\n size_t(int32_t) = 166\n\nWhat the heck is this for ?\n\n>\n>\n> >\n> > > +\t\t\t\tavailableExposureModes_[x] = true;\n> > > +\t\t}\n> > > +\n> > > +\t\tstd::array values;\n> > > +\t\tstd::size_t count = 0;\n> > > +\n> > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> > > +\n> > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> > > +\n> > > +\t\tif (count == 0)\n> > > +\t\t\treturn;\n> > > +\n> > > +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n> > > \t\tbreak;\n> > > \t}\n> > > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > > --\n> > > 2.48.1\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 2C2ACBD808\n\tfor ;\n\tWed, 29 Jan 2025 14:09:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5A1C16855C;\n\tWed, 29 Jan 2025 15:09:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 812266851B\n\tfor ;\n\tWed, 29 Jan 2025 15:09:40 +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 5DF38D21;\n\tWed, 29 Jan 2025 15:08:32 +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=\"CAqppM9O\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738159712;\n\tbh=86oA3PGalZW/CroxkEHp8icqjFnN38NDyMaWLT+0rq4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=CAqppM9OmUeeXQSM5V7mOUS1smSUyQzPVbKTfv7s58JWSmmajtRZAS9sPE5QI69Po\n\tuaUsOH6tFS/CG6LyKNxRx9/B+OxMtv2LXmF1bKUVAI9AxQaH/lyuPM+kN7wKzzLosb\n\tia9seYsF3BUS15S9xs5g/7aWUPUCNuJKaZpk+8Xg=","Date":"Wed, 29 Jan 2025 15:09:36 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t\n\t<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.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\" "}},{"id":33230,"web_url":"https://patchwork.libcamera.org/comment/33230/","msgid":"","date":"2025-01-29T14:59:20","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"2025. január 29., szerda 15:09 keltezéssel, Jacopo Mondi írta:\n\n> Hi Barnabás\n> \n> On Wed, Jan 29, 2025 at 12:13:12PM +0000, Barnabás Pőcze wrote:\n> > Hi\n> >\n> >\n> > 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi írta:\n> >\n> > > Hi Barnabás\n> > >\n> > > On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> > > > To fix this, simply pass a span of `ControlValue` instead.\n> > > >\n> > > > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > > > entirely correct because the control value is retrieved as a `bool`\n> > > > instead of - the correct type - `int32_t`. Additionally, the available\n> > > > modes are not taken into account.\n> > > >\n> > > > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > > > and select the appropriate mode based on the mapping established\n> > > > in the comment.\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 | 81 +++++++++++++-------\n> > > > 1 file changed, 53 insertions(+), 28 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > index dedcac89b..7821cceb0 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,13 @@ public:\n> > > > \tStream stream_;\n> > > > \tstd::map> formats_;\n> > > >\n> > > > +\tstd::bitset > > > +\t\tV4L2_EXPOSURE_AUTO,\n> > > > +\t\tV4L2_EXPOSURE_MANUAL,\n> > > > +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> > > > +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > > > +\t}) + 1> availableExposureModes_;\n> > > > +\n> > >\n> > > I presume a bitset is used for efficiency ?\n> >\n> > I suppose you could say that. I thought an `std::set` was a bit of an overkill.\n> >\n> >\n> > >\n> > >\n> > > > private:\n> > > > \tbool generateId();\n> > > >\n> > > > @@ -95,8 +103,8 @@ public:\n> > > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > >\n> > > > private:\n> > > > -\tint processControl(ControlList *controls, unsigned int id,\n> > > > -\t\t\t const ControlValue &value);\n> > > > +\tint processControl(UVCCameraData *data, ControlList *controls,\n> > > > +\t\t\t unsigned int id, const ControlValue &value);\n> > > > \tint processControls(UVCCameraData *data, Request *request);\n> > > >\n> > > > \tbool acquireDevice(Camera *camera) override;\n> > > > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > > > \tdata->video_->releaseBuffers();\n> > > > }\n> > > >\n> > > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > > -\t\t\t\t const ControlValue &value)\n> > > > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> > > > +\t\t\t\t unsigned int id, const ControlValue &value)\n> > > > {\n> > > > \tuint32_t cid;\n> > > >\n> > > > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > > \t}\n> > > >\n> > > > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > > > -\t\tint32_t ivalue = value.get()\n> > > > -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > > > -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > > > -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > > > +\t\tswitch (value.get()) {\n> > > > +\t\tcase controls::ExposureTimeModeAuto:\n> > > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> > > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> > > > +\t\t\telse\n> > > > +\t\t\t\tASSERT(false);\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase controls::ExposureTimeModeManual:\n> > > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> > > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> > > > +\t\t\telse\n> > > > +\t\t\t\tASSERT(false);\n> > > > +\t\t\tbreak;\n> > > > +\t\tdefault:\n> > > > +\t\t\tASSERT(false);\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > \t\tbreak;\n> > > > \t}\n> > > >\n> > > > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > > > \tControlList controls(data->video_->controls());\n> > > >\n> > > > \tfor (const auto &[id, value] : request->controls())\n> > > > -\t\tprocessControl(&controls, id, value);\n> > > > +\t\tprocessControl(data, &controls, id, value);\n> > > >\n> > > > \tfor (const auto &ctrl : controls)\n> > > > \t\tLOG(UVC, Debug)\n> > > > @@ -725,25 +750,25 @@ 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> > > > +\t\tfor (const ControlValue &value : v4l2Values) {\n> > > > +\t\t\tauto x = value.get();\n> > > > +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> > >\n> > > Can x be negative ? It should come from enum v4l2_exposure_auto_type,\n> > > doesn't it ?\n> >\n> > Yes, it should, but I don't know if it will ever be extended or similar, so\n> > doing the bounds checking seemed the most logical decision.\n> \n> Well, one of the assumptions about Linux is that we don't break\n> userspace, so even if new values can be added to the possible control\n> values, I don't expect the existing ones to be changed or any new\n> negative value to be added there.\n\nThat is my thinking, but keeping the check does not hurt in my opinion.\n\n\n> \n> >\n> >\n> > >\n> > > Also, isn't size_t(x) == 4, why do you need to check it against the\n> > > bitset size ?\n> >\n> > I can't see why `x` would be 4. But `availableExposureModes_.size()` is 4.\n> \n> Because I don't know what std::size_t() does.\n> \n> To me it's the same size_t type as in the one in C (but defined in the\n> std namespace by (which you should probably include)).\n> https://en.cppreference.com/w/cpp/types/size_t\n> \n> and I was expecting be size_t(int32_t) == sizeof(int32_t).\n> \n> When I instead apply it to a variable it always returns me the\n> value of the variable.\n> \n> \tint32_t a = atoi(argv[1]);\n> \tstd::cout << \"size_t(int32_t) = \" << std::size_t(a) << std::endl;\n> \n> $ ./a.out 5\n> size_t(int32_t) = 5\n> $ ./a.out 10\n> size_t(int32_t) = 10\n> $ ./a.out 166\n> size_t(int32_t) = 166\n> \n> What the heck is this for ?\n\n`size_t(x)` just casts `x` to `size_t`. Otherwise one encounters a `-Wsign-compare`\nerror because `x` is signed but `availableExposureModes_.size()` is not.\n\nhttps://en.cppreference.com/w/cpp/language/explicit_cast\n\n\n> \n> >\n> >\n> > >\n> > > > +\t\t\t\tavailableExposureModes_[x] = true;\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tstd::array values;\n> > > > +\t\tstd::size_t count = 0;\n> > > > +\n> > > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > > +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> > > > +\n> > > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > > +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> > > > +\n> > > > +\t\tif (count == 0)\n> > > > +\t\t\treturn;\n> > > > +\n> > > > +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n> > > > \t\tbreak;\n> > > > \t}\n> > > > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > > > --\n> > > > 2.48.1\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 BF134C32C2\n\tfor ;\n\tWed, 29 Jan 2025 14:59:29 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AC2046855C;\n\tWed, 29 Jan 2025 15:59:28 +0100 (CET)","from mail-10628.protonmail.ch (mail-10628.protonmail.ch\n\t[79.135.106.28])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B8696851B\n\tfor ;\n\tWed, 29 Jan 2025 15:59:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"Iz2o3tH0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1738162766; x=1738421966;\n\tbh=hp64+ZwvMFNWoZrFLSqDnV94m+2imZN0bvnnb4LAHk0=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector:List-Unsubscribe:List-Unsubscribe-Post;\n\tb=Iz2o3tH0vJOmqgqMnK3PYxy6Ml+nxj051Mpsg2XRYzjiHPDT3sf9rb9HqVqE+pu4k\n\tyv5sBYW90sDvrYiNH4Af2JUWG8JbQn/Ul0VKie2nYsflaNJJ8vqCtBR/3nKlkT2i5k\n\tsYUhNGBCcjZi9TWJgoi1+VgblRikZQSqB6OfyLW/jLSFdYeEe8TPDrXw1AjuIvy9wP\n\tqgdaR87txWsFgHFiVA4otEVqzYUBdg43F+HsJwNCSHYTgdaFtilcUhy5t+2DuMT9nG\n\toksVQmNTow7XFRYCyWFyU0IoZViryjx8U3T4hpo+F174XAXyYBiBtrSaS6+lSuoj9x\n\t4t+rbGi68pczQ==","Date":"Wed, 29 Jan 2025 14:59:20 +0000","To":"Jacopo Mondi ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"","In-Reply-To":"","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t\n\t<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>\n\t","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"97ba0af265e2568eef3fb4b435e2ac4f0ad89415","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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\" "}},{"id":33231,"web_url":"https://patchwork.libcamera.org/comment/33231/","msgid":"","date":"2025-01-29T15:21:35","subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi\n\nOn Wed, Jan 29, 2025 at 02:59:20PM +0000, Barnabás Pőcze wrote:\n> 2025. január 29., szerda 15:09 keltezéssel, Jacopo Mondi írta:\n>\n> > Hi Barnabás\n> >\n> > On Wed, Jan 29, 2025 at 12:13:12PM +0000, Barnabás Pőcze wrote:\n> > > Hi\n> > >\n> > >\n> > > 2025. január 29., szerda 10:22 keltezéssel, Jacopo Mondi írta:\n> > >\n> > > > Hi Barnabás\n> > > >\n> > > > On Tue, Jan 28, 2025 at 12:14:01PM +0000, 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> > > > > To fix this, simply pass a span of `ControlValue` instead.\n> > > > >\n> > > > > Furthermore, the mapping in `UVCCameraData::processControl()` is also not\n> > > > > entirely correct because the control value is retrieved as a `bool`\n> > > > > instead of - the correct type - `int32_t`. Additionally, the available\n> > > > > modes are not taken into account.\n> > > > >\n> > > > > To fix this, stores the available modes for `V4L2_CID_EXPOSURE_AUTO`\n> > > > > and select the appropriate mode based on the mapping established\n> > > > > in the comment.\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 | 81 +++++++++++++-------\n> > > > > 1 file changed, 53 insertions(+), 28 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > > > index dedcac89b..7821cceb0 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,13 @@ public:\n> > > > > \tStream stream_;\n> > > > > \tstd::map> formats_;\n> > > > >\n> > > > > +\tstd::bitset > > > > +\t\tV4L2_EXPOSURE_AUTO,\n> > > > > +\t\tV4L2_EXPOSURE_MANUAL,\n> > > > > +\t\tV4L2_EXPOSURE_APERTURE_PRIORITY,\n> > > > > +\t\tV4L2_EXPOSURE_SHUTTER_PRIORITY,\n> > > > > +\t}) + 1> availableExposureModes_;\n> > > > > +\n> > > >\n> > > > I presume a bitset is used for efficiency ?\n> > >\n> > > I suppose you could say that. I thought an `std::set` was a bit of an overkill.\n> > >\n> > >\n> > > >\n> > > >\n> > > > > private:\n> > > > > \tbool generateId();\n> > > > >\n> > > > > @@ -95,8 +103,8 @@ public:\n> > > > > \tbool match(DeviceEnumerator *enumerator) override;\n> > > > >\n> > > > > private:\n> > > > > -\tint processControl(ControlList *controls, unsigned int id,\n> > > > > -\t\t\t const ControlValue &value);\n> > > > > +\tint processControl(UVCCameraData *data, ControlList *controls,\n> > > > > +\t\t\t unsigned int id, const ControlValue &value);\n> > > > > \tint processControls(UVCCameraData *data, Request *request);\n> > > > >\n> > > > > \tbool acquireDevice(Camera *camera) override;\n> > > > > @@ -289,8 +297,8 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > > > > \tdata->video_->releaseBuffers();\n> > > > > }\n> > > > >\n> > > > > -int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > > > -\t\t\t\t const ControlValue &value)\n> > > > > +int PipelineHandlerUVC::processControl(UVCCameraData *data, ControlList *controls,\n> > > > > +\t\t\t\t unsigned int id, const ControlValue &value)\n> > > > > {\n> > > > > \tuint32_t cid;\n> > > > >\n> > > > > @@ -334,10 +342,27 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> > > > > \t}\n> > > > >\n> > > > > \tcase V4L2_CID_EXPOSURE_AUTO: {\n> > > > > -\t\tint32_t ivalue = value.get()\n> > > > > -\t\t\t ? V4L2_EXPOSURE_APERTURE_PRIORITY\n> > > > > -\t\t\t : V4L2_EXPOSURE_MANUAL;\n> > > > > -\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, ivalue);\n> > > > > +\t\tswitch (value.get()) {\n> > > > > +\t\tcase controls::ExposureTimeModeAuto:\n> > > > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_AUTO])\n> > > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_AUTO);\n> > > > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_APERTURE_PRIORITY);\n> > > > > +\t\t\telse\n> > > > > +\t\t\t\tASSERT(false);\n> > > > > +\t\t\tbreak;\n> > > > > +\t\tcase controls::ExposureTimeModeManual:\n> > > > > +\t\t\tif (data->availableExposureModes_[V4L2_EXPOSURE_MANUAL])\n> > > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_MANUAL);\n> > > > > +\t\t\telse if (data->availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > > > +\t\t\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, V4L2_EXPOSURE_SHUTTER_PRIORITY);\n> > > > > +\t\t\telse\n> > > > > +\t\t\t\tASSERT(false);\n> > > > > +\t\t\tbreak;\n> > > > > +\t\tdefault:\n> > > > > +\t\t\tASSERT(false);\n> > > > > +\t\t\tbreak;\n> > > > > +\t\t}\n> > > > > \t\tbreak;\n> > > > > \t}\n> > > > >\n> > > > > @@ -375,7 +400,7 @@ int PipelineHandlerUVC::processControls(UVCCameraData *data, Request *request)\n> > > > > \tControlList controls(data->video_->controls());\n> > > > >\n> > > > > \tfor (const auto &[id, value] : request->controls())\n> > > > > -\t\tprocessControl(&controls, id, value);\n> > > > > +\t\tprocessControl(data, &controls, id, value);\n> > > > >\n> > > > > \tfor (const auto &ctrl : controls)\n> > > > > \t\tLOG(UVC, Debug)\n> > > > > @@ -725,25 +750,25 @@ 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> > > > > +\t\tfor (const ControlValue &value : v4l2Values) {\n> > > > > +\t\t\tauto x = value.get();\n> > > > > +\t\t\tif (0 <= x && size_t(x) < availableExposureModes_.size())\n> > > >\n> > > > Can x be negative ? It should come from enum v4l2_exposure_auto_type,\n> > > > doesn't it ?\n> > >\n> > > Yes, it should, but I don't know if it will ever be extended or similar, so\n> > > doing the bounds checking seemed the most logical decision.\n> >\n> > Well, one of the assumptions about Linux is that we don't break\n> > userspace, so even if new values can be added to the possible control\n> > values, I don't expect the existing ones to be changed or any new\n> > negative value to be added there.\n>\n> That is my thinking, but keeping the check does not hurt in my opinion.\n>\n>\n> >\n> > >\n> > >\n> > > >\n> > > > Also, isn't size_t(x) == 4, why do you need to check it against the\n> > > > bitset size ?\n> > >\n> > > I can't see why `x` would be 4. But `availableExposureModes_.size()` is 4.\n> >\n> > Because I don't know what std::size_t() does.\n> >\n> > To me it's the same size_t type as in the one in C (but defined in the\n> > std namespace by (which you should probably include)).\n> > https://en.cppreference.com/w/cpp/types/size_t\n> >\n> > and I was expecting be size_t(int32_t) == sizeof(int32_t).\n> >\n> > When I instead apply it to a variable it always returns me the\n> > value of the variable.\n> >\n> > \tint32_t a = atoi(argv[1]);\n> > \tstd::cout << \"size_t(int32_t) = \" << std::size_t(a) << std::endl;\n> >\n> > $ ./a.out 5\n> > size_t(int32_t) = 5\n> > $ ./a.out 10\n> > size_t(int32_t) = 10\n> > $ ./a.out 166\n> > size_t(int32_t) = 166\n> >\n> > What the heck is this for ?\n>\n> `size_t(x)` just casts `x` to `size_t`. Otherwise one encounters a `-Wsign-compare`\n> error because `x` is signed but `availableExposureModes_.size()` is not.\n>\n> https://en.cppreference.com/w/cpp/language/explicit_cast\n>\n\nahah, ok, this was rather stupid :)\n\nI'll take the occasion to remind you we try to avoid C-style casts in\nthe code base ;)\n\n>\n> >\n> > >\n> > >\n> > > >\n> > > > > +\t\t\t\tavailableExposureModes_[x] = true;\n> > > > > +\t\t}\n> > > > > +\n> > > > > +\t\tstd::array values;\n> > > > > +\t\tstd::size_t count = 0;\n> > > > > +\n> > > > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_AUTO] || availableExposureModes_[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > > > > +\t\t\tvalues[count++] = controls::ExposureTimeModeAuto;\n> > > > > +\n> > > > > +\t\tif (availableExposureModes_[V4L2_EXPOSURE_MANUAL] || availableExposureModes_[V4L2_EXPOSURE_SHUTTER_PRIORITY])\n> > > > > +\t\t\tvalues[count++] = controls::ExposureTimeModeManual;\n> > > > > +\n> > > > > +\t\tif (count == 0)\n> > > > > +\t\t\treturn;\n> > > > > +\n> > > > > +\t\tinfo = ControlInfo{Span{ values.data(), count }, values[0]};\n> > > > > \t\tbreak;\n> > > > > \t}\n> > > > > \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> > > > > --\n> > > > > 2.48.1\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 D25D9BD808\n\tfor ;\n\tWed, 29 Jan 2025 15:21:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DE7316855C;\n\tWed, 29 Jan 2025 16:21: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 47DB86851B\n\tfor ;\n\tWed, 29 Jan 2025 16:21:39 +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 1BDA31E6;\n\tWed, 29 Jan 2025 16:20:31 +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=\"cG+guAs6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738164031;\n\tbh=77fhlzAeYIEZ8w9DLWjXVM0N79iIKgIXtuAqmxa5O90=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=cG+guAs68gpW8NZTetXCDTNZTVa3RzHPtVn9OiHecgF7VBGyWdmHqGEPI4w9TWt3i\n\tqtHkqS0c5iq9s984ZBrxWjZwiNTBnCt9kqoJrrqa0y+XQaGnGTxvi0MkdaKXK29JYe\n\tFSHdoMYASkX7sgqYwnXsVwOvja31CXoDM2cgk++g=","Date":"Wed, 29 Jan 2025 16:21:35 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control","Message-ID":"","References":"<20250128121352.494582-1-pobrn@protonmail.com>\n\t<20250128121352.494582-2-pobrn@protonmail.com>\n\t\n\t<8y8Nn9GikK5ew5-eD4cOP3jiP8q7x75GXJNs_VCsQ_dOHF0-BC1PILVXXu6Qd5PXfA8qfPz5xKFDsRekpV1yk6OYmEHeyg83gyQm017oLXY=@protonmail.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]