[{"id":4492,"web_url":"https://patchwork.libcamera.org/comment/4492/","msgid":"<20200423194334.GL6196@pendragon.ideasonboard.com>","date":"2020-04-23T19:43:34","subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Fri, Apr 03, 2020 at 03:53:04PM +0100, Naushir Patuck wrote:\n> AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type\n> controls used to specify operating modes in the AE algorithm. All modes\n> may not be supported by all platforms.\n> \n> ExposureValue is a new control used to set the log2 exposure adjustment\n> for the AE algorithm.\n> \n> Lux is a new control that returns the current lux estimate.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/control_ids.yaml | 104 +++++++++++++++++++++++++++++++++\n> 1 file changed, 104 insertions(+)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 64e81520..50f9e07b 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -4,6 +4,8 @@\n> #\n> %YAML 1.2\n> ---\n> +# Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> +# set through a Request and returned out through Metadata.\n\nHow about adding this to control_ids.cpp.in, to ensure it will end up in\nthe generated documentation ?\n\ndiff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\nindex 99c511d0e712..cdd5d351ad6a 100644\n--- a/src/libcamera/control_ids.cpp.in\n+++ b/src/libcamera/control_ids.cpp.in\n@@ -33,6 +33,10 @@ ${controls_def}\n\n /**\n * \\brief List of all supported libcamera controls\n+ *\n+ * Unless otherwise stated, all controls are bi-directional, i.e. they can be\n+ * set through Request::controls() and returned out through\n+ * Request::metadata().\n */\n extern const ControlIdMap controls {\n ${controls_map}\n\n> controls:\n> - AeEnable:\n> type: bool\n> @@ -23,6 +25,103 @@ controls:\n> \n> \\sa AeEnable\n> \n\nThe controls below look good to me. I'll add a few comments regarding\npossible future developments, they're not meant to be addressed today.\n\n> + # AeMeteringMode needs further attention:\n> + # - Auto-generate max enum value.\n> + # - Better handling of custom types.\n> + - AeMeteringMode:\n> + type: int32_t\n> + description: |\n> + Specify a metering mode for the AE algorithm to use. The metering\n> + modes determine which parts of the image are used to determine the\n> + scene brightness. Metering modes may be platform specific and not\n> + all metering modes may be supported.\n> + enum:\n> + - name: MeteringCentreWeighted\n> + value: 0\n> + description: Centre-weighted metering mode.\n> + - name: MeteringSpot\n> + value: 1\n> + description: Spot metering mode.\n> + - name: MeteringMatrix\n> + value: 2\n> + description: Matrix metering mode.\n\nI think we'll need a way to configure both the spot and matrix modes\nwith a point and a matrix of weights, respectively. At that point I\nthink we could possibly merge the two together, as spot mode is\nessentially matrix mode with a single region, right ?\n\n> + - name: MeteringCustom\n> + value: 3\n> + description: Custom metering mode.\n> + - name: MeteringModeMax\n> + value: 3\n> + description: Maximum allowed value (place any new values above here).\n> +\n> + # AeConstraintMode needs further attention:\n> + # - Auto-generate max enum value.\n> + # - Better handling of custom types.\n> + - AeConstraintMode:\n> + type: int32_t\n> + description: |\n> + Specify a constraint mode for the AE algorithm to use. These determine\n> + how the measured scene brightness is adjusted to reach the desired\n> + target exposure. Constraint modes may be platform specific, and not\n> + all constraint modes may be supported.\n> + enum:\n> + - name: ConstraintNormal\n> + value: 0\n> + description: Default constraint mode.\n> + This mode aims to balance the exposure of different parts of the\n> + image so as to reach a reasonable average level. However, highlights\n> + in the image may appear over-exposed and lowlights may appear\n> + under-exposed.\n> + - name: ConstraintHighlight\n> + value: 1\n> + description: Highlight constraint mode.\n> + This mode adjusts the exposure levels in order to try and avoid\n> + over-exposing the brightest parts (highlights) of an image.\n> + Other non-highlight parts of the image may appear under-exposed.\n\nDo we need ConstraintLowlight too ?\n\n> + - name: ConstraintCustom\n> + value: 2\n> + description: Custom constraint mode.\n> + - name: ConstraintModeMax\n> + value: 2\n> + description: Maximum allowed value (place any new values above here).\n> +\n> + # AeExposureMode needs further attention:\n> + # - Auto-generate max enum value.\n> + # - Better handling of custom types.\n> + - AeExposureMode:\n> + type: int32_t\n> + description: |\n> + Specify an exposure mode for the AE algorithm to use. These specify\n> + how the desired total exposure is divided between the shutter time\n> + and the sensor's analogue gain. The exposure modes are platform\n> + specific, and not all exposure modes may be supported.\n> + enum:\n> + - name: ExposureNormal\n> + value: 0\n> + description: Default metering mode.\n\nIs this a metering mode, or is that a copy&paste ? :-)\n\n> + - name: ExposureShort\n> + value: 1\n> + description: Exposure mode allowing only short exposure times.\n> + - name: ExposureLong\n> + value: 2\n> + description: Exposure mode allowing long exposure times.\n> + - name: ExposureCustom\n> + value: 3\n> + description: Custom exposure mode.\n> + - name: ExposureModeMax\n> + value: 3\n> + description: Maximum allowed value (place any new values above here).\n> +\n> + - ExposureValue:\n> + type: float\n> + description: |\n> + Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> + applied if the AE algorithm is currently enabled.\n> +\n> + By convention EV adjusts the exposure as log2. For example\n> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> +\n> + \\sa AeEnable\n> +\n> - ExposureTime:\n> type: int32_t\n> description: |\n> @@ -53,6 +152,11 @@ controls:\n> Specify a fixed contrast parameter. Normal contrast is given by the\n> value 1.0; larger values produce images with more contrast.\n> \n> + # Lux can only be returned in Metadata\n> + - Lux:\n> + type: float\n> + description: Report an estimate of the current lux level.\n> +\n\nAnd here, to replace the comment,\n\n - Lux:\n type: float\n description: |\n Report an estimate of the current lux level. The Lux control can only\n be returned in metadata.\n\nand maybe nitpicking, should it be \"estimate of the current illuminance\nin lux\" ? The control could also be called Illuminance. I'll leave this\nup to you.\n\nWith these small issues addressed (except for the comments related to\nAeMeteringMode and AeConstraintMode that are really related to future\nwork),\n\nReviewed-by: Laurent Pinchart \n\n> - AwbEnable:\n> type: bool\n> description: |","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 059D262E45\n\tfor ;\n\tThu, 23 Apr 2020 21:43:49 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7ADDD4F7;\n\tThu, 23 Apr 2020 21:43:48 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"mgZtQfQ9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587671028;\n\tbh=CJ8PotIm0ZL7hO5UC0NQL22xjgjjvZneE2gF3v13800=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=mgZtQfQ9zHqx0sIk/Jg2tKqeRV+UTCUFu20bgbWeZjgaXiKbKSbv39CL44hJElIts\n\tF82d7c4mgvphl6s0lYovBNmcUvCJpJ44KvCFeJzcTvazABTrr8aRV1R0CSLs1tHfRp\n\tODbxSKA4PBqvcOpm0w345L6MCrh/SVUKb3wFj+mg=","Date":"Thu, 23 Apr 2020 22:43:34 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200423194334.GL6196@pendragon.ideasonboard.com>","References":"<20200403145305.10288-1-naush@raspberrypi.com>\n\t<20200403145305.10288-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200403145305.10288-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","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","X-List-Received-Date":"Thu, 23 Apr 2020 19:43:49 -0000"}},{"id":4495,"web_url":"https://patchwork.libcamera.org/comment/4495/","msgid":"","date":"2020-04-24T09:26:37","subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Thu, 23 Apr 2020 at 20:43, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 03, 2020 at 03:53:04PM +0100, Naushir Patuck wrote:\n> > AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type\n> > controls used to specify operating modes in the AE algorithm. All modes\n> > may not be supported by all platforms.\n> >\n> > ExposureValue is a new control used to set the log2 exposure adjustment\n> > for the AE algorithm.\n> >\n> > Lux is a new control that returns the current lux estimate.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/control_ids.yaml | 104 +++++++++++++++++++++++++++++++++\n> > 1 file changed, 104 insertions(+)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 64e81520..50f9e07b 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -4,6 +4,8 @@\n> > #\n> > %YAML 1.2\n> > ---\n> > +# Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > +# set through a Request and returned out through Metadata.\n>\n> How about adding this to control_ids.cpp.in, to ensure it will end up in\n> the generated documentation ?\n>\n\nYes, that's a good idea. Would it make sense if I make this change to\ncontrol_ids.cpp.in as a separate commit and make it the first commit\nin the patchset?\n\n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index 99c511d0e712..cdd5d351ad6a 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -33,6 +33,10 @@ ${controls_def}\n>\n> /**\n> * \\brief List of all supported libcamera controls\n> + *\n> + * Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> + * set through Request::controls() and returned out through\n> + * Request::metadata().\n> */\n> extern const ControlIdMap controls {\n> ${controls_map}\n>\n> > controls:\n> > - AeEnable:\n> > type: bool\n> > @@ -23,6 +25,103 @@ controls:\n> >\n> > \\sa AeEnable\n> >\n>\n> The controls below look good to me. I'll add a few comments regarding\n> possible future developments, they're not meant to be addressed today.\n>\n> > + # AeMeteringMode needs further attention:\n> > + # - Auto-generate max enum value.\n> > + # - Better handling of custom types.\n> > + - AeMeteringMode:\n> > + type: int32_t\n> > + description: |\n> > + Specify a metering mode for the AE algorithm to use. The metering\n> > + modes determine which parts of the image are used to determine the\n> > + scene brightness. Metering modes may be platform specific and not\n> > + all metering modes may be supported.\n> > + enum:\n> > + - name: MeteringCentreWeighted\n> > + value: 0\n> > + description: Centre-weighted metering mode.\n> > + - name: MeteringSpot\n> > + value: 1\n> > + description: Spot metering mode.\n> > + - name: MeteringMatrix\n> > + value: 2\n> > + description: Matrix metering mode.\n>\n> I think we'll need a way to configure both the spot and matrix modes\n> with a point and a matrix of weights, respectively. At that point I\n> think we could possibly merge the two together, as spot mode is\n> essentially matrix mode with a single region, right ?\n\nYes, if we have (somewhat) arbitrary freedom to assign weights and\nregions this should be possible.\n\n>\n> > + - name: MeteringCustom\n> > + value: 3\n> > + description: Custom metering mode.\n> > + - name: MeteringModeMax\n> > + value: 3\n> > + description: Maximum allowed value (place any new values above here).\n> > +\n> > + # AeConstraintMode needs further attention:\n> > + # - Auto-generate max enum value.\n> > + # - Better handling of custom types.\n> > + - AeConstraintMode:\n> > + type: int32_t\n> > + description: |\n> > + Specify a constraint mode for the AE algorithm to use. These determine\n> > + how the measured scene brightness is adjusted to reach the desired\n> > + target exposure. Constraint modes may be platform specific, and not\n> > + all constraint modes may be supported.\n> > + enum:\n> > + - name: ConstraintNormal\n> > + value: 0\n> > + description: Default constraint mode.\n> > + This mode aims to balance the exposure of different parts of the\n> > + image so as to reach a reasonable average level. However, highlights\n> > + in the image may appear over-exposed and lowlights may appear\n> > + under-exposed.\n> > + - name: ConstraintHighlight\n> > + value: 1\n> > + description: Highlight constraint mode.\n> > + This mode adjusts the exposure levels in order to try and avoid\n> > + over-exposing the brightest parts (highlights) of an image.\n> > + Other non-highlight parts of the image may appear under-exposed.\n>\n> Do we need ConstraintLowlight too ?\n\nYes, that makes sense, I'll add a new mode. We don't currently have\ntuning that will handle it, but that can come later.\nOne suggestion, can I call it ConstraintShadows instead of LowLight as\nit's a constraint on the amount of under-exposed area?\n\n>\n> > + - name: ConstraintCustom\n> > + value: 2\n> > + description: Custom constraint mode.\n> > + - name: ConstraintModeMax\n> > + value: 2\n> > + description: Maximum allowed value (place any new values above here).\n> > +\n> > + # AeExposureMode needs further attention:\n> > + # - Auto-generate max enum value.\n> > + # - Better handling of custom types.\n> > + - AeExposureMode:\n> > + type: int32_t\n> > + description: |\n> > + Specify an exposure mode for the AE algorithm to use. These specify\n> > + how the desired total exposure is divided between the shutter time\n> > + and the sensor's analogue gain. The exposure modes are platform\n> > + specific, and not all exposure modes may be supported.\n> > + enum:\n> > + - name: ExposureNormal\n> > + value: 0\n> > + description: Default metering mode.\n>\n> Is this a metering mode, or is that a copy&paste ? :-)\n\nOops, will fix.\n\n>\n> > + - name: ExposureShort\n> > + value: 1\n> > + description: Exposure mode allowing only short exposure times.\n> > + - name: ExposureLong\n> > + value: 2\n> > + description: Exposure mode allowing long exposure times.\n> > + - name: ExposureCustom\n> > + value: 3\n> > + description: Custom exposure mode.\n> > + - name: ExposureModeMax\n> > + value: 3\n> > + description: Maximum allowed value (place any new values above here).\n> > +\n> > + - ExposureValue:\n> > + type: float\n> > + description: |\n> > + Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> > + applied if the AE algorithm is currently enabled.\n> > +\n> > + By convention EV adjusts the exposure as log2. For example\n> > + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> > +\n> > + \\sa AeEnable\n> > +\n> > - ExposureTime:\n> > type: int32_t\n> > description: |\n> > @@ -53,6 +152,11 @@ controls:\n> > Specify a fixed contrast parameter. Normal contrast is given by the\n> > value 1.0; larger values produce images with more contrast.\n> >\n> > + # Lux can only be returned in Metadata\n> > + - Lux:\n> > + type: float\n> > + description: Report an estimate of the current lux level.\n> > +\n>\n> And here, to replace the comment,\n>\n> - Lux:\n> type: float\n> description: |\n> Report an estimate of the current lux level. The Lux control can only\n> be returned in metadata.\n>\n> and maybe nitpicking, should it be \"estimate of the current illuminance\n> in lux\" ? The control could also be called Illuminance. I'll leave this\n> up to you.\n\nDone. I think we should keep the control name as Lux which is a\nstandard imaging term.\n\n> With these small issues addressed (except for the comments related to\n> AeMeteringMode and AeConstraintMode that are really related to future\n> work),\n>\n> Reviewed-by: Laurent Pinchart \n>\n> > - AwbEnable:\n> > type: bool\n> > description: |\n>\n\nRegards,\nNaush\n\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x22f.google.com (mail-lj1-x22f.google.com\n\t[IPv6:2a00:1450:4864:20::22f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7747D603FB\n\tfor ;\n\tFri, 24 Apr 2020 11:26:54 +0200 (CEST)","by mail-lj1-x22f.google.com with SMTP id f18so9190023lja.13\n\tfor ;\n\tFri, 24 Apr 2020 02:26:54 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"YyeavuhE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=sBCGkQzpq9jdBXbrkkSHsdII4aOWq0IBD+IV4yHveVo=;\n\tb=YyeavuhEm2gN6NG/E9tl5Uxl/b0Im3Ye53IHOfSBqoha81SB2Hgek2inUhGdUA7fy6\n\t/+c8mSdWFJqr+f0u88EX88LKB8x4/KHXUuFH1X0M45v3hl6Gx75Ch2kmyu2yC9i0M4JA\n\ttKJG1NdQvNmqYd03vM2U1C6o8/KdhjYhlnm5BCaOHyATo9QMSt1k/0qmMAv21aI1pZ9b\n\teRGOq2U0oQ2NQNYPIJQ22dVFzdf/A5GBSRBavDk6EwxJGAwWnU2RpyUfXPip/EXlIgBO\n\tus4hpurs4I1y7p1yB1wnFECYKnhxV67utaBabPHIMRLvQeUi/AhwEaYmn1gHKisCQdWa\n\tp1lg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=sBCGkQzpq9jdBXbrkkSHsdII4aOWq0IBD+IV4yHveVo=;\n\tb=jYPJO/mwf7j7xBugO6MbTID0CblC/UTCkEsBiKaEi1c5uAwkvekrIho/wO/UHelYpx\n\ttdXLqtPUmwUAhzf5bWPINbFz3cs6ZYpvAy+Q8Ba+8gfxUe0GGQC55aYaL7Ht42nmbkDU\n\tF0BkIV4mzgABAqW5ltJSG8HfS7eR+yDKJSLSn23DgZI39Rh/8gkVFj49YLjGRfhNeftC\n\te+PNuh7Ficd+ylFRdfD04Kns37Pz3LgMl8Pk2EzaHd8kR8INFanoVGSvQM0ghO6xgFEd\n\tZhOAxeLXU/XldsseNllpfbX6PD/neXgiUF35naZqMhaRRHocavaYaP6g3pDYa7D9O9MS\n\t3GqQ==","X-Gm-Message-State":"AGi0PuYltu5VTVaVDkIC6rw2KQR09/uFp3pnvqsRhpza8yyIp6x6fqSp\n\t5ErXzapny7kCqVfEUS43uDIvQZ2rFGng3wrTymyFkw==","X-Google-Smtp-Source":"APiQypKMtZhSNF+SE1wj6Utcoks/3r40T1sEXw/uwyi4RdhDmhJLxLCupwfPceQfYD+MAC7Cx05KAY6VaL06Sdy+qIY=","X-Received":"by 2002:a2e:6a08:: with SMTP id f8mr5625241ljc.8.1587720413357; \n\tFri, 24 Apr 2020 02:26:53 -0700 (PDT)","MIME-Version":"1.0","References":"<20200403145305.10288-1-naush@raspberrypi.com>\n\t<20200403145305.10288-5-naush@raspberrypi.com>\n\t<20200423194334.GL6196@pendragon.ideasonboard.com>","In-Reply-To":"<20200423194334.GL6196@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 24 Apr 2020 10:26:37 +0100","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","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","X-List-Received-Date":"Fri, 24 Apr 2020 09:26:54 -0000"}},{"id":4497,"web_url":"https://patchwork.libcamera.org/comment/4497/","msgid":"<20200424101346.GA5954@pendragon.ideasonboard.com>","date":"2020-04-24T10:13:46","subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Apr 24, 2020 at 10:26:37AM +0100, Naushir Patuck wrote:\n> On Thu, 23 Apr 2020 at 20:43, Laurent Pinchart wrote:\n> > On Fri, Apr 03, 2020 at 03:53:04PM +0100, Naushir Patuck wrote:\n> > > AeMeteringMode, AeConstraintMode, and AeExposureMode are new enum type\n> > > controls used to specify operating modes in the AE algorithm. All modes\n> > > may not be supported by all platforms.\n> > >\n> > > ExposureValue is a new control used to set the log2 exposure adjustment\n> > > for the AE algorithm.\n> > >\n> > > Lux is a new control that returns the current lux estimate.\n> > >\n> > > Signed-off-by: Naushir Patuck \n> > > ---\n> > > src/libcamera/control_ids.yaml | 104 +++++++++++++++++++++++++++++++++\n> > > 1 file changed, 104 insertions(+)\n> > >\n> > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > index 64e81520..50f9e07b 100644\n> > > --- a/src/libcamera/control_ids.yaml\n> > > +++ b/src/libcamera/control_ids.yaml\n> > > @@ -4,6 +4,8 @@\n> > > #\n> > > %YAML 1.2\n> > > ---\n> > > +# Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > > +# set through a Request and returned out through Metadata.\n> >\n> > How about adding this to control_ids.cpp.in, to ensure it will end up in\n> > the generated documentation ?\n> >\n> \n> Yes, that's a good idea. Would it make sense if I make this change to\n> control_ids.cpp.in as a separate commit and make it the first commit\n> in the patchset?\n\nSure, or feel free to squash it in this patch (that was my initial\nplan).\n\n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index 99c511d0e712..cdd5d351ad6a 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -33,6 +33,10 @@ ${controls_def}\n> >\n> > /**\n> > * \\brief List of all supported libcamera controls\n> > + *\n> > + * Unless otherwise stated, all controls are bi-directional, i.e. they can be\n> > + * set through Request::controls() and returned out through\n> > + * Request::metadata().\n> > */\n> > extern const ControlIdMap controls {\n> > ${controls_map}\n> >\n> > > controls:\n> > > - AeEnable:\n> > > type: bool\n> > > @@ -23,6 +25,103 @@ controls:\n> > >\n> > > \\sa AeEnable\n> > >\n> >\n> > The controls below look good to me. I'll add a few comments regarding\n> > possible future developments, they're not meant to be addressed today.\n> >\n> > > + # AeMeteringMode needs further attention:\n> > > + # - Auto-generate max enum value.\n> > > + # - Better handling of custom types.\n> > > + - AeMeteringMode:\n> > > + type: int32_t\n> > > + description: |\n> > > + Specify a metering mode for the AE algorithm to use. The metering\n> > > + modes determine which parts of the image are used to determine the\n> > > + scene brightness. Metering modes may be platform specific and not\n> > > + all metering modes may be supported.\n> > > + enum:\n> > > + - name: MeteringCentreWeighted\n> > > + value: 0\n> > > + description: Centre-weighted metering mode.\n> > > + - name: MeteringSpot\n> > > + value: 1\n> > > + description: Spot metering mode.\n> > > + - name: MeteringMatrix\n> > > + value: 2\n> > > + description: Matrix metering mode.\n> >\n> > I think we'll need a way to configure both the spot and matrix modes\n> > with a point and a matrix of weights, respectively. At that point I\n> > think we could possibly merge the two together, as spot mode is\n> > essentially matrix mode with a single region, right ?\n> \n> Yes, if we have (somewhat) arbitrary freedom to assign weights and\n> regions this should be possible.\n> \n> > > + - name: MeteringCustom\n> > > + value: 3\n> > > + description: Custom metering mode.\n> > > + - name: MeteringModeMax\n> > > + value: 3\n> > > + description: Maximum allowed value (place any new values above here).\n> > > +\n> > > + # AeConstraintMode needs further attention:\n> > > + # - Auto-generate max enum value.\n> > > + # - Better handling of custom types.\n> > > + - AeConstraintMode:\n> > > + type: int32_t\n> > > + description: |\n> > > + Specify a constraint mode for the AE algorithm to use. These determine\n> > > + how the measured scene brightness is adjusted to reach the desired\n> > > + target exposure. Constraint modes may be platform specific, and not\n> > > + all constraint modes may be supported.\n> > > + enum:\n> > > + - name: ConstraintNormal\n> > > + value: 0\n> > > + description: Default constraint mode.\n> > > + This mode aims to balance the exposure of different parts of the\n> > > + image so as to reach a reasonable average level. However, highlights\n> > > + in the image may appear over-exposed and lowlights may appear\n> > > + under-exposed.\n> > > + - name: ConstraintHighlight\n> > > + value: 1\n> > > + description: Highlight constraint mode.\n> > > + This mode adjusts the exposure levels in order to try and avoid\n> > > + over-exposing the brightest parts (highlights) of an image.\n> > > + Other non-highlight parts of the image may appear under-exposed.\n> >\n> > Do we need ConstraintLowlight too ?\n> \n> Yes, that makes sense, I'll add a new mode. We don't currently have\n> tuning that will handle it, but that can come later.\n> One suggestion, can I call it ConstraintShadows instead of LowLight as\n> it's a constraint on the amount of under-exposed area?\n\nWorks for me.\n\n> > > + - name: ConstraintCustom\n> > > + value: 2\n> > > + description: Custom constraint mode.\n> > > + - name: ConstraintModeMax\n> > > + value: 2\n> > > + description: Maximum allowed value (place any new values above here).\n> > > +\n> > > + # AeExposureMode needs further attention:\n> > > + # - Auto-generate max enum value.\n> > > + # - Better handling of custom types.\n> > > + - AeExposureMode:\n> > > + type: int32_t\n> > > + description: |\n> > > + Specify an exposure mode for the AE algorithm to use. These specify\n> > > + how the desired total exposure is divided between the shutter time\n> > > + and the sensor's analogue gain. The exposure modes are platform\n> > > + specific, and not all exposure modes may be supported.\n> > > + enum:\n> > > + - name: ExposureNormal\n> > > + value: 0\n> > > + description: Default metering mode.\n> >\n> > Is this a metering mode, or is that a copy&paste ? :-)\n> \n> Oops, will fix.\n> \n> > > + - name: ExposureShort\n> > > + value: 1\n> > > + description: Exposure mode allowing only short exposure times.\n> > > + - name: ExposureLong\n> > > + value: 2\n> > > + description: Exposure mode allowing long exposure times.\n> > > + - name: ExposureCustom\n> > > + value: 3\n> > > + description: Custom exposure mode.\n> > > + - name: ExposureModeMax\n> > > + value: 3\n> > > + description: Maximum allowed value (place any new values above here).\n> > > +\n> > > + - ExposureValue:\n> > > + type: float\n> > > + description: |\n> > > + Specify an Exposure Value (EV) parameter. The EV parameter will only be\n> > > + applied if the AE algorithm is currently enabled.\n> > > +\n> > > + By convention EV adjusts the exposure as log2. For example\n> > > + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > > + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n> > > +\n> > > + \\sa AeEnable\n> > > +\n> > > - ExposureTime:\n> > > type: int32_t\n> > > description: |\n> > > @@ -53,6 +152,11 @@ controls:\n> > > Specify a fixed contrast parameter. Normal contrast is given by the\n> > > value 1.0; larger values produce images with more contrast.\n> > >\n> > > + # Lux can only be returned in Metadata\n> > > + - Lux:\n> > > + type: float\n> > > + description: Report an estimate of the current lux level.\n> > > +\n> >\n> > And here, to replace the comment,\n> >\n> > - Lux:\n> > type: float\n> > description: |\n> > Report an estimate of the current lux level. The Lux control can only\n> > be returned in metadata.\n> >\n> > and maybe nitpicking, should it be \"estimate of the current illuminance\n> > in lux\" ? The control could also be called Illuminance. I'll leave this\n> > up to you.\n> \n> Done. I think we should keep the control name as Lux which is a\n> standard imaging term.\n\nSure.\n\n> > With these small issues addressed (except for the comments related to\n> > AeMeteringMode and AeConstraintMode that are really related to future\n> > work),\n> >\n> > Reviewed-by: Laurent Pinchart \n> >\n> > > - AwbEnable:\n> > > type: bool\n> > > description: |","headers":{"Return-Path":"","Received":["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 912AD603FC\n\tfor ;\n\tFri, 24 Apr 2020 12:14:01 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0F3E14F7;\n\tFri, 24 Apr 2020 12:14:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rWRSg48z\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1587723241;\n\tbh=DIYpu0csblaNyrNFH912K4GF92LNwaIpNi+ISLt1h4Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rWRSg48zYprzMBsSFuZOKianV5s0lJkpd0EWqy1ZssjdxnHEH7zi5Cqd/dcFp+ciI\n\tFoXHMfwkTY6krMplm6UaN0iGqhxvonRctWIQRc0d7lLWV7koImFvveG4Jr9MMk06JG\n\teBGGKYnuARQp1WxUfG3gdC40NphdIkFVcSUBo1uc=","Date":"Fri, 24 Apr 2020 13:13:46 +0300","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200424101346.GA5954@pendragon.ideasonboard.com>","References":"<20200403145305.10288-1-naush@raspberrypi.com>\n\t<20200403145305.10288-5-naush@raspberrypi.com>\n\t<20200423194334.GL6196@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: controls: Add AE\n\trelated controls","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","X-List-Received-Date":"Fri, 24 Apr 2020 10:14:01 -0000"}}]