[{"id":13717,"web_url":"https://patchwork.libcamera.org/comment/13717/","msgid":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>","date":"2020-11-16T10:40:25","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi David,\n\nOn 27/10/2020 14:12, David Plowman wrote:\n> This control reports the global digital gain applied by the pipeline\n> as a whole, after capturing a raw image from the sensor.\n> \n> Signed-off-by: David Plowman \n> ---\n> src/libcamera/control_ids.yaml | 11 +++++++++++\n> 1 file changed, 11 insertions(+)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index c8874fa9..e6362c74 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -530,4 +530,15 @@ controls:\n> This control is only present when the pipeline supports scaling. Its\n> maximum valid value is given by the properties::ScalerCropMaximum\n> property, and the two can be used to implement digital zoom.\n> +\n> + - DigitalGain:\n> + type: float\n> + description: |\n> + Global digital gain value applied to the image during all the\n> + processing steps after capturing the image from the sensor. Any raw\n> + images, therefore, will not include this gain, but the final images\n> + output by the imaging pipeline as a whole will include it.\n> +\n> + This control is intended to report the value used by the image\n> + processing pipeline.\n\n\nIf this is a per-stream thing anyway, I guess it will then be up to\npipeline handlers to set this to the appropriate value for each stream\nwhen it completes. The fact that this value would not be applicable to a\nRAW stream makes me think it certainly should be a per-stream metadata\nstyle value.\n\n\nI'd hope this could be handled by a common helper in that instance so it\ndoesn't get left out of some pipeline handlers, but included in some,\nand become inconsistent. Not yet sure how we can handle that, but that\nwill be a core issue anyway.\n\n\nI wonder if we should mark this somehow as read-only, at least until we\ndetermine that someone needs to set it.\n\nWe could introduce a control property between type: and description:\n read-only: true\n\n\nOtherwise, I see no objections currently. I think we're just waiting on\ntop-level thoughts from Laurent. (And perhaps per-stream controls, but\nthat brings it's own questions )\n\nReviewed-by: Kieran Bingham \n\n\n> ...\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7F8ECBE081\n\tfor ;\n\tMon, 16 Nov 2020 10:40:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 067E1632B3;\n\tMon, 16 Nov 2020 11:40:30 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71B686033B\n\tfor ;\n\tMon, 16 Nov 2020 11:40:28 +0100 (CET)","from [192.168.0.217]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id F10C8A1B;\n\tMon, 16 Nov 2020 11:40:27 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j9sIbcop\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1605523228;\n\tbh=9TBn+uRZG2CIi1AGitbzGrt3R84V6Q6kbB4IJXK4xuM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=j9sIbcopsZiSpTRjYCJ/WoMWH2gAxikgdfE+tM864a4fmkOwouozKACAzBDsZNl7k\n\tufdni7nI7ysFMumjKXXxFy9yrt8jL2PtxTmgN2mXGoIqsbYTpRQe24zHmS3lAaWEaA\n\t4AB59pgeYtk1Ct7D7dj0SzkHatfg2Fn/OYr2qDTQ=","To":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>","From":"Kieran Bingham ","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":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>","Date":"Mon, 16 Nov 2020 10:40:25 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201027141246.4708-2-david.plowman@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13826,"web_url":"https://patchwork.libcamera.org/comment/13826/","msgid":"<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>","date":"2020-11-23T08:58:35","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n\nOn Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> Hi David,\n>\n> On 27/10/2020 14:12, David Plowman wrote:\n> > This control reports the global digital gain applied by the pipeline\n> > as a whole, after capturing a raw image from the sensor.\n> >\n> > Signed-off-by: David Plowman \n> > ---\n> > src/libcamera/control_ids.yaml | 11 +++++++++++\n> > 1 file changed, 11 insertions(+)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index c8874fa9..e6362c74 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -530,4 +530,15 @@ controls:\n> > This control is only present when the pipeline supports scaling. Its\n> > maximum valid value is given by the properties::ScalerCropMaximum\n> > property, and the two can be used to implement digital zoom.\n> > +\n> > + - DigitalGain:\n> > + type: float\n> > + description: |\n> > + Global digital gain value applied to the image during all the\n> > + processing steps after capturing the image from the sensor. Any raw\n> > + images, therefore, will not include this gain, but the final images\n> > + output by the imaging pipeline as a whole will include it.\n> > +\n> > + This control is intended to report the value used by the image\n> > + processing pipeline.\n>\n>\n> If this is a per-stream thing anyway, I guess it will then be up to\n> pipeline handlers to set this to the appropriate value for each stream\n> when it completes. The fact that this value would not be applicable to a\n> RAW stream makes me think it certainly should be a per-stream metadata\n> style value.\n>\n>\n> I'd hope this could be handled by a common helper in that instance so it\n> doesn't get left out of some pipeline handlers, but included in some,\n> and become inconsistent. Not yet sure how we can handle that, but that\n> will be a core issue anyway.\n>\n>\n> I wonder if we should mark this somehow as read-only, at least until we\n> determine that someone needs to set it.\n>\n> We could introduce a control property between type: and description:\n> read-only: true\n\nIsn't a read-only control just a metadata ?\n\nWouldn't it be enough for a pipeline that does not support changing\nthe control value from applications not reporting it in the list of\nsupported Camera's controls, but only report it as part of a completed\nrequest's metadata ?\n\nThanks\n j\n\n>\n>\n> Otherwise, I see no objections currently. I think we're just waiting on\n> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> that brings it's own questions )\n>\n> Reviewed-by: Kieran Bingham \n>\n>\n> > ...\n> >\n>\n> --\n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9C46FBE176\n\tfor ;\n\tMon, 23 Nov 2020 08:58:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3D11763326;\n\tMon, 23 Nov 2020 09:58:35 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 41AC9615A0\n\tfor ;\n\tMon, 23 Nov 2020 09:58:33 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 4A54B1BF20A;\n\tMon, 23 Nov 2020 08:58:31 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Mon, 23 Nov 2020 09:58:35 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13827,"web_url":"https://patchwork.libcamera.org/comment/13827/","msgid":"<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","date":"2020-11-23T09:17:33","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 23/11/2020 08:58, Jacopo Mondi wrote:\n> Hi Kieran,\n> \n> On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n>> Hi David,\n>>\n>> On 27/10/2020 14:12, David Plowman wrote:\n>>> This control reports the global digital gain applied by the pipeline\n>>> as a whole, after capturing a raw image from the sensor.\n>>>\n>>> Signed-off-by: David Plowman \n>>> ---\n>>> src/libcamera/control_ids.yaml | 11 +++++++++++\n>>> 1 file changed, 11 insertions(+)\n>>>\n>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n>>> index c8874fa9..e6362c74 100644\n>>> --- a/src/libcamera/control_ids.yaml\n>>> +++ b/src/libcamera/control_ids.yaml\n>>> @@ -530,4 +530,15 @@ controls:\n>>> This control is only present when the pipeline supports scaling. Its\n>>> maximum valid value is given by the properties::ScalerCropMaximum\n>>> property, and the two can be used to implement digital zoom.\n>>> +\n>>> + - DigitalGain:\n>>> + type: float\n>>> + description: |\n>>> + Global digital gain value applied to the image during all the\n>>> + processing steps after capturing the image from the sensor. Any raw\n>>> + images, therefore, will not include this gain, but the final images\n>>> + output by the imaging pipeline as a whole will include it.\n>>> +\n>>> + This control is intended to report the value used by the image\n>>> + processing pipeline.\n>>\n>>\n>> If this is a per-stream thing anyway, I guess it will then be up to\n>> pipeline handlers to set this to the appropriate value for each stream\n>> when it completes. The fact that this value would not be applicable to a\n>> RAW stream makes me think it certainly should be a per-stream metadata\n>> style value.\n>>\n>>\n>> I'd hope this could be handled by a common helper in that instance so it\n>> doesn't get left out of some pipeline handlers, but included in some,\n>> and become inconsistent. Not yet sure how we can handle that, but that\n>> will be a core issue anyway.\n>>\n>>\n>> I wonder if we should mark this somehow as read-only, at least until we\n>> determine that someone needs to set it.\n>>\n>> We could introduce a control property between type: and description:\n>> read-only: true\n> \n> Isn't a read-only control just a metadata ?\n> \n> Wouldn't it be enough for a pipeline that does not support changing\n> the control value from applications not reporting it in the list of\n> supported Camera's controls, but only report it as part of a completed\n> request's metadata ?\n\nAh, yes of course - because if the control is not listed as supported it\nwon't be there to set in the first place! I forgot about that.\n\nSo - indeed, no requirement to mark anything as read-only. That will be\nimplicit.\n\n--\nKieran\n\n\n> \n> Thanks\n> j\n> \n>>\n>>\n>> Otherwise, I see no objections currently. I think we're just waiting on\n>> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n>> that brings it's own questions )\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>>\n>>> ...\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id DCA17BE176\n\tfor ;\n\tMon, 23 Nov 2020 09:17:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D5D663326;\n\tMon, 23 Nov 2020 10:17:37 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C0C9D615A0\n\tfor ;\n\tMon, 23 Nov 2020 10:17:35 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 364882A4;\n\tMon, 23 Nov 2020 10:17:35 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"WbqFiyua\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606123055;\n\tbh=s+hfcoYyLgFdSGiy5CYuXyQIcWZ27cFVbYft1I8O084=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=WbqFiyuaAjZ5+gY+Nr94LoCCRL20zXRc/gohKngHHdSQEnJRDHPvD8iuSD0byRUOo\n\tREhvsnaoyAJRj0sEFOYh9JuGxezWkSGbWBQmB6LaWFDdOee3q9EQzM/5f2GbLzlQfd\n\tBo8Toom7i1xFN8G7SjjhfgFLcCr7Vcu5Gqf+TMEE=","To":"Jacopo Mondi ","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>","From":"Kieran Bingham ","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":"<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","Date":"Mon, 23 Nov 2020 09:17:33 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13838,"web_url":"https://patchwork.libcamera.org/comment/13838/","msgid":"","date":"2020-11-24T08:28:09","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nSounds like we're happy enough from the point of view of this thing\nbeing read-only (for Raspberry Pi at least). Would anyone want any\nchanges to the wording? Perhaps the final sentence/paragraph might now\nbe better as\n\n\"This control is present in a request's ControlList only if the\npipeline supports setting the value. Even when it cannot be set by an\napplication, the pipeline may still report the actual value used in\nthe metadata returned with completed requests.\"\n\nAny other thoughts?\n\nThanks\nDavid\n\nOn Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n wrote:\n>\n> Hi Jacopo,\n>\n> On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > Hi Kieran,\n> >\n> > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> >> Hi David,\n> >>\n> >> On 27/10/2020 14:12, David Plowman wrote:\n> >>> This control reports the global digital gain applied by the pipeline\n> >>> as a whole, after capturing a raw image from the sensor.\n> >>>\n> >>> Signed-off-by: David Plowman \n> >>> ---\n> >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> >>> 1 file changed, 11 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>> index c8874fa9..e6362c74 100644\n> >>> --- a/src/libcamera/control_ids.yaml\n> >>> +++ b/src/libcamera/control_ids.yaml\n> >>> @@ -530,4 +530,15 @@ controls:\n> >>> This control is only present when the pipeline supports scaling. Its\n> >>> maximum valid value is given by the properties::ScalerCropMaximum\n> >>> property, and the two can be used to implement digital zoom.\n> >>> +\n> >>> + - DigitalGain:\n> >>> + type: float\n> >>> + description: |\n> >>> + Global digital gain value applied to the image during all the\n> >>> + processing steps after capturing the image from the sensor. Any raw\n> >>> + images, therefore, will not include this gain, but the final images\n> >>> + output by the imaging pipeline as a whole will include it.\n> >>> +\n> >>> + This control is intended to report the value used by the image\n> >>> + processing pipeline.\n> >>\n> >>\n> >> If this is a per-stream thing anyway, I guess it will then be up to\n> >> pipeline handlers to set this to the appropriate value for each stream\n> >> when it completes. The fact that this value would not be applicable to a\n> >> RAW stream makes me think it certainly should be a per-stream metadata\n> >> style value.\n> >>\n> >>\n> >> I'd hope this could be handled by a common helper in that instance so it\n> >> doesn't get left out of some pipeline handlers, but included in some,\n> >> and become inconsistent. Not yet sure how we can handle that, but that\n> >> will be a core issue anyway.\n> >>\n> >>\n> >> I wonder if we should mark this somehow as read-only, at least until we\n> >> determine that someone needs to set it.\n> >>\n> >> We could introduce a control property between type: and description:\n> >> read-only: true\n> >\n> > Isn't a read-only control just a metadata ?\n> >\n> > Wouldn't it be enough for a pipeline that does not support changing\n> > the control value from applications not reporting it in the list of\n> > supported Camera's controls, but only report it as part of a completed\n> > request's metadata ?\n>\n> Ah, yes of course - because if the control is not listed as supported it\n> won't be there to set in the first place! I forgot about that.\n>\n> So - indeed, no requirement to mark anything as read-only. That will be\n> implicit.\n>\n> --\n> Kieran\n>\n>\n> >\n> > Thanks\n> > j\n> >\n> >>\n> >>\n> >> Otherwise, I see no objections currently. I think we're just waiting on\n> >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> >> that brings it's own questions )\n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>\n> >>> ...\n> >>>\n> >>\n> >> --\n> >> Regards\n> >> --\n> >> Kieran\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards\n> --\n> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 68E23BE08A\n\tfor ;\n\tTue, 24 Nov 2020 08:28:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DDE76633F4;\n\tTue, 24 Nov 2020 09:28:23 +0100 (CET)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 29A6660333\n\tfor ;\n\tTue, 24 Nov 2020 09:28:22 +0100 (CET)","by mail-oi1-x242.google.com with SMTP id v202so19764408oia.9\n\tfor ;\n\tTue, 24 Nov 2020 00:28:22 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"Bq5lzc9X\"; 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=hGB45wT4iWuHjGYfmJvLcm0pidmSve8YL6k/by01TGs=;\n\tb=Bq5lzc9XaV/4w0pzfr/8JmPfikezIycecs+4yjvh+ccaDcNxezMJyogIMoIDd7LcXu\n\tS4mSleiZy2XuNplI85lQ4i2wQ172BtHt1sW4oU5g4zXsnQDEcNj3+t3wW5yfpyatpQnG\n\t0AIAmfgqztyyBQDX/BjUkacRHaLkg1e/svVsBra3KGdEnuJvZ8KAUD+z6kVjOmfVW99Z\n\trMOlIPt5gc1bK75XJUHu+7yPtbZ9OPRUklddiRUHefxtS2LR/6igf+PXLTuYDyxOfrP0\n\tn6sqrePQikKN9l0om3qIUT96QTyHkCRHqqqVkdSbf6wkkPDiOkg1Eon2q5H8XHhmq1DL\n\twxNQ==","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=hGB45wT4iWuHjGYfmJvLcm0pidmSve8YL6k/by01TGs=;\n\tb=V8+SSmAI+tb4Aq8jefETMRjf8edTxtxsnpAcxREeVNeXSBt6TV+uSdRNfyXJVoJp/1\n\tlWUcvHFmTHhFgXqsgrqQEeUqY0GQ+vghr+Co+7tJAWQFB3iKdJVk0JKrvOIxGH3dEtnv\n\tdXK5/SwmSYw6X5zTgJpP3IOzamvougcXzF4tyse1tWNlNospZKzghk91bkPASscoJ3sF\n\tfuN4pH+lr5i9d/QsD36azTtW7A9a+f6P1BuYrw7UBKqykdNvbqfFNJny9Ap4eyfPukTf\n\tU2ZeddeXKiFEuxi1mHK6l0pkFDcBgzgOI9bnvBcTRwmif/nysdEnPVSa13BB+S8nre15\n\tq0Lw==","X-Gm-Message-State":"AOAM530noVZT2uAC8NGE4X46MRKymhmQu0I1xSnxAVGilZOu0UOKPzDl\n\tLFyVyfsASgZumTCA71eIEA+T9xSzloUrEeRC4F6Akg==","X-Google-Smtp-Source":"ABdhPJzMZBuTMdYhnMM+pfZb4HY1x+BXl7NjvazFvgj5U7VqXfN0XejWq9ko2Hrh4mo/NwRuW6b19epe+SpaRs0zoAI=","X-Received":"by 2002:aca:de89:: with SMTP id\n\tv131mr1889340oig.55.1606206500726; \n\tTue, 24 Nov 2020 00:28:20 -0800 (PST)","MIME-Version":"1.0","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","In-Reply-To":"<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","From":"David Plowman ","Date":"Tue, 24 Nov 2020 08:28:09 +0000","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13839,"web_url":"https://patchwork.libcamera.org/comment/13839/","msgid":"<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>","date":"2020-11-24T08:43:42","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> Hi everyone\n>\n> Sounds like we're happy enough from the point of view of this thing\n> being read-only (for Raspberry Pi at least). Would anyone want any\n> changes to the wording? Perhaps the final sentence/paragraph might now\n> be better as\n>\n> \"This control is present in a request's ControlList only if the\n> pipeline supports setting the value. Even when it cannot be set by an\n> application, the pipeline may still report the actual value used in\n> the metadata returned with completed requests.\"\n\nI don't think it it necessary. It is implied that if a pipeline\nhandler does not support changing the digital gain it should not\nexpose it a one of the Camera's controls.\n\nLikewise, if it is something that applications should be informed of,\nit will be reported via metadata.\n\nI think we're good to go, except for the point that we've left\nfloating about having this a single value or a per-channel value.\n\nI'm trying to get a feeling how this would be reported by your ISP. I\nsee in example you have two per-channel values for the ColourGains\ncontrol. Is this anyway related ?\n\n>\n> Any other thoughts?\n>\n> Thanks\n> David\n>\n> On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> wrote:\n> >\n> > Hi Jacopo,\n> >\n> > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > Hi Kieran,\n> > >\n> > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > >> Hi David,\n> > >>\n> > >> On 27/10/2020 14:12, David Plowman wrote:\n> > >>> This control reports the global digital gain applied by the pipeline\n> > >>> as a whole, after capturing a raw image from the sensor.\n> > >>>\n> > >>> Signed-off-by: David Plowman \n> > >>> ---\n> > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > >>> 1 file changed, 11 insertions(+)\n> > >>>\n> > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > >>> index c8874fa9..e6362c74 100644\n> > >>> --- a/src/libcamera/control_ids.yaml\n> > >>> +++ b/src/libcamera/control_ids.yaml\n> > >>> @@ -530,4 +530,15 @@ controls:\n> > >>> This control is only present when the pipeline supports scaling. Its\n> > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > >>> property, and the two can be used to implement digital zoom.\n> > >>> +\n> > >>> + - DigitalGain:\n> > >>> + type: float\n> > >>> + description: |\n> > >>> + Global digital gain value applied to the image during all the\n> > >>> + processing steps after capturing the image from the sensor. Any raw\n> > >>> + images, therefore, will not include this gain, but the final images\n> > >>> + output by the imaging pipeline as a whole will include it.\n> > >>> +\n> > >>> + This control is intended to report the value used by the image\n> > >>> + processing pipeline.\n> > >>\n> > >>\n> > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > >> pipeline handlers to set this to the appropriate value for each stream\n> > >> when it completes. The fact that this value would not be applicable to a\n> > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > >> style value.\n> > >>\n> > >>\n> > >> I'd hope this could be handled by a common helper in that instance so it\n> > >> doesn't get left out of some pipeline handlers, but included in some,\n> > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > >> will be a core issue anyway.\n> > >>\n> > >>\n> > >> I wonder if we should mark this somehow as read-only, at least until we\n> > >> determine that someone needs to set it.\n> > >>\n> > >> We could introduce a control property between type: and description:\n> > >> read-only: true\n> > >\n> > > Isn't a read-only control just a metadata ?\n> > >\n> > > Wouldn't it be enough for a pipeline that does not support changing\n> > > the control value from applications not reporting it in the list of\n> > > supported Camera's controls, but only report it as part of a completed\n> > > request's metadata ?\n> >\n> > Ah, yes of course - because if the control is not listed as supported it\n> > won't be there to set in the first place! I forgot about that.\n> >\n> > So - indeed, no requirement to mark anything as read-only. That will be\n> > implicit.\n> >\n> > --\n> > Kieran\n> >\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > >>\n> > >>\n> > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > >> that brings it's own questions )\n> > >>\n> > >> Reviewed-by: Kieran Bingham \n> > >>\n> > >>\n> > >>> ...\n> > >>>\n> > >>\n> > >> --\n> > >> Regards\n> > >> --\n> > >> Kieran\n> > >> _______________________________________________\n> > >> libcamera-devel mailing list\n> > >> libcamera-devel@lists.libcamera.org\n> > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards\n> > --\n> > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3FEE6BE176\n\tfor ;\n\tTue, 24 Nov 2020 08:43:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AF010633F6;\n\tTue, 24 Nov 2020 09:43:40 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 71FB460333\n\tfor ;\n\tTue, 24 Nov 2020 09:43:39 +0100 (CET)","from uno.localdomain (host-79-25-179-116.retail.telecomitalia.it\n\t[79.25.179.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 182AFE001B;\n\tTue, 24 Nov 2020 08:43:37 +0000 (UTC)"],"X-Originating-IP":"79.25.179.116","Date":"Tue, 24 Nov 2020 09:43:42 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13842,"web_url":"https://patchwork.libcamera.org/comment/13842/","msgid":"","date":"2020-11-24T10:15:25","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nYou're right, there's a relationship there. ColourGains obviously\ngives you the red and blue gains determined by the AWB usually. You\nmight get the values 1.6 and 2.0 (for red and blue)\n\nIn our case, if we report a single \"global gain\" value you can kind of\nimagine it as the green gain, where the colour gains were normalised\nfor a green gain of 1. So if the global gain was 1.25, then the actual\nRGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n1.25, 2.5).\n\nIn the per-channel case I guess you'd be reporting these 3 numbers\ndirectly. For me this duplicates information that's already in the\nColourGains, and it seems to muddle things up a bit. Imagine you had a\npipeline that lets you set a global gain - you'd have to query the\ncurrent white balance and work out all three numbers and set them. But\nthen you've set the white balance as well. Or maybe we do something\nspecial so that you haven't? You see why it confuses me! So on balance\nI'm with the single value approach, though I could live either way.\n\nThanks!\nDavid\n\nOn Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > Hi everyone\n> >\n> > Sounds like we're happy enough from the point of view of this thing\n> > being read-only (for Raspberry Pi at least). Would anyone want any\n> > changes to the wording? Perhaps the final sentence/paragraph might now\n> > be better as\n> >\n> > \"This control is present in a request's ControlList only if the\n> > pipeline supports setting the value. Even when it cannot be set by an\n> > application, the pipeline may still report the actual value used in\n> > the metadata returned with completed requests.\"\n>\n> I don't think it it necessary. It is implied that if a pipeline\n> handler does not support changing the digital gain it should not\n> expose it a one of the Camera's controls.\n>\n> Likewise, if it is something that applications should be informed of,\n> it will be reported via metadata.\n>\n> I think we're good to go, except for the point that we've left\n> floating about having this a single value or a per-channel value.\n>\n> I'm trying to get a feeling how this would be reported by your ISP. I\n> see in example you have two per-channel values for the ColourGains\n> control. Is this anyway related ?\n>\n> >\n> > Any other thoughts?\n> >\n> > Thanks\n> > David\n> >\n> > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > wrote:\n> > >\n> > > Hi Jacopo,\n> > >\n> > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > Hi Kieran,\n> > > >\n> > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > >> Hi David,\n> > > >>\n> > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > >>> This control reports the global digital gain applied by the pipeline\n> > > >>> as a whole, after capturing a raw image from the sensor.\n> > > >>>\n> > > >>> Signed-off-by: David Plowman \n> > > >>> ---\n> > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > >>> 1 file changed, 11 insertions(+)\n> > > >>>\n> > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > >>> index c8874fa9..e6362c74 100644\n> > > >>> --- a/src/libcamera/control_ids.yaml\n> > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > >>> @@ -530,4 +530,15 @@ controls:\n> > > >>> This control is only present when the pipeline supports scaling. Its\n> > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > >>> property, and the two can be used to implement digital zoom.\n> > > >>> +\n> > > >>> + - DigitalGain:\n> > > >>> + type: float\n> > > >>> + description: |\n> > > >>> + Global digital gain value applied to the image during all the\n> > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > >>> + images, therefore, will not include this gain, but the final images\n> > > >>> + output by the imaging pipeline as a whole will include it.\n> > > >>> +\n> > > >>> + This control is intended to report the value used by the image\n> > > >>> + processing pipeline.\n> > > >>\n> > > >>\n> > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > >> when it completes. The fact that this value would not be applicable to a\n> > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > >> style value.\n> > > >>\n> > > >>\n> > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > >> will be a core issue anyway.\n> > > >>\n> > > >>\n> > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > >> determine that someone needs to set it.\n> > > >>\n> > > >> We could introduce a control property between type: and description:\n> > > >> read-only: true\n> > > >\n> > > > Isn't a read-only control just a metadata ?\n> > > >\n> > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > the control value from applications not reporting it in the list of\n> > > > supported Camera's controls, but only report it as part of a completed\n> > > > request's metadata ?\n> > >\n> > > Ah, yes of course - because if the control is not listed as supported it\n> > > won't be there to set in the first place! I forgot about that.\n> > >\n> > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > implicit.\n> > >\n> > > --\n> > > Kieran\n> > >\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > >>\n> > > >>\n> > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > >> that brings it's own questions )\n> > > >>\n> > > >> Reviewed-by: Kieran Bingham \n> > > >>\n> > > >>\n> > > >>> ...\n> > > >>>\n> > > >>\n> > > >> --\n> > > >> Regards\n> > > >> --\n> > > >> Kieran\n> > > >> _______________________________________________\n> > > >> libcamera-devel mailing list\n> > > >> libcamera-devel@lists.libcamera.org\n> > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards\n> > > --\n> > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 3993EBE08A\n\tfor ;\n\tTue, 24 Nov 2020 10:15:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A3283633F6;\n\tTue, 24 Nov 2020 11:15:38 +0100 (CET)","from mail-oi1-x243.google.com (mail-oi1-x243.google.com\n\t[IPv6:2607:f8b0:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9C496633E5\n\tfor ;\n\tTue, 24 Nov 2020 11:15:37 +0100 (CET)","by mail-oi1-x243.google.com with SMTP id k26so23245284oiw.0\n\tfor ;\n\tTue, 24 Nov 2020 02:15:37 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ctJBK685\"; 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=cKtAPmBlNUpi/Xw+Q8+bT2VPU7HH1zK7m+x5Ho1SgYk=;\n\tb=ctJBK685/J/oJCs7t4bdh5zXKT9LFIIeTcgtcVOt8GUg20rt/HyBMVlEtl/m8M+TGy\n\tG7l0N34XcwPAmagnD0pOiG0HomMJfclD1Z+0KtPa37B07EjKSz8NpJua+wTNQRRcm6/k\n\tHe2cDGNlOihMcHPW36HvdXusSU7DeeMdZjUr0cYjWJAZeC38w/BQlU3FdxvIAMI7sP9E\n\tqldRTGytCmZbiJ8+LxY4Ia9g2eSf+xwxZjT7ygW9YaAC6rDRA9jJ3T5DZ0xZWDrDEky/\n\tZOg+8JuixJiXM38C3chGJHksn521BfCk4ytsEU+ZP+y6+t7ejY4xKL7xWOkNKWAk1ygR\n\tV61A==","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=cKtAPmBlNUpi/Xw+Q8+bT2VPU7HH1zK7m+x5Ho1SgYk=;\n\tb=rTzV8i4vASvbQPl8SNn5lRCuYfIHSBjUYdoKFTPm2lCm4w5lVaWe/NOpoP2JrWHwFM\n\tC2/Eft3O4OctmrrwlR1tdgbGy6DBt13EUTQ78rwqQXIg3Y1FKrrPQHEEeXpeHaYFyZ8p\n\t6tP78F9JV9dccBD34llTG9p0t2tPTbiFvyDbCjr5qrz12LQroN3+KWMESFiHvKNKSd1Q\n\tnFCLlU18nB1qZGBMHsUfGAcWCMYTfbxQzMHBCQAboM2puY9pHO4yF9wK6gSqsvga+vbM\n\tBGFW5l9pJebUPvyvaWM0Lnet2x4VfK4rV4gk+qdrKA1+AsyV0HMci4LJp7U0LSsqwIpD\n\tZn/w==","X-Gm-Message-State":"AOAM533slf8QQ4kUAVLFEAE9TkHTRVAyn+v1CnKDWj0/k3452VHwn5wv\n\tMcVc5vZZMOKCk3mExHUGewLc26WrrnFfnhFxklOLxw==","X-Google-Smtp-Source":"ABdhPJymMrlPStAOOOdM4n8rUsvaCz45Q8t0vU9PJ5ACKFXSbHUXt9Fy2lv5P5YrxfDg0o+WdKwLpMhpbcbdAbMHLH0=","X-Received":"by 2002:aca:de89:: with SMTP id\n\tv131mr2068796oig.55.1606212936351; \n\tTue, 24 Nov 2020 02:15:36 -0800 (PST)","MIME-Version":"1.0","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>","In-Reply-To":"<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>","From":"David Plowman ","Date":"Tue, 24 Nov 2020 10:15:25 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13843,"web_url":"https://patchwork.libcamera.org/comment/13843/","msgid":"<20201124112808.mwlnk554z2a2esn4@uno.localdomain>","date":"2020-11-24T11:28:08","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> You're right, there's a relationship there. ColourGains obviously\n> gives you the red and blue gains determined by the AWB usually. You\n> might get the values 1.6 and 2.0 (for red and blue)\n>\n> In our case, if we report a single \"global gain\" value you can kind of\n> imagine it as the green gain, where the colour gains were normalised\n> for a green gain of 1. So if the global gain was 1.25, then the actual\n> RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> 1.25, 2.5).\n\nThat's a really helpful explanation for me, thanks\n\n>\n> In the per-channel case I guess you'd be reporting these 3 numbers\n> directly. For me this duplicates information that's already in the\n> ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> pipeline that lets you set a global gain - you'd have to query the\n> current white balance and work out all three numbers and set them. But\n> then you've set the white balance as well. Or maybe we do something\n> special so that you haven't? You see why it confuses me! So on balance\n> I'm with the single value approach, though I could live either way.\n\nI see. With the above explanation I really see why a single global\nvalue is probably enough.\n\nMy question is now: are the above notions (blue/red gains normalized\non a green value of 1, how global gain is used to obtain per-channel\ngains etc) standard knowledge I'm lacking, or:\n\n1) It's worth to describe them in the control documentation as they're\n\"standard\" and won't change per-pipeline\n2) They might differe per-pipeline and they need to be\ndescribed in the pipeline model documentation ?\n3) It is assumed the reader knows she's dealing with\n\nThanks\n j\n\n>\n> Thanks!\n> David\n>\n> On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > Hi everyone\n> > >\n> > > Sounds like we're happy enough from the point of view of this thing\n> > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > be better as\n> > >\n> > > \"This control is present in a request's ControlList only if the\n> > > pipeline supports setting the value. Even when it cannot be set by an\n> > > application, the pipeline may still report the actual value used in\n> > > the metadata returned with completed requests.\"\n> >\n> > I don't think it it necessary. It is implied that if a pipeline\n> > handler does not support changing the digital gain it should not\n> > expose it a one of the Camera's controls.\n> >\n> > Likewise, if it is something that applications should be informed of,\n> > it will be reported via metadata.\n> >\n> > I think we're good to go, except for the point that we've left\n> > floating about having this a single value or a per-channel value.\n> >\n> > I'm trying to get a feeling how this would be reported by your ISP. I\n> > see in example you have two per-channel values for the ColourGains\n> > control. Is this anyway related ?\n> >\n> > >\n> > > Any other thoughts?\n> > >\n> > > Thanks\n> > > David\n> > >\n> > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > wrote:\n> > > >\n> > > > Hi Jacopo,\n> > > >\n> > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > Hi Kieran,\n> > > > >\n> > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > >> Hi David,\n> > > > >>\n> > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > >>>\n> > > > >>> Signed-off-by: David Plowman \n> > > > >>> ---\n> > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > >>> 1 file changed, 11 insertions(+)\n> > > > >>>\n> > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > >>> index c8874fa9..e6362c74 100644\n> > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > >>> property, and the two can be used to implement digital zoom.\n> > > > >>> +\n> > > > >>> + - DigitalGain:\n> > > > >>> + type: float\n> > > > >>> + description: |\n> > > > >>> + Global digital gain value applied to the image during all the\n> > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > >>> +\n> > > > >>> + This control is intended to report the value used by the image\n> > > > >>> + processing pipeline.\n> > > > >>\n> > > > >>\n> > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > >> style value.\n> > > > >>\n> > > > >>\n> > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > >> will be a core issue anyway.\n> > > > >>\n> > > > >>\n> > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > >> determine that someone needs to set it.\n> > > > >>\n> > > > >> We could introduce a control property between type: and description:\n> > > > >> read-only: true\n> > > > >\n> > > > > Isn't a read-only control just a metadata ?\n> > > > >\n> > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > the control value from applications not reporting it in the list of\n> > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > request's metadata ?\n> > > >\n> > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > won't be there to set in the first place! I forgot about that.\n> > > >\n> > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > implicit.\n> > > >\n> > > > --\n> > > > Kieran\n> > > >\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > >>\n> > > > >>\n> > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > >> that brings it's own questions )\n> > > > >>\n> > > > >> Reviewed-by: Kieran Bingham \n> > > > >>\n> > > > >>\n> > > > >>> ...\n> > > > >>>\n> > > > >>\n> > > > >> --\n> > > > >> Regards\n> > > > >> --\n> > > > >> Kieran\n> > > > >> _______________________________________________\n> > > > >> libcamera-devel mailing list\n> > > > >> libcamera-devel@lists.libcamera.org\n> > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > >\n> > > > --\n> > > > Regards\n> > > > --\n> > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C4AABE176\n\tfor ;\n\tTue, 24 Nov 2020 11:28:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC7D5633F8;\n\tTue, 24 Nov 2020 12:28:08 +0100 (CET)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id AE83060335\n\tfor ;\n\tTue, 24 Nov 2020 12:28:07 +0100 (CET)","from uno.localdomain (host-79-25-179-116.retail.telecomitalia.it\n\t[79.25.179.116]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 19677E0012;\n\tTue, 24 Nov 2020 11:28:04 +0000 (UTC)"],"X-Originating-IP":"79.25.179.116","Date":"Tue, 24 Nov 2020 12:28:08 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20201124112808.mwlnk554z2a2esn4@uno.localdomain>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13847,"web_url":"https://patchwork.libcamera.org/comment/13847/","msgid":"","date":"2020-11-24T12:40:38","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the reply. More discussion below...\n\nOn Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > You're right, there's a relationship there. ColourGains obviously\n> > gives you the red and blue gains determined by the AWB usually. You\n> > might get the values 1.6 and 2.0 (for red and blue)\n> >\n> > In our case, if we report a single \"global gain\" value you can kind of\n> > imagine it as the green gain, where the colour gains were normalised\n> > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > 1.25, 2.5).\n>\n> That's a really helpful explanation for me, thanks\n>\n> >\n> > In the per-channel case I guess you'd be reporting these 3 numbers\n> > directly. For me this duplicates information that's already in the\n> > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > pipeline that lets you set a global gain - you'd have to query the\n> > current white balance and work out all three numbers and set them. But\n> > then you've set the white balance as well. Or maybe we do something\n> > special so that you haven't? You see why it confuses me! So on balance\n> > I'm with the single value approach, though I could live either way.\n>\n> I see. With the above explanation I really see why a single global\n> value is probably enough.\n>\n> My question is now: are the above notions (blue/red gains normalized\n> on a green value of 1, how global gain is used to obtain per-channel\n> gains etc) standard knowledge I'm lacking, or:\n>\n> 1) It's worth to describe them in the control documentation as they're\n> \"standard\" and won't change per-pipeline\n> 2) They might differe per-pipeline and they need to be\n> described in the pipeline model documentation ?\n> 3) It is assumed the reader knows she's dealing with\n\nDifficult. I can't really see too many other ways of defining it, but\nyou know, pipelines all have their nuances. You probably can't\nguarantee that what I described is completely \"standard\".\n\nA slightly more qualitative definition seems OK to me - the amount of\nlinear gain applied by the pipeline to all pixels (in addition to the\ncolour gains). Particular pipelines might feel they need to document\nit more carefully if there are any complications in how they deal with\nit.\n\nBest regards\nDavid\n\n>\n> Thanks\n> j\n>\n> >\n> > Thanks!\n> > David\n> >\n> > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > >\n> > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > Hi everyone\n> > > >\n> > > > Sounds like we're happy enough from the point of view of this thing\n> > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > be better as\n> > > >\n> > > > \"This control is present in a request's ControlList only if the\n> > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > application, the pipeline may still report the actual value used in\n> > > > the metadata returned with completed requests.\"\n> > >\n> > > I don't think it it necessary. It is implied that if a pipeline\n> > > handler does not support changing the digital gain it should not\n> > > expose it a one of the Camera's controls.\n> > >\n> > > Likewise, if it is something that applications should be informed of,\n> > > it will be reported via metadata.\n> > >\n> > > I think we're good to go, except for the point that we've left\n> > > floating about having this a single value or a per-channel value.\n> > >\n> > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > see in example you have two per-channel values for the ColourGains\n> > > control. Is this anyway related ?\n> > >\n> > > >\n> > > > Any other thoughts?\n> > > >\n> > > > Thanks\n> > > > David\n> > > >\n> > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > > wrote:\n> > > > >\n> > > > > Hi Jacopo,\n> > > > >\n> > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > Hi Kieran,\n> > > > > >\n> > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > >> Hi David,\n> > > > > >>\n> > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > >>>\n> > > > > >>> Signed-off-by: David Plowman \n> > > > > >>> ---\n> > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > >>>\n> > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > >>> +\n> > > > > >>> + - DigitalGain:\n> > > > > >>> + type: float\n> > > > > >>> + description: |\n> > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > >>> +\n> > > > > >>> + This control is intended to report the value used by the image\n> > > > > >>> + processing pipeline.\n> > > > > >>\n> > > > > >>\n> > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > >> style value.\n> > > > > >>\n> > > > > >>\n> > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > >> will be a core issue anyway.\n> > > > > >>\n> > > > > >>\n> > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > >> determine that someone needs to set it.\n> > > > > >>\n> > > > > >> We could introduce a control property between type: and description:\n> > > > > >> read-only: true\n> > > > > >\n> > > > > > Isn't a read-only control just a metadata ?\n> > > > > >\n> > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > the control value from applications not reporting it in the list of\n> > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > request's metadata ?\n> > > > >\n> > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > won't be there to set in the first place! I forgot about that.\n> > > > >\n> > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > implicit.\n> > > > >\n> > > > > --\n> > > > > Kieran\n> > > > >\n> > > > >\n> > > > > >\n> > > > > > Thanks\n> > > > > > j\n> > > > > >\n> > > > > >>\n> > > > > >>\n> > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > >> that brings it's own questions )\n> > > > > >>\n> > > > > >> Reviewed-by: Kieran Bingham \n> > > > > >>\n> > > > > >>\n> > > > > >>> ...\n> > > > > >>>\n> > > > > >>\n> > > > > >> --\n> > > > > >> Regards\n> > > > > >> --\n> > > > > >> Kieran\n> > > > > >> _______________________________________________\n> > > > > >> libcamera-devel mailing list\n> > > > > >> libcamera-devel@lists.libcamera.org\n> > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > >\n> > > > > --\n> > > > > Regards\n> > > > > --\n> > > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 1F47DBE08A\n\tfor ;\n\tTue, 24 Nov 2020 12:40:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7E31C633F8;\n\tTue, 24 Nov 2020 13:40:53 +0100 (CET)","from mail-ot1-x343.google.com (mail-ot1-x343.google.com\n\t[IPv6:2607:f8b0:4864:20::343])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 32B4B60332\n\tfor ;\n\tTue, 24 Nov 2020 13:40:52 +0100 (CET)","by mail-ot1-x343.google.com with SMTP id 92so16113546otd.5\n\tfor ;\n\tTue, 24 Nov 2020 04:40:51 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"objy+jvH\"; 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=n/VAVhdwyiKndu+swIIhW9siivW4V1hjKmn2/RVfShM=;\n\tb=objy+jvHBAZCyan3xKmhflbM59l/hdfa1DF/LmWQmfaXIadNUNVtjo5ToPzXWyVVBF\n\tyMjjx1mTDYf3gKi1gBamqZM22rZYGKaKCsJPDA9NAYOxJMwanp4hnv2/ymV/hBwkkcaM\n\tAmROaPkfLm3oJl0d/hCLJCQRp9UPrY9ue55cfD4o+n0Zm6S083yhHNOd87tjvPmZM+9u\n\tUk6BZuyjGUvexSkUEa9s42N9jKqKWs4/AC2I3ojlpda1vWxXJMifPERO/9Eqg84NVR9i\n\t+MBA103WyaCiAiCOJxxX4h9YQy4B5tZNHCnadO73+6T58wb2gHz9d10mLTxsML7wRZRi\n\tflmg==","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=n/VAVhdwyiKndu+swIIhW9siivW4V1hjKmn2/RVfShM=;\n\tb=Lg0IBOEJ5sSm08bRw2fLFvvOlZewOO2qYvpTwrizYo1DwvVxbDgz0G9B0BRtKpzonb\n\taQUkufAZCvWjLPb+5orLqv7LEvIVhBEI4PXrrJXCsA3SLcr/zA41ffjXKhXJ7i63z+OU\n\tsbdclNLI96+j3Lz8yi2D+vVNghUO6/jtVga2RbcEu42clHwZTLNTI2P4pO9DwRu5aj+x\n\ty6tz9N76it+uDjgkBxKY09wSxx8eyTTYYQCkPQFCYqjKBoQuDV+RUKfwznVo2bMnsy28\n\tVxTCcym1fYjOnvO7vkhKoXNcmvhTAZgocLJ+CvJqqClkJlD/H629vvoLVezfw8pbZ4rb\n\tntuA==","X-Gm-Message-State":"AOAM531U7Cdz5fQNWUNv6djk8uXX6nwZBjDIdm/AF7UtKFmzLDA6CeDw\n\t4hYNfwX9g/a44OpeGbcQLB/QSV3oI3+sK1v+FgiYImdVbJomHA==","X-Google-Smtp-Source":"ABdhPJz++QStCa+fTUrVELs6PyqDYSW9VuexncOe7LxbCFZKSZhjnwJDctYmJZXOfUibVZ2X4Fbo0uqMwp4UE4VaO7o=","X-Received":"by 2002:a05:6830:18ee:: with SMTP id\n\td14mr3220689otf.317.1606221649630; \n\tTue, 24 Nov 2020 04:40:49 -0800 (PST)","MIME-Version":"1.0","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>","In-Reply-To":"<20201124112808.mwlnk554z2a2esn4@uno.localdomain>","From":"David Plowman ","Date":"Tue, 24 Nov 2020 12:40:38 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13889,"web_url":"https://patchwork.libcamera.org/comment/13889/","msgid":"<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>","date":"2020-11-25T21:03:00","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n I tried proposing a new version based on the outcome of our\ndiscussion\n\nPlease see below\n\nOn Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the reply. More discussion below...\n>\n> On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> >\n> > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > You're right, there's a relationship there. ColourGains obviously\n> > > gives you the red and blue gains determined by the AWB usually. You\n> > > might get the values 1.6 and 2.0 (for red and blue)\n> > >\n> > > In our case, if we report a single \"global gain\" value you can kind of\n> > > imagine it as the green gain, where the colour gains were normalised\n> > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > 1.25, 2.5).\n> >\n> > That's a really helpful explanation for me, thanks\n> >\n> > >\n> > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > directly. For me this duplicates information that's already in the\n> > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > pipeline that lets you set a global gain - you'd have to query the\n> > > current white balance and work out all three numbers and set them. But\n> > > then you've set the white balance as well. Or maybe we do something\n> > > special so that you haven't? You see why it confuses me! So on balance\n> > > I'm with the single value approach, though I could live either way.\n> >\n> > I see. With the above explanation I really see why a single global\n> > value is probably enough.\n> >\n> > My question is now: are the above notions (blue/red gains normalized\n> > on a green value of 1, how global gain is used to obtain per-channel\n> > gains etc) standard knowledge I'm lacking, or:\n> >\n> > 1) It's worth to describe them in the control documentation as they're\n> > \"standard\" and won't change per-pipeline\n> > 2) They might differe per-pipeline and they need to be\n> > described in the pipeline model documentation ?\n> > 3) It is assumed the reader knows she's dealing with\n>\n> Difficult. I can't really see too many other ways of defining it, but\n> you know, pipelines all have their nuances. You probably can't\n> guarantee that what I described is completely \"standard\".\n>\n> A slightly more qualitative definition seems OK to me - the amount of\n> linear gain applied by the pipeline to all pixels (in addition to the\n> colour gains). Particular pipelines might feel they need to document\n> it more carefully if there are any complications in how they deal with\n> it.\n>\n> Best regards\n> David\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > Thanks!\n> > > David\n> > >\n> > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > Hi everyone\n> > > > >\n> > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > be better as\n> > > > >\n> > > > > \"This control is present in a request's ControlList only if the\n> > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > application, the pipeline may still report the actual value used in\n> > > > > the metadata returned with completed requests.\"\n> > > >\n> > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > handler does not support changing the digital gain it should not\n> > > > expose it a one of the Camera's controls.\n> > > >\n> > > > Likewise, if it is something that applications should be informed of,\n> > > > it will be reported via metadata.\n> > > >\n> > > > I think we're good to go, except for the point that we've left\n> > > > floating about having this a single value or a per-channel value.\n> > > >\n> > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > see in example you have two per-channel values for the ColourGains\n> > > > control. Is this anyway related ?\n> > > >\n> > > > >\n> > > > > Any other thoughts?\n> > > > >\n> > > > > Thanks\n> > > > > David\n> > > > >\n> > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > > > wrote:\n> > > > > >\n> > > > > > Hi Jacopo,\n> > > > > >\n> > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > Hi Kieran,\n> > > > > > >\n> > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > >> Hi David,\n> > > > > > >>\n> > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > >>>\n> > > > > > >>> Signed-off-by: David Plowman \n> > > > > > >>> ---\n> > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > >>>\n> > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > >>> +\n> > > > > > >>> + - DigitalGain:\n> > > > > > >>> + type: float\n> > > > > > >>> + description: |\n> > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > >>> +\n> > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > >>> + processing pipeline.\n\n - DigitalGain:\n type: float\n description: |\n Digital gain value applied during the processing steps applied\n to the image as captured from the sensor.\n\n The global digital gain factor is applied to all the colour channels\n of the RAW image. Different pipeline models are free to\n specify how the global gain factor applies to each separate\n channel.\n\n If an imaging pipeline applies digital gain in distinct\n processing steps, this value indicates their total sum.\n Pipelines are free to decide how to adjust each processing\n step to respect the received gain factor and shall report\n their total value in the request metadata.\n\nHowever this deferring to each pipeline model documentation makes me\nwonder if expressing per-channel values would make things more\nexplicit...\n\nFeel free to take in what you consider appropriate.\n\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > >> style value.\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > >> will be a core issue anyway.\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > >> determine that someone needs to set it.\n> > > > > > >>\n> > > > > > >> We could introduce a control property between type: and description:\n> > > > > > >> read-only: true\n> > > > > > >\n> > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > >\n> > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > request's metadata ?\n> > > > > >\n> > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > >\n> > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > implicit.\n> > > > > >\n> > > > > > --\n> > > > > > Kieran\n> > > > > >\n> > > > > >\n> > > > > > >\n> > > > > > > Thanks\n> > > > > > > j\n> > > > > > >\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > >> that brings it's own questions )\n> > > > > > >>\n> > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > >>\n> > > > > > >>\n> > > > > > >>> ...\n> > > > > > >>>\n> > > > > > >>\n> > > > > > >> --\n> > > > > > >> Regards\n> > > > > > >> --\n> > > > > > >> Kieran\n> > > > > > >> _______________________________________________\n> > > > > > >> libcamera-devel mailing list\n> > > > > > >> libcamera-devel@lists.libcamera.org\n> > > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > >\n> > > > > > --\n> > > > > > Regards\n> > > > > > --\n> > > > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 6E611BE176\n\tfor ;\n\tWed, 25 Nov 2020 21:02:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 07C2D63412;\n\tWed, 25 Nov 2020 22:02:59 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2165763402\n\tfor ;\n\tWed, 25 Nov 2020 22:02:58 +0100 (CET)","from uno.localdomain (host-80-104-176-17.retail.telecomitalia.it\n\t[80.104.176.17]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id CB8F6C0006;\n\tWed, 25 Nov 2020 21:02:56 +0000 (UTC)"],"X-Originating-IP":"80.104.176.17","Date":"Wed, 25 Nov 2020 22:03:00 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13897,"web_url":"https://patchwork.libcamera.org/comment/13897/","msgid":"","date":"2020-11-26T09:50:05","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nThanks for the update on this.\n\nOn Wed, 25 Nov 2020 at 21:02, Jacopo Mondi wrote:\n>\n> Hi David,\n> I tried proposing a new version based on the outcome of our\n> discussion\n>\n> Please see below\n>\n> On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the reply. More discussion below...\n> >\n> > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > >\n> > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > > Hi Jacopo\n> > > >\n> > > > You're right, there's a relationship there. ColourGains obviously\n> > > > gives you the red and blue gains determined by the AWB usually. You\n> > > > might get the values 1.6 and 2.0 (for red and blue)\n> > > >\n> > > > In our case, if we report a single \"global gain\" value you can kind of\n> > > > imagine it as the green gain, where the colour gains were normalised\n> > > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > > 1.25, 2.5).\n> > >\n> > > That's a really helpful explanation for me, thanks\n> > >\n> > > >\n> > > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > > directly. For me this duplicates information that's already in the\n> > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > > pipeline that lets you set a global gain - you'd have to query the\n> > > > current white balance and work out all three numbers and set them. But\n> > > > then you've set the white balance as well. Or maybe we do something\n> > > > special so that you haven't? You see why it confuses me! So on balance\n> > > > I'm with the single value approach, though I could live either way.\n> > >\n> > > I see. With the above explanation I really see why a single global\n> > > value is probably enough.\n> > >\n> > > My question is now: are the above notions (blue/red gains normalized\n> > > on a green value of 1, how global gain is used to obtain per-channel\n> > > gains etc) standard knowledge I'm lacking, or:\n> > >\n> > > 1) It's worth to describe them in the control documentation as they're\n> > > \"standard\" and won't change per-pipeline\n> > > 2) They might differe per-pipeline and they need to be\n> > > described in the pipeline model documentation ?\n> > > 3) It is assumed the reader knows she's dealing with\n> >\n> > Difficult. I can't really see too many other ways of defining it, but\n> > you know, pipelines all have their nuances. You probably can't\n> > guarantee that what I described is completely \"standard\".\n> >\n> > A slightly more qualitative definition seems OK to me - the amount of\n> > linear gain applied by the pipeline to all pixels (in addition to the\n> > colour gains). Particular pipelines might feel they need to document\n> > it more carefully if there are any complications in how they deal with\n> > it.\n> >\n> > Best regards\n> > David\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > Thanks!\n> > > > David\n> > > >\n> > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi David,\n> > > > >\n> > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > > Hi everyone\n> > > > > >\n> > > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > > be better as\n> > > > > >\n> > > > > > \"This control is present in a request's ControlList only if the\n> > > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > > application, the pipeline may still report the actual value used in\n> > > > > > the metadata returned with completed requests.\"\n> > > > >\n> > > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > > handler does not support changing the digital gain it should not\n> > > > > expose it a one of the Camera's controls.\n> > > > >\n> > > > > Likewise, if it is something that applications should be informed of,\n> > > > > it will be reported via metadata.\n> > > > >\n> > > > > I think we're good to go, except for the point that we've left\n> > > > > floating about having this a single value or a per-channel value.\n> > > > >\n> > > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > > see in example you have two per-channel values for the ColourGains\n> > > > > control. Is this anyway related ?\n> > > > >\n> > > > > >\n> > > > > > Any other thoughts?\n> > > > > >\n> > > > > > Thanks\n> > > > > > David\n> > > > > >\n> > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > > > > wrote:\n> > > > > > >\n> > > > > > > Hi Jacopo,\n> > > > > > >\n> > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > > Hi Kieran,\n> > > > > > > >\n> > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > > >> Hi David,\n> > > > > > > >>\n> > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > > >>>\n> > > > > > > >>> Signed-off-by: David Plowman \n> > > > > > > >>> ---\n> > > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > > >>>\n> > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > > >>> +\n> > > > > > > >>> + - DigitalGain:\n> > > > > > > >>> + type: float\n> > > > > > > >>> + description: |\n> > > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > > >>> +\n> > > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > > >>> + processing pipeline.\n>\n> - DigitalGain:\n> type: float\n> description: |\n> Digital gain value applied during the processing steps applied\n> to the image as captured from the sensor.\n>\n> The global digital gain factor is applied to all the colour channels\n> of the RAW image. Different pipeline models are free to\n> specify how the global gain factor applies to each separate\n> channel.\n>\n> If an imaging pipeline applies digital gain in distinct\n> processing steps, this value indicates their total sum.\n> Pipelines are free to decide how to adjust each processing\n> step to respect the received gain factor and shall report\n> their total value in the request metadata.\n>\n> However this deferring to each pipeline model documentation makes me\n> wonder if expressing per-channel values would make things more\n> explicit...\n>\n> Feel free to take in what you consider appropriate.\n\nYes, this is all so difficult, we just need to make a decision,\nreally. Per-channel values work for me, but only because we're not\ngoing to let people set them. As a read-only thing, it seems a bit\neasier to understand/explain. We could ignore the question of setting\na global digital gain until someone wants to do it (maybe no one ever\nwill...).\n\nWhat do you think?\n\nThanks!\nDavid\n\n>\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > > >> style value.\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > > >> will be a core issue anyway.\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > > >> determine that someone needs to set it.\n> > > > > > > >>\n> > > > > > > >> We could introduce a control property between type: and description:\n> > > > > > > >> read-only: true\n> > > > > > > >\n> > > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > > >\n> > > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > > request's metadata ?\n> > > > > > >\n> > > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > > >\n> > > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > > implicit.\n> > > > > > >\n> > > > > > > --\n> > > > > > > Kieran\n> > > > > > >\n> > > > > > >\n> > > > > > > >\n> > > > > > > > Thanks\n> > > > > > > > j\n> > > > > > > >\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > > >> that brings it's own questions )\n> > > > > > > >>\n> > > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >>> ...\n> > > > > > > >>>\n> > > > > > > >>\n> > > > > > > >> --\n> > > > > > > >> Regards\n> > > > > > > >> --\n> > > > > > > >> Kieran\n> > > > > > > >> _______________________________________________\n> > > > > > > >> libcamera-devel mailing list\n> > > > > > > >> libcamera-devel@lists.libcamera.org\n> > > > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > > >\n> > > > > > > --\n> > > > > > > Regards\n> > > > > > > --\n> > > > > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B0EF1BE08A\n\tfor ;\n\tThu, 26 Nov 2020 09:50:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7B6F16343F;\n\tThu, 26 Nov 2020 10:50:19 +0100 (CET)","from mail-ot1-x344.google.com (mail-ot1-x344.google.com\n\t[IPv6:2607:f8b0:4864:20::344])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EA76160331\n\tfor ;\n\tThu, 26 Nov 2020 10:50:17 +0100 (CET)","by mail-ot1-x344.google.com with SMTP id n12so1416253otk.0\n\tfor ;\n\tThu, 26 Nov 2020 01:50:17 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"n1/12DWZ\"; 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=I2QLWN7DTJLTreVswp55P38mOdvG/nD0HlulQi8CShg=;\n\tb=n1/12DWZ3BmzG0o1LUTMb9E3s2vKzL4mODvD9xsxXB2ZvvUJBKy5TAr6fusJpbpKQL\n\tvV0Py+6i0updw8EkBsnQhfzvLIRuNBsZ3sjvheGXI6ufCfYxrZJYjUIl24fSgAmgR2b1\n\tTxNTZNmMAti+g9Ok+65lK0QlFDMN/LC8l9Sk2hxAFDHaUvWtQu//1xRu3B9pPZkg3xvJ\n\tFOjBvA1Y4u5Mya3q9rmwDU706L4OTdRRrUuVzN5C5x8SJkqLTsRX038Cz/6Pw0s7ULrP\n\t/DkeVuZ/1P6kWKG7L86Ybj4BlKdVukD8j29CQ2L6K9AZ/W7gMETVUl403d2+wiODu6Ks\n\tNDcg==","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=I2QLWN7DTJLTreVswp55P38mOdvG/nD0HlulQi8CShg=;\n\tb=oPA2NzwXbxMeI/V3C7bTSLeTdCLbkY7c1NCx4+l4Zxl2jlZP7KlDlJROszzqQW0Z9X\n\tzVqvVqUJzTJAng9q4oFoOjnnaoLOp4xCjhtE48MsZ/6vBpTXV0KlTO7vk8yghJVNt3KA\n\t/ZbXeyPezGubaSygRFqf022MMODkJkk1VQCg2+xsxY41tHpdgXXLJD7wz9l2AbykJp2T\n\t6Ftufieh2rVS44HpsjI5KFjUTPi2Jb3/BJ+vxqWlb5OpTuReepTnDtna8rj03JqnbAC6\n\tAhRxRjBBxXa0oZiyM6Yv3ReTybZKKizhKS3lfqUPIU+LUu6bk604l9HdKB6MI9hpbQMu\n\teBug==","X-Gm-Message-State":"AOAM530/ZHMEtT5W8vEweB9Cnx+eD+9AjS9VXEIKvDOer7LMuTQmGXoB\n\tmQrtlSTqfX8fLrixsIkg994pKfzMZHAW5sqDyMT7nbt9XgVz/Q==","X-Google-Smtp-Source":"ABdhPJyWfOHcLnvpbtdc8okv/OjvVZa3mSw2ApZI9Vbp/1O5cbc0Znv5LGsXgpaNmGgXOR7Sh2CxLxFSu/UnwJL4K0o=","X-Received":"by 2002:a9d:6f8f:: with SMTP id\n\th15mr1830686otq.166.1606384216376; \n\tThu, 26 Nov 2020 01:50:16 -0800 (PST)","MIME-Version":"1.0","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>","In-Reply-To":"<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>","From":"David Plowman ","Date":"Thu, 26 Nov 2020 09:50:05 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13902,"web_url":"https://patchwork.libcamera.org/comment/13902/","msgid":"<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","date":"2020-11-26T10:25:22","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Thu, Nov 26, 2020 at 09:50:05AM +0000, David Plowman wrote:\n> Hi Jacopo\n>\n> Thanks for the update on this.\n>\n> On Wed, 25 Nov 2020 at 21:02, Jacopo Mondi wrote:\n> >\n> > Hi David,\n> > I tried proposing a new version based on the outcome of our\n> > discussion\n> >\n> > Please see below\n> >\n> > On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > Thanks for the reply. More discussion below...\n> > >\n> > > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > > >\n> > > > Hi David,\n> > > >\n> > > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > > > Hi Jacopo\n> > > > >\n> > > > > You're right, there's a relationship there. ColourGains obviously\n> > > > > gives you the red and blue gains determined by the AWB usually. You\n> > > > > might get the values 1.6 and 2.0 (for red and blue)\n> > > > >\n> > > > > In our case, if we report a single \"global gain\" value you can kind of\n> > > > > imagine it as the green gain, where the colour gains were normalised\n> > > > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > > > 1.25, 2.5).\n> > > >\n> > > > That's a really helpful explanation for me, thanks\n> > > >\n> > > > >\n> > > > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > > > directly. For me this duplicates information that's already in the\n> > > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > > > pipeline that lets you set a global gain - you'd have to query the\n> > > > > current white balance and work out all three numbers and set them. But\n> > > > > then you've set the white balance as well. Or maybe we do something\n> > > > > special so that you haven't? You see why it confuses me! So on balance\n> > > > > I'm with the single value approach, though I could live either way.\n> > > >\n> > > > I see. With the above explanation I really see why a single global\n> > > > value is probably enough.\n> > > >\n> > > > My question is now: are the above notions (blue/red gains normalized\n> > > > on a green value of 1, how global gain is used to obtain per-channel\n> > > > gains etc) standard knowledge I'm lacking, or:\n> > > >\n> > > > 1) It's worth to describe them in the control documentation as they're\n> > > > \"standard\" and won't change per-pipeline\n> > > > 2) They might differe per-pipeline and they need to be\n> > > > described in the pipeline model documentation ?\n> > > > 3) It is assumed the reader knows she's dealing with\n> > >\n> > > Difficult. I can't really see too many other ways of defining it, but\n> > > you know, pipelines all have their nuances. You probably can't\n> > > guarantee that what I described is completely \"standard\".\n> > >\n> > > A slightly more qualitative definition seems OK to me - the amount of\n> > > linear gain applied by the pipeline to all pixels (in addition to the\n> > > colour gains). Particular pipelines might feel they need to document\n> > > it more carefully if there are any complications in how they deal with\n> > > it.\n> > >\n> > > Best regards\n> > > David\n> > >\n> > > >\n> > > > Thanks\n> > > > j\n> > > >\n> > > > >\n> > > > > Thanks!\n> > > > > David\n> > > > >\n> > > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > > >\n> > > > > > Hi David,\n> > > > > >\n> > > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > > > Hi everyone\n> > > > > > >\n> > > > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > > > be better as\n> > > > > > >\n> > > > > > > \"This control is present in a request's ControlList only if the\n> > > > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > > > application, the pipeline may still report the actual value used in\n> > > > > > > the metadata returned with completed requests.\"\n> > > > > >\n> > > > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > > > handler does not support changing the digital gain it should not\n> > > > > > expose it a one of the Camera's controls.\n> > > > > >\n> > > > > > Likewise, if it is something that applications should be informed of,\n> > > > > > it will be reported via metadata.\n> > > > > >\n> > > > > > I think we're good to go, except for the point that we've left\n> > > > > > floating about having this a single value or a per-channel value.\n> > > > > >\n> > > > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > > > see in example you have two per-channel values for the ColourGains\n> > > > > > control. Is this anyway related ?\n> > > > > >\n> > > > > > >\n> > > > > > > Any other thoughts?\n> > > > > > >\n> > > > > > > Thanks\n> > > > > > > David\n> > > > > > >\n> > > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > > > > > wrote:\n> > > > > > > >\n> > > > > > > > Hi Jacopo,\n> > > > > > > >\n> > > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > > > Hi Kieran,\n> > > > > > > > >\n> > > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > > > >> Hi David,\n> > > > > > > > >>\n> > > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > > > >>>\n> > > > > > > > >>> Signed-off-by: David Plowman \n> > > > > > > > >>> ---\n> > > > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > > > >>>\n> > > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > > > >>> +\n> > > > > > > > >>> + - DigitalGain:\n> > > > > > > > >>> + type: float\n> > > > > > > > >>> + description: |\n> > > > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > > > >>> +\n> > > > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > > > >>> + processing pipeline.\n> >\n> > - DigitalGain:\n> > type: float\n> > description: |\n> > Digital gain value applied during the processing steps applied\n> > to the image as captured from the sensor.\n> >\n> > The global digital gain factor is applied to all the colour channels\n> > of the RAW image. Different pipeline models are free to\n> > specify how the global gain factor applies to each separate\n> > channel.\n> >\n> > If an imaging pipeline applies digital gain in distinct\n> > processing steps, this value indicates their total sum.\n> > Pipelines are free to decide how to adjust each processing\n> > step to respect the received gain factor and shall report\n> > their total value in the request metadata.\n> >\n> > However this deferring to each pipeline model documentation makes me\n> > wonder if expressing per-channel values would make things more\n> > explicit...\n> >\n> > Feel free to take in what you consider appropriate.\n>\n> Yes, this is all so difficult, we just need to make a decision,\n> really. Per-channel values work for me, but only because we're not\n> going to let people set them. As a read-only thing, it seems a bit\n> easier to understand/explain. We could ignore the question of setting\n> a global digital gain until someone wants to do it (maybe no one ever\n> will...).\n>\n> What do you think?\n\nI think that once we have a pipeline that supports per-channel\ndigital gains we will introduce 'DigitalGains' as an array control,\ndeprectate this one, and keep supporting it in pipelines that exposed\nit (through an internal helper maybe) not to break applications that\nmade use of it.\n\nIt's hard for me to have a firm idea without a real use case, but one\nthing that presses me is not to mention this control is read only in\nits definition. If that's the case for RPi just report it in metadata\nonly, but let's not restrict other platforms.\n\nDo you think in the next iteration you will include reporting this\ncontrol from the pipeline too ? if I'm not mistaken this version\ncontains the control definition only.\n\nThanks\n j\n\n\n>\n> Thanks!\n> David\n>\n> >\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > > > >> style value.\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > > > >> will be a core issue anyway.\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > > > >> determine that someone needs to set it.\n> > > > > > > > >>\n> > > > > > > > >> We could introduce a control property between type: and description:\n> > > > > > > > >> read-only: true\n> > > > > > > > >\n> > > > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > > > >\n> > > > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > > > request's metadata ?\n> > > > > > > >\n> > > > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > > > >\n> > > > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > > > implicit.\n> > > > > > > >\n> > > > > > > > --\n> > > > > > > > Kieran\n> > > > > > > >\n> > > > > > > >\n> > > > > > > > >\n> > > > > > > > > Thanks\n> > > > > > > > > j\n> > > > > > > > >\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > > > >> that brings it's own questions )\n> > > > > > > > >>\n> > > > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >>> ...\n> > > > > > > > >>>\n> > > > > > > > >>\n> > > > > > > > >> --\n> > > > > > > > >> Regards\n> > > > > > > > >> --\n> > > > > > > > >> Kieran\n> > > > > > > > >> _______________________________________________\n> > > > > > > > >> libcamera-devel mailing list\n> > > > > > > > >> libcamera-devel@lists.libcamera.org\n> > > > > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > > > >\n> > > > > > > > --\n> > > > > > > > Regards\n> > > > > > > > --\n> > > > > > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 62748BE08A\n\tfor ;\n\tThu, 26 Nov 2020 10:25:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CE46A6344A;\n\tThu, 26 Nov 2020 11:25:20 +0100 (CET)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 87D166343C\n\tfor ;\n\tThu, 26 Nov 2020 11:25:19 +0100 (CET)","from uno.localdomain (host-80-104-176-17.retail.telecomitalia.it\n\t[80.104.176.17]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id CF5D520000E;\n\tThu, 26 Nov 2020 10:25:17 +0000 (UTC)"],"Date":"Thu, 26 Nov 2020 11:25:22 +0100","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","References":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13905,"web_url":"https://patchwork.libcamera.org/comment/13905/","msgid":"<20201126104955.GD3905@pendragon.ideasonboard.com>","date":"2020-11-26T10:49:55","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Nov 23, 2020 at 09:17:33AM +0000, Kieran Bingham wrote:\n> On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> >> On 27/10/2020 14:12, David Plowman wrote:\n> >>> This control reports the global digital gain applied by the pipeline\n> >>> as a whole, after capturing a raw image from the sensor.\n> >>>\n> >>> Signed-off-by: David Plowman \n> >>> ---\n> >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> >>> 1 file changed, 11 insertions(+)\n> >>>\n> >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>> index c8874fa9..e6362c74 100644\n> >>> --- a/src/libcamera/control_ids.yaml\n> >>> +++ b/src/libcamera/control_ids.yaml\n> >>> @@ -530,4 +530,15 @@ controls:\n> >>> This control is only present when the pipeline supports scaling. Its\n> >>> maximum valid value is given by the properties::ScalerCropMaximum\n> >>> property, and the two can be used to implement digital zoom.\n> >>> +\n> >>> + - DigitalGain:\n> >>> + type: float\n> >>> + description: |\n> >>> + Global digital gain value applied to the image during all the\n> >>> + processing steps after capturing the image from the sensor. Any raw\n> >>> + images, therefore, will not include this gain, but the final images\n> >>> + output by the imaging pipeline as a whole will include it.\n> >>> +\n> >>> + This control is intended to report the value used by the image\n> >>> + processing pipeline.\n> >>\n> >> If this is a per-stream thing anyway, I guess it will then be up to\n> >> pipeline handlers to set this to the appropriate value for each stream\n> >> when it completes. The fact that this value would not be applicable to a\n> >> RAW stream makes me think it certainly should be a per-stream metadata\n> >> style value.\n> >>\n> >> I'd hope this could be handled by a common helper in that instance so it\n> >> doesn't get left out of some pipeline handlers, but included in some,\n> >> and become inconsistent. Not yet sure how we can handle that, but that\n> >> will be a core issue anyway.\n> >>\n> >> I wonder if we should mark this somehow as read-only, at least until we\n> >> determine that someone needs to set it.\n> >>\n> >> We could introduce a control property between type: and description:\n> >> read-only: true\n> > \n> > Isn't a read-only control just a metadata ?\n> > \n> > Wouldn't it be enough for a pipeline that does not support changing\n> > the control value from applications not reporting it in the list of\n> > supported Camera's controls, but only report it as part of a completed\n> > request's metadata ?\n> \n> Ah, yes of course - because if the control is not listed as supported it\n> won't be there to set in the first place! I forgot about that.\n> \n> So - indeed, no requirement to mark anything as read-only. That will be\n> implicit.\n\nThere could still be value in specifying this in the yaml file, to\ndocument which controls are meant to be read-only. Instead of writing\n\"This control is read-only\" or \"This control is meant to be used in\nmetadata only\", a read-only property (I'm sure we'll bikeshed the name,\nmaybe \"type: metadata\" would be better) would allow compile-time and\nruntime sanity checks. That's not a prerequisite for this patch though.\n\n> >> Otherwise, I see no objections currently. I think we're just waiting on\n> >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> >> that brings it's own questions )\n> >>\n> >> Reviewed-by: Kieran Bingham \n> >>\n> >>> ...\n> >>>\n> >>\n> >> --\n> >> Regards\n> >> --\n> >> Kieran\n> >> _______________________________________________\n> >> libcamera-devel mailing list\n> >> libcamera-devel@lists.libcamera.org\n> >> https://lists.libcamera.org/listinfo/libcamera-devel\n> \n> -- \n> Regards\n> --\n> Kieran\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D3EA7BE176\n\tfor ;\n\tThu, 26 Nov 2020 10:50:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6636263458;\n\tThu, 26 Nov 2020 11:50:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1F32F633EF\n\tfor ;\n\tThu, 26 Nov 2020 11:50:05 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70A15A1B;\n\tThu, 26 Nov 2020 11:50:04 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"btwgOrg2\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606387804;\n\tbh=T2KQf7AdOU7CbD4wHY2k8yuCenwB+/GmJa+AkO6GLps=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=btwgOrg2N7/sUNo/DrOpeZf0RnuTO9mu4VK7r5fCo9zjGD++h9mp08ySKsoh8RCyJ\n\td0fgf1ty1K6Aaf6QPtaMlBU/KDwvxQ3knYZ/IfgCoL5q6iMcA7NYeNiG3Pwahj71Nv\n\t+BbZ9QWei2lQSW/GHrjUVj7FH0ZrYdWaTdFbroNA=","Date":"Thu, 26 Nov 2020 12:49:55 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20201126104955.GD3905@pendragon.ideasonboard.com>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13906,"web_url":"https://patchwork.libcamera.org/comment/13906/","msgid":"<67accecd-9bba-3bbe-4ca6-b41b3ba35308@ideasonboard.com>","date":"2020-11-26T10:52:43","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 26/11/2020 10:49, Laurent Pinchart wrote:\n> Hello,\n> \n> On Mon, Nov 23, 2020 at 09:17:33AM +0000, Kieran Bingham wrote:\n>> On 23/11/2020 08:58, Jacopo Mondi wrote:\n>>> On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n>>>> On 27/10/2020 14:12, David Plowman wrote:\n>>>>> This control reports the global digital gain applied by the pipeline\n>>>>> as a whole, after capturing a raw image from the sensor.\n>>>>>\n>>>>> Signed-off-by: David Plowman \n>>>>> ---\n>>>>> src/libcamera/control_ids.yaml | 11 +++++++++++\n>>>>> 1 file changed, 11 insertions(+)\n>>>>>\n>>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n>>>>> index c8874fa9..e6362c74 100644\n>>>>> --- a/src/libcamera/control_ids.yaml\n>>>>> +++ b/src/libcamera/control_ids.yaml\n>>>>> @@ -530,4 +530,15 @@ controls:\n>>>>> This control is only present when the pipeline supports scaling. Its\n>>>>> maximum valid value is given by the properties::ScalerCropMaximum\n>>>>> property, and the two can be used to implement digital zoom.\n>>>>> +\n>>>>> + - DigitalGain:\n>>>>> + type: float\n>>>>> + description: |\n>>>>> + Global digital gain value applied to the image during all the\n>>>>> + processing steps after capturing the image from the sensor. Any raw\n>>>>> + images, therefore, will not include this gain, but the final images\n>>>>> + output by the imaging pipeline as a whole will include it.\n>>>>> +\n>>>>> + This control is intended to report the value used by the image\n>>>>> + processing pipeline.\n>>>>\n>>>> If this is a per-stream thing anyway, I guess it will then be up to\n>>>> pipeline handlers to set this to the appropriate value for each stream\n>>>> when it completes. The fact that this value would not be applicable to a\n>>>> RAW stream makes me think it certainly should be a per-stream metadata\n>>>> style value.\n>>>>\n>>>> I'd hope this could be handled by a common helper in that instance so it\n>>>> doesn't get left out of some pipeline handlers, but included in some,\n>>>> and become inconsistent. Not yet sure how we can handle that, but that\n>>>> will be a core issue anyway.\n>>>>\n>>>> I wonder if we should mark this somehow as read-only, at least until we\n>>>> determine that someone needs to set it.\n>>>>\n>>>> We could introduce a control property between type: and description:\n>>>> read-only: true\n>>>\n>>> Isn't a read-only control just a metadata ?\n>>>\n>>> Wouldn't it be enough for a pipeline that does not support changing\n>>> the control value from applications not reporting it in the list of\n>>> supported Camera's controls, but only report it as part of a completed\n>>> request's metadata ?\n>>\n>> Ah, yes of course - because if the control is not listed as supported it\n>> won't be there to set in the first place! I forgot about that.\n>>\n>> So - indeed, no requirement to mark anything as read-only. That will be\n>> implicit.\n> \n> There could still be value in specifying this in the yaml file, to\n> document which controls are meant to be read-only. Instead of writing\n> \"This control is read-only\" or \"This control is meant to be used in\n> metadata only\", a read-only property (I'm sure we'll bikeshed the name,\n> maybe \"type: metadata\" would be better) would allow compile-time and\n> runtime sanity checks. That's not a prerequisite for this patch though.\n\nI don't actually think this has to be a read-only control though.\n\nI can imagine this being an enabled control that will allow increasing\nthe gain of an image (without affecting colour balance).\n\nTo me - this is the same as AnalogueGain but applied at the ISP rather\nthan the sensor right ?\n\n--\nKieran\n\n\n> \n>>>> Otherwise, I see no objections currently. I think we're just waiting on\n>>>> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n>>>> that brings it's own questions )\n>>>>\n>>>> Reviewed-by: Kieran Bingham \n>>>>\n>>>>> ...\n>>>>>\n>>>>\n>>>> --\n>>>> Regards\n>>>> --\n>>>> Kieran\n>>>> _______________________________________________\n>>>> libcamera-devel mailing list\n>>>> libcamera-devel@lists.libcamera.org\n>>>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>\n>> -- \n>> Regards\n>> --\n>> Kieran\n>> _______________________________________________\n>> libcamera-devel mailing list\n>> libcamera-devel@lists.libcamera.org\n>> https://lists.libcamera.org/listinfo/libcamera-devel\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 59E51BE08A\n\tfor ;\n\tThu, 26 Nov 2020 10:52:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EC53763458;\n\tThu, 26 Nov 2020 11:52:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 01707633EF\n\tfor ;\n\tThu, 26 Nov 2020 11:52:46 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 774FFA1B;\n\tThu, 26 Nov 2020 11:52:46 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"EjiEkVYI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606387966;\n\tbh=LZv5udu0U1ghCF/RYgNecqHX5wbw8y/u7fiW1fLJMEg=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=EjiEkVYIlZH3wgYfF/j2vJhxFDnEeOmUC1/FyLyM1yZEdkWE8rRt/OT05NpSLSMlo\n\tQrtxsy/bv/QyL8UBuh/lhvYkoJj1GYUARDb2qGqIHV1g5OX8UGNsJKB7fGUdUdgcah\n\t08EgU7YwbF280XyCsGreRVhwp5LmebwubZrVkB/I=","To":"Laurent Pinchart ","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t<20201126104955.GD3905@pendragon.ideasonboard.com>","From":"Kieran Bingham ","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":"<67accecd-9bba-3bbe-4ca6-b41b3ba35308@ideasonboard.com>","Date":"Thu, 26 Nov 2020 10:52:43 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201126104955.GD3905@pendragon.ideasonboard.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13907,"web_url":"https://patchwork.libcamera.org/comment/13907/","msgid":"<53668e33-095b-6010-d856-5aaeb31f7a5e@ideasonboard.com>","date":"2020-11-26T10:57:14","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo, David,\n\nOn 26/11/2020 10:25, Jacopo Mondi wrote:\n> Hi David,\n> \n> On Thu, Nov 26, 2020 at 09:50:05AM +0000, David Plowman wrote:\n>> Hi Jacopo\n>>\n>> Thanks for the update on this.\n>>\n>> On Wed, 25 Nov 2020 at 21:02, Jacopo Mondi wrote:\n>>>\n>>> Hi David,\n>>> I tried proposing a new version based on the outcome of our\n>>> discussion\n>>>\n>>> Please see below\n>>>\n>>> On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n>>>> Hi Jacopo\n>>>>\n>>>> Thanks for the reply. More discussion below...\n>>>>\n>>>> On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n>>>>>\n>>>>> Hi David,\n>>>>>\n>>>>> On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n>>>>>> Hi Jacopo\n>>>>>>\n>>>>>> You're right, there's a relationship there. ColourGains obviously\n>>>>>> gives you the red and blue gains determined by the AWB usually. You\n>>>>>> might get the values 1.6 and 2.0 (for red and blue)\n>>>>>>\n>>>>>> In our case, if we report a single \"global gain\" value you can kind of\n>>>>>> imagine it as the green gain, where the colour gains were normalised\n>>>>>> for a green gain of 1. So if the global gain was 1.25, then the actual\n>>>>>> RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n>>>>>> 1.25, 2.5).\n>>>>>\n>>>>> That's a really helpful explanation for me, thanks\n>>>>>\n>>>>>>\n>>>>>> In the per-channel case I guess you'd be reporting these 3 numbers\n>>>>>> directly. For me this duplicates information that's already in the\n>>>>>> ColourGains, and it seems to muddle things up a bit. Imagine you had a\n>>>>>> pipeline that lets you set a global gain - you'd have to query the\n>>>>>> current white balance and work out all three numbers and set them. But\n>>>>>> then you've set the white balance as well. Or maybe we do something\n>>>>>> special so that you haven't? You see why it confuses me! So on balance\n>>>>>> I'm with the single value approach, though I could live either way.\n>>>>>\n>>>>> I see. With the above explanation I really see why a single global\n>>>>> value is probably enough.\n>>>>>\n>>>>> My question is now: are the above notions (blue/red gains normalized\n>>>>> on a green value of 1, how global gain is used to obtain per-channel\n>>>>> gains etc) standard knowledge I'm lacking, or:\n>>>>>\n>>>>> 1) It's worth to describe them in the control documentation as they're\n>>>>> \"standard\" and won't change per-pipeline\n>>>>> 2) They might differe per-pipeline and they need to be\n>>>>> described in the pipeline model documentation ?\n>>>>> 3) It is assumed the reader knows she's dealing with\n>>>>\n>>>> Difficult. I can't really see too many other ways of defining it, but\n>>>> you know, pipelines all have their nuances. You probably can't\n>>>> guarantee that what I described is completely \"standard\".\n>>>>\n>>>> A slightly more qualitative definition seems OK to me - the amount of\n>>>> linear gain applied by the pipeline to all pixels (in addition to the\n>>>> colour gains). Particular pipelines might feel they need to document\n>>>> it more carefully if there are any complications in how they deal with\n>>>> it.\n>>>>\n>>>> Best regards\n>>>> David\n>>>>\n>>>>>\n>>>>> Thanks\n>>>>> j\n>>>>>\n>>>>>>\n>>>>>> Thanks!\n>>>>>> David\n>>>>>>\n>>>>>> On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n>>>>>>>\n>>>>>>> Hi David,\n>>>>>>>\n>>>>>>> On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n>>>>>>>> Hi everyone\n>>>>>>>>\n>>>>>>>> Sounds like we're happy enough from the point of view of this thing\n>>>>>>>> being read-only (for Raspberry Pi at least). Would anyone want any\n>>>>>>>> changes to the wording? Perhaps the final sentence/paragraph might now\n>>>>>>>> be better as\n>>>>>>>>\n>>>>>>>> \"This control is present in a request's ControlList only if the\n>>>>>>>> pipeline supports setting the value. Even when it cannot be set by an\n>>>>>>>> application, the pipeline may still report the actual value used in\n>>>>>>>> the metadata returned with completed requests.\"\n>>>>>>>\n>>>>>>> I don't think it it necessary. It is implied that if a pipeline\n>>>>>>> handler does not support changing the digital gain it should not\n>>>>>>> expose it a one of the Camera's controls.\n>>>>>>>\n>>>>>>> Likewise, if it is something that applications should be informed of,\n>>>>>>> it will be reported via metadata.\n>>>>>>>\n>>>>>>> I think we're good to go, except for the point that we've left\n>>>>>>> floating about having this a single value or a per-channel value.\n>>>>>>>\n>>>>>>> I'm trying to get a feeling how this would be reported by your ISP. I\n>>>>>>> see in example you have two per-channel values for the ColourGains\n>>>>>>> control. Is this anyway related ?\n>>>>>>>\n>>>>>>>>\n>>>>>>>> Any other thoughts?\n>>>>>>>>\n>>>>>>>> Thanks\n>>>>>>>> David\n>>>>>>>>\n>>>>>>>> On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n>>>>>>>> wrote:\n>>>>>>>>>\n>>>>>>>>> Hi Jacopo,\n>>>>>>>>>\n>>>>>>>>> On 23/11/2020 08:58, Jacopo Mondi wrote:\n>>>>>>>>>> Hi Kieran,\n>>>>>>>>>>\n>>>>>>>>>> On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n>>>>>>>>>>> Hi David,\n>>>>>>>>>>>\n>>>>>>>>>>> On 27/10/2020 14:12, David Plowman wrote:\n>>>>>>>>>>>> This control reports the global digital gain applied by the pipeline\n>>>>>>>>>>>> as a whole, after capturing a raw image from the sensor.\n>>>>>>>>>>>>\n>>>>>>>>>>>> Signed-off-by: David Plowman \n>>>>>>>>>>>> ---\n>>>>>>>>>>>> src/libcamera/control_ids.yaml | 11 +++++++++++\n>>>>>>>>>>>> 1 file changed, 11 insertions(+)\n>>>>>>>>>>>>\n>>>>>>>>>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n>>>>>>>>>>>> index c8874fa9..e6362c74 100644\n>>>>>>>>>>>> --- a/src/libcamera/control_ids.yaml\n>>>>>>>>>>>> +++ b/src/libcamera/control_ids.yaml\n>>>>>>>>>>>> @@ -530,4 +530,15 @@ controls:\n>>>>>>>>>>>> This control is only present when the pipeline supports scaling. Its\n>>>>>>>>>>>> maximum valid value is given by the properties::ScalerCropMaximum\n>>>>>>>>>>>> property, and the two can be used to implement digital zoom.\n>>>>>>>>>>>> +\n>>>>>>>>>>>> + - DigitalGain:\n>>>>>>>>>>>> + type: float\n>>>>>>>>>>>> + description: |\n>>>>>>>>>>>> + Global digital gain value applied to the image during all the\n>>>>>>>>>>>> + processing steps after capturing the image from the sensor. Any raw\n>>>>>>>>>>>> + images, therefore, will not include this gain, but the final images\n>>>>>>>>>>>> + output by the imaging pipeline as a whole will include it.\n>>>>>>>>>>>> +\n>>>>>>>>>>>> + This control is intended to report the value used by the image\n>>>>>>>>>>>> + processing pipeline.\n>>>\n>>> - DigitalGain:\n>>> type: float\n>>> description: |\n>>> Digital gain value applied during the processing steps applied\n>>> to the image as captured from the sensor.\n>>>\n>>> The global digital gain factor is applied to all the colour channels\n>>> of the RAW image. Different pipeline models are free to\n>>> specify how the global gain factor applies to each separate\n>>> channel.\n>>>\n>>> If an imaging pipeline applies digital gain in distinct\n>>> processing steps, this value indicates their total sum.\n>>> Pipelines are free to decide how to adjust each processing\n>>> step to respect the received gain factor and shall report\n>>> their total value in the request metadata.\n>>>\n>>> However this deferring to each pipeline model documentation makes me\n>>> wonder if expressing per-channel values would make things more\n>>> explicit...\n>>>\n>>> Feel free to take in what you consider appropriate.\n>>\n>> Yes, this is all so difficult, we just need to make a decision,\n>> really. Per-channel values work for me, but only because we're not\n>> going to let people set them. As a read-only thing, it seems a bit\n>> easier to understand/explain. We could ignore the question of setting\n>> a global digital gain until someone wants to do it (maybe no one ever\n>> will...).\n>>\n>> What do you think?\n> \n> I think that once we have a pipeline that supports per-channel\n> digital gains we will introduce 'DigitalGains' as an array control,\n\nThat's called colour balance isn't it ?\n\nTo me - DigitalGain is analogous to AnalogueGain - and would be expected\nto not affect the colour balance.\n\nIf you want to control colour balance, that's manipulating the channels\nindependently, which we have a control for already.\n\n\nIn my opinion, DigitalGain could potentially be something that an\napplication might set, or even that an IPA might set indicating - \"I\nwant to increase the gain/brightness of the whole image (either beyond\nthe capability of the AnalogueGain, or if the AnalogueGain can not be\nchanged)\".\n\nAnd it would be an effect that is /after/ colour balance has occurred.\nPerhaps it might be important to declare specifically that the\nDigitalGain is a factor applied after any balance gains...\n\n--\nKieran\n\n\n\n> deprectate this one, and keep supporting it in pipelines that exposed\n> it (through an internal helper maybe) not to break applications that\n> made use of it.\n> \n> It's hard for me to have a firm idea without a real use case, but one\n> thing that presses me is not to mention this control is read only in\n> its definition. If that's the case for RPi just report it in metadata\n> only, but let's not restrict other platforms.\n> \n> Do you think in the next iteration you will include reporting this\n> control from the pipeline too ? if I'm not mistaken this version\n> contains the control definition only.\n> \n> Thanks\n> j\n> \n> \n>>\n>> Thanks!\n>> David\n>>\n>>>\n>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>> If this is a per-stream thing anyway, I guess it will then be up to\n>>>>>>>>>>> pipeline handlers to set this to the appropriate value for each stream\n>>>>>>>>>>> when it completes. The fact that this value would not be applicable to a\n>>>>>>>>>>> RAW stream makes me think it certainly should be a per-stream metadata\n>>>>>>>>>>> style value.\n>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>> I'd hope this could be handled by a common helper in that instance so it\n>>>>>>>>>>> doesn't get left out of some pipeline handlers, but included in some,\n>>>>>>>>>>> and become inconsistent. Not yet sure how we can handle that, but that\n>>>>>>>>>>> will be a core issue anyway.\n>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>> I wonder if we should mark this somehow as read-only, at least until we\n>>>>>>>>>>> determine that someone needs to set it.\n>>>>>>>>>>>\n>>>>>>>>>>> We could introduce a control property between type: and description:\n>>>>>>>>>>> read-only: true\n>>>>>>>>>>\n>>>>>>>>>> Isn't a read-only control just a metadata ?\n>>>>>>>>>>\n>>>>>>>>>> Wouldn't it be enough for a pipeline that does not support changing\n>>>>>>>>>> the control value from applications not reporting it in the list of\n>>>>>>>>>> supported Camera's controls, but only report it as part of a completed\n>>>>>>>>>> request's metadata ?\n>>>>>>>>>\n>>>>>>>>> Ah, yes of course - because if the control is not listed as supported it\n>>>>>>>>> won't be there to set in the first place! I forgot about that.\n>>>>>>>>>\n>>>>>>>>> So - indeed, no requirement to mark anything as read-only. That will be\n>>>>>>>>> implicit.\n>>>>>>>>>\n>>>>>>>>> --\n>>>>>>>>> Kieran\n>>>>>>>>>\n>>>>>>>>>\n>>>>>>>>>>\n>>>>>>>>>> Thanks\n>>>>>>>>>> j\n>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>> Otherwise, I see no objections currently. I think we're just waiting on\n>>>>>>>>>>> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n>>>>>>>>>>> that brings it's own questions )\n>>>>>>>>>>>\n>>>>>>>>>>> Reviewed-by: Kieran Bingham \n>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>>> ...\n>>>>>>>>>>>>\n>>>>>>>>>>>\n>>>>>>>>>>> --\n>>>>>>>>>>> Regards\n>>>>>>>>>>> --\n>>>>>>>>>>> Kieran\n>>>>>>>>>>> _______________________________________________\n>>>>>>>>>>> libcamera-devel mailing list\n>>>>>>>>>>> libcamera-devel@lists.libcamera.org\n>>>>>>>>>>> https://lists.libcamera.org/listinfo/libcamera-devel\n>>>>>>>>>\n>>>>>>>>> --\n>>>>>>>>> Regards\n>>>>>>>>> --\n>>>>>>>>> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 159CEBE08A\n\tfor ;\n\tThu, 26 Nov 2020 10:57:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 997AD63460;\n\tThu, 26 Nov 2020 11:57:18 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 27705633EF\n\tfor ;\n\tThu, 26 Nov 2020 11:57:17 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9592CA1B;\n\tThu, 26 Nov 2020 11:57:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VrVqTGdU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606388236;\n\tbh=vjJQ9h2gXpJZ/9G0rgcR+9GCwamrGwxHIhA4C9w90RY=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=VrVqTGdUJh4nm9hHKQmoM4DdBCKMT9Hfg9C8sl3mgqzzz0/BKV7fgD9SrruNHm3Ir\n\t7IrEpbdK4WgrMqi6MkWK2njECVEp0NRWjF/6PaKJsyAnB8eo3DXxEgbImCelfY0LtZ\n\tAaiYG3rlNgF/TLtVn1r8Wv8GxisHeDJZMKaGWK4Q=","To":"Jacopo Mondi ,\n\tDavid Plowman ","References":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>\n\t\n\t<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","From":"Kieran Bingham ","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":"<53668e33-095b-6010-d856-5aaeb31f7a5e@ideasonboard.com>","Date":"Thu, 26 Nov 2020 10:57:14 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13908,"web_url":"https://patchwork.libcamera.org/comment/13908/","msgid":"","date":"2020-11-26T11:07:39","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Jacopo\n\nYes, I'll submit a v2 with the text you had earlier, and I'll add an\npatch where the Raspberry Pi IPAs fill it in.\n\nThanks!\nDavid\n\nOn Thu, 26 Nov 2020 at 10:25, Jacopo Mondi wrote:\n>\n> Hi David,\n>\n> On Thu, Nov 26, 2020 at 09:50:05AM +0000, David Plowman wrote:\n> > Hi Jacopo\n> >\n> > Thanks for the update on this.\n> >\n> > On Wed, 25 Nov 2020 at 21:02, Jacopo Mondi wrote:\n> > >\n> > > Hi David,\n> > > I tried proposing a new version based on the outcome of our\n> > > discussion\n> > >\n> > > Please see below\n> > >\n> > > On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> > > > Hi Jacopo\n> > > >\n> > > > Thanks for the reply. More discussion below...\n> > > >\n> > > > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > > > >\n> > > > > Hi David,\n> > > > >\n> > > > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > > > > Hi Jacopo\n> > > > > >\n> > > > > > You're right, there's a relationship there. ColourGains obviously\n> > > > > > gives you the red and blue gains determined by the AWB usually. You\n> > > > > > might get the values 1.6 and 2.0 (for red and blue)\n> > > > > >\n> > > > > > In our case, if we report a single \"global gain\" value you can kind of\n> > > > > > imagine it as the green gain, where the colour gains were normalised\n> > > > > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > > > > 1.25, 2.5).\n> > > > >\n> > > > > That's a really helpful explanation for me, thanks\n> > > > >\n> > > > > >\n> > > > > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > > > > directly. For me this duplicates information that's already in the\n> > > > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > > > > pipeline that lets you set a global gain - you'd have to query the\n> > > > > > current white balance and work out all three numbers and set them. But\n> > > > > > then you've set the white balance as well. Or maybe we do something\n> > > > > > special so that you haven't? You see why it confuses me! So on balance\n> > > > > > I'm with the single value approach, though I could live either way.\n> > > > >\n> > > > > I see. With the above explanation I really see why a single global\n> > > > > value is probably enough.\n> > > > >\n> > > > > My question is now: are the above notions (blue/red gains normalized\n> > > > > on a green value of 1, how global gain is used to obtain per-channel\n> > > > > gains etc) standard knowledge I'm lacking, or:\n> > > > >\n> > > > > 1) It's worth to describe them in the control documentation as they're\n> > > > > \"standard\" and won't change per-pipeline\n> > > > > 2) They might differe per-pipeline and they need to be\n> > > > > described in the pipeline model documentation ?\n> > > > > 3) It is assumed the reader knows she's dealing with\n> > > >\n> > > > Difficult. I can't really see too many other ways of defining it, but\n> > > > you know, pipelines all have their nuances. You probably can't\n> > > > guarantee that what I described is completely \"standard\".\n> > > >\n> > > > A slightly more qualitative definition seems OK to me - the amount of\n> > > > linear gain applied by the pipeline to all pixels (in addition to the\n> > > > colour gains). Particular pipelines might feel they need to document\n> > > > it more carefully if there are any complications in how they deal with\n> > > > it.\n> > > >\n> > > > Best regards\n> > > > David\n> > > >\n> > > > >\n> > > > > Thanks\n> > > > > j\n> > > > >\n> > > > > >\n> > > > > > Thanks!\n> > > > > > David\n> > > > > >\n> > > > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > > > >\n> > > > > > > Hi David,\n> > > > > > >\n> > > > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > > > > Hi everyone\n> > > > > > > >\n> > > > > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > > > > be better as\n> > > > > > > >\n> > > > > > > > \"This control is present in a request's ControlList only if the\n> > > > > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > > > > application, the pipeline may still report the actual value used in\n> > > > > > > > the metadata returned with completed requests.\"\n> > > > > > >\n> > > > > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > > > > handler does not support changing the digital gain it should not\n> > > > > > > expose it a one of the Camera's controls.\n> > > > > > >\n> > > > > > > Likewise, if it is something that applications should be informed of,\n> > > > > > > it will be reported via metadata.\n> > > > > > >\n> > > > > > > I think we're good to go, except for the point that we've left\n> > > > > > > floating about having this a single value or a per-channel value.\n> > > > > > >\n> > > > > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > > > > see in example you have two per-channel values for the ColourGains\n> > > > > > > control. Is this anyway related ?\n> > > > > > >\n> > > > > > > >\n> > > > > > > > Any other thoughts?\n> > > > > > > >\n> > > > > > > > Thanks\n> > > > > > > > David\n> > > > > > > >\n> > > > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham\n> > > > > > > > wrote:\n> > > > > > > > >\n> > > > > > > > > Hi Jacopo,\n> > > > > > > > >\n> > > > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > > > > Hi Kieran,\n> > > > > > > > > >\n> > > > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > > > > >> Hi David,\n> > > > > > > > > >>\n> > > > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > > > > >>>\n> > > > > > > > > >>> Signed-off-by: David Plowman \n> > > > > > > > > >>> ---\n> > > > > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > > > > >>>\n> > > > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > > > > >>> +\n> > > > > > > > > >>> + - DigitalGain:\n> > > > > > > > > >>> + type: float\n> > > > > > > > > >>> + description: |\n> > > > > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > > > > >>> +\n> > > > > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > > > > >>> + processing pipeline.\n> > >\n> > > - DigitalGain:\n> > > type: float\n> > > description: |\n> > > Digital gain value applied during the processing steps applied\n> > > to the image as captured from the sensor.\n> > >\n> > > The global digital gain factor is applied to all the colour channels\n> > > of the RAW image. Different pipeline models are free to\n> > > specify how the global gain factor applies to each separate\n> > > channel.\n> > >\n> > > If an imaging pipeline applies digital gain in distinct\n> > > processing steps, this value indicates their total sum.\n> > > Pipelines are free to decide how to adjust each processing\n> > > step to respect the received gain factor and shall report\n> > > their total value in the request metadata.\n> > >\n> > > However this deferring to each pipeline model documentation makes me\n> > > wonder if expressing per-channel values would make things more\n> > > explicit...\n> > >\n> > > Feel free to take in what you consider appropriate.\n> >\n> > Yes, this is all so difficult, we just need to make a decision,\n> > really. Per-channel values work for me, but only because we're not\n> > going to let people set them. As a read-only thing, it seems a bit\n> > easier to understand/explain. We could ignore the question of setting\n> > a global digital gain until someone wants to do it (maybe no one ever\n> > will...).\n> >\n> > What do you think?\n>\n> I think that once we have a pipeline that supports per-channel\n> digital gains we will introduce 'DigitalGains' as an array control,\n> deprectate this one, and keep supporting it in pipelines that exposed\n> it (through an internal helper maybe) not to break applications that\n> made use of it.\n>\n> It's hard for me to have a firm idea without a real use case, but one\n> thing that presses me is not to mention this control is read only in\n> its definition. If that's the case for RPi just report it in metadata\n> only, but let's not restrict other platforms.\n>\n> Do you think in the next iteration you will include reporting this\n> control from the pipeline too ? if I'm not mistaken this version\n> contains the control definition only.\n>\n> Thanks\n> j\n>\n>\n> >\n> > Thanks!\n> > David\n> >\n> > >\n> > > > > > > > > >>\n> > > > > > > > > >>\n> > > > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > > > > >> style value.\n> > > > > > > > > >>\n> > > > > > > > > >>\n> > > > > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > > > > >> will be a core issue anyway.\n> > > > > > > > > >>\n> > > > > > > > > >>\n> > > > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > > > > >> determine that someone needs to set it.\n> > > > > > > > > >>\n> > > > > > > > > >> We could introduce a control property between type: and description:\n> > > > > > > > > >> read-only: true\n> > > > > > > > > >\n> > > > > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > > > > >\n> > > > > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > > > > request's metadata ?\n> > > > > > > > >\n> > > > > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > > > > >\n> > > > > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > > > > implicit.\n> > > > > > > > >\n> > > > > > > > > --\n> > > > > > > > > Kieran\n> > > > > > > > >\n> > > > > > > > >\n> > > > > > > > > >\n> > > > > > > > > > Thanks\n> > > > > > > > > > j\n> > > > > > > > > >\n> > > > > > > > > >>\n> > > > > > > > > >>\n> > > > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > > > > >> that brings it's own questions )\n> > > > > > > > > >>\n> > > > > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > > > > >>\n> > > > > > > > > >>\n> > > > > > > > > >>> ...\n> > > > > > > > > >>>\n> > > > > > > > > >>\n> > > > > > > > > >> --\n> > > > > > > > > >> Regards\n> > > > > > > > > >> --\n> > > > > > > > > >> Kieran\n> > > > > > > > > >> _______________________________________________\n> > > > > > > > > >> libcamera-devel mailing list\n> > > > > > > > > >> libcamera-devel@lists.libcamera.org\n> > > > > > > > > >> https://lists.libcamera.org/listinfo/libcamera-devel\n> > > > > > > > >\n> > > > > > > > > --\n> > > > > > > > > Regards\n> > > > > > > > > --\n> > > > > > > > > Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 645FBBE176\n\tfor ;\n\tThu, 26 Nov 2020 11:07:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DFDCE6345F;\n\tThu, 26 Nov 2020 12:07:53 +0100 (CET)","from mail-oi1-x243.google.com (mail-oi1-x243.google.com\n\t[IPv6:2607:f8b0:4864:20::243])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9483B63458\n\tfor ;\n\tThu, 26 Nov 2020 12:07:52 +0100 (CET)","by mail-oi1-x243.google.com with SMTP id k26so1919766oiw.0\n\tfor ;\n\tThu, 26 Nov 2020 03:07:52 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"cKJ+a8tF\"; 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=Eo/pWSPDVTzvvVRuxcQWeLxNuUkjkdntmlzQYrzqmqg=;\n\tb=cKJ+a8tFN4BHAwyI9g1z2gQjwvDY8biJmKpyUHduLAJMt8nAdmaL1UNaqmiiVUhxeo\n\toMIf/FJCt2LNxZDrDeLXcPV39qEACNTIGQJ+b5vo8vbozLE69V+3HHFBhLVg/sW74unM\n\tiIoVfegBWViKALM16XWIpw+Ik/nnHWiVsS+goEhpv0NmjKtq6ywFnBxrEhYKVx+kwfPP\n\tAA90wYP7rqVBIH02UEiySoUJaet9s21WnvwqyWTtWTTpehRT2pT+bZrt4ag7skh3DntK\n\trOnzq4KudI7fe6EHTQFWOtMaQb+74ZGR9xz3FfQHSbYc6CWEXMnpccoY94NWnxmL4hqg\n\tLJzg==","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=Eo/pWSPDVTzvvVRuxcQWeLxNuUkjkdntmlzQYrzqmqg=;\n\tb=f22u8gpLEI8BqW24l8kajFWxtOe1WfyobyKLztRgiWa7KuFkqaiTOo8bB5DbkUtdlY\n\tlyoule4u8RCo1mp6dB5+dGzAzzTDWPxG41ozJn2gVdGxbz6zZh7Ky0Km6OaoYRlEKach\n\t/EioZcXvXEqoBrdjEVivZgBYEe/SSOWSG98R9boEM+cl6qdslZymN/duQBl2TFU0JfD5\n\tuIhAYvU9xa6ccdHn5ZVeXHnHxY95/yU1I84tx0Zc76TLvVCb/lOFuJ6ay1u+DF+MjjSB\n\t/nSWhkL/FkrJpQuRJZs7VxrWQjhHcnmhrcpbctHR6MGGFNVnbM1j45PCrTsEjr5DKUJd\n\tsUng==","X-Gm-Message-State":"AOAM532EVA0Dus0X7YsGOHlA41LK/twZB5l15SP198JzGvp6egRrzgUK\n\tH/LF0lWuciQd8B4RpOr6vgXPoYKilhxxhRHXtljdr8n8WDrXSw==","X-Google-Smtp-Source":"ABdhPJzyb1KWbsvzRrxWuhsFGOl7hNbblVJz/kp3kleCFxI4Ybm+NRy4r4guEY3Kg78cmsuTKQNjMDlNeaG8cdoxeVg=","X-Received":"by 2002:aca:3c3:: with SMTP id 186mr1721155oid.22.1606388871393; \n\tThu, 26 Nov 2020 03:07:51 -0800 (PST)","MIME-Version":"1.0","References":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201125210300.7mcl3st3ojcb2iqx@uno.localdomain>\n\t\n\t<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","In-Reply-To":"<20201126102522.d2w5mb5cuhtdfc2w@uno.localdomain>","From":"David Plowman ","Date":"Thu, 26 Nov 2020 11:07:39 +0000","Message-ID":"","To":"Jacopo Mondi ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13909,"web_url":"https://patchwork.libcamera.org/comment/13909/","msgid":"<20201126112550.GE3905@pendragon.ideasonboard.com>","date":"2020-11-26T11:25:50","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Nov 26, 2020 at 10:52:43AM +0000, Kieran Bingham wrote:\n> On 26/11/2020 10:49, Laurent Pinchart wrote:\n> > On Mon, Nov 23, 2020 at 09:17:33AM +0000, Kieran Bingham wrote:\n> >> On 23/11/2020 08:58, Jacopo Mondi wrote:\n> >>> On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> >>>> On 27/10/2020 14:12, David Plowman wrote:\n> >>>>> This control reports the global digital gain applied by the pipeline\n> >>>>> as a whole, after capturing a raw image from the sensor.\n> >>>>>\n> >>>>> Signed-off-by: David Plowman \n> >>>>> ---\n> >>>>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> >>>>> 1 file changed, 11 insertions(+)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> >>>>> index c8874fa9..e6362c74 100644\n> >>>>> --- a/src/libcamera/control_ids.yaml\n> >>>>> +++ b/src/libcamera/control_ids.yaml\n> >>>>> @@ -530,4 +530,15 @@ controls:\n> >>>>> This control is only present when the pipeline supports scaling. Its\n> >>>>> maximum valid value is given by the properties::ScalerCropMaximum\n> >>>>> property, and the two can be used to implement digital zoom.\n> >>>>> +\n> >>>>> + - DigitalGain:\n> >>>>> + type: float\n> >>>>> + description: |\n> >>>>> + Global digital gain value applied to the image during all the\n> >>>>> + processing steps after capturing the image from the sensor. Any raw\n> >>>>> + images, therefore, will not include this gain, but the final images\n> >>>>> + output by the imaging pipeline as a whole will include it.\n> >>>>> +\n> >>>>> + This control is intended to report the value used by the image\n> >>>>> + processing pipeline.\n> >>>>\n> >>>> If this is a per-stream thing anyway, I guess it will then be up to\n> >>>> pipeline handlers to set this to the appropriate value for each stream\n> >>>> when it completes. The fact that this value would not be applicable to a\n> >>>> RAW stream makes me think it certainly should be a per-stream metadata\n> >>>> style value.\n> >>>>\n> >>>> I'd hope this could be handled by a common helper in that instance so it\n> >>>> doesn't get left out of some pipeline handlers, but included in some,\n> >>>> and become inconsistent. Not yet sure how we can handle that, but that\n> >>>> will be a core issue anyway.\n> >>>>\n> >>>> I wonder if we should mark this somehow as read-only, at least until we\n> >>>> determine that someone needs to set it.\n> >>>>\n> >>>> We could introduce a control property between type: and description:\n> >>>> read-only: true\n> >>>\n> >>> Isn't a read-only control just a metadata ?\n> >>>\n> >>> Wouldn't it be enough for a pipeline that does not support changing\n> >>> the control value from applications not reporting it in the list of\n> >>> supported Camera's controls, but only report it as part of a completed\n> >>> request's metadata ?\n> >>\n> >> Ah, yes of course - because if the control is not listed as supported it\n> >> won't be there to set in the first place! I forgot about that.\n> >>\n> >> So - indeed, no requirement to mark anything as read-only. That will be\n> >> implicit.\n> > \n> > There could still be value in specifying this in the yaml file, to\n> > document which controls are meant to be read-only. Instead of writing\n> > \"This control is read-only\" or \"This control is meant to be used in\n> > metadata only\", a read-only property (I'm sure we'll bikeshed the name,\n> > maybe \"type: metadata\" would be better) would allow compile-time and\n> > runtime sanity checks. That's not a prerequisite for this patch though.\n> \n> I don't actually think this has to be a read-only control though.\n> \n> I can imagine this being an enabled control that will allow increasing\n> the gain of an image (without affecting colour balance).\n> \n> To me - this is the same as AnalogueGain but applied at the ISP rather\n> than the sensor right ?\n\nYes, this doesn't necessarily have to be read-only, my comment was about\ngeneral handling of read-only controls, not this specific one.\n\n> >>>> Otherwise, I see no objections currently. I think we're just waiting on\n> >>>> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> >>>> that brings it's own questions )\n> >>>>\n> >>>> Reviewed-by: Kieran Bingham \n> >>>>\n> >>>>> ...","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 62487BE08A\n\tfor ;\n\tThu, 26 Nov 2020 11:26:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D803163459;\n\tThu, 26 Nov 2020 12:26:00 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A169263408\n\tfor ;\n\tThu, 26 Nov 2020 12:25:59 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72E50A1B;\n\tThu, 26 Nov 2020 12:25:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GsK4qMZm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606389958;\n\tbh=pOrhGztGwFUXImNakiq7J6PGAf+L8/A8BBTIZkZvvjs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GsK4qMZmG9pFp1ovyAXLqW87b3EXX+8snCzHky8OBHxJGxy8sCy960wjvK1uyQM4a\n\tk2eWrjcw3qa5399CZv4XlRRQt8TDen07sBEgXwyGhmFkhaipTHZAu3hx7F/W3LoeBk\n\tLOkNPJixyqNkZfbPD+l0Np5g3ss/6fW68c/nc3jA=","Date":"Thu, 26 Nov 2020 13:25:50 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"<20201126112550.GE3905@pendragon.ideasonboard.com>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t<20201126104955.GD3905@pendragon.ideasonboard.com>\n\t<67accecd-9bba-3bbe-4ca6-b41b3ba35308@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<67accecd-9bba-3bbe-4ca6-b41b3ba35308@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13910,"web_url":"https://patchwork.libcamera.org/comment/13910/","msgid":"<20201126124200.GF3905@pendragon.ideasonboard.com>","date":"2020-11-26T12:42:00","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > Hi Jacopo\n> > >\n> > > You're right, there's a relationship there. ColourGains obviously\n> > > gives you the red and blue gains determined by the AWB usually. You\n> > > might get the values 1.6 and 2.0 (for red and blue)\n> > >\n> > > In our case, if we report a single \"global gain\" value you can kind of\n> > > imagine it as the green gain, where the colour gains were normalised\n> > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > 1.25, 2.5).\n> >\n> > That's a really helpful explanation for me, thanks\n> >\n> > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > directly. For me this duplicates information that's already in the\n> > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > pipeline that lets you set a global gain - you'd have to query the\n> > > current white balance and work out all three numbers and set them. But\n> > > then you've set the white balance as well. Or maybe we do something\n> > > special so that you haven't? You see why it confuses me! So on balance\n> > > I'm with the single value approach, though I could live either way.\n> >\n> > I see. With the above explanation I really see why a single global\n> > value is probably enough.\n> >\n> > My question is now: are the above notions (blue/red gains normalized\n> > on a green value of 1, how global gain is used to obtain per-channel\n> > gains etc) standard knowledge I'm lacking, or:\n> >\n> > 1) It's worth to describe them in the control documentation as they're\n> > \"standard\" and won't change per-pipeline\n> > 2) They might differe per-pipeline and they need to be\n> > described in the pipeline model documentation ?\n> > 3) It is assumed the reader knows she's dealing with\n> \n> Difficult. I can't really see too many other ways of defining it, but\n> you know, pipelines all have their nuances. You probably can't\n> guarantee that what I described is completely \"standard\".\n\nThis is really what I'd like to address :-)\n\nHave you seen the \"libcamera: camera: Document the camera and pipeline\nmodel\" patch (which is now in the master branch) ? It's the very first\nstep in standardizing a logical pipeline model that pipeline handlers\nneed to confirm to.\n\nIn a nutshell, the idea is that libcamera needs to standardize the\nbehaviour of cameras as seen from applications in order to make\napplications portable. To do so, we define a logical pipeline model that\ndescribes the processing operations performed by the camera, from the\npixel array to the captured image.\n\nHardware platforms differ in the feature they support and how they\nimplement them, so most operations in the pipeline model are optional.\nFurthermore, there's no requirement to have the hardware performing\nthose operations in the same order, or more generically in the same way,\nas the logical model mandates, as long as the visible effect from an\napplication point of view is the same. Pipeline handlers are responsible\nfor exposing the standard logical model and mapping it to device\nfeatures.\n\nI expect that not all devices will be able to expose the same pipeline\nmodel, so we will need to support multiple options, with a way for the\npipeline handlers to report which options apply to the cameras they\nhandle. This can be reported through properties or through other means.\nWe already support this, albeit implicitly: pipeline handlers that don't\nsupport some of the processing operations simply don't expose the\ncorresponding controls. I expect we will also need properties to report\nthe order of operations, when different orders lead to different results\nand all need to be supported. This should however remain the exception\nrather than the rule.\n\nI would like to expand the pipeline model documentation to include\ncolour processing, and I think digital gain really fits here. We can\nstill apply your patch (or the next version of it) before finalizing\ndocumentation of the colour processing in the pipeline model, but\ncontrols will then likely change soon after, which would require\nadapting your applications. It may end up causing more pain than gain.\n\nAs you have more experience than me in this area, I wonder if you could\nhelp reviewing an initial proposal ? I envision the following\nsensitivity and colour-related operations, in order.\n\n- Total gain in the sensor, made of analog and/or digital gain. This\n applies to all images, RAW and processed, and to all channels. If\n auto-exposure is enabled, the control value is ignored. The actual\n value is always reported in metadata.\n\n Questions:\n\n - Should we mandate that pipeline handlers and IPAs prioritize analog\n gain before applying digital gain ?\n\n - Can there be use cases for per-channel sensor-side gains ? In most\n cases I expect the white balance to be applied in the ISP, but will\n that always be the case ? What if auto-white-balance is disabled,\n and the user sets manual per-channel gains, could there be a reason\n to apply them in the sensor ?\n\n- Digital gain on the ISP side (a.k.a. \"post-RAW sensitivity boost\" in\n Android). This applies to processed images only, and to all channels.\n If auto-exposure is enabled, the control value is ignored. The actual\n value is always reported in metadata.\n\n Questions:\n\n - Should this be renamed ? \"Digital gain\" can be confused with the\n digital gain in the sensor. A name that captures the fact that the\n gain is applied on processed images only may be better (I don't like\n the Android name too much, but it handles this issue).\n\n- Per-channel gains. This is currently handled as red and blue gains,\n with the green gains being hardcoded to 1.0.\n\n Questions:\n\n - Should we change this ? I see two potential issues:\n\n - Not all systems may use a Bayer colour filter array, so other\n gains may be needed (I'm thinking about RGBW patterns for\n instance, or any other from\n https://en.wikipedia.org/wiki/Color_filter_array).\n\n - Hardcoding the green gains to 1.0 means that we will have an\n \"average\" gain different than 1.0 in most cases. This will\n effectively change the sensitivity of the camera, and should be\n taken into account to compute the ISO value. Applications could do\n so, but I wonder if we shouldn't mandate that the \"average\" gain\n remains 1.0, with the \"sensitivity boost\" being handled by the\n digital gain control only. How to compute the \"average\" gain is an\n interesting problem here, as I expect it will need to take a full\n colour model into account.\n\n- Demosaicing. This shouldn't apply any gain.\n\n- RGB-to-RGB colour transformation matrix.\n\n Questions:\n\n - Similarly to the per-channel gains, should we require that this\n matrix applies no gain (i.e. the sum of elements in a line should be\n 1.0) ?\n\n- RGB-to-YUV colour encoding matrix.\n\n Questions:\n\n - Similarly to the RGB-to-RGB matrix, should we forbid gains ?\n\nIs there any other colour processing operation I have missed ?\n\n> A slightly more qualitative definition seems OK to me - the amount of\n> linear gain applied by the pipeline to all pixels (in addition to the\n> colour gains). Particular pipelines might feel they need to document\n> it more carefully if there are any complications in how they deal with\n> it.\n>\n> > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > Hi everyone\n> > > > >\n> > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > be better as\n> > > > >\n> > > > > \"This control is present in a request's ControlList only if the\n> > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > application, the pipeline may still report the actual value used in\n> > > > > the metadata returned with completed requests.\"\n> > > >\n> > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > handler does not support changing the digital gain it should not\n> > > > expose it a one of the Camera's controls.\n> > > >\n> > > > Likewise, if it is something that applications should be informed of,\n> > > > it will be reported via metadata.\n> > > >\n> > > > I think we're good to go, except for the point that we've left\n> > > > floating about having this a single value or a per-channel value.\n> > > >\n> > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > see in example you have two per-channel values for the ColourGains\n> > > > control. Is this anyway related ?\n> > > >\n> > > > > Any other thoughts?\n> > > > >\n> > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham wrote:\n> > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > >>>\n> > > > > > >>> Signed-off-by: David Plowman \n> > > > > > >>> ---\n> > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > >>>\n> > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > >>> +\n> > > > > > >>> + - DigitalGain:\n> > > > > > >>> + type: float\n> > > > > > >>> + description: |\n> > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > >>> +\n> > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > >>> + processing pipeline.\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > >> style value.\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > >> will be a core issue anyway.\n> > > > > > >>\n> > > > > > >>\n> > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > >> determine that someone needs to set it.\n> > > > > > >>\n> > > > > > >> We could introduce a control property between type: and description:\n> > > > > > >> read-only: true\n> > > > > > >\n> > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > >\n> > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > request's metadata ?\n> > > > > >\n> > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > >\n> > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > implicit.\n> > > > > >\n> > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > >> that brings it's own questions )\n> > > > > > >>\n> > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > >>\n> > > > > > >>> ...","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 19388BE08A\n\tfor ;\n\tThu, 26 Nov 2020 12:42:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8409F6345B;\n\tThu, 26 Nov 2020 13:42:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DDD9A63408\n\tfor ;\n\tThu, 26 Nov 2020 13:42:09 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42152A1B;\n\tThu, 26 Nov 2020 13:42:09 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"RehU0K+V\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1606394529;\n\tbh=lKhCPx0kVaiXofNmFOrazF1ICYTFvkUGlWMZ19pBOq4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=RehU0K+VEhKyDw83i6btVauGy/fAjis9TthBjwfAYFfRbgTuomcd4VlmSym4YLxf3\n\t21SOFT2ZcGy+4BdDglmB438K3wlGHZyemq6hw0Uy/4OkBPhdoaT1o3DJLV2rjwHr+c\n\tumYw+cCt18AU4vdIv4QGTbqvv420DrOpRbCkV5/o=","Date":"Thu, 26 Nov 2020 14:42:00 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"<20201126124200.GF3905@pendragon.ideasonboard.com>","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13940,"web_url":"https://patchwork.libcamera.org/comment/13940/","msgid":"","date":"2020-11-26T19:04:13","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Laurent\n\nThanks for sending me this discussion, I agree it's an important\ntopic. Perhaps I can add a few rambling thoughts here and there, and\nalso try to answer your questions. Obviously my answers are rather\ninfluenced by the Broadcom pipeline, and also the logical Raspberry Pi\nview that we put on top of it!\n\nOn Thu, 26 Nov 2020 at 12:42, Laurent Pinchart\n wrote:\n>\n> Hi David,\n>\n> On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > > Hi Jacopo\n> > > >\n> > > > You're right, there's a relationship there. ColourGains obviously\n> > > > gives you the red and blue gains determined by the AWB usually. You\n> > > > might get the values 1.6 and 2.0 (for red and blue)\n> > > >\n> > > > In our case, if we report a single \"global gain\" value you can kind of\n> > > > imagine it as the green gain, where the colour gains were normalised\n> > > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > > 1.25, 2.5).\n> > >\n> > > That's a really helpful explanation for me, thanks\n> > >\n> > > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > > directly. For me this duplicates information that's already in the\n> > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > > pipeline that lets you set a global gain - you'd have to query the\n> > > > current white balance and work out all three numbers and set them. But\n> > > > then you've set the white balance as well. Or maybe we do something\n> > > > special so that you haven't? You see why it confuses me! So on balance\n> > > > I'm with the single value approach, though I could live either way.\n> > >\n> > > I see. With the above explanation I really see why a single global\n> > > value is probably enough.\n> > >\n> > > My question is now: are the above notions (blue/red gains normalized\n> > > on a green value of 1, how global gain is used to obtain per-channel\n> > > gains etc) standard knowledge I'm lacking, or:\n> > >\n> > > 1) It's worth to describe them in the control documentation as they're\n> > > \"standard\" and won't change per-pipeline\n> > > 2) They might differe per-pipeline and they need to be\n> > > described in the pipeline model documentation ?\n> > > 3) It is assumed the reader knows she's dealing with\n> >\n> > Difficult. I can't really see too many other ways of defining it, but\n> > you know, pipelines all have their nuances. You probably can't\n> > guarantee that what I described is completely \"standard\".\n>\n> This is really what I'd like to address :-)\n>\n> Have you seen the \"libcamera: camera: Document the camera and pipeline\n> model\" patch (which is now in the master branch) ? It's the very first\n> step in standardizing a logical pipeline model that pipeline handlers\n> need to confirm to.\n>\n> In a nutshell, the idea is that libcamera needs to standardize the\n> behaviour of cameras as seen from applications in order to make\n> applications portable. To do so, we define a logical pipeline model that\n> describes the processing operations performed by the camera, from the\n> pixel array to the captured image.\n>\n> Hardware platforms differ in the feature they support and how they\n> implement them, so most operations in the pipeline model are optional.\n> Furthermore, there's no requirement to have the hardware performing\n> those operations in the same order, or more generically in the same way,\n> as the logical model mandates, as long as the visible effect from an\n> application point of view is the same. Pipeline handlers are responsible\n> for exposing the standard logical model and mapping it to device\n> features.\n>\n> I expect that not all devices will be able to expose the same pipeline\n> model, so we will need to support multiple options, with a way for the\n> pipeline handlers to report which options apply to the cameras they\n> handle. This can be reported through properties or through other means.\n> We already support this, albeit implicitly: pipeline handlers that don't\n> support some of the processing operations simply don't expose the\n> corresponding controls. I expect we will also need properties to report\n> the order of operations, when different orders lead to different results\n> and all need to be supported. This should however remain the exception\n> rather than the rule.\n>\n> I would like to expand the pipeline model documentation to include\n> colour processing, and I think digital gain really fits here. We can\n> still apply your patch (or the next version of it) before finalizing\n> documentation of the colour processing in the pipeline model, but\n> controls will then likely change soon after, which would require\n> adapting your applications. It may end up causing more pain than gain.\n>\n> As you have more experience than me in this area, I wonder if you could\n> help reviewing an initial proposal ? I envision the following\n> sensitivity and colour-related operations, in order.\n>\n> - Total gain in the sensor, made of analog and/or digital gain. This\n> applies to all images, RAW and processed, and to all channels. If\n> auto-exposure is enabled, the control value is ignored. The actual\n> value is always reported in metadata.\n\nI agree that we obviously need to know the analogue gain applied by\nthe sensor. Optionally there might be digital gain in the sensor too.\nIn many respects I'd quite like to see controls like this:\n\nSensorGain - this is the combined gain applied by the sensor, analogue\nand digital (if any).\nSensorDigitalGain - this is optional and can be used if the sensor\napplies any digital gain.\n\nThe reason for this is that applications can simply look for the\nSensorGain and ignore anything else. Mostly they won't care if the\ngain was analogue or digital. I'd certainly not want application code\nto be forced to go checking for SensorDigitalGain when it won't\nusually be there. Even when a sensor has applied digital gain, I think\nthat reporting the SensorDigitalGain information, while helpful,\nshould be optional. (To be honest I've never seen sensor digital gain\nused.)\n\nI would just add that, when AGC is enabled, setting the gain still\nmakes sense. You still need the AGC (well, AEC/AGC) to control the\nshutter speed to give you those fixed ISO captures. Similarly, there's\nno reason not to let an application fix the shutter speed and let the\nAGC vary the gain.\n\n>\n> Questions:\n>\n> - Should we mandate that pipeline handlers and IPAs prioritize analog\n> gain before applying digital gain ?\n\nI can't really see any reason to mandate this. It would be normal, I\nagree, and it would probably be helpful to see it reported in the\nmetadata if digital gain has been used, but I don't really see a need\nto force any particular behaviour.\n\n>\n> - Can there be use cases for per-channel sensor-side gains ? In most\n> cases I expect the white balance to be applied in the ISP, but will\n> that always be the case ? What if auto-white-balance is disabled,\n> and the user sets manual per-channel gains, could there be a reason\n> to apply them in the sensor ?\n\nAgain, I've never seen sensor-side per channel gains myself, though\nyou'd be brave to assert that no one will ever use them. I don't see\nAWB being disabled as a reason to use them. If you want to fix the\ncolour gains, there's no reason not to do it in the ISP.\n\nHowever, it seems reasonable to me to leave it up to the pipeline\nhandler and its IPAs. If someone wants to set fixed colour gains\n(effectively stop the AWB), and the pipeline handler wants to do it by\nsetting them in the sensor, good luck to them!\n\n>\n> - Digital gain on the ISP side (a.k.a. \"post-RAW sensitivity boost\" in\n> Android). This applies to processed images only, and to all channels.\n> If auto-exposure is enabled, the control value is ignored. The actual\n> value is always reported in metadata.\n>\n> Questions:\n>\n> - Should this be renamed ? \"Digital gain\" can be confused with the\n> digital gain in the sensor. A name that captures the fact that the\n> gain is applied on processed images only may be better (I don't like\n> the Android name too much, but it handles this issue).\n\n\"Post-RAW sensitivity boost\" sounds execrable. Just my opinion,\nthough! I'd be happy with DigitalGain (on its own) being the gain in\nthe ISP, and SensorDigitalGain (if present) being the digital gain in\nthe sensor, but I know opinions will vary! IspDigitalGain? IspGain?\nPipelineDigitalGain? PipelineGain?\n\n>\n> - Per-channel gains. This is currently handled as red and blue gains,\n> with the green gains being hardcoded to 1.0.\n>\n> Questions:\n>\n> - Should we change this ? I see two potential issues:\n>\n> - Not all systems may use a Bayer colour filter array, so other\n> gains may be needed (I'm thinking about RGBW patterns for\n> instance, or any other from\n> https://en.wikipedia.org/wiki/Color_filter_array).\n\nSo I think red and blue gains alone are OK, even with something like\nan RGBW filter. Finding white balance is a problem of finding a single\n2D point in a plane, giving you (for example) red and blue gains. With\nan RGBW filter, I'd expect the AWB calibration process would tell you\nwhat to do with the W pixels once you've been told where in this space\nyou are.\n\nI'm assuming other primaries (e.g. CMYK) or multi-spectral images are\nout of scope!\n\n>\n> - Hardcoding the green gains to 1.0 means that we will have an\n> \"average\" gain different than 1.0 in most cases. This will\n> effectively change the sensitivity of the camera, and should be\n> taken into account to compute the ISO value. Applications could do\n> so, but I wonder if we shouldn't mandate that the \"average\" gain\n> remains 1.0, with the \"sensitivity boost\" being handled by the\n> digital gain control only. How to compute the \"average\" gain is an\n> interesting problem here, as I expect it will need to take a full\n> colour model into account.\n\nMy experience has been as follows. The sensor manufacturer will tell\nyou that an image with analogue gain of 1 corresponds to an ISO of\n(for example) 40. The OEM will then want you to white balance this\nimage properly and then for it to come out of the system with an ISO\nof 40. Yet the RGB colour gains you have applied might actually have\nbeen (1.5, 1.0, 2.0). So perhaps the green gain is really the thing\nthat corresponds to a \"global gain\", it's certainly the one with most\neffect.\n\nNote further that you can *never* apply any gains less than 1. If you\ndo, any saturated pixels will become unsaturated, so if you applied\nRGB gains like (0.8, 1.0, 1.6) all your bright whites would go cyan.\nThe upshot is that you have to apply (1.0, 1.25, 2.0) instead (scale\neverything up by 1 / 0.8 = 1.25). So in this case, what is the \"global\ngain\"? Is it 1.25? Does that mean your ISO (which you might have\nthought was 40) is now 1.25*40 = 50? Maybe. For me, this is probably\nOK if you don't let people set the global gain.\n\nBuf if you do, I think there are further problems. What happens if\nyour user says \"I don't want any global gain, so I'll set it to 1\". Do\nyou let the whites go cyan? Or force it to be 1.25 instead so that all\nvalues between 1 and 1.25 have no effect? Or maybe you always include\nan extra gain of 1 / 0.8 (in this case), so that we can still call the\ngood values (1.0, 1.25, 2.0) a \"global gain of 1\"? Tricky questions.\n\nI end up in a place, therefore, where I feel you kind of have to let\npipelines do what they want. I think we can still maintain the\nfollowing:\n\n* Handling the two colour gains (normalised to a green gain of 1) seems fine.\n* You should be able to query and set the colour gains (effectively,\nthe white balance).\n* You should ideally be able to query the \"global gain\". It gives you\nsome idea of the \"effective gain\" (in some vague sense) applied to all\npixels. You can use it to scale your ISO estimates, for example.\n* If a pipeline wants to let you set the global gain, there's a bit of\na question as to what it really means!\n\n>\n> - Demosaicing. This shouldn't apply any gain.\n\nAgree.\n\n>\n> - RGB-to-RGB colour transformation matrix.\n>\n> Questions:\n>\n> - Similarly to the per-channel gains, should we require that this\n> matrix applies no gain (i.e. the sum of elements in a line should be\n> 1.0) ?\n\nAgree. Even if a pipeline actually mixes its gains and colour matrix\ntogether, there's no reason not to present them as separate things\nexternally.\n\n>\n> - RGB-to-YUV colour encoding matrix.\n>\n> Questions:\n>\n> - Similarly to the RGB-to-RGB matrix, should we forbid gains ?\n\nI think so.\n\n>\n> Is there any other colour processing operation I have missed ?\n\nSome pipelines have relatively sophisticated lookup tables for\nfine-tuning colours, but I don't think those concern anything we've\ndiscussed here.\n\nI think that's everything. Sorry if it's a bit long in places!\n\nDavid\n\n>\n> > A slightly more qualitative definition seems OK to me - the amount of\n> > linear gain applied by the pipeline to all pixels (in addition to the\n> > colour gains). Particular pipelines might feel they need to document\n> > it more carefully if there are any complications in how they deal with\n> > it.\n> >\n> > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > > Hi everyone\n> > > > > >\n> > > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > > be better as\n> > > > > >\n> > > > > > \"This control is present in a request's ControlList only if the\n> > > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > > application, the pipeline may still report the actual value used in\n> > > > > > the metadata returned with completed requests.\"\n> > > > >\n> > > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > > handler does not support changing the digital gain it should not\n> > > > > expose it a one of the Camera's controls.\n> > > > >\n> > > > > Likewise, if it is something that applications should be informed of,\n> > > > > it will be reported via metadata.\n> > > > >\n> > > > > I think we're good to go, except for the point that we've left\n> > > > > floating about having this a single value or a per-channel value.\n> > > > >\n> > > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > > see in example you have two per-channel values for the ColourGains\n> > > > > control. Is this anyway related ?\n> > > > >\n> > > > > > Any other thoughts?\n> > > > > >\n> > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham wrote:\n> > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > > >>>\n> > > > > > > >>> Signed-off-by: David Plowman \n> > > > > > > >>> ---\n> > > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > > >>>\n> > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > > >>> +\n> > > > > > > >>> + - DigitalGain:\n> > > > > > > >>> + type: float\n> > > > > > > >>> + description: |\n> > > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > > >>> +\n> > > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > > >>> + processing pipeline.\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > > >> style value.\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > > >> will be a core issue anyway.\n> > > > > > > >>\n> > > > > > > >>\n> > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > > >> determine that someone needs to set it.\n> > > > > > > >>\n> > > > > > > >> We could introduce a control property between type: and description:\n> > > > > > > >> read-only: true\n> > > > > > > >\n> > > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > > >\n> > > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > > request's metadata ?\n> > > > > > >\n> > > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > > >\n> > > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > > implicit.\n> > > > > > >\n> > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > > >> that brings it's own questions )\n> > > > > > > >>\n> > > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > > >>\n> > > > > > > >>> ...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 4996DBE176\n\tfor ;\n\tThu, 26 Nov 2020 19:04:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id CA43D63466;\n\tThu, 26 Nov 2020 20:04:27 +0100 (CET)","from mail-oi1-x242.google.com (mail-oi1-x242.google.com\n\t[IPv6:2607:f8b0:4864:20::242])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 88CAB633F6\n\tfor ;\n\tThu, 26 Nov 2020 20:04:26 +0100 (CET)","by mail-oi1-x242.google.com with SMTP id o25so3277010oie.5\n\tfor ;\n\tThu, 26 Nov 2020 11:04:26 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"UZpVFF9p\"; 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=NCFxKXrCflP1Kvf+9U5bCCmsn0KPVxc62l50F3/6Yis=;\n\tb=UZpVFF9pZNqogjK8yRzKGp2XvqjvfHGq7XZ4CAaPbG9UdNbge43FiwaoksKaivaO4s\n\tmWk2J6xJi5Pn8pzq2aa66ZbuQh0t9B778plT6Cmxo0nOSfhDkaKj3zXlxJ9qYmSq6ENo\n\tFv5QnDVFoqbicg0gLFrBcKXWe7A7+BrCy2T1NPWRsFmRzosZEpJHH2VQDBWKSQXROLpd\n\tTh1NCCCMY/4mS80nM+AwypgXC+WgQfMUjGKnILKnozpYdICBgYLjubu8/s/KBY6etOZQ\n\tTID+4sIqg5ansteRHWwPf14VHN1/AypXRzvxF5IIZScqm6B/3sjXlvmj9nuCK61ajpSa\n\tWvoA==","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=NCFxKXrCflP1Kvf+9U5bCCmsn0KPVxc62l50F3/6Yis=;\n\tb=RJJxJnDQ+eG2zb3xPTmdigWuvxDQUws+EI2oAIZOFJeuwguFwU79Fu08bPCd2qfZW5\n\t39nuv+kc7AQ8LGSkYyIvjXJQ1zaCmcpg+rkxiqtA9qjvtxjIQ0tYgiOpEblYmOdVv4hq\n\tOx130CbUp24X4EPMkWJCfWT7UdfoaGK/ENifFDi1kycunbL/9hb638/aS0+bxofUBlwb\n\tzHirIdgJLVkx/a4u2ygBfk/3l+WPLIe3hi5ZpttIBjXd6pRXhVOg8OJx1Rhx0JNm0xWF\n\tdqPoU/LGhD2fwAXUjQGK59LvuOhPsZO9mond7WESlDhOCI8J25f8/TR0WdRHYqHZ5mmV\n\tG/Fg==","X-Gm-Message-State":"AOAM531sfFQMtjMyBRjEMP0bBVgRBr3WMXgY6UH7RD+8YbEbxT7UXT3Y\n\toi8GPHfG/ypDT3DUnq+Ka5PtWMtuPid1STvfCrtU5Q==","X-Google-Smtp-Source":"ABdhPJwYAdoVfXjzy4xe0JAlga8Z8qD0iqbfTwRrlPr5WbTYJERlu+E0hWH4UNOpVCdIjWS6D6jubcaUGxfzFhj/0k4=","X-Received":"by 2002:aca:5114:: with SMTP id\n\tf20mr2869382oib.107.1606417464987; \n\tThu, 26 Nov 2020 11:04:24 -0800 (PST)","MIME-Version":"1.0","References":"<20201027141246.4708-1-david.plowman@raspberrypi.com>\n\t<20201027141246.4708-2-david.plowman@raspberrypi.com>\n\t<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201126124200.GF3905@pendragon.ideasonboard.com>","In-Reply-To":"<20201126124200.GF3905@pendragon.ideasonboard.com>","From":"David Plowman ","Date":"Thu, 26 Nov 2020 19:04:13 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14287,"web_url":"https://patchwork.libcamera.org/comment/14287/","msgid":"","date":"2020-12-20T01:37:29","subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi David,\n\nOn Thu, Nov 26, 2020 at 07:04:13PM +0000, David Plowman wrote:\n> Hi Laurent\n> \n> Thanks for sending me this discussion, I agree it's an important\n> topic. Perhaps I can add a few rambling thoughts here and there, and\n> also try to answer your questions. Obviously my answers are rather\n> influenced by the Broadcom pipeline, and also the logical Raspberry Pi\n> view that we put on top of it!\n\nThat's the added value, providing a view point from a real use case :-)\n\n> On Thu, 26 Nov 2020 at 12:42, Laurent Pinchart wrote:\n> > On Tue, Nov 24, 2020 at 12:40:38PM +0000, David Plowman wrote:\n> > > On Tue, 24 Nov 2020 at 11:28, Jacopo Mondi wrote:\n> > > > On Tue, Nov 24, 2020 at 10:15:25AM +0000, David Plowman wrote:\n> > > > > Hi Jacopo\n> > > > >\n> > > > > You're right, there's a relationship there. ColourGains obviously\n> > > > > gives you the red and blue gains determined by the AWB usually. You\n> > > > > might get the values 1.6 and 2.0 (for red and blue)\n> > > > >\n> > > > > In our case, if we report a single \"global gain\" value you can kind of\n> > > > > imagine it as the green gain, where the colour gains were normalised\n> > > > > for a green gain of 1. So if the global gain was 1.25, then the actual\n> > > > > RGB gains used in my example would be 1.25 * (1.6, 1.0, 2.0) = (2.0,\n> > > > > 1.25, 2.5).\n> > > >\n> > > > That's a really helpful explanation for me, thanks\n> > > >\n> > > > > In the per-channel case I guess you'd be reporting these 3 numbers\n> > > > > directly. For me this duplicates information that's already in the\n> > > > > ColourGains, and it seems to muddle things up a bit. Imagine you had a\n> > > > > pipeline that lets you set a global gain - you'd have to query the\n> > > > > current white balance and work out all three numbers and set them. But\n> > > > > then you've set the white balance as well. Or maybe we do something\n> > > > > special so that you haven't? You see why it confuses me! So on balance\n> > > > > I'm with the single value approach, though I could live either way.\n> > > >\n> > > > I see. With the above explanation I really see why a single global\n> > > > value is probably enough.\n> > > >\n> > > > My question is now: are the above notions (blue/red gains normalized\n> > > > on a green value of 1, how global gain is used to obtain per-channel\n> > > > gains etc) standard knowledge I'm lacking, or:\n> > > >\n> > > > 1) It's worth to describe them in the control documentation as they're\n> > > > \"standard\" and won't change per-pipeline\n> > > > 2) They might differe per-pipeline and they need to be\n> > > > described in the pipeline model documentation ?\n> > > > 3) It is assumed the reader knows she's dealing with\n> > >\n> > > Difficult. I can't really see too many other ways of defining it, but\n> > > you know, pipelines all have their nuances. You probably can't\n> > > guarantee that what I described is completely \"standard\".\n> >\n> > This is really what I'd like to address :-)\n> >\n> > Have you seen the \"libcamera: camera: Document the camera and pipeline\n> > model\" patch (which is now in the master branch) ? It's the very first\n> > step in standardizing a logical pipeline model that pipeline handlers\n> > need to confirm to.\n> >\n> > In a nutshell, the idea is that libcamera needs to standardize the\n> > behaviour of cameras as seen from applications in order to make\n> > applications portable. To do so, we define a logical pipeline model that\n> > describes the processing operations performed by the camera, from the\n> > pixel array to the captured image.\n> >\n> > Hardware platforms differ in the feature they support and how they\n> > implement them, so most operations in the pipeline model are optional.\n> > Furthermore, there's no requirement to have the hardware performing\n> > those operations in the same order, or more generically in the same way,\n> > as the logical model mandates, as long as the visible effect from an\n> > application point of view is the same. Pipeline handlers are responsible\n> > for exposing the standard logical model and mapping it to device\n> > features.\n> >\n> > I expect that not all devices will be able to expose the same pipeline\n> > model, so we will need to support multiple options, with a way for the\n> > pipeline handlers to report which options apply to the cameras they\n> > handle. This can be reported through properties or through other means.\n> > We already support this, albeit implicitly: pipeline handlers that don't\n> > support some of the processing operations simply don't expose the\n> > corresponding controls. I expect we will also need properties to report\n> > the order of operations, when different orders lead to different results\n> > and all need to be supported. This should however remain the exception\n> > rather than the rule.\n> >\n> > I would like to expand the pipeline model documentation to include\n> > colour processing, and I think digital gain really fits here. We can\n> > still apply your patch (or the next version of it) before finalizing\n> > documentation of the colour processing in the pipeline model, but\n> > controls will then likely change soon after, which would require\n> > adapting your applications. It may end up causing more pain than gain.\n> >\n> > As you have more experience than me in this area, I wonder if you could\n> > help reviewing an initial proposal ? I envision the following\n> > sensitivity and colour-related operations, in order.\n> >\n> > - Total gain in the sensor, made of analog and/or digital gain. This\n> > applies to all images, RAW and processed, and to all channels. If\n> > auto-exposure is enabled, the control value is ignored. The actual\n> > value is always reported in metadata.\n> \n> I agree that we obviously need to know the analogue gain applied by\n> the sensor. Optionally there might be digital gain in the sensor too.\n> In many respects I'd quite like to see controls like this:\n> \n> SensorGain - this is the combined gain applied by the sensor, analogue\n> and digital (if any).\n> SensorDigitalGain - this is optional and can be used if the sensor\n> applies any digital gain.\n> \n> The reason for this is that applications can simply look for the\n> SensorGain and ignore anything else. Mostly they won't care if the\n> gain was analogue or digital. I'd certainly not want application code\n> to be forced to go checking for SensorDigitalGain when it won't\n> usually be there. Even when a sensor has applied digital gain, I think\n> that reporting the SensorDigitalGain information, while helpful,\n> should be optional. (To be honest I've never seen sensor digital gain\n> used.)\n\nSounds like a good idea to me.\n\n> I would just add that, when AGC is enabled, setting the gain still\n> makes sense. You still need the AGC (well, AEC/AGC) to control the\n> shutter speed to give you those fixed ISO captures. Similarly, there's\n> no reason not to let an application fix the shutter speed and let the\n> AGC vary the gain.\n\nFully agreed, semi-manual modes are important to support.\n\nOn a side note, would you happen to have any recommendation for any good\nreading material covering the concept of ISO speed and exposure index ?\n\n> > Questions:\n> >\n> > - Should we mandate that pipeline handlers and IPAs prioritize analog\n> > gain before applying digital gain ?\n> \n> I can't really see any reason to mandate this. It would be normal, I\n> agree, and it would probably be helpful to see it reported in the\n> metadata if digital gain has been used, but I don't really see a need\n> to force any particular behaviour.\n\nThe reasons why I'm considering mandating a particular behaviour is to\ngive guidelines to both application developers and pipeline handler\ndevelopers. From a pipeline handler point of view, giving clear\nguidelines would help ensuring a more consistent behaviour across\ndifferent platforms. From an application point of view, a more\nconsistent behaviour means better portability. Obviously there's a\nbalance to be found here, the more we mandate a particular behaviour,\nthe higher the risk a platform won't be able to support it. It would\nalso restrict innovation to some extent.\n\nMaybe a good middleground would be to specify a recommended behaviour\nfor pipeline handlers, with an invitation to discuss any proposed\ndeparture from that behaviour ?\n\n> > - Can there be use cases for per-channel sensor-side gains ? In most\n> > cases I expect the white balance to be applied in the ISP, but will\n> > that always be the case ? What if auto-white-balance is disabled,\n> > and the user sets manual per-channel gains, could there be a reason\n> > to apply them in the sensor ?\n> \n> Again, I've never seen sensor-side per channel gains myself, though\n\nThey seem to be fairly common in camera sensors, but probably not\ncommonly used.\n\n> you'd be brave to assert that no one will ever use them. I don't see\n> AWB being disabled as a reason to use them. If you want to fix the\n> colour gains, there's no reason not to do it in the ISP.\n\nColour gains in the ISP (especially when the ISP operates in memory to\nmemory mode) are much easier to synchronize, so that's what I would also\nuse. When AWB is disabled, synchronizing gains to a particular frame is\nless of an issue, so that's why I thought setting them on the sensor\nwould be less of a problem in that case. But thinking about it, the user\ncould change the gains over time... And one could use staggered writes\nto handle colour gains in the sensor the same way as the analog gain and\nexposure time.\n\n> However, it seems reasonable to me to leave it up to the pipeline\n> handler and its IPAs. If someone wants to set fixed colour gains\n> (effectively stop the AWB), and the pipeline handler wants to do it by\n> setting them in the sensor, good luck to them!\n\n:-)\n\n> >\n> > - Digital gain on the ISP side (a.k.a. \"post-RAW sensitivity boost\" in\n> > Android). This applies to processed images only, and to all channels.\n> > If auto-exposure is enabled, the control value is ignored. The actual\n> > value is always reported in metadata.\n> >\n> > Questions:\n> >\n> > - Should this be renamed ? \"Digital gain\" can be confused with the\n> > digital gain in the sensor. A name that captures the fact that the\n> > gain is applied on processed images only may be better (I don't like\n> > the Android name too much, but it handles this issue).\n> \n> \"Post-RAW sensitivity boost\" sounds execrable. Just my opinion,\n\nI fully agree, I don't like the name either.\n\n> though! I'd be happy with DigitalGain (on its own) being the gain in\n> the ISP, and SensorDigitalGain (if present) being the digital gain in\n> the sensor, but I know opinions will vary! IspDigitalGain? IspGain?\n> PipelineDigitalGain? PipelineGain?\n\nI'll add naming to my todo list, to be handled as part of a big rename\nexecise when we'll stabilize the API. I think it's safe to say we won't\ngo for the Android name :-)\n\n> > - Per-channel gains. This is currently handled as red and blue gains,\n> > with the green gains being hardcoded to 1.0.\n> >\n> > Questions:\n> >\n> > - Should we change this ? I see two potential issues:\n> >\n> > - Not all systems may use a Bayer colour filter array, so other\n> > gains may be needed (I'm thinking about RGBW patterns for\n> > instance, or any other from\n> > https://en.wikipedia.org/wiki/Color_filter_array).\n> \n> So I think red and blue gains alone are OK, even with something like\n> an RGBW filter. Finding white balance is a problem of finding a single\n> 2D point in a plane, giving you (for example) red and blue gains. With\n> an RGBW filter, I'd expect the AWB calibration process would tell you\n> what to do with the W pixels once you've been told where in this space\n> you are.\n> \n> I'm assuming other primaries (e.g. CMYK) or multi-spectral images are\n> out of scope!\n\nFor now :-)\n\n> > - Hardcoding the green gains to 1.0 means that we will have an\n> > \"average\" gain different than 1.0 in most cases. This will\n> > effectively change the sensitivity of the camera, and should be\n> > taken into account to compute the ISO value. Applications could do\n> > so, but I wonder if we shouldn't mandate that the \"average\" gain\n> > remains 1.0, with the \"sensitivity boost\" being handled by the\n> > digital gain control only. How to compute the \"average\" gain is an\n> > interesting problem here, as I expect it will need to take a full\n> > colour model into account.\n> \n> My experience has been as follows. The sensor manufacturer will tell\n> you that an image with analogue gain of 1 corresponds to an ISO of\n> (for example) 40. The OEM will then want you to white balance this\n> image properly and then for it to come out of the system with an ISO\n> of 40. Yet the RGB colour gains you have applied might actually have\n> been (1.5, 1.0, 2.0). So perhaps the green gain is really the thing\n> that corresponds to a \"global gain\", it's certainly the one with most\n> effect.\n> \n> Note further that you can *never* apply any gains less than 1. If you\n> do, any saturated pixels will become unsaturated, so if you applied\n> RGB gains like (0.8, 1.0, 1.6) all your bright whites would go cyan.\n> The upshot is that you have to apply (1.0, 1.25, 2.0) instead (scale\n> everything up by 1 / 0.8 = 1.25). So in this case, what is the \"global\n> gain\"? Is it 1.25? Does that mean your ISO (which you might have\n> thought was 40) is now 1.25*40 = 50? Maybe. For me, this is probably\n> OK if you don't let people set the global gain.\n> \n> Buf if you do, I think there are further problems. What happens if\n> your user says \"I don't want any global gain, so I'll set it to 1\". Do\n> you let the whites go cyan? Or force it to be 1.25 instead so that all\n> values between 1 and 1.25 have no effect? Or maybe you always include\n> an extra gain of 1 / 0.8 (in this case), so that we can still call the\n> good values (1.0, 1.25, 2.0) a \"global gain of 1\"? Tricky questions.\n> \n> I end up in a place, therefore, where I feel you kind of have to let\n> pipelines do what they want. I think we can still maintain the\n> following:\n> \n> * Handling the two colour gains (normalised to a green gain of 1) seems fine.\n> * You should be able to query and set the colour gains (effectively,\n> the white balance).\n> * You should ideally be able to query the \"global gain\". It gives you\n> some idea of the \"effective gain\" (in some vague sense) applied to all\n> pixels. You can use it to scale your ISO estimates, for example.\n> * If a pipeline wants to let you set the global gain, there's a bit of\n> a question as to what it really means!\n\nThanks for your input. This is a bit tricky, especially the \"does it\nmean the ISO is 50\" question. This will probably require more thoughts.\n\n> > - Demosaicing. This shouldn't apply any gain.\n> \n> Agree.\n> \n> > - RGB-to-RGB colour transformation matrix.\n> >\n> > Questions:\n> >\n> > - Similarly to the per-channel gains, should we require that this\n> > matrix applies no gain (i.e. the sum of elements in a line should be\n> > 1.0) ?\n> \n> Agree. Even if a pipeline actually mixes its gains and colour matrix\n> together, there's no reason not to present them as separate things\n> externally.\n\nExactly. I think the pipeline model should preclude gains from being\napplied in the RGB-to-RGB matrix, but mapping gains from the pipeline\nmodel to the hardware is the responsibility of the pipeline handler.\n\n> > - RGB-to-YUV colour encoding matrix.\n> >\n> > Questions:\n> >\n> > - Similarly to the RGB-to-RGB matrix, should we forbid gains ?\n> \n> I think so.\n> \n> > Is there any other colour processing operation I have missed ?\n> \n> Some pipelines have relatively sophisticated lookup tables for\n> fine-tuning colours, but I don't think those concern anything we've\n> discussed here.\n\nYes, beside per-component gamma tables, there are also cubic lookup\ntables. In theory you could apply any type of gain there, but I don't\nthink they should be included in the gains discussion.\n\n> I think that's everything. Sorry if it's a bit long in places!\n\nNot at all! It's all great input, than you very much.\n\n> > > A slightly more qualitative definition seems OK to me - the amount of\n> > > linear gain applied by the pipeline to all pixels (in addition to the\n> > > colour gains). Particular pipelines might feel they need to document\n> > > it more carefully if there are any complications in how they deal with\n> > > it.\n> > >\n> > > > > On Tue, 24 Nov 2020 at 08:43, Jacopo Mondi wrote:\n> > > > > > On Tue, Nov 24, 2020 at 08:28:09AM +0000, David Plowman wrote:\n> > > > > > > Hi everyone\n> > > > > > >\n> > > > > > > Sounds like we're happy enough from the point of view of this thing\n> > > > > > > being read-only (for Raspberry Pi at least). Would anyone want any\n> > > > > > > changes to the wording? Perhaps the final sentence/paragraph might now\n> > > > > > > be better as\n> > > > > > >\n> > > > > > > \"This control is present in a request's ControlList only if the\n> > > > > > > pipeline supports setting the value. Even when it cannot be set by an\n> > > > > > > application, the pipeline may still report the actual value used in\n> > > > > > > the metadata returned with completed requests.\"\n> > > > > >\n> > > > > > I don't think it it necessary. It is implied that if a pipeline\n> > > > > > handler does not support changing the digital gain it should not\n> > > > > > expose it a one of the Camera's controls.\n> > > > > >\n> > > > > > Likewise, if it is something that applications should be informed of,\n> > > > > > it will be reported via metadata.\n> > > > > >\n> > > > > > I think we're good to go, except for the point that we've left\n> > > > > > floating about having this a single value or a per-channel value.\n> > > > > >\n> > > > > > I'm trying to get a feeling how this would be reported by your ISP. I\n> > > > > > see in example you have two per-channel values for the ColourGains\n> > > > > > control. Is this anyway related ?\n> > > > > >\n> > > > > > > Any other thoughts?\n> > > > > > >\n> > > > > > > On Mon, 23 Nov 2020 at 09:17, Kieran Bingham wrote:\n> > > > > > > > On 23/11/2020 08:58, Jacopo Mondi wrote:\n> > > > > > > > > On Mon, Nov 16, 2020 at 10:40:25AM +0000, Kieran Bingham wrote:\n> > > > > > > > >> On 27/10/2020 14:12, David Plowman wrote:\n> > > > > > > > >>> This control reports the global digital gain applied by the pipeline\n> > > > > > > > >>> as a whole, after capturing a raw image from the sensor.\n> > > > > > > > >>>\n> > > > > > > > >>> Signed-off-by: David Plowman \n> > > > > > > > >>> ---\n> > > > > > > > >>> src/libcamera/control_ids.yaml | 11 +++++++++++\n> > > > > > > > >>> 1 file changed, 11 insertions(+)\n> > > > > > > > >>>\n> > > > > > > > >>> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > > > > > > > >>> index c8874fa9..e6362c74 100644\n> > > > > > > > >>> --- a/src/libcamera/control_ids.yaml\n> > > > > > > > >>> +++ b/src/libcamera/control_ids.yaml\n> > > > > > > > >>> @@ -530,4 +530,15 @@ controls:\n> > > > > > > > >>> This control is only present when the pipeline supports scaling. Its\n> > > > > > > > >>> maximum valid value is given by the properties::ScalerCropMaximum\n> > > > > > > > >>> property, and the two can be used to implement digital zoom.\n> > > > > > > > >>> +\n> > > > > > > > >>> + - DigitalGain:\n> > > > > > > > >>> + type: float\n> > > > > > > > >>> + description: |\n> > > > > > > > >>> + Global digital gain value applied to the image during all the\n> > > > > > > > >>> + processing steps after capturing the image from the sensor. Any raw\n> > > > > > > > >>> + images, therefore, will not include this gain, but the final images\n> > > > > > > > >>> + output by the imaging pipeline as a whole will include it.\n> > > > > > > > >>> +\n> > > > > > > > >>> + This control is intended to report the value used by the image\n> > > > > > > > >>> + processing pipeline.\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> If this is a per-stream thing anyway, I guess it will then be up to\n> > > > > > > > >> pipeline handlers to set this to the appropriate value for each stream\n> > > > > > > > >> when it completes. The fact that this value would not be applicable to a\n> > > > > > > > >> RAW stream makes me think it certainly should be a per-stream metadata\n> > > > > > > > >> style value.\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> I'd hope this could be handled by a common helper in that instance so it\n> > > > > > > > >> doesn't get left out of some pipeline handlers, but included in some,\n> > > > > > > > >> and become inconsistent. Not yet sure how we can handle that, but that\n> > > > > > > > >> will be a core issue anyway.\n> > > > > > > > >>\n> > > > > > > > >>\n> > > > > > > > >> I wonder if we should mark this somehow as read-only, at least until we\n> > > > > > > > >> determine that someone needs to set it.\n> > > > > > > > >>\n> > > > > > > > >> We could introduce a control property between type: and description:\n> > > > > > > > >> read-only: true\n> > > > > > > > >\n> > > > > > > > > Isn't a read-only control just a metadata ?\n> > > > > > > > >\n> > > > > > > > > Wouldn't it be enough for a pipeline that does not support changing\n> > > > > > > > > the control value from applications not reporting it in the list of\n> > > > > > > > > supported Camera's controls, but only report it as part of a completed\n> > > > > > > > > request's metadata ?\n> > > > > > > >\n> > > > > > > > Ah, yes of course - because if the control is not listed as supported it\n> > > > > > > > won't be there to set in the first place! I forgot about that.\n> > > > > > > >\n> > > > > > > > So - indeed, no requirement to mark anything as read-only. That will be\n> > > > > > > > implicit.\n> > > > > > > >\n> > > > > > > > >> Otherwise, I see no objections currently. I think we're just waiting on\n> > > > > > > > >> top-level thoughts from Laurent. (And perhaps per-stream controls, but\n> > > > > > > > >> that brings it's own questions )\n> > > > > > > > >>\n> > > > > > > > >> Reviewed-by: Kieran Bingham \n> > > > > > > > >>\n> > > > > > > > >>> ...","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D533C0F1A\n\tfor ;\n\tSun, 20 Dec 2020 01:37:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1A22C61597;\n\tSun, 20 Dec 2020 02:37:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31B8360526\n\tfor ;\n\tSun, 20 Dec 2020 02:37:38 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 688AB593;\n\tSun, 20 Dec 2020 02:37:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tCkb2PbZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1608428257;\n\tbh=3vX/fVeGlcxC0V8p1ycU96CNLgfdl2MsaXZEgUAfT+I=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tCkb2PbZGNl3mxsSa+2b9Gn3LDnQhQvRGTP+BFKibEIXL06X/ejLYUR530+ka9kNV\n\tVUG/54aZ+ORAD6G4JRCDS5jVjedRyVo8hDaKdZ+B47RyOpH8a8yN/YZfXo+uPggGRS\n\tOq28c2pYkwFTKdshu0FVI7atGfH1kQuWtRne8Jl0=","Date":"Sun, 20 Dec 2020 03:37:29 +0200","From":"Laurent Pinchart ","To":"David Plowman ","Message-ID":"","References":"<117f0c97-0fd2-5fd6-3ebf-c0acafc0cf68@ideasonboard.com>\n\t<20201123085835.7eqltr3j4hrfrb6i@uno.localdomain>\n\t<4e783c69-476d-2caa-4a8c-2db3ef92d4ad@ideasonboard.com>\n\t\n\t<20201124084342.m5tbelc7gnmcwzlv@uno.localdomain>\n\t\n\t<20201124112808.mwlnk554z2a2esn4@uno.localdomain>\n\t\n\t<20201126124200.GF3905@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] libcamera: controls: Add\n\tDigitalGain control","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","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]