[{"id":32646,"web_url":"https://patchwork.libcamera.org/comment/32646/","msgid":"<20241210023849.GH26531@pendragon.ideasonboard.com>","date":"2024-12-10T02:38:49","subject":"Re: [PATCH v4 3/8] libcamera: uvcvideo: Register ExposureTimeMode\n\tcontrol","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Thu, Dec 05, 2024 at 08:22:36PM +0900, Paul Elder wrote:\n> From: Jacopo Mondi \n> \n> Port the UVC pipeline handler to use the new ExposureTimeMode control\n> when processing Camera controls in place of the AeEnable control.\n> \n> The V4L2_CID_EXPOSURE_AUTO control allows 4 possible values, which\n> map to ExposureTimeModeAuto and ExposureTimeModeManual.\n> \n> Signed-off-by: Jacopo Mondi \n> Signed-off-by: Paul Elder \n> Reviewed-by: Paul Elder \n\nWith have AGC-related issues in the UVC pipeline handler (see\nhttps://bugs.libcamera.org/show_bug.cgi?id=242). This patch doesn't seem\nto make the problem worse, so I'm fine with this. Just one comment\nbelow.\n\n> ---\n> No change in v3\n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 54 ++++++++++++++++++--\n> 1 file changed, 49 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 8c2c6baf3575..a2f0935181d4 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -298,7 +298,7 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id,\n> \t\tcid = V4L2_CID_CONTRAST;\n> \telse if (id == controls::Saturation)\n> \t\tcid = V4L2_CID_SATURATION;\n> -\telse if (id == controls::AeEnable)\n> +\telse if (id == controls::ExposureTimeMode)\n> \t\tcid = V4L2_CID_EXPOSURE_AUTO;\n> \telse if (id == controls::ExposureTime)\n> \t\tcid = V4L2_CID_EXPOSURE_ABSOLUTE;\n> @@ -647,7 +647,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\tid = &controls::Saturation;\n> \t\tbreak;\n> \tcase V4L2_CID_EXPOSURE_AUTO:\n> -\t\tid = &controls::AeEnable;\n> +\t\tid = &controls::ExposureTimeMode;\n> \t\tbreak;\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> \t\tid = &controls::ExposureTime;\n> @@ -660,6 +660,7 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t}\n> \n> \t/* Map the control info. */\n> +\tconst std::vector &v4l2Values = v4l2Info.values();\n> \tint32_t min = v4l2Info.min().get();\n> \tint32_t max = v4l2Info.max().get();\n> \tint32_t def = v4l2Info.def().get();\n> @@ -697,10 +698,53 @@ void UVCCameraData::addControl(uint32_t cid, const ControlInfo &v4l2Info,\n> \t\t};\n> \t\tbreak;\n> \n> -\tcase V4L2_CID_EXPOSURE_AUTO:\n> -\t\tinfo = ControlInfo{ false, true, true };\n> +\tcase V4L2_CID_EXPOSURE_AUTO: {\n> +\t\t/*\n> +\t\t * From the V4L2_CID_EXPOSURE_AUTO documentation:\n> +\t\t *\n> +\t\t * ------------------------------------------------------------\n> +\t\t * V4L2_EXPOSURE_AUTO:\n> +\t\t * Automatic exposure time, automatic iris aperture.\n> +\t\t *\n> +\t\t * V4L2_EXPOSURE_MANUAL:\n> +\t\t * Manual exposure time, manual iris.\n> +\t\t *\n> +\t\t * V4L2_EXPOSURE_SHUTTER_PRIORITY:\n> +\t\t * Manual exposure time, auto iris.\n> +\t\t *\n> +\t\t * V4L2_EXPOSURE_APERTURE_PRIORITY:\n> +\t\t * Auto exposure time, manual iris.\n> +\t\t *-------------------------------------------------------------\n> +\t\t *\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> +\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\tint32_t *data = values.data();\n> +\t\tinfo = ControlInfo{Span(data, 2), values[0]};\n\nYou don't need the data variable, you can write\n\n\t\tinfo = ControlInfo{Span{values}, values[0]};\n\nReviewed-by: Laurent Pinchart \n\n> \t\tbreak;\n> -\n> +\t}\n> \tcase V4L2_CID_EXPOSURE_ABSOLUTE:\n> \t\t/*\n> \t\t * ExposureTime is in units of 1 µs, and UVC expects","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 B7C18C32CE\n\tfor ;\n\tTue, 10 Dec 2024 02:39:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1C47067E79;\n\tTue, 10 Dec 2024 03:39:07 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1338A618AC\n\tfor ;\n\tTue, 10 Dec 2024 03:39:05 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C05156EC;\n\tTue, 10 Dec 2024 03:38: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=\"vCU00nw7\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733798313;\n\tbh=Go2+ypJXpq00Z0ecP3fhK94fY5CfKN05clZe7BN88pw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vCU00nw7rrnm1pj1gXrckEIEa0+7B0CfAeq6HpHdmSbvcdktv9yE7YvJDIFWMSEg8\n\tIoUs9vpd638Zz95Z0yDQGJLpK4dyz71TZsJymA6pEul230DVJ2LLmfSx0tPUYy4ze2\n\tJRxTGt3r22E6WIiUGc0GlQlZtwJQYHRj1XYpCHKM=","Date":"Tue, 10 Dec 2024 04:38:49 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ","Subject":"Re: [PATCH v4 3/8] libcamera: uvcvideo: Register ExposureTimeMode\n\tcontrol","Message-ID":"<20241210023849.GH26531@pendragon.ideasonboard.com>","References":"<20241205112241.641964-1-paul.elder@ideasonboard.com>\n\t<20241205112241.641964-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20241205112241.641964-4-paul.elder@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\" "}}]