[{"id":32999,"web_url":"https://patchwork.libcamera.org/comment/32999/","msgid":"<20250109230504.GG6159@pendragon.ideasonboard.com>","date":"2025-01-09T23:05:04","subject":"Re: [PATCH v6 10/12] libcamera: camera: Pre-process AeEnable control","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 Wed, Jan 08, 2025 at 06:09:40PM -0600, Paul Elder wrote:\n> Handle the AeEnable under-the-hood in the Camera class, such that\n\ns/under-the-hood/under the hood/\n\n> AeEnable activates ExposureTimeMode and AnalogueGain together. This\n> allows applications the convenience of setting auto/manual mode of all\n> of the AE-related controls, as well as protecting applications against a\n> nasty behavior change if an aperture control is added in the future.\n> This also moves common handling code out of the IPA.\n> \n> While we also want to inject AeEnable in Camera::controls() so that IPAs\n> don't have to report it, it is technically difficult at the moment as\n> ControlInfoMaps are not easily modifiable.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> New in v6\n> ---\n> src/libcamera/camera.cpp | 18 ++++++++++++++++++\n> 1 file changed, 18 insertions(+)\n> \n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 69a7ee535..4e0f63930 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -19,6 +19,7 @@\n> #include \n> \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -1325,6 +1326,23 @@ int Camera::queueRequest(Request *request)\n> \t\t}\n> \t}\n> \n> +\t/* Pre-process AeEnable */\n\ns/AeEnable/AeEnable./\n\n> +\tControlList &controls = request->controls();\n> +\tconst auto &aeEnable = controls.get(controls::AeEnable);\n> +\tif (aeEnable) {\n> +\t\tif (!controls.contains(controls::AnalogueGainMode.id())) {\n\nI think we need to first check that the camera supports the\nAnalogueGainMode control. Do we also need to check what values the\ncontrol support, or can we assume that if the camera exposes AeEnable\nand AnalogueGainMode than both auto and manual modes will be available ?\n\n> +\t\t\tcontrols.set(controls::AnalogueGainMode,\n> +\t\t\t\t *aeEnable ? controls::AnalogueGainModeAuto\n> +\t\t\t\t\t : controls::AnalogueGainModeManual);\n> +\t\t}\n\nYou can drop the curly braces here, and below too.\n\n> +\n> +\t\tif (!controls.contains(controls::ExposureTimeMode.id())) {\n\nSeem here, checking it supports the ExposureTimeMode control.\n\nOtherwise this looks fine to me.\n\n> +\t\t\tcontrols.set(controls::ExposureTimeMode,\n> +\t\t\t\t *aeEnable ? controls::ExposureTimeModeAuto\n> +\t\t\t\t\t : controls::ExposureTimeModeManual);\n> +\t\t}\n> +\t}\n> +\n> \td->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> \t\t\t ConnectionTypeQueued, request);\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 D0CA6C32EF\n\tfor ;\n\tThu, 9 Jan 2025 23:05:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6E92684EA;\n\tFri, 10 Jan 2025 00:05:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 439F3608AB\n\tfor ;\n\tFri, 10 Jan 2025 00:05:08 +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 03714161;\n\tFri, 10 Jan 2025 00:04:13 +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=\"Ww3kt4lQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736463854;\n\tbh=Ue3lv/kk/F+WxOs//jZjH21JJ3LFjuH4386l2puynmY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ww3kt4lQEHx0sH8vc0NVd/ecHT3m80Nt/ywJzM1yu5qNnIO+C5n5wZUcvykqVb/J+\n\tUmlQqy87lGWcKe5TEUqu4HNuXojMyWU21Z72gbzSf3yXwZxrXkHB15jclp4bmLSD5W\n\thpqpxA89nXt+YtQ77f6JdYTv1vPLqaHKYiikb8vI=","Date":"Fri, 10 Jan 2025 01:05:04 +0200","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, stefan.klug@ideasonboard.com","Subject":"Re: [PATCH v6 10/12] libcamera: camera: Pre-process AeEnable control","Message-ID":"<20250109230504.GG6159@pendragon.ideasonboard.com>","References":"<20250109000942.1616565-1-paul.elder@ideasonboard.com>\n\t<20250109000942.1616565-11-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250109000942.1616565-11-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\" "}},{"id":33027,"web_url":"https://patchwork.libcamera.org/comment/33027/","msgid":"","date":"2025-01-10T21:59:23","subject":"Re: [PATCH v6 10/12] libcamera: camera: Pre-process AeEnable control","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Fri, Jan 10, 2025 at 01:05:04AM +0200, Laurent Pinchart wrote:\n> Hi Paul,\n> \n> Thank you for the patch.\n> \n> On Wed, Jan 08, 2025 at 06:09:40PM -0600, Paul Elder wrote:\n> > Handle the AeEnable under-the-hood in the Camera class, such that\n> \n> s/under-the-hood/under the hood/\n> \n> > AeEnable activates ExposureTimeMode and AnalogueGain together. This\n> > allows applications the convenience of setting auto/manual mode of all\n> > of the AE-related controls, as well as protecting applications against a\n> > nasty behavior change if an aperture control is added in the future.\n> > This also moves common handling code out of the IPA.\n> > \n> > While we also want to inject AeEnable in Camera::controls() so that IPAs\n> > don't have to report it, it is technically difficult at the moment as\n> > ControlInfoMaps are not easily modifiable.\n> > \n> > Signed-off-by: Paul Elder \n> > \n> > ---\n> > New in v6\n> > ---\n> > src/libcamera/camera.cpp | 18 ++++++++++++++++++\n> > 1 file changed, 18 insertions(+)\n> > \n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 69a7ee535..4e0f63930 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -19,6 +19,7 @@\n> > #include \n> > \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -1325,6 +1326,23 @@ int Camera::queueRequest(Request *request)\n> > \t\t}\n> > \t}\n> > \n> > +\t/* Pre-process AeEnable */\n> \n> s/AeEnable/AeEnable./\n> \n> > +\tControlList &controls = request->controls();\n> > +\tconst auto &aeEnable = controls.get(controls::AeEnable);\n> > +\tif (aeEnable) {\n> > +\t\tif (!controls.contains(controls::AnalogueGainMode.id())) {\n> \n> I think we need to first check that the camera supports the\n> AnalogueGainMode control.\n\nOh right good point.\n\n> Do we also need to check what values the\n> control support, or can we assume that if the camera exposes AeEnable\n> and AnalogueGainMode than both auto and manual modes will be available ?\n\nIn non-black box cameras, yes we can assume that. In black box cameras,\nwe had arguments about \"if you can't change it then there's no point in\nexposing the control\" vs \"the control should be exposed so the\napplication knows about which mode it's forced to\". Or something like\nthat iirc.\n\n> \n> > +\t\t\tcontrols.set(controls::AnalogueGainMode,\n> > +\t\t\t\t *aeEnable ? controls::AnalogueGainModeAuto\n> > +\t\t\t\t\t : controls::AnalogueGainModeManual);\n> > +\t\t}\n> \n> You can drop the curly braces here, and below too.\n\nIt's multiple lines with weird indentation so I thought the curly braces\nare nicer.\n\n> \n> > +\n> > +\t\tif (!controls.contains(controls::ExposureTimeMode.id())) {\n> \n> Seem here, checking it supports the ExposureTimeMode control.\n> \n> Otherwise this looks fine to me.\n> \n\nThanks,\n\nPaul\n\n> > +\t\t\tcontrols.set(controls::ExposureTimeMode,\n> > +\t\t\t\t *aeEnable ? controls::ExposureTimeModeAuto\n> > +\t\t\t\t\t : controls::ExposureTimeModeManual);\n> > +\t\t}\n> > +\t}\n> > +\n> > \td->pipe_->invokeMethod(&PipelineHandler::queueRequest,\n> > \t\t\t ConnectionTypeQueued, request);\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 1666DC32F9\n\tfor ;\n\tFri, 10 Jan 2025 21:59:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 31A1F61891;\n\tFri, 10 Jan 2025 22:59:32 +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 D8393608AA\n\tfor ;\n\tFri, 10 Jan 2025 22:59:30 +0100 (CET)","from pyrite.rasen.tech (h175-177-049-030.catv02.itscom.jp\n\t[175.177.49.30])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E25D79FC;\n\tFri, 10 Jan 2025 22:58:34 +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=\"dnBzKiao\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1736546316;\n\tbh=R9mLWQ1Onwk3RVJMp6ynpMPJcvtNOJ9dJXiSJB/112Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dnBzKiaoyyOaaUdOoBivL0h6kGBxsYzToqM8jMRGGufTvO7sHmZ/qbSPif9DAsL6c\n\t6nHob40YhCNfC2E0a9X5Ux4FrHB3yKIKQx+BdYLiBSQUoulpjQZz3/9mKfWkdJU1CQ\n\tTIrVJBe4U3AViQzrlmVVokHg+4chi3uRY9anm5/o=","Date":"Sat, 11 Jan 2025 06:59:23 +0900","From":"Paul Elder ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, stefan.klug@ideasonboard.com","Subject":"Re: [PATCH v6 10/12] libcamera: camera: Pre-process AeEnable control","Message-ID":"","References":"<20250109000942.1616565-1-paul.elder@ideasonboard.com>\n\t<20250109000942.1616565-11-paul.elder@ideasonboard.com>\n\t<20250109230504.GG6159@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250109230504.GG6159@pendragon.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\" "}}]