[{"id":33678,"web_url":"https://patchwork.libcamera.org/comment/33678/","msgid":"<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>","date":"2025-03-21T14:02:06","subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote:\n> If `ExposureTimeMode` is supported, then retrieve the current value\n> of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and\n> convert it to the appropriate `ExposureTimeModeEnum` value. If it\n> is not supported or the conversion fails, then fall back to assuming\n> that `ExposureTimeModeManual` is in effect.\n>\n> This is necessary to be able to properly handle the `ExposureTime`\n> control as its value is required to be ignored if `ExposureTimeMode`\n> is not manual.\n>\n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++\n> 1 file changed, 20 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 4ff79c291..7d882ebe1 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -62,6 +62,15 @@ public:\n> \tstd::optional autoExposureMode_;\n> \tstd::optional manualExposureMode_;\n>\n> +\tstruct State {\n> +\t\tstd::optional exp;\n> +\n> +\t\tvoid reset()\n> +\t\t{\n> +\t\t\texp.reset();\n> +\t\t}\n> +\t} state_;\n\nDo you expect this to grow ? Otherwise there's no much difference than\njust using an optional<> directly.\n\n> +\n> private:\n> \tbool generateId();\n>\n> @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> \t\treturn ret;\n> \t}\n>\n> +\tif (data->autoExposureMode_ || data->manualExposureMode_) {\n> +\t\tconst auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO });\n> +\t\tconst auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n\nIs this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it\nsafer to check\n\n if (!ctrls.empty()) {\n const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n\t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n\n }\n> +\t\tif (!value.isNone())\n> +\t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n> +\t}\n> +\n> +\tif (!data->state_.exp)\n> +\t\tdata->state_.exp = controls::ExposureTimeModeManual;\n> +\n\nI presume this is safer than just relying on the control default\nvalue, specifically in start-stop-start sessions (if controls are not\nreset in between ofc)\n\n> \treturn 0;\n> }\n>\n> @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> \tUVCCameraData *data = cameraData(camera);\n> \tdata->video_->streamOff();\n> \tdata->video_->releaseBuffers();\n> +\tdata->state_.reset();\n> }\n>\n> int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,\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 377DBC3272\n\tfor ;\n\tFri, 21 Mar 2025 14:02:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 318D268953;\n\tFri, 21 Mar 2025 15:02:14 +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 414346894B\n\tfor ;\n\tFri, 21 Mar 2025 15:02:12 +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 8DC6A220;\n\tFri, 21 Mar 2025 15:00:26 +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=\"auTfQGQk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742565627;\n\tbh=Lv4ekpTZK2TIpOTP5pT+fTIX3++Ga7H7Hz55qhgZgUA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=auTfQGQkCEe0FNqF8mJmHqtkTEydWppaRwla2yv9Qp4crUKXI3KGWFOtGNMPBMoN3\n\t6sQRcyTG6cZCURjgH7M8DJQUUenpISBMtpwXtxpNJzJMDkpJLVPK/EAVLqLj0xLszl\n\tFh40wU9skkD4DpBV7ok0EpZ5xAuWoHBM+D6zv9bg=","Date":"Fri, 21 Mar 2025 15:02:06 +0100","From":"Jacopo Mondi ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","Message-ID":"<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>","References":"<20250314174248.1015718-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174248.1015718-4-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250314174248.1015718-4-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":33682,"web_url":"https://patchwork.libcamera.org/comment/33682/","msgid":"","date":"2025-03-21T14:36:10","subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote:\n>> If `ExposureTimeMode` is supported, then retrieve the current value\n>> of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and\n>> convert it to the appropriate `ExposureTimeModeEnum` value. If it\n>> is not supported or the conversion fails, then fall back to assuming\n>> that `ExposureTimeModeManual` is in effect.\n>>\n>> This is necessary to be able to properly handle the `ExposureTime`\n>> control as its value is required to be ignored if `ExposureTimeMode`\n>> is not manual.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++\n>> 1 file changed, 20 insertions(+)\n>>\n>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> index 4ff79c291..7d882ebe1 100644\n>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> @@ -62,6 +62,15 @@ public:\n>> \tstd::optional autoExposureMode_;\n>> \tstd::optional manualExposureMode_;\n>>\n>> +\tstruct State {\n>> +\t\tstd::optional exp;\n>> +\n>> +\t\tvoid reset()\n>> +\t\t{\n>> +\t\t\texp.reset();\n>> +\t\t}\n>> +\t} state_;\n> \n> Do you expect this to grow ? Otherwise there's no much difference than\n> just using an optional<> directly.\n\nNo idea to be honest, but state objects are passed to functions, so\nit's certainly less typing.\n\n\n> \n>> +\n>> private:\n>> \tbool generateId();\n>>\n>> @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n>> \t\treturn ret;\n>> \t}\n>>\n>> +\tif (data->autoExposureMode_ || data->manualExposureMode_) {\n>> +\t\tconst auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO });\n>> +\t\tconst auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n> \n> Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it\n> safer to check\n\nIt is apparently not, documentation says undefined behaviour. I have\nadded a `ctrls.contains(...)` check.\n\n\n> \n> if (!ctrls.empty()) {\n> const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n> \t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n> \n> }\n>> +\t\tif (!value.isNone())\n>> +\t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n>> +\t}\n>> +\n>> +\tif (!data->state_.exp)\n>> +\t\tdata->state_.exp = controls::ExposureTimeModeManual;\n>> +\n> \n> I presume this is safer than just relying on the control default\n> value, specifically in start-stop-start sessions (if controls are not\n> reset in between ofc)\n\nAt this point I have different thoughts every time I look at this...\nThis is a fallback path for very unlikely scenarios, but now I am\nthinking that the current value shouldn't even be retrieved for\ntwo reasons:\n\n * the current values of controls are not retrievable by the user application\n (with libcamera at least);\n * the user application will have to set `ExposureTimeMode=Manual` explicitly\n before setting `ExposureTime` at least once (otherwise correct operation is\n not guaranteed by the docs) and at that point up to date state information\n will be available.\n\nOr is there possibly an expectation that the controls will have their \"default\"\nvalues when streaming starts? That is surely not guaranteed by the UVC pipeline\nhandler, do others guarantee that?\n\n\nRegards,\nBarnabás Pőcze\n\n> \n>> \treturn 0;\n>> }\n>>\n>> @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n>> \tUVCCameraData *data = cameraData(camera);\n>> \tdata->video_->streamOff();\n>> \tdata->video_->releaseBuffers();\n>> +\tdata->state_.reset();\n>> }\n>>\n>> int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,\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 63457C3272\n\tfor ;\n\tFri, 21 Mar 2025 14:36:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D62A68953;\n\tFri, 21 Mar 2025 15:36:15 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0427868947\n\tfor ;\n\tFri, 21 Mar 2025 15:36:13 +0100 (CET)","from [192.168.33.18] (185.221.143.221.nat.pool.zt.hu\n\t[185.221.143.221])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 777EC465;\n\tFri, 21 Mar 2025 15:34:29 +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=\"eCBmqk2L\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742567669;\n\tbh=g8jV5ehSzhGByP+DPTjqS1t3iOij3O6d3a7+X6P1Zw4=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=eCBmqk2LwYBSsg4g4MMB/t3Wa6V+Yj//lUIJsSZH8rZpcToYJaJg8VGx5yAHk41PM\n\tvUmJDriaXB8jlo7dAiZEQvLIIOglpNVS7veTumqC284c1bdb4D2/lOFsbamRqzJuxy\n\tcHbDAFYJ2J5ZgmEyTQXru9jRR1Xt3IITBTbF0g1k=","Message-ID":"","Date":"Fri, 21 Mar 2025 15:36:10 +0100","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250314174248.1015718-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174248.1015718-4-barnabas.pocze@ideasonboard.com>\n\t<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>","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":33684,"web_url":"https://patchwork.libcamera.org/comment/33684/","msgid":"","date":"2025-03-21T14:53:29","subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Barnabás\n\nOn Fri, Mar 21, 2025 at 03:36:10PM +0100, Barnabás Pőcze wrote:\n> Hi\n>\n> 2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta:\n> > Hi Barnabás\n> >\n> > On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote:\n> > > If `ExposureTimeMode` is supported, then retrieve the current value\n> > > of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and\n> > > convert it to the appropriate `ExposureTimeModeEnum` value. If it\n> > > is not supported or the conversion fails, then fall back to assuming\n> > > that `ExposureTimeModeManual` is in effect.\n> > >\n> > > This is necessary to be able to properly handle the `ExposureTime`\n> > > control as its value is required to be ignored if `ExposureTimeMode`\n> > > is not manual.\n> > >\n> > > Signed-off-by: Barnabás Pőcze \n> > > ---\n> > > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++\n> > > 1 file changed, 20 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > index 4ff79c291..7d882ebe1 100644\n> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > > @@ -62,6 +62,15 @@ public:\n> > > \tstd::optional autoExposureMode_;\n> > > \tstd::optional manualExposureMode_;\n> > >\n> > > +\tstruct State {\n> > > +\t\tstd::optional exp;\n> > > +\n> > > +\t\tvoid reset()\n> > > +\t\t{\n> > > +\t\t\texp.reset();\n> > > +\t\t}\n> > > +\t} state_;\n> >\n> > Do you expect this to grow ? Otherwise there's no much difference than\n> > just using an optional<> directly.\n>\n> No idea to be honest, but state objects are passed to functions, so\n> it's certainly less typing.\n>\n\nTrue, and probably more tidy than just passing the optional<> around..\n\nLet's make it clear this is about controls maybe naming the type\nControlsState ? (uvc doesn't have an IPA, but in IPA-terms this would\nactually be the ActiveState..)\n\n>\n> >\n> > > +\n> > > private:\n> > > \tbool generateId();\n> > >\n> > > @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n> > > \t\treturn ret;\n> > > \t}\n> > >\n> > > +\tif (data->autoExposureMode_ || data->manualExposureMode_) {\n> > > +\t\tconst auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO });\n> > > +\t\tconst auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n> >\n> > Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it\n> > safer to check\n>\n> It is apparently not, documentation says undefined behaviour. I have\n> added a `ctrls.contains(...)` check.\n>\n>\n> >\n> > if (!ctrls.empty()) {\n> > const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n> > \t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n> >\n> > }\n> > > +\t\tif (!value.isNone())\n> > > +\t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n> > > +\t}\n> > > +\n> > > +\tif (!data->state_.exp)\n> > > +\t\tdata->state_.exp = controls::ExposureTimeModeManual;\n> > > +\n> >\n> > I presume this is safer than just relying on the control default\n> > value, specifically in start-stop-start sessions (if controls are not\n> > reset in between ofc)\n>\n> At this point I have different thoughts every time I look at this...\n> This is a fallback path for very unlikely scenarios, but now I am\n> thinking that the current value shouldn't even be retrieved for\n> two reasons:\n>\n> * the current values of controls are not retrievable by the user application\n> (with libcamera at least);\n> * the user application will have to set `ExposureTimeMode=Manual` explicitly\n> before setting `ExposureTime` at least once (otherwise correct operation is\n> not guaranteed by the docs) and at that point up to date state information\n> will be available.\n>\n> Or is there possibly an expectation that the controls will have their \"default\"\n> values when streaming starts? That is surely not guaranteed by the UVC pipeline\n> handler, do others guarantee that?\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> >\n> > > \treturn 0;\n> > > }\n> > >\n> > > @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n> > > \tUVCCameraData *data = cameraData(camera);\n> > > \tdata->video_->streamOff();\n> > > \tdata->video_->releaseBuffers();\n> > > +\tdata->state_.reset();\n> > > }\n> > >\n> > > int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,\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 5D82DC3272\n\tfor ;\n\tFri, 21 Mar 2025 14:53:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 866BE68952;\n\tFri, 21 Mar 2025 15:53:36 +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 CB7EF68947\n\tfor ;\n\tFri, 21 Mar 2025 15:53:35 +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 31FDF465;\n\tFri, 21 Mar 2025 15:51:50 +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=\"RRyeJnpb\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1742568711;\n\tbh=EEIF59hYrQVTQX35bRwtK7jZnTncPCmjvpSfVRmvWvM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RRyeJnpboupv1wW3aGuuNPNqbfbjxjqRAMCF/+eg4l5f+2dGQ28nq7dH0xtnu1ri5\n\tiwS5uulpaWJeX2vYIgJJZmTeDiuss1/wVdh73aVKhJVVMwgzrivaVorLEMwLRUAxux\n\tOGtkfwYICSqyxqvRH+GmSxn0e5YBHQNQpcGHkXyQ=","Date":"Fri, 21 Mar 2025 15:53:29 +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 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","Message-ID":"","References":"<20250314174248.1015718-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174248.1015718-4-barnabas.pocze@ideasonboard.com>\n\t<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>\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\" "}},{"id":34134,"web_url":"https://patchwork.libcamera.org/comment/34134/","msgid":"<5c6e5e29-e695-4e13-8258-6eb6bf0ac11c@ideasonboard.com>","date":"2025-05-06T15:53:44","subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","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. 21. 15:53 keltezéssel, Jacopo Mondi írta:\n> Hi Barnabás\n> \n> On Fri, Mar 21, 2025 at 03:36:10PM +0100, Barnabás Pőcze wrote:\n>> Hi\n>>\n>> 2025. 03. 21. 15:02 keltezéssel, Jacopo Mondi írta:\n>>> Hi Barnabás\n>>>\n>>> On Fri, Mar 14, 2025 at 06:42:47PM +0100, Barnabás Pőcze wrote:\n>>>> If `ExposureTimeMode` is supported, then retrieve the current value\n>>>> of `V4L2_CID_EXPOSURE_AUTO` in `PipelineHandlerUVC::start()`, and\n>>>> convert it to the appropriate `ExposureTimeModeEnum` value. If it\n>>>> is not supported or the conversion fails, then fall back to assuming\n>>>> that `ExposureTimeModeManual` is in effect.\n>>>>\n>>>> This is necessary to be able to properly handle the `ExposureTime`\n>>>> control as its value is required to be ignored if `ExposureTimeMode`\n>>>> is not manual.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 20 ++++++++++++++++++++\n>>>> 1 file changed, 20 insertions(+)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> index 4ff79c291..7d882ebe1 100644\n>>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>>>> @@ -62,6 +62,15 @@ public:\n>>>> \tstd::optional autoExposureMode_;\n>>>> \tstd::optional manualExposureMode_;\n>>>>\n>>>> +\tstruct State {\n>>>> +\t\tstd::optional exp;\n>>>> +\n>>>> +\t\tvoid reset()\n>>>> +\t\t{\n>>>> +\t\t\texp.reset();\n>>>> +\t\t}\n>>>> +\t} state_;\n>>>\n>>> Do you expect this to grow ? Otherwise there's no much difference than\n>>> just using an optional<> directly.\n>>\n>> No idea to be honest, but state objects are passed to functions, so\n>> it's certainly less typing.\n>>\n> \n> True, and probably more tidy than just passing the optional<> around..\n> \n> Let's make it clear this is about controls maybe naming the type\n> ControlsState ? (uvc doesn't have an IPA, but in IPA-terms this would\n> actually be the ActiveState..)\n\nHmm... I like `ActiveState` better than `ControlsState`, but let's continue\nthis discussion at the new version of the patch.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n>>\n>>>\n>>>> +\n>>>> private:\n>>>> \tbool generateId();\n>>>>\n>>>> @@ -303,6 +312,16 @@ int PipelineHandlerUVC::start(Camera *camera, [[maybe_unused]] const ControlList\n>>>> \t\treturn ret;\n>>>> \t}\n>>>>\n>>>> +\tif (data->autoExposureMode_ || data->manualExposureMode_) {\n>>>> +\t\tconst auto &ctrls = data->video_->getControls({ V4L2_CID_EXPOSURE_AUTO });\n>>>> +\t\tconst auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n>>>\n>>> Is this safe in case there's no V4L2_CID_EXPOSURE_AUTO ? Isn't it\n>>> safer to check\n>>\n>> It is apparently not, documentation says undefined behaviour. I have\n>> added a `ctrls.contains(...)` check.\n>>\n>>\n>>>\n>>> if (!ctrls.empty()) {\n>>> const auto &value = ctrls.get(V4L2_CID_EXPOSURE_AUTO);\n>>> \t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n>>>\n>>> }\n>>>> +\t\tif (!value.isNone())\n>>>> +\t\t\tdata->state_.exp = v4l2ToExposureMode(value.get());\n>>>> +\t}\n>>>> +\n>>>> +\tif (!data->state_.exp)\n>>>> +\t\tdata->state_.exp = controls::ExposureTimeModeManual;\n>>>> +\n>>>\n>>> I presume this is safer than just relying on the control default\n>>> value, specifically in start-stop-start sessions (if controls are not\n>>> reset in between ofc)\n>>\n>> At this point I have different thoughts every time I look at this...\n>> This is a fallback path for very unlikely scenarios, but now I am\n>> thinking that the current value shouldn't even be retrieved for\n>> two reasons:\n>>\n>> * the current values of controls are not retrievable by the user application\n>> (with libcamera at least);\n>> * the user application will have to set `ExposureTimeMode=Manual` explicitly\n>> before setting `ExposureTime` at least once (otherwise correct operation is\n>> not guaranteed by the docs) and at that point up to date state information\n>> will be available.\n>>\n>> Or is there possibly an expectation that the controls will have their \"default\"\n>> values when streaming starts? That is surely not guaranteed by the UVC pipeline\n>> handler, do others guarantee that?\n>>\n>>\n>> Regards,\n>> Barnabás Pőcze\n>>\n>>>\n>>>> \treturn 0;\n>>>> }\n>>>>\n>>>> @@ -311,6 +330,7 @@ void PipelineHandlerUVC::stopDevice(Camera *camera)\n>>>> \tUVCCameraData *data = cameraData(camera);\n>>>> \tdata->video_->streamOff();\n>>>> \tdata->video_->releaseBuffers();\n>>>> +\tdata->state_.reset();\n>>>> }\n>>>>\n>>>> int PipelineHandlerUVC::processControl(const UVCCameraData *data, ControlList *controls,\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 E5A94C3226\n\tfor ;\n\tTue, 6 May 2025 15:53:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A71868B25;\n\tTue, 6 May 2025 17:53:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D23E768ADC\n\tfor ;\n\tTue, 6 May 2025 17:53:47 +0200 (CEST)","from [192.168.33.16] (185.221.141.56.nat.pool.zt.hu\n\t[185.221.141.56])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1BC8156D;\n\tTue, 6 May 2025 17:53:37 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"adDYL1Ad\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1746546817;\n\tbh=5b9bbgmMXp3H/5tuUlW5OQMHniYJ5/sIvbOIoJYq3Wo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=adDYL1AdtPuBGZZjOJDrpc+mkK/2eQ0oc3K38ly3GNoBi4RmGeDXZwM5jHTVew2XL\n\tfnm2V/VPEn1eoqUeKV25JAMjIf9VXx97afc+VCqLEZR6e+sGL++OtNWkz8r2VBO4jH\n\tp2g7RbaEKgi+R3yUoCaUAM0rrTr1ETGPR06TOZn8=","Message-ID":"<5c6e5e29-e695-4e13-8258-6eb6bf0ac11c@ideasonboard.com>","Date":"Tue, 6 May 2025 17:53:44 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v3 3/4] libcamera: pipeline: uvcvideo: Retrieve current\n\texposure mode on start","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250314174248.1015718-1-barnabas.pocze@ideasonboard.com>\n\t<20250314174248.1015718-4-barnabas.pocze@ideasonboard.com>\n\t<6tu3u6q44cbntwlfl2cvpe7po2xr3jl6btvey22notwxwfnhj7@tvz35hgbxyg2>\n\t\n\t","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"","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\" "}}]