[{"id":4147,"web_url":"https://patchwork.libcamera.org/comment/4147/","msgid":"<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>","date":"2020-03-20T14:58:09","subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE\n\trelated controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> adjustment for the AE algorithm.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> 1 file changed, 109 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 5bbe65ae..da1a7b43 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -23,24 +23,104 @@ controls:\n> \n> \\sa AeEnable\n> \n> - - AwbEnable:\n> - type: bool\n> + - AeMeteringMode:\n> + type: int32_t\n> description: |\n> - Enable or disable the AWB.\n> -\n> - \\sa ManualGain\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\nOnly caught because of the spelling error below, but in the other\ninstances you do not hyphenate platform-specific. Maybe the simple\noption is to not hyphenate here either :-)\n\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> + - name: MeteringCustom1\n> + value: 3\n\nThis feels a bit painful if we want to add more (non-custom, or custom)\nmodes.\n\nWe would only be able to append to prevent ABI breakage - Then the enums\ncould seem a bit ugly:\n\n enum {\n\tMeteringCentreWeighted,\n\tMeteringSpot,\n\tMeteringMatrix,\n\tMeteringCustom1,\n\tMeteringCustom2,\n\tMeteringCustom3,\n\tMeteringMadeUpGeneralControl,\n\tMeteringCustom4,\n\tMeteringMadeupButGeneralControl2,\n }\n\nBut I fear that may not be something we can easily avoid unless we know\n*all* of the metering mode names in advance...\n\nOf course we're intentionally not yet ABI stable anyway, so this could\nstill be an intermediate step...\n\n\nOne option could be to have MeteringCustom, with the actual 'custom'\nchoice provided by another means ('yet another' control?), but maybe\nthat doesn't scale well either. I'm not sure yet...\n\n> + description: Custom metering mode 1.\n> + - name: MeteringCustom2\n> + value: 4\n> + description: Custom metering mode 2.\n> + - name: MeteringCustom3\n> + value: 5\n> + description: Custom metering mode 3.\n\nAre you able to express what you currently envisage the 3 custom modes\nwould be used for?\n\n> + - name: MeteringModeMax\n> + value: 5\nIt's a shame we have to express the enum max value manually here :-(\nI haven't looked at if it could be identified through code though.\n\n> + description: Maximum allowed value (place any new values above here).\n> \n> - - Brightness:\n> + - AeConstraintMode:\n> type: int32_t\n> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n\n/constaint/constraint/\n\n> + enum:\n> + - name: ConstraintNormal\n> + value: 0\n> + description: Default constraint mode.\n> + - name: ConstraintHighlight\n> + value: 1\n> + description: Highlight constraint mode.\n> + - name: ConstraintCustom1\n> + value: 2\n> + description: Custom constraint mode 1.\n> + - name: ConstraintCustom2\n> + value: 3\n> + description: Custom constraint mode 2.\n> + - name: ConstraintCustom3\n> + value: 4\n> + description: Custom constraint mode 3.\n> + - name: ConstraintModeMax\n> + value: 4\n> + description: Maximum allowed value (place any new values above here).\n\nSame comments apply here of course.\n\nPerhaps we should expand the initial set from the modes/constraints that\nwe will need for supporting the Android HAL...\n\n\n> \n> - - Contrast:\n> + - AeExposureMode:\n> type: int32_t\n> - description: Specify a fixed contrast parameter\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> + spcific, and not all exposure modes may be supported.\n\ns/spcific/specific/\n\n> + enum:\n> + - name: ExposureNormal\n> + value: 0\n> + description: Default metering mode.\n> + - name: ExposureSport\n> + value: 1\n> + description: Sport metering mode (favouring short shutter times).\n> + - name: ExposureLong\n> + value: 2\n> + description: Exposure mode allowing long exposure times.\n> + - name: ExposureCustom1\n> + value: 3\n> + description: Custom exposure mode 1.\n> + - name: ExposureCustom2\n> + value: 4\n> + description: Custom exposure mode 2.\n> + - name: ExposureCustom3\n> + value: 5\n> + description: Custom exposure mode 3.\n> + - name: ExposureModeMax\n> + value: 5\n> + description: Maximum allowed value (place any new values above here).\n> \n> - - Saturation:\n> - type: int32_t\n> - description: Specify a fixed saturation parameter\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 alogrithm 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> - ManualExposure:\n> type: int32_t\n> @@ -57,4 +137,21 @@ controls:\n> applied to all colour channels.\n> \n> \\sa ManualExposure\n> +\n\nWhat changes were made to the properties below ? (I don't see anything\nin particular) or is that just git being a pain and adding churn?\n\n\n\n> + - AwbEnable:\n> + type: bool\n> + description: |\n> + Enable or disable the AWB.\n> +\n> + - Brightness:\n> + type: int32_t\n> + description: Specify a fixed brightness parameter\n> +\n> + - Contrast:\n> + type: int32_t\n> + description: Specify a fixed contrast parameter\n> +\n> + - Saturation:\n> + type: int32_t\n> + description: Specify a fixed saturation parameter\n> ...\n>","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 8F40860416\n\tfor ;\n\tFri, 20 Mar 2020 15:58:12 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F3DA2504;\n\tFri, 20 Mar 2020 15:58:11 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584716292;\n\tbh=mO/z7Ufoui/ebPwBgM99ypJhk1OSyo4IguahGpQ0DVw=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=TKXMVHDSJRzAB41sN25fyefQV13isGF4gNKSBl0u514wvO7D/Jfg5qvKvws12AldX\n\ti9eXov5Q/IMbBTM/3fDO2AwEMHLXQmqJLjvKIKESyXdl2zVOf+4f8JajJl9koBrGDu\n\tRf3azACtG0ifxTIy3/KrKVY8OWRBZFhKkvmP7r/U=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>","Date":"Fri, 20 Mar 2020 14:58:09 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"<20200309123319.630-4-naush@raspberrypi.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 20 Mar 2020 14:58:12 -0000"}},{"id":4214,"web_url":"https://patchwork.libcamera.org/comment/4214/","msgid":"","date":"2020-03-23T15:29:18","subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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 Kieran,\n\nOn Fri, 20 Mar 2020 at 14:58, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> > adjustment for the AE algorithm.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> > 1 file changed, 109 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 5bbe65ae..da1a7b43 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -23,24 +23,104 @@ controls:\n> >\n> > \\sa AeEnable\n> >\n> > - - AwbEnable:\n> > - type: bool\n> > + - AeMeteringMode:\n> > + type: int32_t\n> > description: |\n> > - Enable or disable the AWB.\n> > -\n> > - \\sa ManualGain\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>\n> Only caught because of the spelling error below, but in the other\n> instances you do not hyphenate platform-specific. Maybe the simple\n> option is to not hyphenate here either :-)\n>\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> > + - name: MeteringCustom1\n> > + value: 3\n>\n> This feels a bit painful if we want to add more (non-custom, or custom)\n> modes.\n>\n> We would only be able to append to prevent ABI breakage - Then the enums\n> could seem a bit ugly:\n>\n> enum {\n> MeteringCentreWeighted,\n> MeteringSpot,\n> MeteringMatrix,\n> MeteringCustom1,\n> MeteringCustom2,\n> MeteringCustom3,\n> MeteringMadeUpGeneralControl,\n> MeteringCustom4,\n> MeteringMadeupButGeneralControl2,\n> }\n>\n> But I fear that may not be something we can easily avoid unless we know\n> *all* of the metering mode names in advance...\n>\n> Of course we're intentionally not yet ABI stable anyway, so this could\n> still be an intermediate step...\n>\n>\n> One option could be to have MeteringCustom, with the actual 'custom'\n> choice provided by another means ('yet another' control?), but maybe\n> that doesn't scale well either. I'm not sure yet...\n>\n> > + description: Custom metering mode 1.\n> > + - name: MeteringCustom2\n> > + value: 4\n> > + description: Custom metering mode 2.\n> > + - name: MeteringCustom3\n> > + value: 5\n> > + description: Custom metering mode 3.\n>\n> Are you able to express what you currently envisage the 3 custom modes\n> would be used for?\n>\n\nThe intention here was to allow users to add new modes to the\nalgorithm, perhaps something very specific to their application. If\nthe tuning parameters for the mode could be added/tweaked via a\ntext/config file, then the user could directly use the new mode\nwithout ever having to download/re-compile any code. Initially (as in\npatch v1), these modes were defined as std::string types, but we\nconcluded that added too much vagueness to the list of options.\nPerhaps 3 custom modes is too much, and we can make do with just one?\n\n> > + - name: MeteringModeMax\n> > + value: 5\n> It's a shame we have to express the enum max value manually here :-(\n> I haven't looked at if it could be identified through code though.\n>\n\nThis is primarily needed for setting up ControlRange for the control.\nIt should be entirely possible for the gen_controls script to add the\nMax item to the end of the enum?\n\n> > + description: Maximum allowed value (place any new values above here).\n> >\n> > - - Brightness:\n> > + - AeConstraintMode:\n> > type: int32_t\n> > - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n>\n> /constaint/constraint/\n>\n> > + enum:\n> > + - name: ConstraintNormal\n> > + value: 0\n> > + description: Default constraint mode.\n> > + - name: ConstraintHighlight\n> > + value: 1\n> > + description: Highlight constraint mode.\n> > + - name: ConstraintCustom1\n> > + value: 2\n> > + description: Custom constraint mode 1.\n> > + - name: ConstraintCustom2\n> > + value: 3\n> > + description: Custom constraint mode 2.\n> > + - name: ConstraintCustom3\n> > + value: 4\n> > + description: Custom constraint mode 3.\n> > + - name: ConstraintModeMax\n> > + value: 4\n> > + description: Maximum allowed value (place any new values above here).\n>\n> Same comments apply here of course.\n>\n> Perhaps we should expand the initial set from the modes/constraints that\n> we will need for supporting the Android HAL...\n>\n>\n> >\n> > - - Contrast:\n> > + - AeExposureMode:\n> > type: int32_t\n> > - description: Specify a fixed contrast parameter\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> > + spcific, and not all exposure modes may be supported.\n>\n> s/spcific/specific/\n>\n> > + enum:\n> > + - name: ExposureNormal\n> > + value: 0\n> > + description: Default metering mode.\n> > + - name: ExposureSport\n> > + value: 1\n> > + description: Sport metering mode (favouring short shutter times).\n> > + - name: ExposureLong\n> > + value: 2\n> > + description: Exposure mode allowing long exposure times.\n> > + - name: ExposureCustom1\n> > + value: 3\n> > + description: Custom exposure mode 1.\n> > + - name: ExposureCustom2\n> > + value: 4\n> > + description: Custom exposure mode 2.\n> > + - name: ExposureCustom3\n> > + value: 5\n> > + description: Custom exposure mode 3.\n> > + - name: ExposureModeMax\n> > + value: 5\n> > + description: Maximum allowed value (place any new values above here).\n> >\n> > - - Saturation:\n> > - type: int32_t\n> > - description: Specify a fixed saturation parameter\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 alogrithm 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> > - ManualExposure:\n> > type: int32_t\n> > @@ -57,4 +137,21 @@ controls:\n> > applied to all colour channels.\n> >\n> > \\sa ManualExposure\n> > +\n>\n> What changes were made to the properties below ? (I don't see anything\n> in particular) or is that just git being a pain and adding churn?\n>\n\nI only moved them around - so all the controls in the file are ordered\nin appropriate groups (AGC, AWB, General...).\n\n>\n>\n> > + - AwbEnable:\n> > + type: bool\n> > + description: |\n> > + Enable or disable the AWB.\n> > +\n> > + - Brightness:\n> > + type: int32_t\n> > + description: Specify a fixed brightness parameter\n> > +\n> > + - Contrast:\n> > + type: int32_t\n> > + description: Specify a fixed contrast parameter\n> > +\n> > + - Saturation:\n> > + type: int32_t\n> > + description: Specify a fixed saturation parameter\n> > ...\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9613460417\n\tfor ;\n\tMon, 23 Mar 2020 16:29:37 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id t17so4356086ljc.12\n\tfor ;\n\tMon, 23 Mar 2020 08:29:37 -0700 (PDT)"],"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=YvJbVrZQpDWo0J4biZf7+ap/t+xxY6eHPmctqu+5tk4=;\n\tb=SX2p0Uc+DNcHcR4L1bULnDfCP1+pIfNOm/RlkUCtx/lM2ZtKiIHLR2A0qdpdsu7sx1\n\t7O79WS8oyLaaTIVwt9AFjDTDN7yYaJsj29F+yud9kXYeYty5x/qFbOjTHF8HaAmOs4g/\n\tn3Hp77yuK4nHozFMQV/VDOyHJmEVPlJi6OhynE7W1sYgbkxvCyfgU+Cwf1pJBaZb5FWO\n\t8LD+YWQyL1xGGrSdYbeiF75iKWULbm6b0p0uHlsAlnGZHs+cne8W+/2FIkvsDWUul5S8\n\tGID9Lbm9YK0TgaQ1qv3ATwXTNuoH04jYI6ojwWHehzePRCKQl1obI3Jh9expENJ2kJB+\n\tYaaQ==","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=YvJbVrZQpDWo0J4biZf7+ap/t+xxY6eHPmctqu+5tk4=;\n\tb=UpB74onSMHtQNtiV7ZAm5Xrs5qOLVXrkURBh/98IgwQ0cKc16cpEsR1Li02MJcIdLh\n\ttVBCNshd5xdLHzjlmD8jWwc2ZBhlmyxuuiHb7f8+tUe53sH/an9zCxZNW0d+ooXGAPHG\n\tR3UZddW2zTtJqMNa1ZooFCS8Xa8CfNsieNXkclNhsIEBAsN1GTvwe9oiUq0yizS+/Exh\n\tXRD5rn4KXBnLJj04vTPwuwMT/1m5JwXEJWf9xuchTYCtMXfGaXpM6Pjzxaze+ZdQMYy5\n\tDP/bzVg9id72g5rbepcjmSfCdsoqviY9X4ni9Bm0z4bYZAdgehjiu2W02hJzD/WR8bZt\n\tGkSA==","X-Gm-Message-State":"ANhLgQ2ZAjnS4LWEpbNWgFtFVtLRZZ8S3KI2DnV+JRA8Ww7Mc/o1LJ9W\n\t/FJJn3eaCkXc1HRD0c5gklU+L5t9lF3M3oZFZgaKDP+g","X-Google-Smtp-Source":"ADFU+vu7XYy4Lg1tVmxU9EjCwZKTrlMNP2WnYJwvfNFIxsd40D+RUvt+iB3+G8/jeH6RSh4FtfSCsOJXgE9chK3EOPg=","X-Received":"by 2002:a2e:9ccd:: with SMTP id\n\tg13mr5117831ljj.147.1584977375573; \n\tMon, 23 Mar 2020 08:29:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>","In-Reply-To":"<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 23 Mar 2020 15:29:18 +0000","Message-ID":"","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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":"Mon, 23 Mar 2020 15:29:37 -0000"}},{"id":4229,"web_url":"https://patchwork.libcamera.org/comment/4229/","msgid":"","date":"2020-03-23T16:42:27","subject":"Re: [libcamera-devel] [PATCH v2 3/6] libcamera: controls: Add AE\n\trelated controls","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 23/03/2020 15:29, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham\n> wrote:\n>>\n>> Hi Naush,\n>>\n>> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n>>> adjustment for the AE algorithm.\n>>>\n>>> Signed-off-by: Naushir Patuck \n>>> ---\n>>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n>>> 1 file changed, 109 insertions(+), 12 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n>>> index 5bbe65ae..da1a7b43 100644\n>>> --- a/src/libcamera/control_ids.yaml\n>>> +++ b/src/libcamera/control_ids.yaml\n>>> @@ -23,24 +23,104 @@ controls:\n>>>\n>>> \\sa AeEnable\n>>>\n>>> - - AwbEnable:\n>>> - type: bool\n>>> + - AeMeteringMode:\n>>> + type: int32_t\n>>> description: |\n>>> - Enable or disable the AWB.\n>>> -\n>>> - \\sa ManualGain\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>>\n>> Only caught because of the spelling error below, but in the other\n>> instances you do not hyphenate platform-specific. Maybe the simple\n>> option is to not hyphenate here either :-)\n>>\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>>> + - name: MeteringCustom1\n>>> + value: 3\n>>\n>> This feels a bit painful if we want to add more (non-custom, or custom)\n>> modes.\n>>\n>> We would only be able to append to prevent ABI breakage - Then the enums\n>> could seem a bit ugly:\n>>\n>> enum {\n>> MeteringCentreWeighted,\n>> MeteringSpot,\n>> MeteringMatrix,\n>> MeteringCustom1,\n>> MeteringCustom2,\n>> MeteringCustom3,\n>> MeteringMadeUpGeneralControl,\n>> MeteringCustom4,\n>> MeteringMadeupButGeneralControl2,\n>> }\n>>\n>> But I fear that may not be something we can easily avoid unless we know\n>> *all* of the metering mode names in advance...\n>>\n>> Of course we're intentionally not yet ABI stable anyway, so this could\n>> still be an intermediate step...\n>>\n>>\n>> One option could be to have MeteringCustom, with the actual 'custom'\n>> choice provided by another means ('yet another' control?), but maybe\n>> that doesn't scale well either. I'm not sure yet...\n>>\n>>> + description: Custom metering mode 1.\n>>> + - name: MeteringCustom2\n>>> + value: 4\n>>> + description: Custom metering mode 2.\n>>> + - name: MeteringCustom3\n>>> + value: 5\n>>> + description: Custom metering mode 3.\n>>\n>> Are you able to express what you currently envisage the 3 custom modes\n>> would be used for?\n>>\n> \n> The intention here was to allow users to add new modes to the\n> algorithm, perhaps something very specific to their application. If\n> the tuning parameters for the mode could be added/tweaked via a\n> text/config file, then the user could directly use the new mode\n> without ever having to download/re-compile any code. Initially (as in\n> patch v1), these modes were defined as std::string types, but we\n> concluded that added too much vagueness to the list of options.\n> Perhaps 3 custom modes is too much, and we can make do with just one?\n\nMaybe one is enough I don't know. That's why I wondered about one custom\ncontrol which has a secondary option to give more flexibility.\n\nOtherwise '3' seems like a very arbitrary definition ...\n\n\n> \n>>> + - name: MeteringModeMax\n>>> + value: 5\n>> It's a shame we have to express the enum max value manually here :-(\n>> I haven't looked at if it could be identified through code though.\n>>\n> \n> This is primarily needed for setting up ControlRange for the control.\n> It should be entirely possible for the gen_controls script to add the\n> Max item to the end of the enum?\n\nAha, then indeed perhaps we should automate the creation somehow\n\n> \n>>> + description: Maximum allowed value (place any new values above here).\n>>>\n>>> - - Brightness:\n>>> + - AeConstraintMode:\n>>> type: int32_t\n>>> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n>>\n>> /constaint/constraint/\n>>\n>>> + enum:\n>>> + - name: ConstraintNormal\n>>> + value: 0\n>>> + description: Default constraint mode.\n>>> + - name: ConstraintHighlight\n>>> + value: 1\n>>> + description: Highlight constraint mode.\n>>> + - name: ConstraintCustom1\n>>> + value: 2\n>>> + description: Custom constraint mode 1.\n>>> + - name: ConstraintCustom2\n>>> + value: 3\n>>> + description: Custom constraint mode 2.\n>>> + - name: ConstraintCustom3\n>>> + value: 4\n>>> + description: Custom constraint mode 3.\n>>> + - name: ConstraintModeMax\n>>> + value: 4\n>>> + description: Maximum allowed value (place any new values above here).\n>>\n>> Same comments apply here of course.\n>>\n>> Perhaps we should expand the initial set from the modes/constraints that\n>> we will need for supporting the Android HAL...\n>>\n>>\n>>>\n>>> - - Contrast:\n>>> + - AeExposureMode:\n>>> type: int32_t\n>>> - description: Specify a fixed contrast parameter\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>>> + spcific, and not all exposure modes may be supported.\n>>\n>> s/spcific/specific/\n>>\n>>> + enum:\n>>> + - name: ExposureNormal\n>>> + value: 0\n>>> + description: Default metering mode.\n>>> + - name: ExposureSport\n>>> + value: 1\n>>> + description: Sport metering mode (favouring short shutter times).\n>>> + - name: ExposureLong\n>>> + value: 2\n>>> + description: Exposure mode allowing long exposure times.\n>>> + - name: ExposureCustom1\n>>> + value: 3\n>>> + description: Custom exposure mode 1.\n>>> + - name: ExposureCustom2\n>>> + value: 4\n>>> + description: Custom exposure mode 2.\n>>> + - name: ExposureCustom3\n>>> + value: 5\n>>> + description: Custom exposure mode 3.\n>>> + - name: ExposureModeMax\n>>> + value: 5\n>>> + description: Maximum allowed value (place any new values above here).\n>>>\n>>> - - Saturation:\n>>> - type: int32_t\n>>> - description: Specify a fixed saturation parameter\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 alogrithm 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>>> - ManualExposure:\n>>> type: int32_t\n>>> @@ -57,4 +137,21 @@ controls:\n>>> applied to all colour channels.\n>>>\n>>> \\sa ManualExposure\n>>> +\n>>\n>> What changes were made to the properties below ? (I don't see anything\n>> in particular) or is that just git being a pain and adding churn?\n>>\n> \n> I only moved them around - so all the controls in the file are ordered\n> in appropriate groups (AGC, AWB, General...).\n\nAha - in that case, it would generally be better to commit any\nnon-functional-change code move as a separate patch, so that it's clear\nand distinct on it's own.\n\n\n>>> + - AwbEnable:\n>>> + type: bool\n>>> + description: |\n>>> + Enable or disable the AWB.\n>>> +\n>>> + - Brightness:\n>>> + type: int32_t\n>>> + description: Specify a fixed brightness parameter\n>>> +\n>>> + - Contrast:\n>>> + type: int32_t\n>>> + description: Specify a fixed contrast parameter\n>>> +\n>>> + - Saturation:\n>>> + type: int32_t\n>>> + description: Specify a fixed saturation parameter\n>>> ...\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran\n> \n> Regards,\n> Naush\n>","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 7E8D760429\n\tfor ;\n\tMon, 23 Mar 2020 17:42:31 +0100 (CET)","from [192.168.0.20]\n\t(cpc89242-aztw30-2-0-cust488.18-1.cable.virginm.net [86.31.129.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id ED5A0308;\n\tMon, 23 Mar 2020 17:42:30 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1584981751;\n\tbh=slfiTK3Nq00mh5cfk6LmjmGgQnW136zu9db+/jMyoW8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=NWidW65WiWYZocDeOZTxeO+kSAn+ig8zCdf/hmos3tdXmANc/bVUNsyNWYJQQidox\n\tXQpRs/GyjjgmKb+go1O/Xn1LdxgWQzLoJLoumqlJ8bd1DflSfAYBECl49VTBMGiajT\n\tZWZKMeU6/ZhKlTiVcxzEd7tFNhlK/GgLMQdOcitM=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>\n\t","From":"Kieran Bingham ","Openpgp":"preference=signencrypt","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Mon, 23 Mar 2020 16:42:27 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.9.1","MIME-Version":"1.0","In-Reply-To":"","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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":"Mon, 23 Mar 2020 16:42:31 -0000"}},{"id":4315,"web_url":"https://patchwork.libcamera.org/comment/4315/","msgid":"<20200326162600.GV20581@pendragon.ideasonboard.com>","date":"2020-03-26T16:26:00","subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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 Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote:\n> On 23/03/2020 15:29, Naushir Patuck wrote:\n> > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote:\n> >> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> >>> adjustment for the AE algorithm.\n> >>>\n> >>> Signed-off-by: Naushir Patuck \n> >>> ---\n> >>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> >>> 1 file changed, 109 insertions(+), 12 deletions(-)\n> >>>\n> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>> index 5bbe65ae..da1a7b43 100644\n> >>> --- a/src/libcamera/control_ids.yaml\n> >>> +++ b/src/libcamera/control_ids.yaml\n> >>> @@ -23,24 +23,104 @@ controls:\n> >>>\n> >>> \\sa AeEnable\n> >>>\n> >>> - - AwbEnable:\n> >>> - type: bool\n> >>> + - AeMeteringMode:\n> >>> + type: int32_t\n> >>> description: |\n> >>> - Enable or disable the AWB.\n> >>> -\n> >>> - \\sa ManualGain\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> >>\n> >> Only caught because of the spelling error below, but in the other\n> >> instances you do not hyphenate platform-specific. Maybe the simple\n> >> option is to not hyphenate here either :-)\n> >>\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> >>> + - name: MeteringCustom1\n> >>> + value: 3\n> >>\n> >> This feels a bit painful if we want to add more (non-custom, or custom)\n> >> modes.\n> >>\n> >> We would only be able to append to prevent ABI breakage - Then the enums\n> >> could seem a bit ugly:\n> >>\n> >> enum {\n> >> MeteringCentreWeighted,\n> >> MeteringSpot,\n> >> MeteringMatrix,\n> >> MeteringCustom1,\n> >> MeteringCustom2,\n> >> MeteringCustom3,\n> >> MeteringMadeUpGeneralControl,\n> >> MeteringCustom4,\n> >> MeteringMadeupButGeneralControl2,\n> >> }\n> >>\n> >> But I fear that may not be something we can easily avoid unless we know\n> >> *all* of the metering mode names in advance...\n> >>\n> >> Of course we're intentionally not yet ABI stable anyway, so this could\n> >> still be an intermediate step...\n> >>\n> >> One option could be to have MeteringCustom, with the actual 'custom'\n> >> choice provided by another means ('yet another' control?), but maybe\n> >> that doesn't scale well either. I'm not sure yet...\n> >>\n> >>> + description: Custom metering mode 1.\n> >>> + - name: MeteringCustom2\n> >>> + value: 4\n> >>> + description: Custom metering mode 2.\n> >>> + - name: MeteringCustom3\n> >>> + value: 5\n> >>> + description: Custom metering mode 3.\n> >>\n> >> Are you able to express what you currently envisage the 3 custom modes\n> >> would be used for?\n> > \n> > The intention here was to allow users to add new modes to the\n> > algorithm, perhaps something very specific to their application. If\n> > the tuning parameters for the mode could be added/tweaked via a\n> > text/config file, then the user could directly use the new mode\n> > without ever having to download/re-compile any code. Initially (as in\n> > patch v1), these modes were defined as std::string types, but we\n> > concluded that added too much vagueness to the list of options.\n> > Perhaps 3 custom modes is too much, and we can make do with just one?\n> \n> Maybe one is enough I don't know. That's why I wondered about one custom\n> control which has a secondary option to give more flexibility.\n> \n> Otherwise '3' seems like a very arbitrary definition ...\n\nThis is indeed an issue, I'll try to sleep over it. Having a single\nvalue wouldn't be too bad when it comes to extending the enum, but that\nmay not be enough.\n\nThat being said, for this specific control, I wonder if we should make\nit a bit more finer-grained by specifying a list of weighted regions.\nJust brainstorming, the ControlInfo (previously known as ControlRange)\nclass could then possibly offer a set of defaults based on an enum.\n\n> >>> + - name: MeteringModeMax\n> >>> + value: 5\n> >>\n> >> It's a shame we have to express the enum max value manually here :-(\n> >> I haven't looked at if it could be identified through code though.\n> > \n> > This is primarily needed for setting up ControlRange for the control.\n> > It should be entirely possible for the gen_controls script to add the\n> > Max item to the end of the enum?\n> \n> Aha, then indeed perhaps we should automate the creation somehow\n\nYes, we should do that.\n\n> >>> + description: Maximum allowed value (place any new values above here).\n> >>>\n> >>> - - Brightness:\n> >>> + - AeConstraintMode:\n> >>> type: int32_t\n> >>> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n> >>\n> >> /constaint/constraint/\n> >>\n> >>> + enum:\n> >>> + - name: ConstraintNormal\n> >>> + value: 0\n> >>> + description: Default constraint mode.\n> >>> + - name: ConstraintHighlight\n> >>> + value: 1\n> >>> + description: Highlight constraint mode.\n\nWould it be possible to give a more detailed description of those modes\n?\n\n> >>> + - name: ConstraintCustom1\n> >>> + value: 2\n> >>> + description: Custom constraint mode 1.\n> >>> + - name: ConstraintCustom2\n> >>> + value: 3\n> >>> + description: Custom constraint mode 2.\n> >>> + - name: ConstraintCustom3\n> >>> + value: 4\n> >>> + description: Custom constraint mode 3.\n> >>> + - name: ConstraintModeMax\n> >>> + value: 4\n> >>> + description: Maximum allowed value (place any new values above here).\n> >>\n> >> Same comments apply here of course.\n> >>\n> >> Perhaps we should expand the initial set from the modes/constraints that\n> >> we will need for supporting the Android HAL...\n> >>\n> >>> - - Contrast:\n> >>> + - AeExposureMode:\n> >>> type: int32_t\n> >>> - description: Specify a fixed contrast parameter\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> >>> + spcific, and not all exposure modes may be supported.\n> >>\n> >> s/spcific/specific/\n> >>\n> >>> + enum:\n> >>> + - name: ExposureNormal\n> >>> + value: 0\n> >>> + description: Default metering mode.\n> >>> + - name: ExposureSport\n> >>> + value: 1\n> >>> + description: Sport metering mode (favouring short shutter times).\n\nReading the description, I wonder if this should be turned into a scene\nmode control, as it could also have an effect on other algorithms\n(auto-focus comes to mind for instance).\n\n> >>> + - name: ExposureLong\n> >>> + value: 2\n> >>> + description: Exposure mode allowing long exposure times.\n> >>> + - name: ExposureCustom1\n> >>> + value: 3\n> >>> + description: Custom exposure mode 1.\n> >>> + - name: ExposureCustom2\n> >>> + value: 4\n> >>> + description: Custom exposure mode 2.\n> >>> + - name: ExposureCustom3\n> >>> + value: 5\n> >>> + description: Custom exposure mode 3.\n> >>> + - name: ExposureModeMax\n> >>> + value: 5\n> >>> + description: Maximum allowed value (place any new values above here).\n> >>>\n> >>> - - Saturation:\n> >>> - type: int32_t\n> >>> - description: Specify a fixed saturation parameter\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 alogrithm 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> >>> - ManualExposure:\n> >>> type: int32_t\n> >>> @@ -57,4 +137,21 @@ controls:\n> >>> applied to all colour channels.\n> >>>\n> >>> \\sa ManualExposure\n> >>> +\n> >>\n> >> What changes were made to the properties below ? (I don't see anything\n> >> in particular) or is that just git being a pain and adding churn?\n> > \n> > I only moved them around - so all the controls in the file are ordered\n> > in appropriate groups (AGC, AWB, General...).\n> \n> Aha - in that case, it would generally be better to commit any\n> non-functional-change code move as a separate patch, so that it's clear\n> and distinct on it's own.\n> \n> >>> + - AwbEnable:\n> >>> + type: bool\n> >>> + description: |\n> >>> + Enable or disable the AWB.\n> >>> +\n> >>> + - Brightness:\n> >>> + type: int32_t\n> >>> + description: Specify a fixed brightness parameter\n> >>> +\n> >>> + - Contrast:\n> >>> + type: int32_t\n> >>> + description: Specify a fixed contrast parameter\n> >>> +\n> >>> + - Saturation:\n> >>> + type: int32_t\n> >>> + description: Specify a fixed saturation parameter\n> >>> ...","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 DF18460414\n\tfor ;\n\tThu, 26 Mar 2020 17:26:04 +0100 (CET)","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 4A6EF2DC;\n\tThu, 26 Mar 2020 17:26:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"FXF/w5qh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585239964;\n\tbh=kY6Ex9pBhYhiiElrmDXU9+nEZ4lJSaOQ/AJ3fMf9OHk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FXF/w5qhdPJfnk89kzu30IDMi27GuwQZwuGEgem2t7HxaG75fA6YLtI6IKu6WcmW/\n\thdIBrvTFqgWTGZUFH4cbYPdlDDHwob+fSWZnDi/lTBhOIPRPhQ2qnzOi3Itd74dJI6\n\tyq46UUojqf72q1Okox8k9I2Er/30to/MfzBdcTrI=","Date":"Thu, 26 Mar 2020 18:26:00 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200326162600.GV20581@pendragon.ideasonboard.com>","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 26 Mar 2020 16:26:05 -0000"}},{"id":4335,"web_url":"https://patchwork.libcamera.org/comment/4335/","msgid":"","date":"2020-03-27T10:56:14","subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 26 Mar 2020 at 16:26, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote:\n> > On 23/03/2020 15:29, Naushir Patuck wrote:\n> > > On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote:\n> > >> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> > >>> adjustment for the AE algorithm.\n> > >>>\n> > >>> Signed-off-by: Naushir Patuck \n> > >>> ---\n> > >>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> > >>> 1 file changed, 109 insertions(+), 12 deletions(-)\n> > >>>\n> > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > >>> index 5bbe65ae..da1a7b43 100644\n> > >>> --- a/src/libcamera/control_ids.yaml\n> > >>> +++ b/src/libcamera/control_ids.yaml\n> > >>> @@ -23,24 +23,104 @@ controls:\n> > >>>\n> > >>> \\sa AeEnable\n> > >>>\n> > >>> - - AwbEnable:\n> > >>> - type: bool\n> > >>> + - AeMeteringMode:\n> > >>> + type: int32_t\n> > >>> description: |\n> > >>> - Enable or disable the AWB.\n> > >>> -\n> > >>> - \\sa ManualGain\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> > >>\n> > >> Only caught because of the spelling error below, but in the other\n> > >> instances you do not hyphenate platform-specific. Maybe the simple\n> > >> option is to not hyphenate here either :-)\n> > >>\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> > >>> + - name: MeteringCustom1\n> > >>> + value: 3\n> > >>\n> > >> This feels a bit painful if we want to add more (non-custom, or custom)\n> > >> modes.\n> > >>\n> > >> We would only be able to append to prevent ABI breakage - Then the enums\n> > >> could seem a bit ugly:\n> > >>\n> > >> enum {\n> > >> MeteringCentreWeighted,\n> > >> MeteringSpot,\n> > >> MeteringMatrix,\n> > >> MeteringCustom1,\n> > >> MeteringCustom2,\n> > >> MeteringCustom3,\n> > >> MeteringMadeUpGeneralControl,\n> > >> MeteringCustom4,\n> > >> MeteringMadeupButGeneralControl2,\n> > >> }\n> > >>\n> > >> But I fear that may not be something we can easily avoid unless we know\n> > >> *all* of the metering mode names in advance...\n> > >>\n> > >> Of course we're intentionally not yet ABI stable anyway, so this could\n> > >> still be an intermediate step...\n> > >>\n> > >> One option could be to have MeteringCustom, with the actual 'custom'\n> > >> choice provided by another means ('yet another' control?), but maybe\n> > >> that doesn't scale well either. I'm not sure yet...\n> > >>\n> > >>> + description: Custom metering mode 1.\n> > >>> + - name: MeteringCustom2\n> > >>> + value: 4\n> > >>> + description: Custom metering mode 2.\n> > >>> + - name: MeteringCustom3\n> > >>> + value: 5\n> > >>> + description: Custom metering mode 3.\n> > >>\n> > >> Are you able to express what you currently envisage the 3 custom modes\n> > >> would be used for?\n> > >\n> > > The intention here was to allow users to add new modes to the\n> > > algorithm, perhaps something very specific to their application. If\n> > > the tuning parameters for the mode could be added/tweaked via a\n> > > text/config file, then the user could directly use the new mode\n> > > without ever having to download/re-compile any code. Initially (as in\n> > > patch v1), these modes were defined as std::string types, but we\n> > > concluded that added too much vagueness to the list of options.\n> > > Perhaps 3 custom modes is too much, and we can make do with just one?\n> >\n> > Maybe one is enough I don't know. That's why I wondered about one custom\n> > control which has a secondary option to give more flexibility.\n> >\n> > Otherwise '3' seems like a very arbitrary definition ...\n>\n> This is indeed an issue, I'll try to sleep over it. Having a single\n> value wouldn't be too bad when it comes to extending the enum, but that\n> may not be enough.\n>\n> That being said, for this specific control, I wonder if we should make\n> it a bit more finer-grained by specifying a list of weighted regions.\n> Just brainstorming, the ControlInfo (previously known as ControlRange)\n> class could then possibly offer a set of defaults based on an enum.\n>\n\nThis is possible, but it might get tricky trying to interoperate with\ndifferent vendor IPAs where the HW/SW capabilities may not be the\nsame.\n\n> > >>> + - name: MeteringModeMax\n> > >>> + value: 5\n> > >>\n> > >> It's a shame we have to express the enum max value manually here :-(\n> > >> I haven't looked at if it could be identified through code though.\n> > >\n> > > This is primarily needed for setting up ControlRange for the control.\n> > > It should be entirely possible for the gen_controls script to add the\n> > > Max item to the end of the enum?\n> >\n> > Aha, then indeed perhaps we should automate the creation somehow\n>\n> Yes, we should do that.\n\nAs mentioned previously, the max value is only used for ControlInfo\n(previously known as ControlRange). I do wonder if ControlInfo is a\nsensible thing to have for an enum though. Suppose an IPA allows only\nmetering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be\nspecified as (min, max) == (0, 5), but a value of 3 is invalid.\n\n>\n> > >>> + description: Maximum allowed value (place any new values above here).\n> > >>>\n> > >>> - - Brightness:\n> > >>> + - AeConstraintMode:\n> > >>> type: int32_t\n> > >>> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n> > >>\n> > >> /constaint/constraint/\n> > >>\n> > >>> + enum:\n> > >>> + - name: ConstraintNormal\n> > >>> + value: 0\n> > >>> + description: Default constraint mode.\n> > >>> + - name: ConstraintHighlight\n> > >>> + value: 1\n> > >>> + description: Highlight constraint mode.\n>\n> Would it be possible to give a more detailed description of those modes\n> ?\n\nWill do.\n\n>\n> > >>> + - name: ConstraintCustom1\n> > >>> + value: 2\n> > >>> + description: Custom constraint mode 1.\n> > >>> + - name: ConstraintCustom2\n> > >>> + value: 3\n> > >>> + description: Custom constraint mode 2.\n> > >>> + - name: ConstraintCustom3\n> > >>> + value: 4\n> > >>> + description: Custom constraint mode 3.\n> > >>> + - name: ConstraintModeMax\n> > >>> + value: 4\n> > >>> + description: Maximum allowed value (place any new values above here).\n> > >>\n> > >> Same comments apply here of course.\n> > >>\n> > >> Perhaps we should expand the initial set from the modes/constraints that\n> > >> we will need for supporting the Android HAL...\n> > >>\n> > >>> - - Contrast:\n> > >>> + - AeExposureMode:\n> > >>> type: int32_t\n> > >>> - description: Specify a fixed contrast parameter\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> > >>> + spcific, and not all exposure modes may be supported.\n> > >>\n> > >> s/spcific/specific/\n> > >>\n> > >>> + enum:\n> > >>> + - name: ExposureNormal\n> > >>> + value: 0\n> > >>> + description: Default metering mode.\n> > >>> + - name: ExposureSport\n> > >>> + value: 1\n> > >>> + description: Sport metering mode (favouring short shutter times).\n>\n> Reading the description, I wonder if this should be turned into a scene\n> mode control, as it could also have an effect on other algorithms\n> (auto-focus comes to mind for instance).\n>\n\nI always looked as scene mode as a higher level construct within the\ncamera application. The application bunches a group of settings for a\nparticular scene mode (e.g. for Sport scene mode, AE turned to sport,\nfocus turned to fast converge, flash turned to fast sync) and sets\nthem individually. This way it could tailor them exactly to what it\nneeds.\n\n> > >>> + - name: ExposureLong\n> > >>> + value: 2\n> > >>> + description: Exposure mode allowing long exposure times.\n> > >>> + - name: ExposureCustom1\n> > >>> + value: 3\n> > >>> + description: Custom exposure mode 1.\n> > >>> + - name: ExposureCustom2\n> > >>> + value: 4\n> > >>> + description: Custom exposure mode 2.\n> > >>> + - name: ExposureCustom3\n> > >>> + value: 5\n> > >>> + description: Custom exposure mode 3.\n> > >>> + - name: ExposureModeMax\n> > >>> + value: 5\n> > >>> + description: Maximum allowed value (place any new values above here).\n> > >>>\n> > >>> - - Saturation:\n> > >>> - type: int32_t\n> > >>> - description: Specify a fixed saturation parameter\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 alogrithm 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> > >>> - ManualExposure:\n> > >>> type: int32_t\n> > >>> @@ -57,4 +137,21 @@ controls:\n> > >>> applied to all colour channels.\n> > >>>\n> > >>> \\sa ManualExposure\n> > >>> +\n> > >>\n> > >> What changes were made to the properties below ? (I don't see anything\n> > >> in particular) or is that just git being a pain and adding churn?\n> > >\n> > > I only moved them around - so all the controls in the file are ordered\n> > > in appropriate groups (AGC, AWB, General...).\n> >\n> > Aha - in that case, it would generally be better to commit any\n> > non-functional-change code move as a separate patch, so that it's clear\n> > and distinct on it's own.\n> >\n> > >>> + - AwbEnable:\n> > >>> + type: bool\n> > >>> + description: |\n> > >>> + Enable or disable the AWB.\n> > >>> +\n> > >>> + - Brightness:\n> > >>> + type: int32_t\n> > >>> + description: Specify a fixed brightness parameter\n> > >>> +\n> > >>> + - Contrast:\n> > >>> + type: int32_t\n> > >>> + description: Specify a fixed contrast parameter\n> > >>> +\n> > >>> + - Saturation:\n> > >>> + type: int32_t\n> > >>> + description: Specify a fixed saturation parameter\n> > >>> ...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lj1-x242.google.com (mail-lj1-x242.google.com\n\t[IPv6:2a00:1450:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5693E6040E\n\tfor ;\n\tFri, 27 Mar 2020 11:56:34 +0100 (CET)","by mail-lj1-x242.google.com with SMTP id r24so9726567ljd.4\n\tfor ;\n\tFri, 27 Mar 2020 03:56:34 -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=\"SXbj/ylC\"; 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=no0rNQoosLarMOgGR1bDAjlH3zc1F+PxsCHRkYJkYDs=;\n\tb=SXbj/ylCcwVRaij9Xn/WmQo3hP891B151dETC5rQ4eRu5xqBmWSuu5g1B7p53O14+d\n\tHbK+yJNCsCWlFllWBBB56u0bG1ya/0fB82v1zMN4Ielyp9P5vP8xoJI2pRAU7lXCRJiY\n\tSnh3sTZjTH/bme/Iy6FFty860R+NW7dXHEbT+rNyNwziXyQpPyqIlMYanqxsOc+t3qYR\n\teczcUcBX8lhxQur7asEOdZ9WYJqCiCKqJJ/oXJZrSyXCp3sFGj7ZLgZxJBSxbAUY/lJ6\n\tqc2qsa4YqVJrX2vDtT9So0soAC/bi54CeHeSU0ZsVyqUQZ0/ld5yJsnL86svfg5EF37d\n\tPUlQ==","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=no0rNQoosLarMOgGR1bDAjlH3zc1F+PxsCHRkYJkYDs=;\n\tb=ueLwTeWZ/KQqnIH09zyZySXf3RI7Ribw8q4NuhsRnyvy170IxCCWbq74LU6UucvulO\n\tD+SYyvL/l32nsipIdbIDfYEAF+5GyGDCg8snMg9tip8ss21vddnpjAI9Gt+QG4wa9DsI\n\t/3xpZwUHwx/vzULjBroWGzafSEJFbO4cwY3BDrz6qj39r615HFas6kj2l/lBVRLjE935\n\tvmoz2l0IDXDSdz8xPxIj/f9grmOTcZO9DjVlJ7cYC26+8SAZcLpOXuLOpt9z8CaEELTH\n\tOxRj772zDvJsTGFuCl8jTEo8kd6xpuhM7Gpp3P8L4nBMTxcV14DKjqm27WtbNrMvipMf\n\tgkdg==","X-Gm-Message-State":"AGi0PubTougLrxWkXiIlcY2Z6p/WSm9bTZ1E29b2ZqR6fE5QEhcUW+Kp\n\tcffD373QrXNIiU015kq2iWY1mnVc7gUMhokJOjT4CA==","X-Google-Smtp-Source":"ADFU+vsR4+gaeLTlTP9lyMYtOOx1hCNvUpyTsSmCBZXQS1j2q7Sa7WKE9c6DQ+FaiVlQpKIvT01mo7QiNyNlrBJa0AM=","X-Received":"by 2002:a2e:85cb:: with SMTP id h11mr8142376ljj.55.1585306593263;\n\tFri, 27 Mar 2020 03:56:33 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>\n\t\n\t\n\t<20200326162600.GV20581@pendragon.ideasonboard.com>","In-Reply-To":"<20200326162600.GV20581@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 27 Mar 2020 10:56:14 +0000","Message-ID":"","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 27 Mar 2020 10:56:34 -0000"}},{"id":4337,"web_url":"https://patchwork.libcamera.org/comment/4337/","msgid":"<20200327110310.GE5040@pendragon.ideasonboard.com>","date":"2020-03-27T11:03:10","subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, Mar 27, 2020 at 10:56:14AM +0000, Naushir Patuck wrote:\n> On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart wrote:\n> > On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote:\n> >> On 23/03/2020 15:29, Naushir Patuck wrote:\n> >>> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote:\n> >>>> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> >>>>> adjustment for the AE algorithm.\n> >>>>>\n> >>>>> Signed-off-by: Naushir Patuck \n> >>>>> ---\n> >>>>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> >>>>> 1 file changed, 109 insertions(+), 12 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>>>> index 5bbe65ae..da1a7b43 100644\n> >>>>> --- a/src/libcamera/control_ids.yaml\n> >>>>> +++ b/src/libcamera/control_ids.yaml\n> >>>>> @@ -23,24 +23,104 @@ controls:\n> >>>>>\n> >>>>> \\sa AeEnable\n> >>>>>\n> >>>>> - - AwbEnable:\n> >>>>> - type: bool\n> >>>>> + - AeMeteringMode:\n> >>>>> + type: int32_t\n> >>>>> description: |\n> >>>>> - Enable or disable the AWB.\n> >>>>> -\n> >>>>> - \\sa ManualGain\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> >>>>\n> >>>> Only caught because of the spelling error below, but in the other\n> >>>> instances you do not hyphenate platform-specific. Maybe the simple\n> >>>> option is to not hyphenate here either :-)\n> >>>>\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> >>>>> + - name: MeteringCustom1\n> >>>>> + value: 3\n> >>>>\n> >>>> This feels a bit painful if we want to add more (non-custom, or custom)\n> >>>> modes.\n> >>>>\n> >>>> We would only be able to append to prevent ABI breakage - Then the enums\n> >>>> could seem a bit ugly:\n> >>>>\n> >>>> enum {\n> >>>> MeteringCentreWeighted,\n> >>>> MeteringSpot,\n> >>>> MeteringMatrix,\n> >>>> MeteringCustom1,\n> >>>> MeteringCustom2,\n> >>>> MeteringCustom3,\n> >>>> MeteringMadeUpGeneralControl,\n> >>>> MeteringCustom4,\n> >>>> MeteringMadeupButGeneralControl2,\n> >>>> }\n> >>>>\n> >>>> But I fear that may not be something we can easily avoid unless we know\n> >>>> *all* of the metering mode names in advance...\n> >>>>\n> >>>> Of course we're intentionally not yet ABI stable anyway, so this could\n> >>>> still be an intermediate step...\n> >>>>\n> >>>> One option could be to have MeteringCustom, with the actual 'custom'\n> >>>> choice provided by another means ('yet another' control?), but maybe\n> >>>> that doesn't scale well either. I'm not sure yet...\n> >>>>\n> >>>>> + description: Custom metering mode 1.\n> >>>>> + - name: MeteringCustom2\n> >>>>> + value: 4\n> >>>>> + description: Custom metering mode 2.\n> >>>>> + - name: MeteringCustom3\n> >>>>> + value: 5\n> >>>>> + description: Custom metering mode 3.\n> >>>>\n> >>>> Are you able to express what you currently envisage the 3 custom modes\n> >>>> would be used for?\n> >>>\n> >>> The intention here was to allow users to add new modes to the\n> >>> algorithm, perhaps something very specific to their application. If\n> >>> the tuning parameters for the mode could be added/tweaked via a\n> >>> text/config file, then the user could directly use the new mode\n> >>> without ever having to download/re-compile any code. Initially (as in\n> >>> patch v1), these modes were defined as std::string types, but we\n> >>> concluded that added too much vagueness to the list of options.\n> >>> Perhaps 3 custom modes is too much, and we can make do with just one?\n> >>\n> >> Maybe one is enough I don't know. That's why I wondered about one custom\n> >> control which has a secondary option to give more flexibility.\n> >>\n> >> Otherwise '3' seems like a very arbitrary definition ...\n> >\n> > This is indeed an issue, I'll try to sleep over it. Having a single\n> > value wouldn't be too bad when it comes to extending the enum, but that\n> > may not be enough.\n> >\n> > That being said, for this specific control, I wonder if we should make\n> > it a bit more finer-grained by specifying a list of weighted regions.\n> > Just brainstorming, the ControlInfo (previously known as ControlRange)\n> > class could then possibly offer a set of defaults based on an enum.\n> \n> This is possible, but it might get tricky trying to interoperate with\n> different vendor IPAs where the HW/SW capabilities may not be the\n> same.\n\nI agree, that's why I was wondering what would be a good way to express\nmetering (which will apply to AF too, and potentially AWB ?) in a\nflexible way that could give finer-grained control when the hardware and\nIPA support it, but wouldn't be awkward to use for devices that have\nmore limited capabilities. We don't have to fix all of this now, it\ncould come as a later rework of the controls, but I wanted to raise the\nissue already in case we could figure out a solution in the short term.\n\n> >>>>> + - name: MeteringModeMax\n> >>>>> + value: 5\n> >>>>\n> >>>> It's a shame we have to express the enum max value manually here :-(\n> >>>> I haven't looked at if it could be identified through code though.\n> >>>\n> >>> This is primarily needed for setting up ControlRange for the control.\n> >>> It should be entirely possible for the gen_controls script to add the\n> >>> Max item to the end of the enum?\n> >>\n> >> Aha, then indeed perhaps we should automate the creation somehow\n> >\n> > Yes, we should do that.\n> \n> As mentioned previously, the max value is only used for ControlInfo\n> (previously known as ControlRange). I do wonder if ControlInfo is a\n> sensible thing to have for an enum though. Suppose an IPA allows only\n> metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be\n> specified as (min, max) == (0, 5), but a value of 3 is invalid.\n\nI agree with you. We need ControlInfo to report the valid values for\nenumerated controls. Another item on our todo list :-)\n\n> >>>>> + description: Maximum allowed value (place any new values above here).\n> >>>>>\n> >>>>> - - Brightness:\n> >>>>> + - AeConstraintMode:\n> >>>>> type: int32_t\n> >>>>> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n> >>>>\n> >>>> /constaint/constraint/\n> >>>>\n> >>>>> + enum:\n> >>>>> + - name: ConstraintNormal\n> >>>>> + value: 0\n> >>>>> + description: Default constraint mode.\n> >>>>> + - name: ConstraintHighlight\n> >>>>> + value: 1\n> >>>>> + description: Highlight constraint mode.\n> >\n> > Would it be possible to give a more detailed description of those modes\n> > ?\n> \n> Will do.\n> \n> >>>>> + - name: ConstraintCustom1\n> >>>>> + value: 2\n> >>>>> + description: Custom constraint mode 1.\n> >>>>> + - name: ConstraintCustom2\n> >>>>> + value: 3\n> >>>>> + description: Custom constraint mode 2.\n> >>>>> + - name: ConstraintCustom3\n> >>>>> + value: 4\n> >>>>> + description: Custom constraint mode 3.\n> >>>>> + - name: ConstraintModeMax\n> >>>>> + value: 4\n> >>>>> + description: Maximum allowed value (place any new values above here).\n> >>>>\n> >>>> Same comments apply here of course.\n> >>>>\n> >>>> Perhaps we should expand the initial set from the modes/constraints that\n> >>>> we will need for supporting the Android HAL...\n> >>>>\n> >>>>> - - Contrast:\n> >>>>> + - AeExposureMode:\n> >>>>> type: int32_t\n> >>>>> - description: Specify a fixed contrast parameter\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> >>>>> + spcific, and not all exposure modes may be supported.\n> >>>>\n> >>>> s/spcific/specific/\n> >>>>\n> >>>>> + enum:\n> >>>>> + - name: ExposureNormal\n> >>>>> + value: 0\n> >>>>> + description: Default metering mode.\n> >>>>> + - name: ExposureSport\n> >>>>> + value: 1\n> >>>>> + description: Sport metering mode (favouring short shutter times).\n> >\n> > Reading the description, I wonder if this should be turned into a scene\n> > mode control, as it could also have an effect on other algorithms\n> > (auto-focus comes to mind for instance).\n> \n> I always looked as scene mode as a higher level construct within the\n> camera application. The application bunches a group of settings for a\n> particular scene mode (e.g. for Sport scene mode, AE turned to sport,\n> focus turned to fast converge, flash turned to fast sync) and sets\n> them individually. This way it could tailor them exactly to what it\n> needs.\n\nThat makes sense to me, but should the lower-level settings then use\nscene mode names ? We could for instance name this ExposureShort (very\neasy to mistype ExposureSport :-)) for instance (or any other name that\nwould convey the underlying behaviour). I trust your expertise on this\ntopic.\n\n> >>>>> + - name: ExposureLong\n> >>>>> + value: 2\n> >>>>> + description: Exposure mode allowing long exposure times.\n> >>>>> + - name: ExposureCustom1\n> >>>>> + value: 3\n> >>>>> + description: Custom exposure mode 1.\n> >>>>> + - name: ExposureCustom2\n> >>>>> + value: 4\n> >>>>> + description: Custom exposure mode 2.\n> >>>>> + - name: ExposureCustom3\n> >>>>> + value: 5\n> >>>>> + description: Custom exposure mode 3.\n> >>>>> + - name: ExposureModeMax\n> >>>>> + value: 5\n> >>>>> + description: Maximum allowed value (place any new values above here).\n> >>>>>\n> >>>>> - - Saturation:\n> >>>>> - type: int32_t\n> >>>>> - description: Specify a fixed saturation parameter\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 alogrithm 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> >>>>> - ManualExposure:\n> >>>>> type: int32_t\n> >>>>> @@ -57,4 +137,21 @@ controls:\n> >>>>> applied to all colour channels.\n> >>>>>\n> >>>>> \\sa ManualExposure\n> >>>>> +\n> >>>>\n> >>>> What changes were made to the properties below ? (I don't see anything\n> >>>> in particular) or is that just git being a pain and adding churn?\n> >>>\n> >>> I only moved them around - so all the controls in the file are ordered\n> >>> in appropriate groups (AGC, AWB, General...).\n> >>\n> >> Aha - in that case, it would generally be better to commit any\n> >> non-functional-change code move as a separate patch, so that it's clear\n> >> and distinct on it's own.\n> >>\n> >>>>> + - AwbEnable:\n> >>>>> + type: bool\n> >>>>> + description: |\n> >>>>> + Enable or disable the AWB.\n> >>>>> +\n> >>>>> + - Brightness:\n> >>>>> + type: int32_t\n> >>>>> + description: Specify a fixed brightness parameter\n> >>>>> +\n> >>>>> + - Contrast:\n> >>>>> + type: int32_t\n> >>>>> + description: Specify a fixed contrast parameter\n> >>>>> +\n> >>>>> + - Saturation:\n> >>>>> + type: int32_t\n> >>>>> + description: Specify a fixed saturation parameter\n> >>>>> ...","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 CCEE7605D3\n\tfor ;\n\tFri, 27 Mar 2020 12:03:14 +0100 (CET)","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 37E152DC;\n\tFri, 27 Mar 2020 12:03:14 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"s/WREDIK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1585306994;\n\tbh=iExzhHiLrekYkfGLX9tbWmQNamNujVJqr7FomvW2XgA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=s/WREDIKIUf5i1zv4ZMJ/QEo8xFxU+Bg6gfWQfL7CRBxv9ZQ9dYOwy0VWr2MIXnCf\n\tNxeq5GLR48ABySZRS/Ttij7o/fOeyCsYZLGL9obK70+mouyepRAam6iiDzsuqUY6Pp\n\tRm+Pl4M8Paj29YuS6KhH8ECR471OZLLkdmAXT1lI=","Date":"Fri, 27 Mar 2020 13:03:10 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20200327110310.GE5040@pendragon.ideasonboard.com>","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>\n\t\n\t\n\t<20200326162600.GV20581@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 27 Mar 2020 11:03:15 -0000"}},{"id":4340,"web_url":"https://patchwork.libcamera.org/comment/4340/","msgid":"","date":"2020-03-27T11:29:03","subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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 Fri, 27 Mar 2020 at 11:03, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> On Fri, Mar 27, 2020 at 10:56:14AM +0000, Naushir Patuck wrote:\n> > On Thu, 26 Mar 2020 at 16:26, Laurent Pinchart wrote:\n> > > On Mon, Mar 23, 2020 at 04:42:27PM +0000, Kieran Bingham wrote:\n> > >> On 23/03/2020 15:29, Naushir Patuck wrote:\n> > >>> On Fri, 20 Mar 2020 at 14:58, Kieran Bingham wrote:\n> > >>>> On 09/03/2020 12:33, 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 double type control used to set the log2 exposure\n> > >>>>> adjustment for the AE algorithm.\n> > >>>>>\n> > >>>>> Signed-off-by: Naushir Patuck \n> > >>>>> ---\n> > >>>>> src/libcamera/control_ids.yaml | 121 +++++++++++++++++++++++++++++----\n> > >>>>> 1 file changed, 109 insertions(+), 12 deletions(-)\n> > >>>>>\n> > >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > >>>>> index 5bbe65ae..da1a7b43 100644\n> > >>>>> --- a/src/libcamera/control_ids.yaml\n> > >>>>> +++ b/src/libcamera/control_ids.yaml\n> > >>>>> @@ -23,24 +23,104 @@ controls:\n> > >>>>>\n> > >>>>> \\sa AeEnable\n> > >>>>>\n> > >>>>> - - AwbEnable:\n> > >>>>> - type: bool\n> > >>>>> + - AeMeteringMode:\n> > >>>>> + type: int32_t\n> > >>>>> description: |\n> > >>>>> - Enable or disable the AWB.\n> > >>>>> -\n> > >>>>> - \\sa ManualGain\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> > >>>>\n> > >>>> Only caught because of the spelling error below, but in the other\n> > >>>> instances you do not hyphenate platform-specific. Maybe the simple\n> > >>>> option is to not hyphenate here either :-)\n> > >>>>\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> > >>>>> + - name: MeteringCustom1\n> > >>>>> + value: 3\n> > >>>>\n> > >>>> This feels a bit painful if we want to add more (non-custom, or custom)\n> > >>>> modes.\n> > >>>>\n> > >>>> We would only be able to append to prevent ABI breakage - Then the enums\n> > >>>> could seem a bit ugly:\n> > >>>>\n> > >>>> enum {\n> > >>>> MeteringCentreWeighted,\n> > >>>> MeteringSpot,\n> > >>>> MeteringMatrix,\n> > >>>> MeteringCustom1,\n> > >>>> MeteringCustom2,\n> > >>>> MeteringCustom3,\n> > >>>> MeteringMadeUpGeneralControl,\n> > >>>> MeteringCustom4,\n> > >>>> MeteringMadeupButGeneralControl2,\n> > >>>> }\n> > >>>>\n> > >>>> But I fear that may not be something we can easily avoid unless we know\n> > >>>> *all* of the metering mode names in advance...\n> > >>>>\n> > >>>> Of course we're intentionally not yet ABI stable anyway, so this could\n> > >>>> still be an intermediate step...\n> > >>>>\n> > >>>> One option could be to have MeteringCustom, with the actual 'custom'\n> > >>>> choice provided by another means ('yet another' control?), but maybe\n> > >>>> that doesn't scale well either. I'm not sure yet...\n> > >>>>\n> > >>>>> + description: Custom metering mode 1.\n> > >>>>> + - name: MeteringCustom2\n> > >>>>> + value: 4\n> > >>>>> + description: Custom metering mode 2.\n> > >>>>> + - name: MeteringCustom3\n> > >>>>> + value: 5\n> > >>>>> + description: Custom metering mode 3.\n> > >>>>\n> > >>>> Are you able to express what you currently envisage the 3 custom modes\n> > >>>> would be used for?\n> > >>>\n> > >>> The intention here was to allow users to add new modes to the\n> > >>> algorithm, perhaps something very specific to their application. If\n> > >>> the tuning parameters for the mode could be added/tweaked via a\n> > >>> text/config file, then the user could directly use the new mode\n> > >>> without ever having to download/re-compile any code. Initially (as in\n> > >>> patch v1), these modes were defined as std::string types, but we\n> > >>> concluded that added too much vagueness to the list of options.\n> > >>> Perhaps 3 custom modes is too much, and we can make do with just one?\n> > >>\n> > >> Maybe one is enough I don't know. That's why I wondered about one custom\n> > >> control which has a secondary option to give more flexibility.\n> > >>\n> > >> Otherwise '3' seems like a very arbitrary definition ...\n> > >\n> > > This is indeed an issue, I'll try to sleep over it. Having a single\n> > > value wouldn't be too bad when it comes to extending the enum, but that\n> > > may not be enough.\n> > >\n> > > That being said, for this specific control, I wonder if we should make\n> > > it a bit more finer-grained by specifying a list of weighted regions.\n> > > Just brainstorming, the ControlInfo (previously known as ControlRange)\n> > > class could then possibly offer a set of defaults based on an enum.\n> >\n> > This is possible, but it might get tricky trying to interoperate with\n> > different vendor IPAs where the HW/SW capabilities may not be the\n> > same.\n>\n> I agree, that's why I was wondering what would be a good way to express\n> metering (which will apply to AF too, and potentially AWB ?) in a\n> flexible way that could give finer-grained control when the hardware and\n> IPA support it, but wouldn't be awkward to use for devices that have\n> more limited capabilities. We don't have to fix all of this now, it\n> could come as a later rework of the controls, but I wanted to raise the\n> issue already in case we could figure out a solution in the short term.\n>\n> > >>>>> + - name: MeteringModeMax\n> > >>>>> + value: 5\n> > >>>>\n> > >>>> It's a shame we have to express the enum max value manually here :-(\n> > >>>> I haven't looked at if it could be identified through code though.\n> > >>>\n> > >>> This is primarily needed for setting up ControlRange for the control.\n> > >>> It should be entirely possible for the gen_controls script to add the\n> > >>> Max item to the end of the enum?\n> > >>\n> > >> Aha, then indeed perhaps we should automate the creation somehow\n> > >\n> > > Yes, we should do that.\n> >\n> > As mentioned previously, the max value is only used for ControlInfo\n> > (previously known as ControlRange). I do wonder if ControlInfo is a\n> > sensible thing to have for an enum though. Suppose an IPA allows only\n> > metering modes 0, 1, 2, 4, 5. In this case, the ControlInfo must be\n> > specified as (min, max) == (0, 5), but a value of 3 is invalid.\n>\n> I agree with you. We need ControlInfo to report the valid values for\n> enumerated controls. Another item on our todo list :-)\n>\n> > >>>>> + description: Maximum allowed value (place any new values above here).\n> > >>>>>\n> > >>>>> - - Brightness:\n> > >>>>> + - AeConstraintMode:\n> > >>>>> type: int32_t\n> > >>>>> - description: Specify a fixed brightness parameter\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 constaint modes may be supported.\n> > >>>>\n> > >>>> /constaint/constraint/\n> > >>>>\n> > >>>>> + enum:\n> > >>>>> + - name: ConstraintNormal\n> > >>>>> + value: 0\n> > >>>>> + description: Default constraint mode.\n> > >>>>> + - name: ConstraintHighlight\n> > >>>>> + value: 1\n> > >>>>> + description: Highlight constraint mode.\n> > >\n> > > Would it be possible to give a more detailed description of those modes\n> > > ?\n> >\n> > Will do.\n> >\n> > >>>>> + - name: ConstraintCustom1\n> > >>>>> + value: 2\n> > >>>>> + description: Custom constraint mode 1.\n> > >>>>> + - name: ConstraintCustom2\n> > >>>>> + value: 3\n> > >>>>> + description: Custom constraint mode 2.\n> > >>>>> + - name: ConstraintCustom3\n> > >>>>> + value: 4\n> > >>>>> + description: Custom constraint mode 3.\n> > >>>>> + - name: ConstraintModeMax\n> > >>>>> + value: 4\n> > >>>>> + description: Maximum allowed value (place any new values above here).\n> > >>>>\n> > >>>> Same comments apply here of course.\n> > >>>>\n> > >>>> Perhaps we should expand the initial set from the modes/constraints that\n> > >>>> we will need for supporting the Android HAL...\n> > >>>>\n> > >>>>> - - Contrast:\n> > >>>>> + - AeExposureMode:\n> > >>>>> type: int32_t\n> > >>>>> - description: Specify a fixed contrast parameter\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> > >>>>> + spcific, and not all exposure modes may be supported.\n> > >>>>\n> > >>>> s/spcific/specific/\n> > >>>>\n> > >>>>> + enum:\n> > >>>>> + - name: ExposureNormal\n> > >>>>> + value: 0\n> > >>>>> + description: Default metering mode.\n> > >>>>> + - name: ExposureSport\n> > >>>>> + value: 1\n> > >>>>> + description: Sport metering mode (favouring short shutter times).\n> > >\n> > > Reading the description, I wonder if this should be turned into a scene\n> > > mode control, as it could also have an effect on other algorithms\n> > > (auto-focus comes to mind for instance).\n> >\n> > I always looked as scene mode as a higher level construct within the\n> > camera application. The application bunches a group of settings for a\n> > particular scene mode (e.g. for Sport scene mode, AE turned to sport,\n> > focus turned to fast converge, flash turned to fast sync) and sets\n> > them individually. This way it could tailor them exactly to what it\n> > needs.\n>\n> That makes sense to me, but should the lower-level settings then use\n> scene mode names ? We could for instance name this ExposureShort (very\n> easy to mistype ExposureSport :-)) for instance (or any other name that\n> would convey the underlying behaviour). I trust your expertise on this\n> topic.\n\nYes, that seems reasonable.\n\n>\n> > >>>>> + - name: ExposureLong\n> > >>>>> + value: 2\n> > >>>>> + description: Exposure mode allowing long exposure times.\n> > >>>>> + - name: ExposureCustom1\n> > >>>>> + value: 3\n> > >>>>> + description: Custom exposure mode 1.\n> > >>>>> + - name: ExposureCustom2\n> > >>>>> + value: 4\n> > >>>>> + description: Custom exposure mode 2.\n> > >>>>> + - name: ExposureCustom3\n> > >>>>> + value: 5\n> > >>>>> + description: Custom exposure mode 3.\n> > >>>>> + - name: ExposureModeMax\n> > >>>>> + value: 5\n> > >>>>> + description: Maximum allowed value (place any new values above here).\n> > >>>>>\n> > >>>>> - - Saturation:\n> > >>>>> - type: int32_t\n> > >>>>> - description: Specify a fixed saturation parameter\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 alogrithm 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> > >>>>> - ManualExposure:\n> > >>>>> type: int32_t\n> > >>>>> @@ -57,4 +137,21 @@ controls:\n> > >>>>> applied to all colour channels.\n> > >>>>>\n> > >>>>> \\sa ManualExposure\n> > >>>>> +\n> > >>>>\n> > >>>> What changes were made to the properties below ? (I don't see anything\n> > >>>> in particular) or is that just git being a pain and adding churn?\n> > >>>\n> > >>> I only moved them around - so all the controls in the file are ordered\n> > >>> in appropriate groups (AGC, AWB, General...).\n> > >>\n> > >> Aha - in that case, it would generally be better to commit any\n> > >> non-functional-change code move as a separate patch, so that it's clear\n> > >> and distinct on it's own.\n> > >>\n> > >>>>> + - AwbEnable:\n> > >>>>> + type: bool\n> > >>>>> + description: |\n> > >>>>> + Enable or disable the AWB.\n> > >>>>> +\n> > >>>>> + - Brightness:\n> > >>>>> + type: int32_t\n> > >>>>> + description: Specify a fixed brightness parameter\n> > >>>>> +\n> > >>>>> + - Contrast:\n> > >>>>> + type: int32_t\n> > >>>>> + description: Specify a fixed contrast parameter\n> > >>>>> +\n> > >>>>> + - Saturation:\n> > >>>>> + type: int32_t\n> > >>>>> + description: Specify a fixed saturation parameter\n> > >>>>> ...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA60A6040E\n\tfor ;\n\tFri, 27 Mar 2020 12:29:22 +0100 (CET)","by mail-lj1-x241.google.com with SMTP id n17so9796865lji.8\n\tfor ;\n\tFri, 27 Mar 2020 04:29:22 -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=\"Sv3+4q+x\"; 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=x3OmBHxGWDpXLZEiYAd1hZJB+y5KL3UHOhyLiyJDpbg=;\n\tb=Sv3+4q+x6SrBBjdcyEcJfu8YsfhFDwBk5YH3e+a29KitErAp7m/JGhU5OLdwh6vCVa\n\tnBHLSa7gYmcVEF9Zvo2PfmCHactXI+GW9q2lRc4jz8lTTAtalTeX5rWQPWHSSHFdRpCN\n\te7BESdP68yEj9f9xEFVUiFzqE1+m7LoEkgGTJrM5IL5aHbsF8TcFyMGleZe4/6Ro4T9U\n\tFYBER++M7Cr5VRpGpnIIonoBdWTY2DNxWsTxfOMFUddxHoBx17yTctULjCIVc0EefXZa\n\tti0miI7WbCPmoRl0TBPobkDQFd7fCatEAqc1xevVZMEqF2tZNvQDbZhXlHCqRUphQgSm\n\tfl/Q==","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=x3OmBHxGWDpXLZEiYAd1hZJB+y5KL3UHOhyLiyJDpbg=;\n\tb=jnrcm0Qe5wPMFICbjWGpD7WB1fd46dXifuO4GipXJqoSqNVveBWSfcmu5CVduv/jYc\n\tcA2iFQoEScH/MV7XydfyuRBxMDjCCYi+J2XfRCEkTTDh5hvzUjENJpL9jcOS59HALZmo\n\tJO35CVtLFXf+i6lbRoxQMK/J5TI7VkNNTUssqbm0mAriJcutDclrJQv3RgaZoD1VOiw0\n\tb+9QbGTdwJFLyTsQgjER2Qk0kxSkAPuuysFKCmkyv3O/rqtBK8eerEQVS5Bibu2mdH4v\n\tba7bLpKufjruwrRwz5701JGxsoayXIrR/Wj6nsI61oa751ncHCJakBr2y0eb7txKK8yP\n\trDxw==","X-Gm-Message-State":"AGi0PubIaoSEjHLBQpegEjAdWZ3FMBRDdP76wOKoiax2DHnQ5k4dKBrV\n\tYJv6XMukRcuGXLX+h6F1MOk4cj/L6v9ZPMzIy4OFIjmgvEQ=","X-Google-Smtp-Source":"APiQypJxehCnG5q6ugfZIxg+Pi50vV+oW2JaktNorqd0vRBOg4A0W4kOXqtDqCC8exLa7kzfw8aFbsuzg/oKc+Phx4c=","X-Received":"by 2002:a2e:81cc:: with SMTP id s12mr6557835ljg.90.1585308562176;\n\tFri, 27 Mar 2020 04:29:22 -0700 (PDT)","MIME-Version":"1.0","References":"<20200309123319.630-1-naush@raspberrypi.com>\n\t<20200309123319.630-4-naush@raspberrypi.com>\n\t<22c4cca6-017f-f991-3e87-785da1ec0311@ideasonboard.com>\n\t\n\t\n\t<20200326162600.GV20581@pendragon.ideasonboard.com>\n\t\n\t<20200327110310.GE5040@pendragon.ideasonboard.com>","In-Reply-To":"<20200327110310.GE5040@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 27 Mar 2020 11:29:03 +0000","Message-ID":"","To":"Laurent Pinchart ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v2 3/6] 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, 27 Mar 2020 11:29:23 -0000"}}]