[{"id":3795,"web_url":"https://patchwork.libcamera.org/comment/3795/","msgid":"<20200218005913.GM4830@pendragon.ideasonboard.com>","date":"2020-02-18T00:59:13","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: controls: Add AE/AWB\n\tmode, manual and EV controls.","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Mon, Feb 17, 2020 at 02:26:09PM +0000, Naushir Patuck wrote:\n> AeMode is a new std::string type control used to set the AE algorithm\n> mode. The mode may not always be supported by all platforms.\n> \n> AwbMode is a new std::string type control used to set the AWB algorithm\n> illuminant mode. The mode may not always be supported by all platforms.\n> \n> EV is a new double type control used to set the log2 exposure adjustment\n> for the AE algorithm.\n> \n> ManualGainR is a new double type control used to set the Red channel\n> gain in manual AWB mode.\n> \n> ManualGainB is a new double type control used to set the Blue channel\n> gain in manual AWB mode.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++\n> 1 file changed, 40 insertions(+)\n> \n> diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> index 33062d6..21d2065 100644\n> --- a/src/libcamera/control_ids.yaml\n> +++ b/src/libcamera/control_ids.yaml\n> @@ -50,4 +50,44 @@ controls:\n> type: double\n> description: Specify a fixed gain parameter.\n> \n> + - AeMode:\n> + type: std::string\n> + description: |\n> + Specify an exposure mode for the AE algorithm to use. The exposure modes\n> + supported are platform spcific, and not all modes may be supported.\n\ns/spcific/specific/\n\n> +\n> + Examples include \"normal\", \"sport\", \"low light\", etc.\n> +\n> + - AwbMode:\n> + type: std::string\n> + description: |\n> + Specify the range of illuminant to use for the AWB algorihtm. The modes\n> + supported are platform specific, and not all modes may be supported.\n> +\n> + Examples include \"auto\", \"incandescent\", \"daylight\", etc.\n\nI think we should use enumerated values instead of strings for both\ncontrols. I do agree that strings have the nice benefit of being easily\nextensible, but they will be much more difficult to use for\napplications. A typical end-user camera application will display icons\nfor the different modes, and will thus need to map strings to icons. In\norder to allow doing so, we would need to specify what mode strings are\nsupported in this spec, which will remove the extensibility feature of\nstrings. Strings would then have no advantage over numerical values\nanymore.\n\nI do however agree that an extension mechanism can be useful, but to\ndesign it properly, I think we need to discuss how such extensions would\nbe used by applications.\n\n> +\n> + - EV:\n\nWould it make sense to spell it out to ExposureValue ?\n\n> + type: double\n> + description: |\n> + Specify an Exposure Value (EV) parameter.\n> +\n> + By convention, EV adjusts the exposure by a factor of 2^EV. For example\n> + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n\nYou should also explain here how this interacts with the AeEnable\ncontrol. One option would be to mention that the control only applies\nwhen AE is enabled, and that if AE is disabled, the EV value being set\nwill take effect next time AE is enabled. I'm open to discussing\nalternative proposals.\n\nI would also like to know what values you envision being supported for\nthis control. Would an IPA typically support a continuous range, or\ndiscrete values ?\n\n> + - ManualGainR:\n> + type: double\n> + description: |\n> + Specify a fixed gain parameter for the Red colour channel. This must be\n> + set together with ManualGainB to be applied.\n> +\n> + \\sa ManualGainB\n> +\n> + - ManualGainB:\n> + type: double\n> + description: |\n> + Specify a fixed gain parameter for the Green colour channel. This must\n> + bet together with AwbManualGainR to be applied.\n> +\n> + \\sa ManualGainR\n\nHow do those two controls interact with ManualGain ? And where in the\npipeline do they apply ?\n\n> ...","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1A86661A44\n\tfor ;\n\tTue, 18 Feb 2020 01:59:33 +0100 (CET)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6FFEC1220;\n\tTue, 18 Feb 2020 01:59:32 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1581987572;\n\tbh=Kv4eBJ6f6F8RrvAjeyUs/T+u+nGx6WnUAeWEUfzVDKM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ZCjK7lA/ttFcw8pTE5R0nJQcUkTX23hirAUSHLPKtHUA2uqBssNznXKYT4jUWFbbz\n\tnEbyfvPs7CAuI0dsNprqep4kXox9ozfQlPYzfZk5Huc1DpTn5i2Y/6GIoiOFuejHAZ\n\tziM0zwjbqNVZmQPDtrAtVrpPcYnHGb5dnvuiNJV4=","Date":"Tue, 18 Feb 2020 02:59:13 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org,\n\tNaushir Patuck ","Message-ID":"<20200218005913.GM4830@pendragon.ideasonboard.com>","References":"<20200217142609.22837-1-naush@raspberrypi.com>\n\t<20200217142609.22837-4-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20200217142609.22837-4-naush@raspberrypi.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: controls: Add AE/AWB\n\tmode, manual and EV controls.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Feb 2020 00:59:33 -0000"}},{"id":3802,"web_url":"https://patchwork.libcamera.org/comment/3802/","msgid":"","date":"2020-02-18T09:32:33","subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: controls: Add AE/AWB\n\tmode, manual and EV controls.","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nOn Tue, 18 Feb 2020 at 00:59, Laurent Pinchart\n wrote:\n>\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Mon, Feb 17, 2020 at 02:26:09PM +0000, Naushir Patuck wrote:\n> > AeMode is a new std::string type control used to set the AE algorithm\n> > mode. The mode may not always be supported by all platforms.\n> >\n> > AwbMode is a new std::string type control used to set the AWB algorithm\n> > illuminant mode. The mode may not always be supported by all platforms.\n> >\n> > EV is a new double type control used to set the log2 exposure adjustment\n> > for the AE algorithm.\n> >\n> > ManualGainR is a new double type control used to set the Red channel\n> > gain in manual AWB mode.\n> >\n> > ManualGainB is a new double type control used to set the Blue channel\n> > gain in manual AWB mode.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > src/libcamera/control_ids.yaml | 40 ++++++++++++++++++++++++++++++++++\n> > 1 file changed, 40 insertions(+)\n> >\n> > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids.yaml\n> > index 33062d6..21d2065 100644\n> > --- a/src/libcamera/control_ids.yaml\n> > +++ b/src/libcamera/control_ids.yaml\n> > @@ -50,4 +50,44 @@ controls:\n> > type: double\n> > description: Specify a fixed gain parameter.\n> >\n> > + - AeMode:\n> > + type: std::string\n> > + description: |\n> > + Specify an exposure mode for the AE algorithm to use. The exposure modes\n> > + supported are platform spcific, and not all modes may be supported.\n>\n> s/spcific/specific/\n>\n> > +\n> > + Examples include \"normal\", \"sport\", \"low light\", etc.\n> > +\n> > + - AwbMode:\n> > + type: std::string\n> > + description: |\n> > + Specify the range of illuminant to use for the AWB algorihtm. The modes\n> > + supported are platform specific, and not all modes may be supported.\n> > +\n> > + Examples include \"auto\", \"incandescent\", \"daylight\", etc.\n>\n> I think we should use enumerated values instead of strings for both\n> controls. I do agree that strings have the nice benefit of being easily\n> extensible, but they will be much more difficult to use for\n> applications. A typical end-user camera application will display icons\n> for the different modes, and will thus need to map strings to icons. In\n> order to allow doing so, we would need to specify what mode strings are\n> supported in this spec, which will remove the extensibility feature of\n> strings. Strings would then have no advantage over numerical values\n> anymore.\n>\n> I do however agree that an extension mechanism can be useful, but to\n> design it properly, I think we need to discuss how such extensions would\n> be used by applications.\n\nYes, this is a tricky one, and the choice here will not please\neveryone. I will switch to using enums in the next version of the\npatchset. One strong argument for using strings is to allow users\n(who may not be technically inclined) to add new modes to the tuning\nand be able to use them without having the need to every recompile any\ncode. To that end, what if I add, for example, {Custom1 Custom2\nCustom3} enum values to the AE Mode? This will allow the user to set\nthe mode in the tuning configuration, and select it in the application\nwithout needing to ever recompile. What do you think? On a related\nnote, do you expect any mechanism where the IPAs advertise which enums\nvalues are supported - this will be entirely platform/sensor/tuning\ndependent.\n\n>\n> > +\n> > + - EV:\n>\n> Would it make sense to spell it out to ExposureValue ?\n>\n\nYes, that sounds better.\n\n> > + type: double\n> > + description: |\n> > + Specify an Exposure Value (EV) parameter.\n> > +\n> > + By convention, EV adjusts the exposure by a factor of 2^EV. For example\n> > + EV = [-2, -1, 0.5, 0, 0.5, 1, 2] results in an exposure adjustment\n> > + of [1/4x, 1/2x, 1/sqrt(2)x, 1x, sqrt(2)x, 2x, 4x].\n>\n> You should also explain here how this interacts with the AeEnable\n> control. One option would be to mention that the control only applies\n> when AE is enabled, and that if AE is disabled, the EV value being set\n> will take effect next time AE is enabled. I'm open to discussing\n> alternative proposals.\n>\n\nI think having EV applied only when AE is enabled makes sense - and\nindeed, this is what our platform will do. I will update the\ndescription.\n\n> I would also like to know what values you envision being supported for\n> this control. Would an IPA typically support a continuous range, or\n> discrete values ?\n>\n\nOur platform supports a continuous range, I expect others will do as\nwell. Normally I would expect discretizing the allowable values\nshould be handled by the camera application.\n\n> > + - ManualGainR:\n> > + type: double\n> > + description: |\n> > + Specify a fixed gain parameter for the Red colour channel. This must be\n> > + set together with ManualGainB to be applied.\n> > +\n> > + \\sa ManualGainB\n> > +\n> > + - ManualGainB:\n> > + type: double\n> > + description: |\n> > + Specify a fixed gain parameter for the Green colour channel. This must\n> > + bet together with AwbManualGainR to be applied.\n> > +\n> > + \\sa ManualGainR\n>\n> How do those two controls interact with ManualGain ? And where in the\n> pipeline do they apply ?\n>\n\nI took ManualGain to be the sensor analogue gain value (although, it\ncould equally be applied to sensor digital gain I suppose). This\npairs with the ManualExposure sensor ctrl. ManualGainR and\nManualGainB are specific to the white balance control when AWB is off\nand we provide these fixed gains to apply in the ISP pipeline. So\nthere is no link between ManualGain and ManualGainR/ManualGainB. Does\nthis seem reasonable?\n\n> > ...\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n\nRegards,\nNaush","headers":{"Return-Path":"","Received":["from mail-lj1-x244.google.com (mail-lj1-x244.google.com\n\t[IPv6:2a00:1450:4864:20::244])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C11F061D61\n\tfor ;\n\tTue, 18 Feb 2020 10:32:49 +0100 (CET)","by mail-lj1-x244.google.com with SMTP id e18so22048291ljn.12\n\tfor ;\n\tTue, 18 Feb 2020 01:32:49 -0800 (PST)"],"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=13ItFj5Yd6rLs3iYTqgxhmmsvporDAGe3nKjg2zYLxU=;\n\tb=AIIIWzN8clBV+s23BKaCUAR9aLgv/PoSOMDculsNISroUr/1It86edXyJ2k50/q4OK\n\t4kfQw6RdCL91VO+9T+i84ocfuPRlgAaT2qrpi4mQ6vghwkjbs6EwdC973s86m4fJt3Dn\n\tW81zsImb4PZBp1iGWU4LdGZtzM5uGVgQPU6piImfOmSUiuwyZez6Usc5URx/uWhM2+b0\n\tWZdEMCRJKPN0RmX5jj4NH1Z905MkB3utelrBolzk8afxDpm7ft43XDOcyzyNUyRO9IEh\n\tZqNQsjlUyk8BABwzYbSCGK2Xb4Z0969pP0XyeHN7fvlbhxPFEDzmQdqxiFhcMWSrlJc4\n\tUspg==","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=13ItFj5Yd6rLs3iYTqgxhmmsvporDAGe3nKjg2zYLxU=;\n\tb=PIIsMe4pO8AkAQQSIKe4xeETzvUqtreFUeIY+j67+l0ea9qzfteHzwOXmi23ZzefM5\n\tpzPdXP576Nz3uGuN9PXFgs9RfXYzTkez04418ubw22iM8vzkhw+tFxVrEoNW8DPUyabi\n\tdZSxDtZwUQqNzvwVqeMQGAgHiEKBfFJKd63YH3AQgz95Ag0H0eafmixpkBqsM0/LHiOy\n\tQrVWOkp/MNY95PysxkcSqdwjdF1WM3nxOQmlUcUQD7z1PfzsrMuAK62Owyk/UAaw4rbe\n\toJjP706C0JRR6nsKnBtY3p/9gPRQePpCSNqDieMizq5+0oS5cUdp5H0s40XBSHiNyST3\n\tr1Zw==","X-Gm-Message-State":"APjAAAVu28DrBa7DHyYqG0FVuEunIVr7+FnFIOxtOT7nWKDzXaxhVWlE\n\tW1JH1FSifw0SGqBWeGySny0FbmJoClTNZngCfLJ8oA==","X-Google-Smtp-Source":"APXvYqxw7n3Sztw6e5iXoOP/ghXOcfzzmbpDNVr3bQiKv0Gne0rwm8aNIUmuHQBUHpkOJAp6XUndJPbqfqFjyuXeN2s=","X-Received":"by 2002:a2e:9b03:: with SMTP id u3mr12392584lji.87.1582018368901;\n\tTue, 18 Feb 2020 01:32:48 -0800 (PST)","MIME-Version":"1.0","References":"<20200217142609.22837-1-naush@raspberrypi.com>\n\t<20200217142609.22837-4-naush@raspberrypi.com>\n\t<20200218005913.GM4830@pendragon.ideasonboard.com>","In-Reply-To":"<20200218005913.GM4830@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Tue, 18 Feb 2020 09:32:33 +0000","Message-ID":"","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tNaushir Patuck ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 3/3] libcamera: controls: Add AE/AWB\n\tmode, manual and EV controls.","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Tue, 18 Feb 2020 09:32:49 -0000"}}]