[{"id":33566,"web_url":"https://patchwork.libcamera.org/comment/33566/","msgid":"<6juw4q5bfgrgfogomwbhrgyl7bopd2ieex7pttdqkkgc5nm5kf@jta34hkh5v4s>","date":"2025-03-04T10:09:12","subject":"Re: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","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 Mon, Mar 03, 2025 at 02:42:34PM +0100, Barnabás Pőcze wrote:\n> The mapping in `UVCCameraData::processControl()` is not entirely correct\n> because the control value is retrieved as a `bool` instead of `int32_t`.\n> Additionally, the available modes are not taken into account.\n>\n> To fix this, retrieve the control with the proper type - `int32_t` -, and\n> use the V4L2 mode stored in `UVCCameraData` that was selected earlier in\n> `UVCCameraData::addControl()` for the given libcamera exposure time mode.\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 | 32 ++++++++++++++------\n> 1 file changed, 23 insertions(+), 9 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index a6cc37366..dc3e85bd8 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -97,8 +97,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> @@ -291,8 +291,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> @@ -336,10 +336,24 @@ 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\tint32_t exposureMode;\n> +\n> +\t\tswitch (value.get()) {\n> +\t\tcase controls::ExposureTimeModeAuto:\n> +\t\t\tif (!data->autoExposureMode_)\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\texposureMode = *data->autoExposureMode_;\n\nThat's more an open question, but according to your previous patch\n\n if (exposureModes[V4L2_EXPOSURE_AUTO])\n autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n\nIf the camera supported both V4L2_EXPOSURE_AUTO and\nV4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be\nV4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to\nsupport APERTURE_PRIORITY by using auto exposure and manual gain.\n\nI wonder if the above condition shouldn't be changed to\n\n if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n else if (exposureModes[V4L2_EXPOSURE_AUTO])\n autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n\nso that if the camera supports _PRIORITY modes we use them to allow\nimplementing _PRIORITY modes in libcamera with ExposureTimeMode and\nAnalogueGainMode.\n\nTrue that, if a user sets controls::ExposureTimeModeAuto and\ncontrols::AnalogueGainModeAuto and here we set APERTURE_PRIORITY,\nwe're breaking it.\n\nAgain, I'm not sure how many uvc cameras support PRIORITY modes, but\nif we want to support the full handling of PRIORITY we should also\ninspect AnalogueGainMode.\n\nThe pipeline at the moment doesn't even register AnalogueGainMode so I\nguess we can don't care about PRIORITIES, so what you have here is\nfine.\n\nIf someone wants to actually support priority modes in uvc, then there\nis a bigger rework to be done most probably. I wonder if we should\ncapture it in a comment at least (can be a separate patch)\n\n> +\t\t\tbreak;\n> +\t\tcase controls::ExposureTimeModeManual:\n> +\t\t\tif (!data->manualExposureMode_)\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\texposureMode = *data->manualExposureMode_;\n> +\t\t\tbreak;\n> +\t\tdefault:\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \t\tbreak;\n> \t}\n>\n> @@ -377,7 +391,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> --\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 D2F63C32DC\n\tfor ;\n\tTue, 4 Mar 2025 10:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 09FFB68779;\n\tTue, 4 Mar 2025 11:09:17 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6BBA668754\n\tfor ;\n\tTue, 4 Mar 2025 11:09:15 +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 432091083;\n\tTue, 4 Mar 2025 11:07:43 +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=\"Y73WPOEU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741082863;\n\tbh=O4+jLhT9ZFzvXjEAYjk9j7hYoQcmPd4vJktBJEDCYyM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Y73WPOEUCghbP1c8t1lDycSGiResDxnlIO8XF5tTT7yjg690+PUQBxmVFrpcyBtg7\n\tL9L8tsz+uu41vNgfYmk5BW8yS6WhPCiUYoMG7fRqp7lOoDJvt04bmroeh/ETIfPPd9\n\tpdqr5fx7TChSbPEgzt+fMYAlJp8+YtmUYD+rpgsM=","Date":"Tue, 4 Mar 2025 11:09:12 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","Message-ID":"<6juw4q5bfgrgfogomwbhrgyl7bopd2ieex7pttdqkkgc5nm5kf@jta34hkh5v4s>","References":"<20250303134234.699293-1-barnabas.pocze@ideasonboard.com>\n\t<20250303134234.699293-3-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":"<20250303134234.699293-3-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\" "}},{"id":33568,"web_url":"https://patchwork.libcamera.org/comment/33568/","msgid":"<02e2b6e4-e915-4b96-bcc1-7b86792c4750@ideasonboard.com>","date":"2025-03-04T10:46:24","subject":"Re: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n\n2025. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Mon, Mar 03, 2025 at 02:42:34PM +0100, Barnabás Pőcze wrote:\n>> The mapping in `UVCCameraData::processControl()` is not entirely correct\n>> because the control value is retrieved as a `bool` instead of `int32_t`.\n>> Additionally, the available modes are not taken into account.\n>>\n>> To fix this, retrieve the control with the proper type - `int32_t` -, and\n>> use the V4L2 mode stored in `UVCCameraData` that was selected earlier in\n>> `UVCCameraData::addControl()` for the given libcamera exposure time mode.\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 | 32 ++++++++++++++------\n>> 1 file changed, 23 insertions(+), 9 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index a6cc37366..dc3e85bd8 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -97,8 +97,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>> @@ -291,8 +291,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>> @@ -336,10 +336,24 @@ 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\tint32_t exposureMode;\n>> +\n>> +\t\tswitch (value.get()) {\n>> +\t\tcase controls::ExposureTimeModeAuto:\n>> +\t\t\tif (!data->autoExposureMode_)\n>> +\t\t\t\treturn -EINVAL;\n>> +\t\t\texposureMode = *data->autoExposureMode_;\n> \n> That's more an open question, but according to your previous patch\n> \n> if (exposureModes[V4L2_EXPOSURE_AUTO])\n> autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n> else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n> \n> If the camera supported both V4L2_EXPOSURE_AUTO and\n> V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be\n> V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to\n> support APERTURE_PRIORITY by using auto exposure and manual gain.\n\nI based the order on the comment in the code:\n\n\t\t * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,\n\t\t * \t\t\t V4L2_EXPOSURE_APERTURE_PRIORITY }\n\t\t *\n\t\t *\n\t\t * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,\n\t\t *\t\t\t V4L2_EXPOSURE_SHUTTER_PRIORITY }\n\n\n> \n> I wonder if the above condition shouldn't be changed to\n> \n> if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n> else if (exposureModes[V4L2_EXPOSURE_AUTO])\n> autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n> \n> so that if the camera supports _PRIORITY modes we use them to allow\n> implementing _PRIORITY modes in libcamera with ExposureTimeMode and\n> AnalogueGainMode.\n> \n> True that, if a user sets controls::ExposureTimeModeAuto and\n> controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY,\n> we're breaking it.\n> \n> Again, I'm not sure how many uvc cameras support PRIORITY modes, but\n> if we want to support the full handling of PRIORITY we should also\n> inspect AnalogueGainMode.\n\nI only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and\nit implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`.\n\n\n> \n> The pipeline at the moment doesn't even register AnalogueGainMode so I\n> guess we can don't care about PRIORITIES, so what you have here is\n> fine.\n> \n> If someone wants to actually support priority modes in uvc, then there\n> is a bigger rework to be done most probably. I wonder if we should\n> capture it in a comment at least (can be a separate patch)\n\nI will open a bug report about this if that's fine?\n\nIf I understand it correctly:\n\n gain\n A M\nexposure A auto aperture\n M shutter manual\n\nThis is how the two libcamera controls would be mapped to the v4l2 control.\nWill it not be an issue that you cannot express the supported feature set\nwith ControlInfo exactly? E.g. if three are supported, or if two that are\nplaced diagonally in the above table. I don't know if this is just theoretical,\nif such scenario is even possible/allowed.\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> +\t\t\tbreak;\n>> +\t\tcase controls::ExposureTimeModeManual:\n>> +\t\t\tif (!data->manualExposureMode_)\n>> +\t\t\t\treturn -EINVAL;\n>> +\t\t\texposureMode = *data->manualExposureMode_;\n>> +\t\t\tbreak;\n>> +\t\tdefault:\n>> +\t\t\treturn -EINVAL;\n>> +\t\t}\n>> +\n>> +\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> \t\tbreak;\n>> \t}\n>>\n>> @@ -377,7 +391,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>> --\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 636DEC32DC\n\tfor ;\n\tTue, 4 Mar 2025 10:46:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7C27368779;\n\tTue, 4 Mar 2025 11:46:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8966F68777\n\tfor ;\n\tTue, 4 Mar 2025 11:46:39 +0100 (CET)","from [192.168.33.3] (185.221.143.4.nat.pool.zt.hu [185.221.143.4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A22DD11D9; \n\tTue, 4 Mar 2025 11:45:01 +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=\"qex9URcM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741085101;\n\tbh=G0EJzLsVAcKlT9/IO9BcJii2qO67ndDsUlOcD2A/rHg=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=qex9URcMwa0OVV3LluNXeaMSW+bRSIT8ua3VW+I5o/On9ijB9bU68vfEV80QQmHly\n\tACqErDt86OXUjuUEzVd3LzfT+Q4QJeClOpn1Okj7/ZTaulpr7FvKgBcEqzH/WJWHla\n\tmbV7VZ3qVb3nRQMVoYt0Uy/cEN2y0tGrbmsAKs2I=","Message-ID":"<02e2b6e4-e915-4b96-bcc1-7b86792c4750@ideasonboard.com>","Date":"Tue, 4 Mar 2025 11:46:24 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250303134234.699293-1-barnabas.pocze@ideasonboard.com>\n\t<20250303134234.699293-3-barnabas.pocze@ideasonboard.com>\n\t<6juw4q5bfgrgfogomwbhrgyl7bopd2ieex7pttdqkkgc5nm5kf@jta34hkh5v4s>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<6juw4q5bfgrgfogomwbhrgyl7bopd2ieex7pttdqkkgc5nm5kf@jta34hkh5v4s>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":33569,"web_url":"https://patchwork.libcamera.org/comment/33569/","msgid":"","date":"2025-03-04T10:56:36","subject":"Re: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","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, Mar 04, 2025 at 11:46:24AM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n>\n> 2025. 03. 04. 11:09 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Mon, Mar 03, 2025 at 02:42:34PM +0100, Barnabás Pőcze wrote:\n> > > The mapping in `UVCCameraData::processControl()` is not entirely correct\n> > > because the control value is retrieved as a `bool` instead of `int32_t`.\n> > > Additionally, the available modes are not taken into account.\n> > >\n> > > To fix this, retrieve the control with the proper type - `int32_t` -, and\n> > > use the V4L2 mode stored in `UVCCameraData` that was selected earlier in\n> > > `UVCCameraData::addControl()` for the given libcamera exposure time mode.\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 | 32 ++++++++++++++------\n> > > 1 file changed, 23 insertions(+), 9 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index a6cc37366..dc3e85bd8 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -97,8 +97,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> > > @@ -291,8 +291,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> > > @@ -336,10 +336,24 @@ 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\tint32_t exposureMode;\n> > > +\n> > > +\t\tswitch (value.get()) {\n> > > +\t\tcase controls::ExposureTimeModeAuto:\n> > > +\t\t\tif (!data->autoExposureMode_)\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\texposureMode = *data->autoExposureMode_;\n> >\n> > That's more an open question, but according to your previous patch\n> >\n> > if (exposureModes[V4L2_EXPOSURE_AUTO])\n> > autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n> > else if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n> >\n> > If the camera supported both V4L2_EXPOSURE_AUTO and\n> > V4L2_EXPOSURE_APERTURE_PRIORITY autoExposureMode_ will always be\n> > V4L2_EXPOSURE_AUTO meaning that we effectively make it impossible to\n> > support APERTURE_PRIORITY by using auto exposure and manual gain.\n>\n> I based the order on the comment in the code:\n>\n> \t\t * ExposureTimeModeAuto = { V4L2_EXPOSURE_AUTO,\n> \t\t * \t\t\t V4L2_EXPOSURE_APERTURE_PRIORITY }\n> \t\t *\n> \t\t *\n> \t\t * ExposureTimeModeManual = { V4L2_EXPOSURE_MANUAL,\n> \t\t *\t\t\t V4L2_EXPOSURE_SHUTTER_PRIORITY }\n>\n>\n> >\n> > I wonder if the above condition shouldn't be changed to\n> >\n> > if (exposureModes[V4L2_EXPOSURE_APERTURE_PRIORITY])\n> > autoExposureMode_ = V4L2_EXPOSURE_APERTURE_PRIORITY;\n> > else if (exposureModes[V4L2_EXPOSURE_AUTO])\n> > autoExposureMode_ = V4L2_EXPOSURE_AUTO;\n> >\n> > so that if the camera supports _PRIORITY modes we use them to allow\n> > implementing _PRIORITY modes in libcamera with ExposureTimeMode and\n> > AnalogueGainMode.\n> >\n> > True that, if a user sets controls::ExposureTimeModeAuto and\n> > controls::AnalogueGainModeAuto and here we set APERTURE_PRIORITY,\n> > we're breaking it.\n> >\n> > Again, I'm not sure how many uvc cameras support PRIORITY modes, but\n> > if we want to support the full handling of PRIORITY we should also\n> > inspect AnalogueGainMode.\n>\n> I only have a single UVC camera that supports `V4L2_CID_EXPOSURE_AUTO`, and\n> it implements `V4L2_EXPOSURE_MANUAL` and `V4L2_EXPOSURE_APERTURE_PRIORITY`.\n>\n\nAh see, so it's not that uncommon\n\n>\n> >\n> > The pipeline at the moment doesn't even register AnalogueGainMode so I\n> > guess we can don't care about PRIORITIES, so what you have here is\n> > fine.\n> >\n> > If someone wants to actually support priority modes in uvc, then there\n> > is a bigger rework to be done most probably. I wonder if we should\n> > capture it in a comment at least (can be a separate patch)\n>\n> I will open a bug report about this if that's fine?\n\nYes please, but also record in a comment that we don't support\npriority modes but only Auto/Manual modes for both exposure and gain\n\n>\n> If I understand it correctly:\n>\n> gain\n> A M\n> exposure A auto aperture\n> M shutter manual\n\nThat's my understanding as well\n\n>\n> This is how the two libcamera controls would be mapped to the v4l2 control.\n> Will it not be an issue that you cannot express the supported feature set\n> with ControlInfo exactly? E.g. if three are supported, or if two that are\n> placed diagonally in the above table. I don't know if this is just theoretical,\n> if such scenario is even possible/allowed.\n\naperture and shutter priorty modes should be implemented by\nregistering AnalogueGainMode as well.\n\nMode AnalogueGainMode ExposureTimeMode\nAUTO { Auto} { Auto }\nMANUAL { Manual } { Manual }\nAPERTURE { Manual } { Auto }\nSHUTTER { Auto } { Manual }\n\nUsers should inspect the control info for both controls, and in the\ncase of your webcam that supports {AUTO, MANUAL, APERTURE }\nthey should read\n\nAnalogueGainMode: { Auto, Manual }\nExposureTimeMode: { Auto, Manual }\n\nValid combinations will be\n\nAnalogueGainMode, ExposureTimeMode = { Auto, Auto }\nAnalogueGainMode, ExposureTimeMode = { Manual, Manual }\nAnalogueGainMode, ExposureTimeMode = { Manual, Auto }\n\nSo yes, we can't express {Auto, Manual} (SHUTTER) is not valid until a\nuser tries it...\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > > +\t\t\tbreak;\n> > > +\t\tcase controls::ExposureTimeModeManual:\n> > > +\t\t\tif (!data->manualExposureMode_)\n> > > +\t\t\t\treturn -EINVAL;\n> > > +\t\t\texposureMode = *data->manualExposureMode_;\n> > > +\t\t\tbreak;\n> > > +\t\tdefault:\n> > > +\t\t\treturn -EINVAL;\n> > > +\t\t}\n> > > +\n> > > +\t\tcontrols->set(V4L2_CID_EXPOSURE_AUTO, exposureMode);\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > Thanks\n> > j\n> >\n> > > \t\tbreak;\n> > > \t}\n> > >\n> > > @@ -377,7 +391,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> > > --\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 E7FA3C3257\n\tfor ;\n\tTue, 4 Mar 2025 10:56:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0EBD68777;\n\tTue, 4 Mar 2025 11:56:41 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BB9AB68754\n\tfor ;\n\tTue, 4 Mar 2025 11:56: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 98B181083;\n\tTue, 4 Mar 2025 11:55:07 +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=\"fINhFRz8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1741085707;\n\tbh=uGIKSulSs9zXbi+QzexFluCKVLpIGNcMTf20pRomTq0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fINhFRz8UU6xkbuMQI305Z8/CFWqfRWnH0Ec0zIDNDgVCmZ0kkhm6Zz/XDHzxt88O\n\tSEhReLazU9VvC4/D6U8ekKCJ2W4uL5TG4BMH70dsVCUVs0gmLzw4zsSG+1x5WUafUG\n\t76Oc1W+3K98OFOG8Md8sc9sR+ICldseD+F256NL0=","Date":"Tue, 4 Mar 2025 11:56: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: [PATCH v3 2/2] libcamera: pipeline: uvcvideo: Fix\n\t`ExposureTimeMode` control setting","Message-ID":"","References":"<20250303134234.699293-1-barnabas.pocze@ideasonboard.com>\n\t<20250303134234.699293-3-barnabas.pocze@ideasonboard.com>\n\t<6juw4q5bfgrgfogomwbhrgyl7bopd2ieex7pttdqkkgc5nm5kf@jta34hkh5v4s>\n\t<02e2b6e4-e915-4b96-bcc1-7b86792c4750@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<02e2b6e4-e915-4b96-bcc1-7b86792c4750@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\" "}}]