[{"id":30912,"web_url":"https://patchwork.libcamera.org/comment/30912/","msgid":"","date":"2024-08-27T07:49:54","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> For manual control it is helpful to be able to specify a fixed colour\n> temperature. It also provides an easy way to apply the temperature\n> specific CCMs and colour gains that are contained in the tuning files.\n> \n> Document this and update the control dependencies.\n> \n> Signed-off-by: Stefan Klug \n\nReviewed-by: Paul Elder \n\n> ---\n> src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> 1 file changed, 20 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index cf40771d3896..04dcc4af67fc 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -252,9 +252,12 @@ controls:\n> - AwbEnable:\n> type: bool\n> description: |\n> - Enable or disable the AWB.\n> + Enable or disable the AWB. Disabling AWB stops updates to the\n> + ColourGains and to the ColourCorrectionMatrix.\n> \n> + \\sa ColourCorrectionMatrix\n> \\sa ColourGains\n> + \\sa ColourTemperature\n> \n> # AwbMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -309,13 +312,24 @@ controls:\n> disabled.\n> \n> \\sa AwbEnable\n> + \\sa ColourTemperature\n> size: [2]\n> \n> - ColourTemperature:\n> type: int32_t\n> - description: Report the current estimate of the colour temperature, in\n> - kelvin, for this frame. The ColourTemperature control can only be\n> - returned in metadata.\n> + description: |\n> + Report the current estimate of the colour temperature, in kelvin, for\n> + this frame. An implementation may also allow this control to be set when\n> + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> + specified at the same time, they take precedence.\n> +\n> + The metadata will only report measured colour temperature values when\n> + available, even if set manually.\n> +\n> + \\sa AwbEnable\n> + \\sa ColourCorrectionMatrix\n> + \\sa ColourGains\n> \n> - Saturation:\n> type: float\n> @@ -365,6 +379,8 @@ controls:\n> transformation. The 3x3 matrix is stored in conventional reading\n> order in an array of 9 floating point values.\n> \n> + \\sa AwbEnable\n> + \\sa ColourTemperature\n> size: [3,3]\n> \n> - ScalerCrop:\n> -- \n> 2.43.0\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 1975AC324C\n\tfor ;\n\tTue, 27 Aug 2024 07:50:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AE83363442;\n\tTue, 27 Aug 2024 09:50:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 806586343E\n\tfor ;\n\tTue, 27 Aug 2024 09:50:02 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:b8b8:7adf:d5d3:ce44])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D2B569FF;\n\tTue, 27 Aug 2024 09:48:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uETmG460\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724744935;\n\tbh=SpilJyd5kP9BMdvezOoKNHKGEut53778yL4wIm3mrOo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=uETmG460BaNTHA3wRrk5j8fxq37+yyhdnTO3AJzgQMxD1wRP3V9wqiXQ1DsZRWESO\n\tMT83joxFZ1YLerFtxQWpFNjOaJ1pfLaSPVQ0l+gBz39UFuOL9YwRgg9iiZGW/O4pp+\n\tXiPIDL1BJh+1rvfdF2aLj5/WCyBz8cVfdniGAWdU=","Date":"Tue, 27 Aug 2024 16:49:54 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20240813084451.44099-2-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30971,"web_url":"https://patchwork.libcamera.org/comment/30971/","msgid":"<20240829230119.GA25163@pendragon.ideasonboard.com>","date":"2024-08-29T23:01:19","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> For manual control it is helpful to be able to specify a fixed colour\n> temperature. It also provides an easy way to apply the temperature\n> specific CCMs and colour gains that are contained in the tuning files.\n> \n> Document this and update the control dependencies.\n> \n> Signed-off-by: Stefan Klug \n> ---\n> src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> 1 file changed, 20 insertions(+), 4 deletions(-)\n> \n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index cf40771d3896..04dcc4af67fc 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -252,9 +252,12 @@ controls:\n> - AwbEnable:\n> type: bool\n> description: |\n> - Enable or disable the AWB.\n> + Enable or disable the AWB. Disabling AWB stops updates to the\n> + ColourGains and to the ColourCorrectionMatrix.\n\nPlease split this in two paragraphs. The first paragraph is translated\nto a \\brief doxygen command, and should be a single sentence. Same\nbelow.\n\nI would like to clarify this a bit. Here's an attempt, is it what you\nmean ?\n\n\tEnable or disable the AWB.\n\n\tWhen AWB is enabled, the algorithm estimates the colour\ttemperature of\n\tthe scene, and computes colour gains and the colour correction matrix\n\tautomatically. The computed colour temperate, gains and correction\n\tmatrix are reported in metadata. The corresponding controls are ignored\n\tif set in a request.\n\n\tWhen AWB is disabled, the colour temperature, gains and correction\n\tmatrix are not updated automatically and can be set manually in\n\trequests.\n\n> \n> + \\sa ColourCorrectionMatrix\n> \\sa ColourGains\n> + \\sa ColourTemperature\n> \n> # AwbMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -309,13 +312,24 @@ controls:\n> disabled.\n> \n> \\sa AwbEnable\n> + \\sa ColourTemperature\n> size: [2]\n> \n> - ColourTemperature:\n> type: int32_t\n> - description: Report the current estimate of the colour temperature, in\n> - kelvin, for this frame. The ColourTemperature control can only be\n> - returned in metadata.\n> + description: |\n> + Report the current estimate of the colour temperature, in kelvin, for\n> + this frame. An implementation may also allow this control to be set when\n\ns/also //\n\n> + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> + specified at the same time, they take precedence.\n\nmaybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\nif an application sets ColourTemperature and ColourGains but not\nColourCorrectionMatrix, do you expect the implementation to apply the\nrequested ColourGains and set ColourCorrectionMatrix based on the\ntemperature ?\n\n> +\n> + The metadata will only report measured colour temperature values when\n> + available, even if set manually.\n\nI'm not sure to understand this.\n\n> +\n> + \\sa AwbEnable\n> + \\sa ColourCorrectionMatrix\n> + \\sa ColourGains\n> \n> - Saturation:\n> type: float\n> @@ -365,6 +379,8 @@ controls:\n> transformation. The 3x3 matrix is stored in conventional reading\n> order in an array of 9 floating point values.\n> \n> + \\sa AwbEnable\n> + \\sa ColourTemperature\n> size: [3,3]\n> \n> - ScalerCrop:","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 D00B0C324C\n\tfor ;\n\tThu, 29 Aug 2024 23:01:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C116F63458;\n\tFri, 30 Aug 2024 01:01:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C435C6002E\n\tfor ;\n\tFri, 30 Aug 2024 01:01:50 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C20D4524;\n\tFri, 30 Aug 2024 01:00:41 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Lt8gw5gl\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724972442;\n\tbh=YIBcA0/Z0HTXuJh2HsiiE7Ct5KHTgzAW/yvdi/13dwI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Lt8gw5glUG2WK81QsrNWUzR/VYtlQg+nLbwQCHGnWKGe0fJmULUBz9QZu0Vg6liZ7\n\tIv3CxNIEkXKvK2Z2cagRTkHPCNVki17QAG1CXGo0jaTrh77JtHroYCeJ9pWELLicgW\n\tGoeIwA/V0gkk6m5bb3pSWAQoRSBoTxUfyPr1ygVE=","Date":"Fri, 30 Aug 2024 02:01:19 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20240829230119.GA25163@pendragon.ideasonboard.com>","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240813084451.44099-2-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30977,"web_url":"https://patchwork.libcamera.org/comment/30977/","msgid":"","date":"2024-08-30T05:58:31","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nThank you for the review.\n\nOn Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> Thank you for the patch.\n> \n> On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > For manual control it is helpful to be able to specify a fixed colour\n> > temperature. It also provides an easy way to apply the temperature\n> > specific CCMs and colour gains that are contained in the tuning files.\n> > \n> > Document this and update the control dependencies.\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > \n> > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > index cf40771d3896..04dcc4af67fc 100644\n> > --- a/src/libcamera/control_ids_core.yaml\n> > +++ b/src/libcamera/control_ids_core.yaml\n> > @@ -252,9 +252,12 @@ controls:\n> > - AwbEnable:\n> > type: bool\n> > description: |\n> > - Enable or disable the AWB.\n> > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > + ColourGains and to the ColourCorrectionMatrix.\n> \n> Please split this in two paragraphs. The first paragraph is translated\n> to a \\brief doxygen command, and should be a single sentence. Same\n> below.\n> \n> I would like to clarify this a bit. Here's an attempt, is it what you\n> mean ?\n> \n> \tEnable or disable the AWB.\n> \n> \tWhen AWB is enabled, the algorithm estimates the colour\ttemperature of\n> \tthe scene, and computes colour gains and the colour correction matrix\n> \tautomatically. The computed colour temperate, gains and correction\n> \tmatrix are reported in metadata. The corresponding controls are ignored\n> \tif set in a request.\n> \n> \tWhen AWB is disabled, the colour temperature, gains and correction\n> \tmatrix are not updated automatically and can be set manually in\n> \trequests.\n\nLooks good, I'll apply that.\n\n> \n> > \n> > + \\sa ColourCorrectionMatrix\n> > \\sa ColourGains\n> > + \\sa ColourTemperature\n> > \n> > # AwbMode needs further attention:\n> > # - Auto-generate max enum value.\n> > @@ -309,13 +312,24 @@ controls:\n> > disabled.\n> > \n> > \\sa AwbEnable\n> > + \\sa ColourTemperature\n> > size: [2]\n> > \n> > - ColourTemperature:\n> > type: int32_t\n> > - description: Report the current estimate of the colour temperature, in\n> > - kelvin, for this frame. The ColourTemperature control can only be\n> > - returned in metadata.\n> > + description: |\n> > + Report the current estimate of the colour temperature, in kelvin, for\n> > + this frame. An implementation may also allow this control to be set when\n> \n> s/also //\n> \n> > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > + specified at the same time, they take precedence.\n> \n> maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> if an application sets ColourTemperature and ColourGains but not\n> ColourCorrectionMatrix, do you expect the implementation to apply the\n> requested ColourGains and set ColourCorrectionMatrix based on the\n> temperature ?\n\nThe latter is exactly what happens. The following table lists the\npossible options:\n\nCT -> CG(CT), CCM(CT)\nCT, CG -> CG, CCM(CT)\nCT, CCM -> CG(CT), CCM\nCT, CG, CCM -> CG, CCM\n\nDo you have a nice sentence for that?\n\n> \n> > +\n> > + The metadata will only report measured colour temperature values when\n> > + available, even if set manually.\n> \n> I'm not sure to understand this.\n\nThis is based on the comment from Kieran:\nhttps://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\nfor cases (RPi) where no temperature gets estimated. Only measurements\nare returned in metadata. Manually set temperature is not reflected in\nthe metadata. This has the nice side effect, that you can set\nAwbEnable=false, and still get temperature estimations in the metadata.\n\n> \n> > +\n> > + \\sa AwbEnable\n> > + \\sa ColourCorrectionMatrix\n> > + \\sa ColourGains\n> > \n> > - Saturation:\n> > type: float\n> > @@ -365,6 +379,8 @@ controls:\n> > transformation. The 3x3 matrix is stored in conventional reading\n> > order in an array of 9 floating point values.\n> > \n> > + \\sa AwbEnable\n> > + \\sa ColourTemperature\n> > size: [3,3]\n> > \n> > - ScalerCrop:\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 81F5BC323E\n\tfor ;\n\tFri, 30 Aug 2024 05:58:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2366B63458;\n\tFri, 30 Aug 2024 07:58:36 +0200 (CEST)","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 6557E61900\n\tfor ;\n\tFri, 30 Aug 2024 07:58:34 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e99d:2bef:156e:346a])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 42B8C227;\n\tFri, 30 Aug 2024 07:57:25 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"kp2r5uHs\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1724997445;\n\tbh=4ZUJ3TWaIrMlIvPOKG1SbVR0T+scf1mKjaV6+NrABY4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=kp2r5uHsYTksmdu6oOzGEi/Bz+d9kVBBWW5m5K5X5qQhRsWjOioP7enVBTRWZvrG3\n\thMlBDn8X1goVmk2Junbu5qnbHFNb/qb3+cigEW8Ve3suaxkc5IZ1ytp1BPDeYXulpi\n\t7i2Egcno/sXE6kBRXmC3WZY4DBKPIILo94gpgR/g=","Date":"Fri, 30 Aug 2024 07:58:31 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240829230119.GA25163@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30987,"web_url":"https://patchwork.libcamera.org/comment/30987/","msgid":"<20240830101555.GG25163@pendragon.ideasonboard.com>","date":"2024-08-30T10:15:55","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\n(CC'ing David)\n\nOn Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > For manual control it is helpful to be able to specify a fixed colour\n> > > temperature. It also provides an easy way to apply the temperature\n> > > specific CCMs and colour gains that are contained in the tuning files.\n> > > \n> > > Document this and update the control dependencies.\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > \n> > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > index cf40771d3896..04dcc4af67fc 100644\n> > > --- a/src/libcamera/control_ids_core.yaml\n> > > +++ b/src/libcamera/control_ids_core.yaml\n> > > @@ -252,9 +252,12 @@ controls:\n> > > - AwbEnable:\n> > > type: bool\n> > > description: |\n> > > - Enable or disable the AWB.\n> > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > + ColourGains and to the ColourCorrectionMatrix.\n> > \n> > Please split this in two paragraphs. The first paragraph is translated\n> > to a \\brief doxygen command, and should be a single sentence. Same\n> > below.\n> > \n> > I would like to clarify this a bit. Here's an attempt, is it what you\n> > mean ?\n> > \n> > \tEnable or disable the AWB.\n> > \n> > \tWhen AWB is enabled, the algorithm estimates the colour\ttemperature of\n> > \tthe scene, and computes colour gains and the colour correction matrix\n> > \tautomatically. The computed colour temperate, gains and correction\n> > \tmatrix are reported in metadata. The corresponding controls are ignored\n> > \tif set in a request.\n> > \n> > \tWhen AWB is disabled, the colour temperature, gains and correction\n> > \tmatrix are not updated automatically and can be set manually in\n> > \trequests.\n> \n> Looks good, I'll apply that.\n> \n> > > \n> > > + \\sa ColourCorrectionMatrix\n> > > \\sa ColourGains\n> > > + \\sa ColourTemperature\n> > > \n> > > # AwbMode needs further attention:\n> > > # - Auto-generate max enum value.\n> > > @@ -309,13 +312,24 @@ controls:\n> > > disabled.\n> > > \n> > > \\sa AwbEnable\n> > > + \\sa ColourTemperature\n> > > size: [2]\n> > > \n> > > - ColourTemperature:\n> > > type: int32_t\n> > > - description: Report the current estimate of the colour temperature, in\n> > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > - returned in metadata.\n> > > + description: |\n> > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > + this frame. An implementation may also allow this control to be set when\n> > \n> > s/also //\n> > \n> > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > + specified at the same time, they take precedence.\n> > \n> > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > if an application sets ColourTemperature and ColourGains but not\n> > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > requested ColourGains and set ColourCorrectionMatrix based on the\n> > temperature ?\n> \n> The latter is exactly what happens. The following table lists the\n> possible options:\n> \n> CT -> CG(CT), CCM(CT)\n> CT, CG -> CG, CCM(CT)\n> CT, CCM -> CG(CT), CCM\n> CT, CG, CCM -> CG, CCM\n\nDavid, is this the behaviour you would also expect for Raspberry Pi ?\n\n> Do you have a nice sentence for that?\n\n/me checks... I have a set of French proverbs, but none of the seem very\nappropriate. Sorry :-)\n\n> > > +\n> > > + The metadata will only report measured colour temperature values when\n> > > + available, even if set manually.\n> > \n> > I'm not sure to understand this.\n> \n> This is based on the comment from Kieran:\n> https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> for cases (RPi) where no temperature gets estimated. Only measurements\n> are returned in metadata. Manually set temperature is not reflected in\n> the metadata. This has the nice side effect, that you can set\n> AwbEnable=false, and still get temperature estimations in the metadata.\n\nThis means that whether or not the estimated colour temperature will be\nreturned in metadata will be platform-dependent. Can we avoid that ? It\nmakes writing portable applications much more difficult.\n\nMy other concern is that metadata is supposed to report the setting\napplied to a frame. The general rule is that, if a control can be set,\nthe value that has been set for a frame is reported in metadata. There\nare exception for trigger-like controls. As this example clearly shows,\nhaving multiple controls that ultimately set the same values is also\nproblematic from this point of view. Do we need to set a rule that\nhigher-level controls that get translated to lower-level controls are\nnever reported in metadata ?\n\nIf we do that, then we'll have ColourTemperature as a control being\ndefined differently from ColourTemperature as metadata. I'm not sure I\nlike it much. Should we have two different controls ?\n\n> > > +\n> > > + \\sa AwbEnable\n> > > + \\sa ColourCorrectionMatrix\n> > > + \\sa ColourGains\n> > > \n> > > - Saturation:\n> > > type: float\n> > > @@ -365,6 +379,8 @@ controls:\n> > > transformation. The 3x3 matrix is stored in conventional reading\n> > > order in an array of 9 floating point values.\n> > > \n> > > + \\sa AwbEnable\n> > > + \\sa ColourTemperature\n> > > size: [3,3]\n> > > \n> > > - ScalerCrop:","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 CAC05C324C\n\tfor ;\n\tFri, 30 Aug 2024 10:16:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A6C5A63466;\n\tFri, 30 Aug 2024 12:16:27 +0200 (CEST)","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 7D85161E4A\n\tfor ;\n\tFri, 30 Aug 2024 12:16:26 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 257C7227;\n\tFri, 30 Aug 2024 12:15:17 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vfAJmN6B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725012917;\n\tbh=i07A3MFKc6xzl9hhIXaMavGpQ0I/B837PnnyFAc7oMo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=vfAJmN6BF0qrQrcEQpRXU/M1UG7Sb1D7/oFFjwmIjXSCBEg7y8n8ul0qk58uwIVcP\n\tUHiI16k/esCemz1CKjdKvvmUBXf1EJsSv1WjypEWSXrBakRIgjvvmzwiHMwuPDQy1I\n\t3v6hqoaWqE+0k7QSAnEXLMRPNS88PabT7mlPegFw=","Date":"Fri, 30 Aug 2024 13:15:55 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tDavid Plowman ","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20240830101555.GG25163@pendragon.ideasonboard.com>","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30989,"web_url":"https://patchwork.libcamera.org/comment/30989/","msgid":"","date":"2024-08-30T11:21:36","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> Hi Stefan,\n> \n> (CC'ing David)\n> \n> On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > temperature. It also provides an easy way to apply the temperature\n> > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > \n> > > > Document this and update the control dependencies.\n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > \n> > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > @@ -252,9 +252,12 @@ controls:\n> > > > - AwbEnable:\n> > > > type: bool\n> > > > description: |\n> > > > - Enable or disable the AWB.\n> > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > \n> > > Please split this in two paragraphs. The first paragraph is translated\n> > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > below.\n> > > \n> > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > mean ?\n> > > \n> > > \tEnable or disable the AWB.\n> > > \n> > > \tWhen AWB is enabled, the algorithm estimates the colour\ttemperature of\n> > > \tthe scene, and computes colour gains and the colour correction matrix\n> > > \tautomatically. The computed colour temperate, gains and correction\n> > > \tmatrix are reported in metadata. The corresponding controls are ignored\n> > > \tif set in a request.\n> > > \n> > > \tWhen AWB is disabled, the colour temperature, gains and correction\n> > > \tmatrix are not updated automatically and can be set manually in\n> > > \trequests.\n> > \n> > Looks good, I'll apply that.\n> > \n> > > > \n> > > > + \\sa ColourCorrectionMatrix\n> > > > \\sa ColourGains\n> > > > + \\sa ColourTemperature\n> > > > \n> > > > # AwbMode needs further attention:\n> > > > # - Auto-generate max enum value.\n> > > > @@ -309,13 +312,24 @@ controls:\n> > > > disabled.\n> > > > \n> > > > \\sa AwbEnable\n> > > > + \\sa ColourTemperature\n> > > > size: [2]\n> > > > \n> > > > - ColourTemperature:\n> > > > type: int32_t\n> > > > - description: Report the current estimate of the colour temperature, in\n> > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > - returned in metadata.\n> > > > + description: |\n> > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > + this frame. An implementation may also allow this control to be set when\n> > > \n> > > s/also //\n> > > \n> > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > + specified at the same time, they take precedence.\n> > > \n> > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > if an application sets ColourTemperature and ColourGains but not\n> > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > temperature ?\n> > \n> > The latter is exactly what happens. The following table lists the\n> > possible options:\n> > \n> > CT -> CG(CT), CCM(CT)\n> > CT, CG -> CG, CCM(CT)\n> > CT, CCM -> CG(CT), CCM\n> > CT, CG, CCM -> CG, CCM\n> \n> David, is this the behaviour you would also expect for Raspberry Pi ?\n> \n> > Do you have a nice sentence for that?\n> \n> /me checks... I have a set of French proverbs, but none of the seem very\n> appropriate. Sorry :-)\n>\n\nWhat about \"If ColourGains and ColourTemperature are specified at the\nsame time, ColourGains takes precedence. The same applies to\nColourCorrectionMatrix.\"?\n\nWe can discuss the final wording after feedback from David.\n\n> \n> > > > +\n> > > > + The metadata will only report measured colour temperature values when\n> > > > + available, even if set manually.\n> > > \n> > > I'm not sure to understand this.\n> > \n> > This is based on the comment from Kieran:\n> > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > for cases (RPi) where no temperature gets estimated. Only measurements\n> > are returned in metadata. Manually set temperature is not reflected in\n> > the metadata. This has the nice side effect, that you can set\n> > AwbEnable=false, and still get temperature estimations in the metadata.\n> \n> This means that whether or not the estimated colour temperature will be\n> returned in metadata will be platform-dependent. Can we avoid that ? It\n> makes writing portable applications much more difficult.\n> \n> My other concern is that metadata is supposed to report the setting\n> applied to a frame. The general rule is that, if a control can be set,\n> the value that has been set for a frame is reported in metadata. There\n> are exception for trigger-like controls. As this example clearly shows,\n> having multiple controls that ultimately set the same values is also\n> problematic from this point of view. Do we need to set a rule that\n> higher-level controls that get translated to lower-level controls are\n> never reported in metadata ?\n> \n> If we do that, then we'll have ColourTemperature as a control being\n> defined differently from ColourTemperature as metadata. I'm not sure I\n> like it much. Should we have two different controls ?\n>\n\nWe could split that to \"MeasuredColourTemperature\" and\n\"AppliedColorTemperature\". But there are always cases where one of them\n(or both) is not available (as discussed on Patch 3). I think on rkisp\nwe could ensure that MeasuredColourTemperature is always available and\ncontains \"something\" as the statistics are always available. But in the\nend as a user I'd prefer to know when the algorithm failed to deduce a\nvalid colour temperature.\n\nRegards,\nStefan\n\n> \n> > > > +\n> > > > + \\sa AwbEnable\n> > > > + \\sa ColourCorrectionMatrix\n> > > > + \\sa ColourGains\n> > > > \n> > > > - Saturation:\n> > > > type: float\n> > > > @@ -365,6 +379,8 @@ controls:\n> > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > order in an array of 9 floating point values.\n> > > > \n> > > > + \\sa AwbEnable\n> > > > + \\sa ColourTemperature\n> > > > size: [3,3]\n> > > > \n> > > > - ScalerCrop:\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 B2591C323E\n\tfor ;\n\tFri, 30 Aug 2024 11:21:42 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DADF663466;\n\tFri, 30 Aug 2024 13:21:41 +0200 (CEST)","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 D432D61E4A\n\tfor ;\n\tFri, 30 Aug 2024 13:21:39 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:cdac:b850:c793:ea45])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id A5E91229;\n\tFri, 30 Aug 2024 13:20:30 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LRSnfu4D\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725016830;\n\tbh=CXi+dmsA4xGaeqzpTZruXFqCU/eOFWELmliPLIgBARI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LRSnfu4DV+9pVpSRB6zGXlT9rdZNhZAxyVx+f9JTKaxA+3IXYffDuMrEKIpANJmT7\n\tFuDUuFuyqRN+TyFOmoaBYqTQTzVjp4lCb3YrX/fSbB2xUVEmw0wdjwK8nIMyAYjbTp\n\tCBfI3vg1u7hjlAfD6CQdZi77AQlAtGKO4BQY27rw=","Date":"Fri, 30 Aug 2024 13:21:36 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman ","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240830101555.GG25163@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30994,"web_url":"https://patchwork.libcamera.org/comment/30994/","msgid":"","date":"2024-08-30T15:31:50","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi everyone\n\nOn Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n>\n> Hi Laurent,\n>\n> On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > Hi Stefan,\n> >\n> > (CC'ing David)\n> >\n> > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > >\n> > > > > Document this and update the control dependencies.\n> > > > >\n> > > > > Signed-off-by: Stefan Klug \n> > > > > ---\n> > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > - AwbEnable:\n> > > > > type: bool\n> > > > > description: |\n> > > > > - Enable or disable the AWB.\n> > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > >\n> > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > below.\n> > > >\n> > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > mean ?\n> > > >\n> > > > Enable or disable the AWB.\n> > > >\n> > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > the scene, and computes colour gains and the colour correction matrix\n> > > > automatically. The computed colour temperate, gains and correction\n> > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > if set in a request.\n> > > >\n> > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > matrix are not updated automatically and can be set manually in\n> > > > requests.\n> > >\n> > > Looks good, I'll apply that.\n> > >\n> > > > >\n> > > > > + \\sa ColourCorrectionMatrix\n> > > > > \\sa ColourGains\n> > > > > + \\sa ColourTemperature\n> > > > >\n> > > > > # AwbMode needs further attention:\n> > > > > # - Auto-generate max enum value.\n> > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > disabled.\n> > > > >\n> > > > > \\sa AwbEnable\n> > > > > + \\sa ColourTemperature\n> > > > > size: [2]\n> > > > >\n> > > > > - ColourTemperature:\n> > > > > type: int32_t\n> > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > - returned in metadata.\n> > > > > + description: |\n> > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > + this frame. An implementation may also allow this control to be set when\n> > > >\n> > > > s/also //\n> > > >\n> > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > + specified at the same time, they take precedence.\n> > > >\n> > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > if an application sets ColourTemperature and ColourGains but not\n> > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > temperature ?\n> > >\n> > > The latter is exactly what happens. The following table lists the\n> > > possible options:\n> > >\n> > > CT -> CG(CT), CCM(CT)\n> > > CT, CG -> CG, CCM(CT)\n> > > CT, CCM -> CG(CT), CCM\n> > > CT, CG, CCM -> CG, CCM\n> >\n> > David, is this the behaviour you would also expect for Raspberry Pi ?\n\nI'm guessing the question here is really what to do if an application\nsets both CT and CG together? The other cases seem fairly\nuncontroversial (?).\n\nFor us, I'm not sure it really makes much sense to supply both CT and\nCG at once. When someone sets CG, the algorithm estimates the CT and\npasses this to other algorithms, like lens shading and CCM. (So we\nactually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n\nWhen I add this new control, it will calculate CG from the calibrated\ncolour temperature curve in the camera tuning and use those, and pass\nthe CT to those other algorithms (so like the first row in the table).\n\nI don't think I particularly want to implement cases where it takes a\nCG value, uses them, and also a random CT which it then passes on?\nPossibly I'd rather leave these cases (where we have both CT and CG on\nthe LHS) as either \"implementation dependent\" or just \"undefined\", and\nrecommend setting one or other.\n\nDoes that help or just confuse further?\n\nThanks!\nDavid\n\n> >\n> > > Do you have a nice sentence for that?\n> >\n> > /me checks... I have a set of French proverbs, but none of the seem very\n> > appropriate. Sorry :-)\n> >\n>\n> What about \"If ColourGains and ColourTemperature are specified at the\n> same time, ColourGains takes precedence. The same applies to\n> ColourCorrectionMatrix.\"?\n>\n> We can discuss the final wording after feedback from David.\n>\n> >\n> > > > > +\n> > > > > + The metadata will only report measured colour temperature values when\n> > > > > + available, even if set manually.\n> > > >\n> > > > I'm not sure to understand this.\n> > >\n> > > This is based on the comment from Kieran:\n> > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > are returned in metadata. Manually set temperature is not reflected in\n> > > the metadata. This has the nice side effect, that you can set\n> > > AwbEnable=false, and still get temperature estimations in the metadata.\n> >\n> > This means that whether or not the estimated colour temperature will be\n> > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > makes writing portable applications much more difficult.\n> >\n> > My other concern is that metadata is supposed to report the setting\n> > applied to a frame. The general rule is that, if a control can be set,\n> > the value that has been set for a frame is reported in metadata. There\n> > are exception for trigger-like controls. As this example clearly shows,\n> > having multiple controls that ultimately set the same values is also\n> > problematic from this point of view. Do we need to set a rule that\n> > higher-level controls that get translated to lower-level controls are\n> > never reported in metadata ?\n> >\n> > If we do that, then we'll have ColourTemperature as a control being\n> > defined differently from ColourTemperature as metadata. I'm not sure I\n> > like it much. Should we have two different controls ?\n> >\n>\n> We could split that to \"MeasuredColourTemperature\" and\n> \"AppliedColorTemperature\". But there are always cases where one of them\n> (or both) is not available (as discussed on Patch 3). I think on rkisp\n> we could ensure that MeasuredColourTemperature is always available and\n> contains \"something\" as the statistics are always available. But in the\n> end as a user I'd prefer to know when the algorithm failed to deduce a\n> valid colour temperature.\n>\n> Regards,\n> Stefan\n>\n> >\n> > > > > +\n> > > > > + \\sa AwbEnable\n> > > > > + \\sa ColourCorrectionMatrix\n> > > > > + \\sa ColourGains\n> > > > >\n> > > > > - Saturation:\n> > > > > type: float\n> > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > order in an array of 9 floating point values.\n> > > > >\n> > > > > + \\sa AwbEnable\n> > > > > + \\sa ColourTemperature\n> > > > > size: [3,3]\n> > > > >\n> > > > > - ScalerCrop:\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 DE509C323E\n\tfor ;\n\tFri, 30 Aug 2024 15:32:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6E89263469;\n\tFri, 30 Aug 2024 17:32:04 +0200 (CEST)","from mail-qk1-x72a.google.com (mail-qk1-x72a.google.com\n\t[IPv6:2607:f8b0:4864:20::72a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D0E4161E4A\n\tfor ;\n\tFri, 30 Aug 2024 17:32:02 +0200 (CEST)","by mail-qk1-x72a.google.com with SMTP id\n\taf79cd13be357-7a80fe481a9so69005885a.0\n\tfor ;\n\tFri, 30 Aug 2024 08:32:02 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"aO/HLzg3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1725031922; x=1725636722;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=F+OeqzJHUz6sFK2EG0+G9s1bn2P0KiIGJB2PXND8yEw=;\n\tb=aO/HLzg3DDgkM5o3/ZL6PaG6gPEqdrTJ2kXL9RLEc+dep11HypKwuHDXN6NPTuapfA\n\tXQwLDdMLGOXl1P9E8wMFSQWUYFRiW6xjabgzXY3eXlPHgO70gxh+3/5Z+1ZlBvgya9tk\n\tJig8LWnTva/dPXmPUSORzYLGKzz6Gj4REi3rdayJGN6U34O9ayxAZpsZk5kkK6m5yS3v\n\tWBrG+Z2uavpovss87hET9rjvvBD68rl4zjXpP/KziDj6oYPLwDN8SUBwAYLvd1t6cwpQ\n\tt7y8lNDabbyR4pghbaVxhwMc2CNqlMFFtUW5f5Knh9zZJCIsfGaV3o59k9BhAGwnmB1y\n\tEFCA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725031922; x=1725636722;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=F+OeqzJHUz6sFK2EG0+G9s1bn2P0KiIGJB2PXND8yEw=;\n\tb=pQqj1HjDXVTDVhOCtiNcc3N03DzvLyUzzsti0oZ2Kw9LaYAs32Vzarkbo1Wt/J992S\n\tvMsbDo9dYA38dB/xZiDss6YUY0dDoHVVv9aw7XVAv6AjUhGilILhJfXnb+Dm30UKwr4O\n\t3wp4AKu6sehZXDZnPIKIO4uPo9J3fqyda9fEFbdNy73yA9NbatjxpGFhK6apIurH0plM\n\tRjF9m1BfV2/X2C4u1cUHBc7R8C5S5sMakkTAYSRxgvT6gvc9lV0LFdPmX7zQbSt8AC1T\n\tT3dtBo1/eCqGZIaGuc+fdYsu77CHugSN7WknAwD71M3lR5/i+v85XcVzZvbUjtOUCNwz\n\tlQuQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWQfkP7QCuK+4ycufpMoxFEZXC8erEH6c7UahEx9/DZasb7MRM8zPAQpXMYiC4guIVTgxGOnTKFOPqUuT/zRc8=@lists.libcamera.org","X-Gm-Message-State":"AOJu0Yw1T8prqrJg0+JtqK2NhTuHpfm8NkV+sdq48dzok9xTS+6FYk9d\n\tcV5m0jHhMTfWrLGNuy08cgI+0xrJGPdKqDfSBSyNyWRJ76DHGFgtR+M3DyB1TNHNaF2gXpImyOF\n\tcyi65oo2xNdWZGnPXGicbZwNKry3QafpU0Cngr279StDad7HV","X-Google-Smtp-Source":"AGHT+IFcErM4CE0DchCIDAKgS5/DnZbbYJQgzUgpM/ZYohx0n/FWzkwxgzZ5jrXVAJH4K4PzbtN1HFxPFkI09hSJ4sA=","X-Received":"by 2002:a05:620a:2404:b0:7a1:e888:c852 with SMTP id\n\taf79cd13be357-7a81d67fee8mr29278085a.24.1725031921486;\n\tFri, 30 Aug 2024 08:32:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t","In-Reply-To":"","From":"David Plowman ","Date":"Fri, 30 Aug 2024 16:31:50 +0100","Message-ID":"","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","To":"Stefan Klug ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31022,"web_url":"https://patchwork.libcamera.org/comment/31022/","msgid":"<20240831151610.GX3811@pendragon.ideasonboard.com>","date":"2024-08-31T15:16:10","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > >\n> > > > > > Document this and update the control dependencies.\n> > > > > >\n> > > > > > Signed-off-by: Stefan Klug \n> > > > > > ---\n> > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > >\n> > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > - AwbEnable:\n> > > > > > type: bool\n> > > > > > description: |\n> > > > > > - Enable or disable the AWB.\n> > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > >\n> > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > below.\n> > > > >\n> > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > mean ?\n> > > > >\n> > > > > Enable or disable the AWB.\n> > > > >\n> > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > automatically. The computed colour temperate, gains and correction\n> > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > if set in a request.\n> > > > >\n> > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > matrix are not updated automatically and can be set manually in\n> > > > > requests.\n> > > >\n> > > > Looks good, I'll apply that.\n> > > >\n> > > > > >\n> > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > \\sa ColourGains\n> > > > > > + \\sa ColourTemperature\n> > > > > >\n> > > > > > # AwbMode needs further attention:\n> > > > > > # - Auto-generate max enum value.\n> > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > disabled.\n> > > > > >\n> > > > > > \\sa AwbEnable\n> > > > > > + \\sa ColourTemperature\n> > > > > > size: [2]\n> > > > > >\n> > > > > > - ColourTemperature:\n> > > > > > type: int32_t\n> > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > - returned in metadata.\n> > > > > > + description: |\n> > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > >\n> > > > > s/also //\n> > > > >\n> > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > + specified at the same time, they take precedence.\n> > > > >\n> > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > temperature ?\n> > > >\n> > > > The latter is exactly what happens. The following table lists the\n> > > > possible options:\n> > > >\n> > > > CT -> CG(CT), CCM(CT)\n> > > > CT, CG -> CG, CCM(CT)\n> > > > CT, CCM -> CG(CT), CCM\n> > > > CT, CG, CCM -> CG, CCM\n> > >\n> > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> \n> I'm guessing the question here is really what to do if an application\n> sets both CT and CG together? The other cases seem fairly\n> uncontroversial (?).\n\nAs far as I understand, for the rkisp1, the colour gains and the CCM are\nboth calculated based on the colour temperature. The case where the\napplication sets CT and CG is conceptually as problematic as the case\nwhere it sets CT and CCM. Stefan, is that correct ?\n\n> For us, I'm not sure it really makes much sense to supply both CT and\n> CG at once. When someone sets CG, the algorithm estimates the CT and\n> passes this to other algorithms, like lens shading and CCM. (So we\n> actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n\nWhen an application sets CG, does the algorithm estimate CT from the\nstats, or from the CG alone for RPi ? Stefan, how about your plans for\nrkisp1 ?\n\n> When I add this new control, it will calculate CG from the calibrated\n> colour temperature curve in the camera tuning and use those, and pass\n> the CT to those other algorithms (so like the first row in the table).\n\nThat's the main use, and I think we all agree on the behaviour. It seems\nquite uncontroversial.\n\n> I don't think I particularly want to implement cases where it takes a\n> CG value, uses them, and also a random CT which it then passes on?\n> Possibly I'd rather leave these cases (where we have both CT and CG on\n> the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> recommend setting one or other.\n\nI dislike implementation-dependent behaviours if we can avoid them, at\nleast in cases that we consider valid use cases. Is there a valid use\ncase for setting CT along with CG or CCM ? It sounds to me like there\nwouldn't be one for RPi. Stefan, how about you ?\n\n> Does that help or just confuse further?\n\nLet's see how the discussion progresses :-) Thank you for your input.\n\n> > > > Do you have a nice sentence for that?\n> > >\n> > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > appropriate. Sorry :-)\n> >\n> > What about \"If ColourGains and ColourTemperature are specified at the\n> > same time, ColourGains takes precedence. The same applies to\n> > ColourCorrectionMatrix.\"?\n> >\n> > We can discuss the final wording after feedback from David.\n> >\n> > > > > > +\n> > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > + available, even if set manually.\n> > > > >\n> > > > > I'm not sure to understand this.\n> > > >\n> > > > This is based on the comment from Kieran:\n> > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > the metadata. This has the nice side effect, that you can set\n> > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > >\n> > > This means that whether or not the estimated colour temperature will be\n> > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > makes writing portable applications much more difficult.\n> > >\n> > > My other concern is that metadata is supposed to report the setting\n> > > applied to a frame. The general rule is that, if a control can be set,\n> > > the value that has been set for a frame is reported in metadata. There\n> > > are exception for trigger-like controls. As this example clearly shows,\n> > > having multiple controls that ultimately set the same values is also\n> > > problematic from this point of view. Do we need to set a rule that\n> > > higher-level controls that get translated to lower-level controls are\n> > > never reported in metadata ?\n> > >\n> > > If we do that, then we'll have ColourTemperature as a control being\n> > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > like it much. Should we have two different controls ?\n> >\n> > We could split that to \"MeasuredColourTemperature\" and\n> > \"AppliedColorTemperature\". But there are always cases where one of them\n> > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > we could ensure that MeasuredColourTemperature is always available and\n> > contains \"something\" as the statistics are always available. But in the\n> > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > valid colour temperature.\n\nIf the algorithm failed to estimate the colour temperature, that should\nbe indicated by the absence of an estimated colour temperature in\nmetadata.\n\nDavid, why do you (if I understand correctly) stop estimating the colour\ntemperature from the statistics in manual AWB mode ?\n\n> > > > > > +\n> > > > > > + \\sa AwbEnable\n> > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > + \\sa ColourGains\n> > > > > >\n> > > > > > - Saturation:\n> > > > > > type: float\n> > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > order in an array of 9 floating point values.\n> > > > > >\n> > > > > > + \\sa AwbEnable\n> > > > > > + \\sa ColourTemperature\n> > > > > > size: [3,3]\n> > > > > >\n> > > > > > - ScalerCrop:","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 1FB04C324C\n\tfor ;\n\tSat, 31 Aug 2024 15:16:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F104063471;\n\tSat, 31 Aug 2024 17:16:43 +0200 (CEST)","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 67F9661900\n\tfor ;\n\tSat, 31 Aug 2024 17:16:42 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54C56735;\n\tSat, 31 Aug 2024 17:15:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"adrFTkSm\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725117331;\n\tbh=2NU3I0ujnI5+Wec1IeRNbn0YRYUZ85bKO5nm4oK4IgE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=adrFTkSmXGR64gOBeCNhIlmn5EjEk3EDCG1w9Mn2kJEbOCsAMgFb/otGtYCjCQY/8\n\t6wsbpX6sWHIarMn44L6YcemiSfoTvT9kJO4sATI4eso05HW7Opgd8OzcDsJ8q6GGup\n\tVkRFNYu67g2aA6INAfHwqDBe1yAUZoUYFAPwj+fg=","Date":"Sat, 31 Aug 2024 18:16:10 +0300","From":"Laurent Pinchart ","To":"David Plowman ","Cc":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20240831151610.GX3811@pendragon.ideasonboard.com>","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31036,"web_url":"https://patchwork.libcamera.org/comment/31036/","msgid":"","date":"2024-09-02T07:28:24","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\nDavid's away this week, so I'll answer on his behalf.\n\nOn Sat, 31 Aug 2024 at 16:16, Laurent Pinchart\n wrote:\n>\n> On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > >\n> > > > > > > Document this and update the control dependencies.\n> > > > > > >\n> > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > ---\n> > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > - AwbEnable:\n> > > > > > > type: bool\n> > > > > > > description: |\n> > > > > > > - Enable or disable the AWB.\n> > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > >\n> > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > below.\n> > > > > >\n> > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > mean ?\n> > > > > >\n> > > > > > Enable or disable the AWB.\n> > > > > >\n> > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > if set in a request.\n> > > > > >\n> > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > requests.\n> > > > >\n> > > > > Looks good, I'll apply that.\n> > > > >\n> > > > > > >\n> > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > \\sa ColourGains\n> > > > > > > + \\sa ColourTemperature\n> > > > > > >\n> > > > > > > # AwbMode needs further attention:\n> > > > > > > # - Auto-generate max enum value.\n> > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > disabled.\n> > > > > > >\n> > > > > > > \\sa AwbEnable\n> > > > > > > + \\sa ColourTemperature\n> > > > > > > size: [2]\n> > > > > > >\n> > > > > > > - ColourTemperature:\n> > > > > > > type: int32_t\n> > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > - returned in metadata.\n> > > > > > > + description: |\n> > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > >\n> > > > > > s/also //\n> > > > > >\n> > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > + specified at the same time, they take precedence.\n> > > > > >\n> > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > temperature ?\n> > > > >\n> > > > > The latter is exactly what happens. The following table lists the\n> > > > > possible options:\n> > > > >\n> > > > > CT -> CG(CT), CCM(CT)\n> > > > > CT, CG -> CG, CCM(CT)\n> > > > > CT, CCM -> CG(CT), CCM\n> > > > > CT, CG, CCM -> CG, CCM\n> > > >\n> > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> >\n> > I'm guessing the question here is really what to do if an application\n> > sets both CT and CG together? The other cases seem fairly\n> > uncontroversial (?).\n>\n> As far as I understand, for the rkisp1, the colour gains and the CCM are\n> both calculated based on the colour temperature. The case where the\n> application sets CT and CG is conceptually as problematic as the case\n> where it sets CT and CCM. Stefan, is that correct ?\n>\n> > For us, I'm not sure it really makes much sense to supply both CT and\n> > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > passes this to other algorithms, like lens shading and CCM. (So we\n> > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n>\n> When an application sets CG, does the algorithm estimate CT from the\n> stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> rkisp1 ?\n\nWhen an application sets manual CGs, the algorithm estimates the CT\nfrom the CT-curve in the tuning directly.\n\n>\n> > When I add this new control, it will calculate CG from the calibrated\n> > colour temperature curve in the camera tuning and use those, and pass\n> > the CT to those other algorithms (so like the first row in the table).\n>\n> That's the main use, and I think we all agree on the behaviour. It seems\n> quite uncontroversial.\n>\n> > I don't think I particularly want to implement cases where it takes a\n> > CG value, uses them, and also a random CT which it then passes on?\n> > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > recommend setting one or other.\n>\n> I dislike implementation-dependent behaviours if we can avoid them, at\n> least in cases that we consider valid use cases. Is there a valid use\n> case for setting CT along with CG or CCM ? It sounds to me like there\n> wouldn't be one for RPi. Stefan, how about you ?\n\nI don't think RPi has a use case that requires the CT and CGs to be both set.\n\n>\n> > Does that help or just confuse further?\n>\n> Let's see how the discussion progresses :-) Thank you for your input.\n>\n> > > > > Do you have a nice sentence for that?\n> > > >\n> > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > appropriate. Sorry :-)\n> > >\n> > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > same time, ColourGains takes precedence. The same applies to\n> > > ColourCorrectionMatrix.\"?\n> > >\n> > > We can discuss the final wording after feedback from David.\n> > >\n> > > > > > > +\n> > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > + available, even if set manually.\n> > > > > >\n> > > > > > I'm not sure to understand this.\n> > > > >\n> > > > > This is based on the comment from Kieran:\n> > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > the metadata. This has the nice side effect, that you can set\n> > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > >\n> > > > This means that whether or not the estimated colour temperature will be\n> > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > makes writing portable applications much more difficult.\n> > > >\n> > > > My other concern is that metadata is supposed to report the setting\n> > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > the value that has been set for a frame is reported in metadata. There\n> > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > having multiple controls that ultimately set the same values is also\n> > > > problematic from this point of view. Do we need to set a rule that\n> > > > higher-level controls that get translated to lower-level controls are\n> > > > never reported in metadata ?\n> > > >\n> > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > like it much. Should we have two different controls ?\n> > >\n> > > We could split that to \"MeasuredColourTemperature\" and\n> > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > we could ensure that MeasuredColourTemperature is always available and\n> > > contains \"something\" as the statistics are always available. But in the\n> > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > valid colour temperature.\n>\n> If the algorithm failed to estimate the colour temperature, that should\n> be indicated by the absence of an estimated colour temperature in\n> metadata.\n>\n> David, why do you (if I understand correctly) stop estimating the colour\n> temperature from the statistics in manual AWB mode ?\n\nI think it's to keep things consistent. The CT-curve in the tuning\nfile is taken as the canonical truth. So if manual CGs are applied,\nand assuming the users know what they are doing(!), we return a simple\nlookup without going through all the computations again. Of course,\nthis is simple enough to change in our code if the need arises.\n\nRegards,\nNaush\n\n>\n> > > > > > > +\n> > > > > > > + \\sa AwbEnable\n> > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > + \\sa ColourGains\n> > > > > > >\n> > > > > > > - Saturation:\n> > > > > > > type: float\n> > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > order in an array of 9 floating point values.\n> > > > > > >\n> > > > > > > + \\sa AwbEnable\n> > > > > > > + \\sa ColourTemperature\n> > > > > > > size: [3,3]\n> > > > > > >\n> > > > > > > - ScalerCrop:\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 78B6CBF415\n\tfor ;\n\tMon, 2 Sep 2024 07:29:04 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6F0A763458;\n\tMon, 2 Sep 2024 09:29:03 +0200 (CEST)","from mail-yw1-x112d.google.com (mail-yw1-x112d.google.com\n\t[IPv6:2607:f8b0:4864:20::112d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C879F618FE\n\tfor ;\n\tMon, 2 Sep 2024 09:29:01 +0200 (CEST)","by mail-yw1-x112d.google.com with SMTP id\n\t00721157ae682-6da426b619cso38447b3.0\n\tfor ;\n\tMon, 02 Sep 2024 00:29:01 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"p4xJb58C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1725262140; x=1725866940;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=1qlOLPh+DtxS3gAPMZiXdOlxoSId/rSnPZnvTooHbAQ=;\n\tb=p4xJb58C2/3OMA4hhf0bpjE3aqYfBbthgrOnPnVevrcglHi6V9gOkwjzzoSTHu0grm\n\tKILyYFO0v3sx3MLoJUzXTWHNVNngUPAKfOOo20hfr2UaL2o/6Ho0nNPGwuI/rPs0X6T0\n\tjmZCYmO4OdLx69nWFy+2kaOXQLDDeL3gnf/K5hF5X/VUYVJDC8PCzm/6eA9JX7V247hv\n\tH29cJXlgoAX0ykW0IUX/xSUi7F13i1Grpr0eYCScYMjGdndhA3JmBMOo8r5wtP9O40uF\n\t7+q1w3WTuq6lRxHNIONFRFWImbweNzCty4k6uF3MVVZUi4eobONc+vE/3/mXEiSD0k04\n\tqFRw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1725262140; x=1725866940;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=1qlOLPh+DtxS3gAPMZiXdOlxoSId/rSnPZnvTooHbAQ=;\n\tb=Ry27dmFPZt15BPminpOqqh3Ix4pyd/f7wToxoVmfOXV1yLzClm6JHZw57NIoaaVCqG\n\tEFs6veOmsmd4oPoS0LX6RQjQ2j6ARib/2Oy4NPnLHMsKgfv00OqqCLWYqTyUUvu61RpD\n\tt95itne9S4DDQzQV2vBf5JGLXbQrH7C6S378EHeUA7FqESP6JTylZYX5CTfR8yfdUr+M\n\tqGv9lMWnitm3XOXT5zfdtgGHOqIy19mXHqD4z7u8rX2gsQtI/isqg8eJQQgctz+4vXSo\n\taSdoKSPo25jt9l04jXojCx+69HN5+PCWl+/xVZgMvqU0QH0/44VhmKX/iApcFJQAPDPQ\n\tXbag==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCU264lzCbFbkVUawevJUcf/xEgtmiiyWUmUUEQoV2Dxtk5Nz8mLzIRXe4Ag/PRIgpgBb4GQVayUHcVgtdI5jhU=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YwjqtVxru+Dm1ucAjbWcjWS/XnfC3tXDqN9xNil3iTshm3cbCuQ\n\tsIwEH+0ptzpEO/UMlz7Ltr36P6zZsjK5v7RsyOgBZcyazxoGaUkrnwopasUS3eQhz8OpYKJq9HW\n\tNfO6WM5sZdIIEutSgKWgPJyPV/YrKJedSNy2KMA==","X-Google-Smtp-Source":"AGHT+IHVqOSVxTMvJZYz7UkbVwTWBiehUVLxA3aP3xJ5d3eZ/z66ES0gIwBqT/kJEZpgHxEtOIg9fL7uxxkL2Gw0IEY=","X-Received":"by 2002:a05:690c:6909:b0:6d5:86df:d17c with SMTP id\n\t00721157ae682-6d586dfd248mr35367457b3.9.1725262140598;\n\tMon, 02 Sep 2024 00:29:00 -0700 (PDT)","MIME-Version":"1.0","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>","In-Reply-To":"<20240831151610.GX3811@pendragon.ideasonboard.com>","From":"Naushir Patuck ","Date":"Mon, 2 Sep 2024 08:28:24 +0100","Message-ID":"","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","To":"Laurent Pinchart ","Cc":"David Plowman , \n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31038,"web_url":"https://patchwork.libcamera.org/comment/31038/","msgid":"","date":"2024-09-02T08:32:59","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Sat, Aug 31, 2024 at 06:16:10PM +0300, Laurent Pinchart wrote:\n> On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > >\n> > > > > > > Document this and update the control dependencies.\n> > > > > > >\n> > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > ---\n> > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > >\n> > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > - AwbEnable:\n> > > > > > > type: bool\n> > > > > > > description: |\n> > > > > > > - Enable or disable the AWB.\n> > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > >\n> > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > below.\n> > > > > >\n> > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > mean ?\n> > > > > >\n> > > > > > Enable or disable the AWB.\n> > > > > >\n> > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > if set in a request.\n> > > > > >\n> > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > requests.\n> > > > >\n> > > > > Looks good, I'll apply that.\n> > > > >\n> > > > > > >\n> > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > \\sa ColourGains\n> > > > > > > + \\sa ColourTemperature\n> > > > > > >\n> > > > > > > # AwbMode needs further attention:\n> > > > > > > # - Auto-generate max enum value.\n> > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > disabled.\n> > > > > > >\n> > > > > > > \\sa AwbEnable\n> > > > > > > + \\sa ColourTemperature\n> > > > > > > size: [2]\n> > > > > > >\n> > > > > > > - ColourTemperature:\n> > > > > > > type: int32_t\n> > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > - returned in metadata.\n> > > > > > > + description: |\n> > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > >\n> > > > > > s/also //\n> > > > > >\n> > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > + specified at the same time, they take precedence.\n> > > > > >\n> > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > temperature ?\n> > > > >\n> > > > > The latter is exactly what happens. The following table lists the\n> > > > > possible options:\n> > > > >\n> > > > > CT -> CG(CT), CCM(CT)\n> > > > > CT, CG -> CG, CCM(CT)\n> > > > > CT, CCM -> CG(CT), CCM\n> > > > > CT, CG, CCM -> CG, CCM\n> > > >\n> > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > \n> > I'm guessing the question here is really what to do if an application\n> > sets both CT and CG together? The other cases seem fairly\n> > uncontroversial (?).\n> \n> As far as I understand, for the rkisp1, the colour gains and the CCM are\n> both calculated based on the colour temperature. The case where the\n> application sets CT and CG is conceptually as problematic as the case\n> where it sets CT and CCM. Stefan, is that correct ?\n\nOnly CCM is set based on CT. CG is set based on the stats and these are\nalso used to calculate the CT. As David noted CG -> CT(CG) -> CCM(CT) is\npossible. CT(CCM) is not possible afaik. So the cases are similar but\nnot the same.\n\n> \n> > For us, I'm not sure it really makes much sense to supply both CT and\n> > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > passes this to other algorithms, like lens shading and CCM. (So we\n> > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> \n> When an application sets CG, does the algorithm estimate CT from the\n> stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> rkisp1 ?\n\nAt the moment, we do not estimate CT from CG. But that might be worth to\ndo.\n\n> \n> > When I add this new control, it will calculate CG from the calibrated\n> > colour temperature curve in the camera tuning and use those, and pass\n> > the CT to those other algorithms (so like the first row in the table).\n> \n> That's the main use, and I think we all agree on the behaviour. It seems\n> quite uncontroversial.\n\nYes, I think on that one we are all fine.\nSo CT -> CG(CT), CCM(CT) is accepted. Metadata might be an open point\nhere.\n\n> \n> > I don't think I particularly want to implement cases where it takes a\n> > CG value, uses them, and also a random CT which it then passes on?\n> > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > recommend setting one or other.\n> \n> I dislike implementation-dependent behaviours if we can avoid them, at\n> least in cases that we consider valid use cases. Is there a valid use\n> case for setting CT along with CG or CCM ? It sounds to me like there\n> wouldn't be one for RPi. Stefan, how about you ?\n\nTo me the most prominent use case is manual WB. So the user takes a\npicture of a gray patch and the application (not libcamera) calculates\nwb gains based on that. If you compare to a typical digital camera I\nthink one would expect the following to happen: CG -> CT(CG) -> CCM(CT)\n\nIn rkisp1 we don't have that CG->CT(CG) step, but that doesn't mean we\nshouldn't do it.\n\nLooking at that with the eyes of a machine-vision user it would be\ncounter intuitive that the CCM suddenly changes when I change the CG.\nBut that can be worked around from user side, by supplying a manual CCM\nin the request. Which brings me to the next point. The CCM is the only\nparameter here, that is basically impossible to handcraft in an\napplication. So there might be use cases where I'd like to set a CCM\nbased on a color temperature and then hand tweak the CG.\n\nSo I would be fine if we say CG -> CG, CCM(CT(CG)). That is new to me\nand I haven't look into the implementation effort yet.\n\nCG, CT -> CG, CCM(CT) is something I'd like to have, but I don't know if\nwe need to force it (@David: would it be much effort on your side, just\nfor completeness sake?)\n\nCCM, CT -> CG(CT), CCM also feels natural to me, but without any known\nusecase. (Maybe in calibration, where you want to force a unit ccm\nand check the correctness of the gains)\n\nCT, CG, CCM -> CG, CCM I think we can agree on that one. It is\noverspecified, but the lower level controls should win.\n\n> \n> > Does that help or just confuse further?\n> \n> Let's see how the discussion progresses :-) Thank you for your input.\n> \n> > > > > Do you have a nice sentence for that?\n> > > >\n> > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > appropriate. Sorry :-)\n> > >\n> > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > same time, ColourGains takes precedence. The same applies to\n> > > ColourCorrectionMatrix.\"?\n> > >\n> > > We can discuss the final wording after feedback from David.\n> > >\n> > > > > > > +\n> > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > + available, even if set manually.\n> > > > > >\n> > > > > > I'm not sure to understand this.\n> > > > >\n> > > > > This is based on the comment from Kieran:\n> > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > the metadata. This has the nice side effect, that you can set\n> > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > >\n> > > > This means that whether or not the estimated colour temperature will be\n> > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > makes writing portable applications much more difficult.\n> > > >\n> > > > My other concern is that metadata is supposed to report the setting\n> > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > the value that has been set for a frame is reported in metadata. There\n> > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > having multiple controls that ultimately set the same values is also\n> > > > problematic from this point of view. Do we need to set a rule that\n> > > > higher-level controls that get translated to lower-level controls are\n> > > > never reported in metadata ?\n> > > >\n> > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > like it much. Should we have two different controls ?\n> > >\n> > > We could split that to \"MeasuredColourTemperature\" and\n> > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > we could ensure that MeasuredColourTemperature is always available and\n> > > contains \"something\" as the statistics are always available. But in the\n> > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > valid colour temperature.\n> \n> If the algorithm failed to estimate the colour temperature, that should\n> be indicated by the absence of an estimated colour temperature in\n> metadata.\n> \n> David, why do you (if I understand correctly) stop estimating the colour\n> temperature from the statistics in manual AWB mode ?\n> \n> > > > > > > +\n> > > > > > > + \\sa AwbEnable\n> > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > + \\sa ColourGains\n> > > > > > >\n> > > > > > > - Saturation:\n> > > > > > > type: float\n> > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > order in an array of 9 floating point values.\n> > > > > > >\n> > > > > > > + \\sa AwbEnable\n> > > > > > > + \\sa ColourTemperature\n> > > > > > > size: [3,3]\n> > > > > > >\n> > > > > > > - ScalerCrop:\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n\nRegards,\nStefan","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 09CC9BF415\n\tfor ;\n\tMon, 2 Sep 2024 08:33:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D438E634CB;\n\tMon, 2 Sep 2024 10:33:04 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A6B92618FE\n\tfor ;\n\tMon, 2 Sep 2024 10:33:02 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:4216:b2b9:2e87:3f42])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 71FF24CE;\n\tMon, 2 Sep 2024 10:31:51 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"OIOol96c\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725265911;\n\tbh=Kijs4qSjUOpjc7m1sETVFbKMbTy/nzO50Ru783lMXKE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=OIOol96cNISVk15TwkAZldIQHfoX97LzYF8ZL1QtreccQr36DaRJ/c/mf8NRyKjpU\n\tys4xyva3AfSVMu7p7zrWozIM8KotVqhMWqebEwBK+RYvNuOewKd5HJShqMyQoU+YwB\n\t+JhMi8aLMaU8RV6KdLbvjr/GNIb6Ve7ValT6w0MM=","Date":"Mon, 2 Sep 2024 10:32:59 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"David Plowman , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240831151610.GX3811@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32611,"web_url":"https://patchwork.libcamera.org/comment/32611/","msgid":"<20241209010209.GD6230@pendragon.ideasonboard.com>","date":"2024-12-09T01:02:09","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Looks like I forgot to reply to this.\n\nOn Mon, Sep 02, 2024 at 08:28:24AM +0100, Naushir Patuck wrote:\n> On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart wrote:\n> > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > >\n> > > > > > > > Document this and update the control dependencies.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > ---\n> > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > - AwbEnable:\n> > > > > > > > type: bool\n> > > > > > > > description: |\n> > > > > > > > - Enable or disable the AWB.\n> > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > >\n> > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > below.\n> > > > > > >\n> > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > mean ?\n> > > > > > >\n> > > > > > > Enable or disable the AWB.\n> > > > > > >\n> > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > if set in a request.\n> > > > > > >\n> > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > requests.\n> > > > > >\n> > > > > > Looks good, I'll apply that.\n> > > > > >\n> > > > > > > >\n> > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > \\sa ColourGains\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > >\n> > > > > > > > # AwbMode needs further attention:\n> > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > disabled.\n> > > > > > > >\n> > > > > > > > \\sa AwbEnable\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > > size: [2]\n> > > > > > > >\n> > > > > > > > - ColourTemperature:\n> > > > > > > > type: int32_t\n> > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > - returned in metadata.\n> > > > > > > > + description: |\n> > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > >\n> > > > > > > s/also //\n> > > > > > >\n> > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > + specified at the same time, they take precedence.\n> > > > > > >\n> > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > temperature ?\n> > > > > >\n> > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > possible options:\n> > > > > >\n> > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > CT, CG, CCM -> CG, CCM\n> > > > >\n> > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > >\n> > > I'm guessing the question here is really what to do if an application\n> > > sets both CT and CG together? The other cases seem fairly\n> > > uncontroversial (?).\n> >\n> > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > both calculated based on the colour temperature. The case where the\n> > application sets CT and CG is conceptually as problematic as the case\n> > where it sets CT and CCM. Stefan, is that correct ?\n> >\n> > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> >\n> > When an application sets CG, does the algorithm estimate CT from the\n> > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > rkisp1 ?\n> \n> When an application sets manual CGs, the algorithm estimates the CT\n> from the CT-curve in the tuning directly.\n\nI'll try to recap what we have (hopefully) agreed on so far.\n\nOK, so in manual AWB mode, CT and CG are interchangeable, one will be\ncalculated from the other based on the tuning data, without taking\nstatistics into account. As this is a pure software implementation, if\nboth controls are specified, we can freely specify which one takes\nprecedence, and this can be applied to all platforms. I have a\npreference for making the gains take precedence (as Stefan did in v5) as\nthey provide more precise control.\n\nIn auto AWB mode, CG and CT are both calculated by the AWB algorithm and\nany value set through controls is ignored.\n\nSo far, so good ?\n\nThe next question is how to handle CCM. Auto mode is easy, CCM is\ncomputed automatically, and any value set through controls is ignored.\nIn manual mode, if CCM is set in a request, its value should be applied.\nWhat if a request contains CG or CT and no CCM ? Would CCM be computed\nby the AWB algorithm (in manual mode), or its previous value retained ?\n\n> > > When I add this new control, it will calculate CG from the calibrated\n> > > colour temperature curve in the camera tuning and use those, and pass\n> > > the CT to those other algorithms (so like the first row in the table).\n> >\n> > That's the main use, and I think we all agree on the behaviour. It seems\n> > quite uncontroversial.\n> >\n> > > I don't think I particularly want to implement cases where it takes a\n> > > CG value, uses them, and also a random CT which it then passes on?\n> > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > recommend setting one or other.\n> >\n> > I dislike implementation-dependent behaviours if we can avoid them, at\n> > least in cases that we consider valid use cases. Is there a valid use\n> > case for setting CT along with CG or CCM ? It sounds to me like there\n> > wouldn't be one for RPi. Stefan, how about you ?\n> \n> I don't think RPi has a use case that requires the CT and CGs to be both set.\n> \n> > > Does that help or just confuse further?\n> >\n> > Let's see how the discussion progresses :-) Thank you for your input.\n> >\n> > > > > > Do you have a nice sentence for that?\n> > > > >\n> > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > appropriate. Sorry :-)\n> > > >\n> > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > same time, ColourGains takes precedence. The same applies to\n> > > > ColourCorrectionMatrix.\"?\n> > > >\n> > > > We can discuss the final wording after feedback from David.\n> > > >\n> > > > > > > > +\n> > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > + available, even if set manually.\n> > > > > > >\n> > > > > > > I'm not sure to understand this.\n> > > > > >\n> > > > > > This is based on the comment from Kieran:\n> > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > >\n> > > > > This means that whether or not the estimated colour temperature will be\n> > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > makes writing portable applications much more difficult.\n> > > > >\n> > > > > My other concern is that metadata is supposed to report the setting\n> > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > having multiple controls that ultimately set the same values is also\n> > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > higher-level controls that get translated to lower-level controls are\n> > > > > never reported in metadata ?\n> > > > >\n> > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > like it much. Should we have two different controls ?\n> > > >\n> > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > contains \"something\" as the statistics are always available. But in the\n> > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > valid colour temperature.\n> >\n> > If the algorithm failed to estimate the colour temperature, that should\n> > be indicated by the absence of an estimated colour temperature in\n> > metadata.\n> >\n> > David, why do you (if I understand correctly) stop estimating the colour\n> > temperature from the statistics in manual AWB mode ?\n> \n> I think it's to keep things consistent. The CT-curve in the tuning\n> file is taken as the canonical truth. So if manual CGs are applied,\n> and assuming the users know what they are doing(!), we return a simple\n> lookup without going through all the computations again. Of course,\n> this is simple enough to change in our code if the need arises.\n\nI'm fine with that behaviour, but I would like to keep the metadata\nbehaviour consistent between platforms.\n\nFor auto-mode, I think we all agree that the CG, CT and CCM metadata\nwill report the values computed by the AWB algorithm (please wave if\nyou disagree).\n\nFor manual mode, we've discussed two different behaviours that apply to\nCG, CT and CCM:\n\n- Report the controls that are applied to the device. This matches the\n standard libcamera metadata behaviour.\n\n- Let the AWB algorithm continue to process statistics, and report the\n estimated values. As far as I understand, this was requested for CT\n estimation only, not for CG or CCM estimation.\n\nWhile I understand that continuous CT estimation in manual mode can be\ntempting, I think it would require more discussions to specify it\nunambiguously. Stefan, Kieran, do you have a use case for this now, or\nis this something you considered as a nice-to-have feature ?\n\n> > > > > > > > +\n> > > > > > > > + \\sa AwbEnable\n> > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > + \\sa ColourGains\n> > > > > > > >\n> > > > > > > > - Saturation:\n> > > > > > > > type: float\n> > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > order in an array of 9 floating point values.\n> > > > > > > >\n> > > > > > > > + \\sa AwbEnable\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > > size: [3,3]\n> > > > > > > >\n> > > > > > > > - ScalerCrop:","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 E3A80BD80A\n\tfor ;\n\tMon, 9 Dec 2024 01:02:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C327367E3E;\n\tMon, 9 Dec 2024 02:02:26 +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 E9A96660E7\n\tfor ;\n\tMon, 9 Dec 2024 02:02:24 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6CE24752;\n\tMon, 9 Dec 2024 02:01:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GBeImonN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733706113;\n\tbh=WFFONzubUSt8I5e5y7qZwWnTMhooi4eZVV8pDGIkHHE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GBeImonNZw1s96K+Kn54gJO2K8Pji+dmEXiqtAwTc7yCIwG8QTZTZez7MaVQTGRXN\n\tTbPcS7Ppiq8uMgsoJlrsGmUorKdkAXBJ95ZuXfYs/jRjOy645jN7LnzSplwVXePjJT\n\t4xxwIhelylMHZ1QOwzOvsdQPPuaKmMp5rj/n573c=","Date":"Mon, 9 Dec 2024 03:02:09 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Cc":"David Plowman ,\n\tStefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20241209010209.GD6230@pendragon.ideasonboard.com>","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32613,"web_url":"https://patchwork.libcamera.org/comment/32613/","msgid":"<20241209014729.GA31303@pendragon.ideasonboard.com>","date":"2024-12-09T01:47:29","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Mon, Sep 02, 2024 at 10:32:59AM +0200, Stefan Klug wrote:\n> On Sat, Aug 31, 2024 at 06:16:10PM +0300, Laurent Pinchart wrote:\n> > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > >\n> > > > > > > > Document this and update the control dependencies.\n> > > > > > > >\n> > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > ---\n> > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > >\n> > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > - AwbEnable:\n> > > > > > > > type: bool\n> > > > > > > > description: |\n> > > > > > > > - Enable or disable the AWB.\n> > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > >\n> > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > below.\n> > > > > > >\n> > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > mean ?\n> > > > > > >\n> > > > > > > Enable or disable the AWB.\n> > > > > > >\n> > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > if set in a request.\n> > > > > > >\n> > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > requests.\n> > > > > >\n> > > > > > Looks good, I'll apply that.\n> > > > > >\n> > > > > > > >\n> > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > \\sa ColourGains\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > >\n> > > > > > > > # AwbMode needs further attention:\n> > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > disabled.\n> > > > > > > >\n> > > > > > > > \\sa AwbEnable\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > > size: [2]\n> > > > > > > >\n> > > > > > > > - ColourTemperature:\n> > > > > > > > type: int32_t\n> > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > - returned in metadata.\n> > > > > > > > + description: |\n> > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > >\n> > > > > > > s/also //\n> > > > > > >\n> > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > + specified at the same time, they take precedence.\n> > > > > > >\n> > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > temperature ?\n> > > > > >\n> > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > possible options:\n> > > > > >\n> > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > CT, CG, CCM -> CG, CCM\n> > > > >\n> > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > \n> > > I'm guessing the question here is really what to do if an application\n> > > sets both CT and CG together? The other cases seem fairly\n> > > uncontroversial (?).\n> > \n> > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > both calculated based on the colour temperature. The case where the\n> > application sets CT and CG is conceptually as problematic as the case\n> > where it sets CT and CCM. Stefan, is that correct ?\n> \n> Only CCM is set based on CT. CG is set based on the stats and these are\n> also used to calculate the CT. As David noted CG -> CT(CG) -> CCM(CT) is\n> possible. CT(CCM) is not possible afaik. So the cases are similar but\n> not the same.\n> \n> > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > \n> > When an application sets CG, does the algorithm estimate CT from the\n> > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > rkisp1 ?\n> \n> At the moment, we do not estimate CT from CG. But that might be worth to\n> do.\n> \n> > > When I add this new control, it will calculate CG from the calibrated\n> > > colour temperature curve in the camera tuning and use those, and pass\n> > > the CT to those other algorithms (so like the first row in the table).\n> > \n> > That's the main use, and I think we all agree on the behaviour. It seems\n> > quite uncontroversial.\n> \n> Yes, I think on that one we are all fine.\n> So CT -> CG(CT), CCM(CT) is accepted. Metadata might be an open point\n> here.\n> \n> > > I don't think I particularly want to implement cases where it takes a\n> > > CG value, uses them, and also a random CT which it then passes on?\n> > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > recommend setting one or other.\n> > \n> > I dislike implementation-dependent behaviours if we can avoid them, at\n> > least in cases that we consider valid use cases. Is there a valid use\n> > case for setting CT along with CG or CCM ? It sounds to me like there\n> > wouldn't be one for RPi. Stefan, how about you ?\n> \n> To me the most prominent use case is manual WB. So the user takes a\n> picture of a gray patch and the application (not libcamera) calculates\n> wb gains based on that. If you compare to a typical digital camera I\n> think one would expect the following to happen: CG -> CT(CG) -> CCM(CT)\n> \n> In rkisp1 we don't have that CG->CT(CG) step, but that doesn't mean we\n> shouldn't do it.\n\nTo make sure I understand you correctly, what you're proposing is\n\n- AWB disabled\n- Application sets CG\n- IPA computes CT(CG) and CCM(CG,CT), and applies CCM\n\nNote that I wrote CCM(CG,CT) to let implementations decide how to\ncompute CCM. Whether they use CG, CT or both is an implementation\ndetail. Statistics are *not* used to compute either CT or CCM, they are\ncompletely ignored.\n\nIs that right ? That seems easy to specify and implement, so I'm fine\nwith it. The use case with CG and CT swapped would be\n\n- AWB disabled\n- Application sets CT\n- IPA computes CG(CT) and CCM(CG,CT), and applies CCM\n\ncan easily be supported too, and the two options don't conflict with\neach other as they're user-selectable (by setting either CG or CT).\n\n> Looking at that with the eyes of a machine-vision user it would be\n> counter intuitive that the CCM suddenly changes when I change the CG.\n\nCould you elaborate on why that is the case ?\n\n> But that can be worked around from user side, by supplying a manual CCM\n> in the request. Which brings me to the next point. The CCM is the only\n> parameter here, that is basically impossible to handcraft in an\n> application. So there might be use cases where I'd like to set a CCM\n> based on a color temperature and then hand tweak the CG.\n\nTo do so, you would need to set the CG, wait until it gets applied,\nretrieved the CCM from the metadata, and then change the CG while\nspecifying the CCM in the same request. Is that what you have in mind ?\n\n> So I would be fine if we say CG -> CG, CCM(CT(CG)). That is new to me\n> and I haven't look into the implementation effort yet.\n>\n> CG, CT -> CG, CCM(CT) is something I'd like to have, but I don't know if\n> we need to force it (@David: would it be much effort on your side, just\n> for completeness sake?)\n\nHow would we document that ? We can't say anymore that CG takes\nprecedence over CT. This would require documenting precisely how the\ndifferent values are computed from each other in all cases, and would\nlimit options for IPA modules. I fear we may corner ourselves.\n\n> CCM, CT -> CG(CT), CCM also feels natural to me, but without any known\n> usecase. (Maybe in calibration, where you want to force a unit ccm\n> and check the correctness of the gains)\n\nI'm fine with this, we can document that CCM, when specified, overrides\nany computed value.\n\n> CT, CG, CCM -> CG, CCM I think we can agree on that one. It is\n> overspecified, but the lower level controls should win.\n\nYes, this is fine.\n\n> > > Does that help or just confuse further?\n> > \n> > Let's see how the discussion progresses :-) Thank you for your input.\n> > \n> > > > > > Do you have a nice sentence for that?\n> > > > >\n> > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > appropriate. Sorry :-)\n> > > >\n> > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > same time, ColourGains takes precedence. The same applies to\n> > > > ColourCorrectionMatrix.\"?\n> > > >\n> > > > We can discuss the final wording after feedback from David.\n> > > >\n> > > > > > > > +\n> > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > + available, even if set manually.\n> > > > > > >\n> > > > > > > I'm not sure to understand this.\n> > > > > >\n> > > > > > This is based on the comment from Kieran:\n> > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > >\n> > > > > This means that whether or not the estimated colour temperature will be\n> > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > makes writing portable applications much more difficult.\n> > > > >\n> > > > > My other concern is that metadata is supposed to report the setting\n> > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > having multiple controls that ultimately set the same values is also\n> > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > higher-level controls that get translated to lower-level controls are\n> > > > > never reported in metadata ?\n> > > > >\n> > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > like it much. Should we have two different controls ?\n> > > >\n> > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > contains \"something\" as the statistics are always available. But in the\n> > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > valid colour temperature.\n> > \n> > If the algorithm failed to estimate the colour temperature, that should\n> > be indicated by the absence of an estimated colour temperature in\n> > metadata.\n> > \n> > David, why do you (if I understand correctly) stop estimating the colour\n> > temperature from the statistics in manual AWB mode ?\n> > \n> > > > > > > > +\n> > > > > > > > + \\sa AwbEnable\n> > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > + \\sa ColourGains\n> > > > > > > >\n> > > > > > > > - Saturation:\n> > > > > > > > type: float\n> > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > order in an array of 9 floating point values.\n> > > > > > > >\n> > > > > > > > + \\sa AwbEnable\n> > > > > > > > + \\sa ColourTemperature\n> > > > > > > > size: [3,3]\n> > > > > > > >\n> > > > > > > > - ScalerCrop:","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 BFE1CBD80A\n\tfor ;\n\tMon, 9 Dec 2024 01:47:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7298E67E41;\n\tMon, 9 Dec 2024 02:47:48 +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 0AA99660E7\n\tfor ;\n\tMon, 9 Dec 2024 02:47:44 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 80580496;\n\tMon, 9 Dec 2024 02:47:13 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Mh/yRSmH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733708833;\n\tbh=ey0HewFzXdDjiq0QBloMRM8fizyQBtILDWbooNNQhLA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Mh/yRSmHzCK1Bfpomfk1C4MLUz2IjsyhaUtE0npjZpQ79VAnvG0vyXcq8FAFYJWys\n\tdFyVRGCUZthi8S981qvBrzFuWu4R17sVBAs/Gzcyxbjg9Jq7z7DlRw4zc+Bk7C9zgD\n\t2+KvGLRzxHgbNJE883tUJIT2k81pEGUhY5jQx37U=","Date":"Mon, 9 Dec 2024 03:47:29 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20241209014729.GA31303@pendragon.ideasonboard.com>","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32660,"web_url":"https://patchwork.libcamera.org/comment/32660/","msgid":"","date":"2024-12-10T15:34:11","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, Dec 09, 2024 at 03:02:09AM +0200, Laurent Pinchart wrote:\n> Looks like I forgot to reply to this.\n> \n> On Mon, Sep 02, 2024 at 08:28:24AM +0100, Naushir Patuck wrote:\n> > On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart wrote:\n> > > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > > >\n> > > > > > > > > Document this and update the control dependencies.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > > ---\n> > > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > > >\n> > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > > - AwbEnable:\n> > > > > > > > > type: bool\n> > > > > > > > > description: |\n> > > > > > > > > - Enable or disable the AWB.\n> > > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > > >\n> > > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > > below.\n> > > > > > > >\n> > > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > > mean ?\n> > > > > > > >\n> > > > > > > > Enable or disable the AWB.\n> > > > > > > >\n> > > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > > if set in a request.\n> > > > > > > >\n> > > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > > requests.\n> > > > > > >\n> > > > > > > Looks good, I'll apply that.\n> > > > > > >\n> > > > > > > > >\n> > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > \\sa ColourGains\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > >\n> > > > > > > > > # AwbMode needs further attention:\n> > > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > > disabled.\n> > > > > > > > >\n> > > > > > > > > \\sa AwbEnable\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > size: [2]\n> > > > > > > > >\n> > > > > > > > > - ColourTemperature:\n> > > > > > > > > type: int32_t\n> > > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > > - returned in metadata.\n> > > > > > > > > + description: |\n> > > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > > >\n> > > > > > > > s/also //\n> > > > > > > >\n> > > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > > + specified at the same time, they take precedence.\n> > > > > > > >\n> > > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > > temperature ?\n> > > > > > >\n> > > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > > possible options:\n> > > > > > >\n> > > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > > CT, CG, CCM -> CG, CCM\n> > > > > >\n> > > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > >\n> > > > I'm guessing the question here is really what to do if an application\n> > > > sets both CT and CG together? The other cases seem fairly\n> > > > uncontroversial (?).\n> > >\n> > > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > > both calculated based on the colour temperature. The case where the\n> > > application sets CT and CG is conceptually as problematic as the case\n> > > where it sets CT and CCM. Stefan, is that correct ?\n> > >\n> > > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > >\n> > > When an application sets CG, does the algorithm estimate CT from the\n> > > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > > rkisp1 ?\n> > \n> > When an application sets manual CGs, the algorithm estimates the CT\n> > from the CT-curve in the tuning directly.\n> \n> I'll try to recap what we have (hopefully) agreed on so far.\n> \n> OK, so in manual AWB mode, CT and CG are interchangeable, one will be\n> calculated from the other based on the tuning data, without taking\n> statistics into account. As this is a pure software implementation, if\n> both controls are specified, we can freely specify which one takes\n> precedence, and this can be applied to all platforms. I have a\n> preference for making the gains take precedence (as Stefan did in v5) as\n> they provide more precise control.\n\nAs written in the next paragraph, I propose for no precedence between CG\nand CT at all as we would unnecessarily limit our options.\n\n> \n> In auto AWB mode, CG and CT are both calculated by the AWB algorithm and\n> any value set through controls is ignored.\n> \n> So far, so good ?\n> \n> The next question is how to handle CCM. Auto mode is easy, CCM is\n> computed automatically, and any value set through controls is ignored.\n> In manual mode, if CCM is set in a request, its value should be applied.\n> What if a request contains CG or CT and no CCM ? Would CCM be computed\n> by the AWB algorithm (in manual mode), or its previous value retained ?\n> \n\nI prepared a new update to the controls documentation, that I'll post\nshortly. The proposal there is (for manual mode only):\n\n- CG updates CT if CT is not set.\n- CT updates CG if CG is not set.\n- CT updates CCM if CCM is not set (Which could be triggered by the CG ->\n CT -> CCM chain)\n\nThat seems to be easiest to explain at the moment captures most use\ncases.\n\n> > > > When I add this new control, it will calculate CG from the calibrated\n> > > > colour temperature curve in the camera tuning and use those, and pass\n> > > > the CT to those other algorithms (so like the first row in the table).\n> > >\n> > > That's the main use, and I think we all agree on the behaviour. It seems\n> > > quite uncontroversial.\n> > >\n> > > > I don't think I particularly want to implement cases where it takes a\n> > > > CG value, uses them, and also a random CT which it then passes on?\n> > > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > > recommend setting one or other.\n> > >\n> > > I dislike implementation-dependent behaviours if we can avoid them, at\n> > > least in cases that we consider valid use cases. Is there a valid use\n> > > case for setting CT along with CG or CCM ? It sounds to me like there\n> > > wouldn't be one for RPi. Stefan, how about you ?\n> > \n> > I don't think RPi has a use case that requires the CT and CGs to be both set.\n> > \n> > > > Does that help or just confuse further?\n> > >\n> > > Let's see how the discussion progresses :-) Thank you for your input.\n> > >\n> > > > > > > Do you have a nice sentence for that?\n> > > > > >\n> > > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > > appropriate. Sorry :-)\n> > > > >\n> > > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > > same time, ColourGains takes precedence. The same applies to\n> > > > > ColourCorrectionMatrix.\"?\n> > > > >\n> > > > > We can discuss the final wording after feedback from David.\n> > > > >\n> > > > > > > > > +\n> > > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > > + available, even if set manually.\n> > > > > > > >\n> > > > > > > > I'm not sure to understand this.\n> > > > > > >\n> > > > > > > This is based on the comment from Kieran:\n> > > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > > >\n> > > > > > This means that whether or not the estimated colour temperature will be\n> > > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > > makes writing portable applications much more difficult.\n> > > > > >\n> > > > > > My other concern is that metadata is supposed to report the setting\n> > > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > > having multiple controls that ultimately set the same values is also\n> > > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > > higher-level controls that get translated to lower-level controls are\n> > > > > > never reported in metadata ?\n> > > > > >\n> > > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > > like it much. Should we have two different controls ?\n> > > > >\n> > > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > > contains \"something\" as the statistics are always available. But in the\n> > > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > > valid colour temperature.\n> > >\n> > > If the algorithm failed to estimate the colour temperature, that should\n> > > be indicated by the absence of an estimated colour temperature in\n> > > metadata.\n> > >\n> > > David, why do you (if I understand correctly) stop estimating the colour\n> > > temperature from the statistics in manual AWB mode ?\n> > \n> > I think it's to keep things consistent. The CT-curve in the tuning\n> > file is taken as the canonical truth. So if manual CGs are applied,\n> > and assuming the users know what they are doing(!), we return a simple\n> > lookup without going through all the computations again. Of course,\n> > this is simple enough to change in our code if the need arises.\n> \n> I'm fine with that behaviour, but I would like to keep the metadata\n> behaviour consistent between platforms.\n> \n> For auto-mode, I think we all agree that the CG, CT and CCM metadata\n> will report the values computed by the AWB algorithm (please wave if\n> you disagree).\n> \n> For manual mode, we've discussed two different behaviours that apply to\n> CG, CT and CCM:\n> \n> - Report the controls that are applied to the device. This matches the\n> standard libcamera metadata behaviour.\n> \n> - Let the AWB algorithm continue to process statistics, and report the\n> estimated values. As far as I understand, this was requested for CT\n> estimation only, not for CG or CCM estimation.\n> \n> While I understand that continuous CT estimation in manual mode can be\n> tempting, I think it would require more discussions to specify it\n> unambiguously. Stefan, Kieran, do you have a use case for this now, or\n> is this something you considered as a nice-to-have feature ?\n\nThe discussion roughly went as follows:\n\nThere are valid cases for both colour temperatures. The one from the\nstatistics engine (measuredTemp) would be very useful to do any manual\nregulation (outside of libcamera). The one used in the ISP for\nprocessing the frame at hand (usedTemp) is useful to store in image\nmetadata or similar things. But for none of them there is a hard use\ncase.\n\nThe issue with usedTemp is that it is not well defined in manual mode.\nAt the time this was discussed we also had no way on rkisp1 to map from\nCG to CT. As you lean towards that value I propose the following:\n\nCT: usedTemp = CT, correct\nCG: usedTemp = CT(CG), correct (now possible on rkisp1 also)\nCT,CG: usedTemp = CT, incorrect because CG not taken into account but best\neffort\nCCM: usedTemp is unchanged, incorrect but best effort\n\nI think we can live with that incorrectness as it only affects corner\ncases.\n\nFor the measuredTemp we can easily introduce a MeasuredColourTemperature\nmetadata at a later stage.\n\nRegards,\nStefan\n\n> \n> > > > > > > > > +\n> > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > + \\sa ColourGains\n> > > > > > > > >\n> > > > > > > > > - Saturation:\n> > > > > > > > > type: float\n> > > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > > order in an array of 9 floating point values.\n> > > > > > > > >\n> > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > size: [3,3]\n> > > > > > > > >\n> > > > > > > > > - ScalerCrop:\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 671CBBD80A\n\tfor ;\n\tTue, 10 Dec 2024 15:34:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85DC367E8E;\n\tTue, 10 Dec 2024 16:34:15 +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 0595B618AF\n\tfor ;\n\tTue, 10 Dec 2024 16:34:14 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e00:d0ba:841f:f989])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8B9E16EC;\n\tTue, 10 Dec 2024 16:33:41 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hTAjjGP9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733844821;\n\tbh=UVEpeaHhfZahakpwx6IUGuVx29WpwCNUSJvdBashaJg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hTAjjGP9guPL8C2k7zUcyzFkcXkYsHRfdQhWRmAi0Fss1ziU25bGJRnHceXTH7GAt\n\tqV8ID9SdmTx6SWqRuKzJNUBn6yWA3n2jmQFsSmhs2ZHyeopHhHrE2CjxFrkpyY3AwF\n\t/4ix0GeowW79oNWO7Yc9JWrtVfLnQVPkZJJDxuP4=","Date":"Tue, 10 Dec 2024 16:34:11 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Naushir Patuck , \n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t\n\t<20241209010209.GD6230@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241209010209.GD6230@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32661,"web_url":"https://patchwork.libcamera.org/comment/32661/","msgid":"","date":"2024-12-10T16:00:29","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Mon, Dec 09, 2024 at 03:47:29AM +0200, Laurent Pinchart wrote:\n> On Mon, Sep 02, 2024 at 10:32:59AM +0200, Stefan Klug wrote:\n> > On Sat, Aug 31, 2024 at 06:16:10PM +0300, Laurent Pinchart wrote:\n> > > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > > >\n> > > > > > > > > Document this and update the control dependencies.\n> > > > > > > > >\n> > > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > > ---\n> > > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > > >\n> > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > > - AwbEnable:\n> > > > > > > > > type: bool\n> > > > > > > > > description: |\n> > > > > > > > > - Enable or disable the AWB.\n> > > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > > >\n> > > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > > below.\n> > > > > > > >\n> > > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > > mean ?\n> > > > > > > >\n> > > > > > > > Enable or disable the AWB.\n> > > > > > > >\n> > > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > > if set in a request.\n> > > > > > > >\n> > > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > > requests.\n> > > > > > >\n> > > > > > > Looks good, I'll apply that.\n> > > > > > >\n> > > > > > > > >\n> > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > \\sa ColourGains\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > >\n> > > > > > > > > # AwbMode needs further attention:\n> > > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > > disabled.\n> > > > > > > > >\n> > > > > > > > > \\sa AwbEnable\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > size: [2]\n> > > > > > > > >\n> > > > > > > > > - ColourTemperature:\n> > > > > > > > > type: int32_t\n> > > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > > - returned in metadata.\n> > > > > > > > > + description: |\n> > > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > > >\n> > > > > > > > s/also //\n> > > > > > > >\n> > > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > > + specified at the same time, they take precedence.\n> > > > > > > >\n> > > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > > temperature ?\n> > > > > > >\n> > > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > > possible options:\n> > > > > > >\n> > > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > > CT, CG, CCM -> CG, CCM\n> > > > > >\n> > > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > > \n> > > > I'm guessing the question here is really what to do if an application\n> > > > sets both CT and CG together? The other cases seem fairly\n> > > > uncontroversial (?).\n> > > \n> > > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > > both calculated based on the colour temperature. The case where the\n> > > application sets CT and CG is conceptually as problematic as the case\n> > > where it sets CT and CCM. Stefan, is that correct ?\n> > \n> > Only CCM is set based on CT. CG is set based on the stats and these are\n> > also used to calculate the CT. As David noted CG -> CT(CG) -> CCM(CT) is\n> > possible. CT(CCM) is not possible afaik. So the cases are similar but\n> > not the same.\n> > \n> > > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > > \n> > > When an application sets CG, does the algorithm estimate CT from the\n> > > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > > rkisp1 ?\n> > \n> > At the moment, we do not estimate CT from CG. But that might be worth to\n> > do.\n> > \n> > > > When I add this new control, it will calculate CG from the calibrated\n> > > > colour temperature curve in the camera tuning and use those, and pass\n> > > > the CT to those other algorithms (so like the first row in the table).\n> > > \n> > > That's the main use, and I think we all agree on the behaviour. It seems\n> > > quite uncontroversial.\n> > \n> > Yes, I think on that one we are all fine.\n> > So CT -> CG(CT), CCM(CT) is accepted. Metadata might be an open point\n> > here.\n> > \n> > > > I don't think I particularly want to implement cases where it takes a\n> > > > CG value, uses them, and also a random CT which it then passes on?\n> > > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > > recommend setting one or other.\n> > > \n> > > I dislike implementation-dependent behaviours if we can avoid them, at\n> > > least in cases that we consider valid use cases. Is there a valid use\n> > > case for setting CT along with CG or CCM ? It sounds to me like there\n> > > wouldn't be one for RPi. Stefan, how about you ?\n> > \n> > To me the most prominent use case is manual WB. So the user takes a\n> > picture of a gray patch and the application (not libcamera) calculates\n> > wb gains based on that. If you compare to a typical digital camera I\n> > think one would expect the following to happen: CG -> CT(CG) -> CCM(CT)\n> > \n> > In rkisp1 we don't have that CG->CT(CG) step, but that doesn't mean we\n> > shouldn't do it.\n> \n> To make sure I understand you correctly, what you're proposing is\n> \n> - AWB disabled\n> - Application sets CG\n> - IPA computes CT(CG) and CCM(CG,CT), and applies CCM\n> \n> Note that I wrote CCM(CG,CT) to let implementations decide how to\n> compute CCM. Whether they use CG, CT or both is an implementation\n> detail. Statistics are *not* used to compute either CT or CCM, they are\n> completely ignored.\n> \n> Is that right ? That seems easy to specify and implement, so I'm fine\n> with it. The use case with CG and CT swapped would be\n> \n> - AWB disabled\n> - Application sets CT\n> - IPA computes CG(CT) and CCM(CG,CT), and applies CCM\n> \n> can easily be supported too, and the two options don't conflict with\n> each other as they're user-selectable (by setting either CG or CT).\n> \n> > Looking at that with the eyes of a machine-vision user it would be\n> > counter intuitive that the CCM suddenly changes when I change the CG.\n> \n> Could you elaborate on why that is the case ?\n\nI guess it stems from the model of MV cameras. In SFNC there is no\nnotion of colour temperature at all, but colour gains and ccm. ccm for\ncolour space transform and cg for the white balance. Both can be\ndescribed using calibration and a colour temperature but are otherwise\ncompletely independent. As these cameras tend to have no ct estimation\nat all and only do grey world model white balance, this is also not\nneeded. So I'm used to loading a ccm for a given light source (in our\ncase by specifying a CT) and then adjust the cg to reproduce grays as\ngrays. But that is quit theoretical. \n(See also https://docs.baslerweb.com/light-source-preset )\n\n> \n> > But that can be worked around from user side, by supplying a manual CCM\n> > in the request. Which brings me to the next point. The CCM is the only\n> > parameter here, that is basically impossible to handcraft in an\n> > application. So there might be use cases where I'd like to set a CCM\n> > based on a color temperature and then hand tweak the CG.\n> \n> To do so, you would need to set the CG, wait until it gets applied,\n> retrieved the CCM from the metadata, and then change the CG while\n> specifying the CCM in the same request. Is that what you have in mind ?\n\nYes, that would be the workaround, but I don't see a strong argument why\nwe should cut the CT, CG -> CG, CCM(CT) case. \n\n> \n> > So I would be fine if we say CG -> CG, CCM(CT(CG)). That is new to me\n> > and I haven't look into the implementation effort yet.\n> >\n> > CG, CT -> CG, CCM(CT) is something I'd like to have, but I don't know if\n> > we need to force it (@David: would it be much effort on your side, just\n> > for completeness sake?)\n> \n> How would we document that ? We can't say anymore that CG takes\n> precedence over CT. This would require documenting precisely how the\n> different values are computed from each other in all cases, and would\n> limit options for IPA modules. I fear we may corner ourselves.\n\nThis overlaps with the other mails of the thread and is answered there.\nI'm going to post a proposal for that.\n\nRegards,\nStefan\n\n> \n> > CCM, CT -> CG(CT), CCM also feels natural to me, but without any known\n> > usecase. (Maybe in calibration, where you want to force a unit ccm\n> > and check the correctness of the gains)\n> \n> I'm fine with this, we can document that CCM, when specified, overrides\n> any computed value.\n> \n> > CT, CG, CCM -> CG, CCM I think we can agree on that one. It is\n> > overspecified, but the lower level controls should win.\n> \n> Yes, this is fine.\n> \n> > > > Does that help or just confuse further?\n> > > \n> > > Let's see how the discussion progresses :-) Thank you for your input.\n> > > \n> > > > > > > Do you have a nice sentence for that?\n> > > > > >\n> > > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > > appropriate. Sorry :-)\n> > > > >\n> > > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > > same time, ColourGains takes precedence. The same applies to\n> > > > > ColourCorrectionMatrix.\"?\n> > > > >\n> > > > > We can discuss the final wording after feedback from David.\n> > > > >\n> > > > > > > > > +\n> > > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > > + available, even if set manually.\n> > > > > > > >\n> > > > > > > > I'm not sure to understand this.\n> > > > > > >\n> > > > > > > This is based on the comment from Kieran:\n> > > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > > >\n> > > > > > This means that whether or not the estimated colour temperature will be\n> > > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > > makes writing portable applications much more difficult.\n> > > > > >\n> > > > > > My other concern is that metadata is supposed to report the setting\n> > > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > > having multiple controls that ultimately set the same values is also\n> > > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > > higher-level controls that get translated to lower-level controls are\n> > > > > > never reported in metadata ?\n> > > > > >\n> > > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > > like it much. Should we have two different controls ?\n> > > > >\n> > > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > > contains \"something\" as the statistics are always available. But in the\n> > > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > > valid colour temperature.\n> > > \n> > > If the algorithm failed to estimate the colour temperature, that should\n> > > be indicated by the absence of an estimated colour temperature in\n> > > metadata.\n> > > \n> > > David, why do you (if I understand correctly) stop estimating the colour\n> > > temperature from the statistics in manual AWB mode ?\n> > > \n> > > > > > > > > +\n> > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > + \\sa ColourGains\n> > > > > > > > >\n> > > > > > > > > - Saturation:\n> > > > > > > > > type: float\n> > > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > > order in an array of 9 floating point values.\n> > > > > > > > >\n> > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > size: [3,3]\n> > > > > > > > >\n> > > > > > > > > - ScalerCrop:\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 C0CE0BD80A\n\tfor ;\n\tTue, 10 Dec 2024 16:00:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2086A67E92;\n\tTue, 10 Dec 2024 17:00:34 +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 2D46A618AF\n\tfor ;\n\tTue, 10 Dec 2024 17:00:32 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e00:d0ba:841f:f989])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id BD41B352;\n\tTue, 10 Dec 2024 16:59:59 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"NgsbArWD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733846399;\n\tbh=+PB8uLFNKBNL/adIhuz9hIYm1Q3Fx5vKxwEXV6Vw2Hc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NgsbArWDELVCjzPePBhWOxekdTKfaPPaZnTcGkQrWlUXymGKIBsKvdBG1ScI7NIz3\n\tDulEhkmoNudqZRdmC4l/jnTp3OuZR5OjF8ayAaqsh8XE6uVnZgdo7vaf10PKQJD5qC\n\trKJIT5pfDoSVQ8FfuHwwLGMyqFD7FONVj6frG10g=","Date":"Tue, 10 Dec 2024 17:00:29 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"David Plowman , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-1-stefan.klug@ideasonboard.com>\n\t<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t\n\t<20241209014729.GA31303@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241209014729.GA31303@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32662,"web_url":"https://patchwork.libcamera.org/comment/32662/","msgid":"","date":"2024-12-10T16:14:30","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nmaybe it's easiest to post the updated documentation diff before posting\nthe v6. I believe the rest is easy...\n\ndiff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\nindex d45cf8e56187..2317515d6e8d 100644\n--- a/src/libcamera/control_ids_core.yaml\n+++ b/src/libcamera/control_ids_core.yaml\n@@ -283,7 +283,19 @@ controls:\n description: |\n Enable or disable the AWB.\n\n+ When AWB is enabled, the algorithm estimates the colour temperature of\n+ the scene and computes colour gains and the colour correction matrix\n+ automatically. The computed colour temperature, gains and correction\n+ matrix are reported in metadata. The corresponding controls are ignored\n+ if set in a request.\n+\n+ When AWB is disabled, the colour temperature, gains and correction\n+ matrix are not updated automatically and can be set manually in\n+ requests.\n+\n+ \\sa ColourCorrectionMatrix\n \\sa ColourGains\n+ \\sa ColourTemperature\n\n # AwbMode needs further attention:\n # - Auto-generate max enum value.\n@@ -338,17 +350,34 @@ controls:\n Pair of gain values for the Red and Blue colour channels, in that\n order.\n\n- ColourGains can only be applied in a Request when the AWB is disabled.\n+ ColourGains can only be applied in a Request when the AWB is disabled.\n+ If ColourGains is set in a request but ColourTemperature is not, the\n+ implementation shall calculate and set the ColourTemperature based on\n+ the ColourGains.\n\n \\sa AwbEnable\n+ \\sa ColourTemperature\n size: [2]\n\n - ColourTemperature:\n type: int32_t\n description: |\n- Report the estimate of the colour temperature for the frame, in kelvin.\n+ ColourTemperature of the frame, in kelvin.\n+\n+ ColourTemperature can only be applied in a Request when the AWB is\n+ disabled.\n+\n+ If ColourTemperature is set in a request but ColourGains is not, the\n+ implementation shall calculate and set the ColourGains based on the\n+ given ColourTemperature. If ColourTemperature is set but\n+ ColourCorrectionMarix is not, the ColourCorrectionMarix is updated based\n+ on the ColourTemperature.\n+\n+ The ColourTemperature used to process the frame is reported in metadata.\n\n- The ColourTemperature control can only be returned in metadata.\n+ \\sa AwbEnable\n+ \\sa ColourCorrectionMatrix\n+ \\sa ColourGains\n\n - Saturation:\n type: float\n@@ -405,6 +434,11 @@ controls:\n stored in conventional reading order in an array of 9 floating point\n values.\n\n+ ColourCorrectionMatrix can only be applied in a Request when the AWB is\n+ disabled.\n+\n+ \\sa AwbEnable\n+ \\sa ColourTemperature\n size: [3,3]\n\n - ScalerCrop:\n\n\nCheers,\nStefan\n\nOn Tue, Dec 10, 2024 at 04:34:11PM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> On Mon, Dec 09, 2024 at 03:02:09AM +0200, Laurent Pinchart wrote:\n> > Looks like I forgot to reply to this.\n> > \n> > On Mon, Sep 02, 2024 at 08:28:24AM +0100, Naushir Patuck wrote:\n> > > On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart wrote:\n> > > > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > > > >\n> > > > > > > > > > Document this and update the control dependencies.\n> > > > > > > > > >\n> > > > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > > > ---\n> > > > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > > > >\n> > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > > > - AwbEnable:\n> > > > > > > > > > type: bool\n> > > > > > > > > > description: |\n> > > > > > > > > > - Enable or disable the AWB.\n> > > > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > > > >\n> > > > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > > > below.\n> > > > > > > > >\n> > > > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > > > mean ?\n> > > > > > > > >\n> > > > > > > > > Enable or disable the AWB.\n> > > > > > > > >\n> > > > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > > > if set in a request.\n> > > > > > > > >\n> > > > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > > > requests.\n> > > > > > > >\n> > > > > > > > Looks good, I'll apply that.\n> > > > > > > >\n> > > > > > > > > >\n> > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > \\sa ColourGains\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > >\n> > > > > > > > > > # AwbMode needs further attention:\n> > > > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > > > disabled.\n> > > > > > > > > >\n> > > > > > > > > > \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > size: [2]\n> > > > > > > > > >\n> > > > > > > > > > - ColourTemperature:\n> > > > > > > > > > type: int32_t\n> > > > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > > > - returned in metadata.\n> > > > > > > > > > + description: |\n> > > > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > > > >\n> > > > > > > > > s/also //\n> > > > > > > > >\n> > > > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > > > + specified at the same time, they take precedence.\n> > > > > > > > >\n> > > > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > > > temperature ?\n> > > > > > > >\n> > > > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > > > possible options:\n> > > > > > > >\n> > > > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > > > CT, CG, CCM -> CG, CCM\n> > > > > > >\n> > > > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > > >\n> > > > > I'm guessing the question here is really what to do if an application\n> > > > > sets both CT and CG together? The other cases seem fairly\n> > > > > uncontroversial (?).\n> > > >\n> > > > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > > > both calculated based on the colour temperature. The case where the\n> > > > application sets CT and CG is conceptually as problematic as the case\n> > > > where it sets CT and CCM. Stefan, is that correct ?\n> > > >\n> > > > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > > >\n> > > > When an application sets CG, does the algorithm estimate CT from the\n> > > > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > > > rkisp1 ?\n> > > \n> > > When an application sets manual CGs, the algorithm estimates the CT\n> > > from the CT-curve in the tuning directly.\n> > \n> > I'll try to recap what we have (hopefully) agreed on so far.\n> > \n> > OK, so in manual AWB mode, CT and CG are interchangeable, one will be\n> > calculated from the other based on the tuning data, without taking\n> > statistics into account. As this is a pure software implementation, if\n> > both controls are specified, we can freely specify which one takes\n> > precedence, and this can be applied to all platforms. I have a\n> > preference for making the gains take precedence (as Stefan did in v5) as\n> > they provide more precise control.\n> \n> As written in the next paragraph, I propose for no precedence between CG\n> and CT at all as we would unnecessarily limit our options.\n> \n> > \n> > In auto AWB mode, CG and CT are both calculated by the AWB algorithm and\n> > any value set through controls is ignored.\n> > \n> > So far, so good ?\n> > \n> > The next question is how to handle CCM. Auto mode is easy, CCM is\n> > computed automatically, and any value set through controls is ignored.\n> > In manual mode, if CCM is set in a request, its value should be applied.\n> > What if a request contains CG or CT and no CCM ? Would CCM be computed\n> > by the AWB algorithm (in manual mode), or its previous value retained ?\n> > \n> \n> I prepared a new update to the controls documentation, that I'll post\n> shortly. The proposal there is (for manual mode only):\n> \n> - CG updates CT if CT is not set.\n> - CT updates CG if CG is not set.\n> - CT updates CCM if CCM is not set (Which could be triggered by the CG ->\n> CT -> CCM chain)\n> \n> That seems to be easiest to explain at the moment captures most use\n> cases.\n> \n> > > > > When I add this new control, it will calculate CG from the calibrated\n> > > > > colour temperature curve in the camera tuning and use those, and pass\n> > > > > the CT to those other algorithms (so like the first row in the table).\n> > > >\n> > > > That's the main use, and I think we all agree on the behaviour. It seems\n> > > > quite uncontroversial.\n> > > >\n> > > > > I don't think I particularly want to implement cases where it takes a\n> > > > > CG value, uses them, and also a random CT which it then passes on?\n> > > > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > > > recommend setting one or other.\n> > > >\n> > > > I dislike implementation-dependent behaviours if we can avoid them, at\n> > > > least in cases that we consider valid use cases. Is there a valid use\n> > > > case for setting CT along with CG or CCM ? It sounds to me like there\n> > > > wouldn't be one for RPi. Stefan, how about you ?\n> > > \n> > > I don't think RPi has a use case that requires the CT and CGs to be both set.\n> > > \n> > > > > Does that help or just confuse further?\n> > > >\n> > > > Let's see how the discussion progresses :-) Thank you for your input.\n> > > >\n> > > > > > > > Do you have a nice sentence for that?\n> > > > > > >\n> > > > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > > > appropriate. Sorry :-)\n> > > > > >\n> > > > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > > > same time, ColourGains takes precedence. The same applies to\n> > > > > > ColourCorrectionMatrix.\"?\n> > > > > >\n> > > > > > We can discuss the final wording after feedback from David.\n> > > > > >\n> > > > > > > > > > +\n> > > > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > > > + available, even if set manually.\n> > > > > > > > >\n> > > > > > > > > I'm not sure to understand this.\n> > > > > > > >\n> > > > > > > > This is based on the comment from Kieran:\n> > > > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > > > >\n> > > > > > > This means that whether or not the estimated colour temperature will be\n> > > > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > > > makes writing portable applications much more difficult.\n> > > > > > >\n> > > > > > > My other concern is that metadata is supposed to report the setting\n> > > > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > > > having multiple controls that ultimately set the same values is also\n> > > > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > > > higher-level controls that get translated to lower-level controls are\n> > > > > > > never reported in metadata ?\n> > > > > > >\n> > > > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > > > like it much. Should we have two different controls ?\n> > > > > >\n> > > > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > > > contains \"something\" as the statistics are always available. But in the\n> > > > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > > > valid colour temperature.\n> > > >\n> > > > If the algorithm failed to estimate the colour temperature, that should\n> > > > be indicated by the absence of an estimated colour temperature in\n> > > > metadata.\n> > > >\n> > > > David, why do you (if I understand correctly) stop estimating the colour\n> > > > temperature from the statistics in manual AWB mode ?\n> > > \n> > > I think it's to keep things consistent. The CT-curve in the tuning\n> > > file is taken as the canonical truth. So if manual CGs are applied,\n> > > and assuming the users know what they are doing(!), we return a simple\n> > > lookup without going through all the computations again. Of course,\n> > > this is simple enough to change in our code if the need arises.\n> > \n> > I'm fine with that behaviour, but I would like to keep the metadata\n> > behaviour consistent between platforms.\n> > \n> > For auto-mode, I think we all agree that the CG, CT and CCM metadata\n> > will report the values computed by the AWB algorithm (please wave if\n> > you disagree).\n> > \n> > For manual mode, we've discussed two different behaviours that apply to\n> > CG, CT and CCM:\n> > \n> > - Report the controls that are applied to the device. This matches the\n> > standard libcamera metadata behaviour.\n> > \n> > - Let the AWB algorithm continue to process statistics, and report the\n> > estimated values. As far as I understand, this was requested for CT\n> > estimation only, not for CG or CCM estimation.\n> > \n> > While I understand that continuous CT estimation in manual mode can be\n> > tempting, I think it would require more discussions to specify it\n> > unambiguously. Stefan, Kieran, do you have a use case for this now, or\n> > is this something you considered as a nice-to-have feature ?\n> \n> The discussion roughly went as follows:\n> \n> There are valid cases for both colour temperatures. The one from the\n> statistics engine (measuredTemp) would be very useful to do any manual\n> regulation (outside of libcamera). The one used in the ISP for\n> processing the frame at hand (usedTemp) is useful to store in image\n> metadata or similar things. But for none of them there is a hard use\n> case.\n> \n> The issue with usedTemp is that it is not well defined in manual mode.\n> At the time this was discussed we also had no way on rkisp1 to map from\n> CG to CT. As you lean towards that value I propose the following:\n> \n> CT: usedTemp = CT, correct\n> CG: usedTemp = CT(CG), correct (now possible on rkisp1 also)\n> CT,CG: usedTemp = CT, incorrect because CG not taken into account but best\n> effort\n> CCM: usedTemp is unchanged, incorrect but best effort\n> \n> I think we can live with that incorrectness as it only affects corner\n> cases.\n> \n> For the measuredTemp we can easily introduce a MeasuredColourTemperature\n> metadata at a later stage.\n> \n> Regards,\n> Stefan\n> \n> > \n> > > > > > > > > > +\n> > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > + \\sa ColourGains\n> > > > > > > > > >\n> > > > > > > > > > - Saturation:\n> > > > > > > > > > type: float\n> > > > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > > > order in an array of 9 floating point values.\n> > > > > > > > > >\n> > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > size: [3,3]\n> > > > > > > > > >\n> > > > > > > > > > - ScalerCrop:\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 A447FC3257\n\tfor ;\n\tTue, 10 Dec 2024 16:14:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8A8E467E94;\n\tTue, 10 Dec 2024 17:14:35 +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 8200B618AF\n\tfor ;\n\tTue, 10 Dec 2024 17:14:33 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:e00:d0ba:841f:f989])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EE127352;\n\tTue, 10 Dec 2024 17:14:00 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"IvHOM8P4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1733847241;\n\tbh=EharuDcudwYAtxZ+kkyYaAV+ND4Th98M/ThjRVau4/8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IvHOM8P4WHKnKWGzu5+oU4xjhv7WFUvin+9HS60rVGevVw5w2WUKsCFtfliBh5vNV\n\tUyEqwfPaKZnlqP7iCh1xp0/GSlA4I5H9d2x4zeSTLYIGZUZkrsi2WedOoyvLf+Av43\n\thFR/SJahuoe8zHBRGCfziGJmFrsqdOXpJ1KuzWqo=","Date":"Tue, 10 Dec 2024 17:14:30 +0100","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"Naushir Patuck , \n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"","References":"<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t\n\t<20241209010209.GD6230@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32864,"web_url":"https://patchwork.libcamera.org/comment/32864/","msgid":"<20241218035115.GB23470@pendragon.ideasonboard.com>","date":"2024-12-18T03:51:15","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nOn Tue, Dec 10, 2024 at 05:00:29PM +0100, Stefan Klug wrote:\n> On Mon, Dec 09, 2024 at 03:47:29AM +0200, Laurent Pinchart wrote:\n> > On Mon, Sep 02, 2024 at 10:32:59AM +0200, Stefan Klug wrote:\n> > > On Sat, Aug 31, 2024 at 06:16:10PM +0300, Laurent Pinchart wrote:\n> > > > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > > > >\n> > > > > > > > > > Document this and update the control dependencies.\n> > > > > > > > > >\n> > > > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > > > ---\n> > > > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > > > >\n> > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > > > - AwbEnable:\n> > > > > > > > > > type: bool\n> > > > > > > > > > description: |\n> > > > > > > > > > - Enable or disable the AWB.\n> > > > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > > > >\n> > > > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > > > below.\n> > > > > > > > >\n> > > > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > > > mean ?\n> > > > > > > > >\n> > > > > > > > > Enable or disable the AWB.\n> > > > > > > > >\n> > > > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > > > if set in a request.\n> > > > > > > > >\n> > > > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > > > requests.\n> > > > > > > >\n> > > > > > > > Looks good, I'll apply that.\n> > > > > > > >\n> > > > > > > > > >\n> > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > \\sa ColourGains\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > >\n> > > > > > > > > > # AwbMode needs further attention:\n> > > > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > > > disabled.\n> > > > > > > > > >\n> > > > > > > > > > \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > size: [2]\n> > > > > > > > > >\n> > > > > > > > > > - ColourTemperature:\n> > > > > > > > > > type: int32_t\n> > > > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > > > - returned in metadata.\n> > > > > > > > > > + description: |\n> > > > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > > > >\n> > > > > > > > > s/also //\n> > > > > > > > >\n> > > > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > > > + specified at the same time, they take precedence.\n> > > > > > > > >\n> > > > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > > > temperature ?\n> > > > > > > >\n> > > > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > > > possible options:\n> > > > > > > >\n> > > > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > > > CT, CG, CCM -> CG, CCM\n> > > > > > >\n> > > > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > > > \n> > > > > I'm guessing the question here is really what to do if an application\n> > > > > sets both CT and CG together? The other cases seem fairly\n> > > > > uncontroversial (?).\n> > > > \n> > > > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > > > both calculated based on the colour temperature. The case where the\n> > > > application sets CT and CG is conceptually as problematic as the case\n> > > > where it sets CT and CCM. Stefan, is that correct ?\n> > > \n> > > Only CCM is set based on CT. CG is set based on the stats and these are\n> > > also used to calculate the CT. As David noted CG -> CT(CG) -> CCM(CT) is\n> > > possible. CT(CCM) is not possible afaik. So the cases are similar but\n> > > not the same.\n> > > \n> > > > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > > > \n> > > > When an application sets CG, does the algorithm estimate CT from the\n> > > > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > > > rkisp1 ?\n> > > \n> > > At the moment, we do not estimate CT from CG. But that might be worth to\n> > > do.\n> > > \n> > > > > When I add this new control, it will calculate CG from the calibrated\n> > > > > colour temperature curve in the camera tuning and use those, and pass\n> > > > > the CT to those other algorithms (so like the first row in the table).\n> > > > \n> > > > That's the main use, and I think we all agree on the behaviour. It seems\n> > > > quite uncontroversial.\n> > > \n> > > Yes, I think on that one we are all fine.\n> > > So CT -> CG(CT), CCM(CT) is accepted. Metadata might be an open point\n> > > here.\n> > > \n> > > > > I don't think I particularly want to implement cases where it takes a\n> > > > > CG value, uses them, and also a random CT which it then passes on?\n> > > > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > > > recommend setting one or other.\n> > > > \n> > > > I dislike implementation-dependent behaviours if we can avoid them, at\n> > > > least in cases that we consider valid use cases. Is there a valid use\n> > > > case for setting CT along with CG or CCM ? It sounds to me like there\n> > > > wouldn't be one for RPi. Stefan, how about you ?\n> > > \n> > > To me the most prominent use case is manual WB. So the user takes a\n> > > picture of a gray patch and the application (not libcamera) calculates\n> > > wb gains based on that. If you compare to a typical digital camera I\n> > > think one would expect the following to happen: CG -> CT(CG) -> CCM(CT)\n> > > \n> > > In rkisp1 we don't have that CG->CT(CG) step, but that doesn't mean we\n> > > shouldn't do it.\n> > \n> > To make sure I understand you correctly, what you're proposing is\n> > \n> > - AWB disabled\n> > - Application sets CG\n> > - IPA computes CT(CG) and CCM(CG,CT), and applies CCM\n> > \n> > Note that I wrote CCM(CG,CT) to let implementations decide how to\n> > compute CCM. Whether they use CG, CT or both is an implementation\n> > detail. Statistics are *not* used to compute either CT or CCM, they are\n> > completely ignored.\n> > \n> > Is that right ? That seems easy to specify and implement, so I'm fine\n> > with it. The use case with CG and CT swapped would be\n> > \n> > - AWB disabled\n> > - Application sets CT\n> > - IPA computes CG(CT) and CCM(CG,CT), and applies CCM\n> > \n> > can easily be supported too, and the two options don't conflict with\n> > each other as they're user-selectable (by setting either CG or CT).\n> > \n> > > Looking at that with the eyes of a machine-vision user it would be\n> > > counter intuitive that the CCM suddenly changes when I change the CG.\n> > \n> > Could you elaborate on why that is the case ?\n> \n> I guess it stems from the model of MV cameras. In SFNC there is no\n> notion of colour temperature at all, but colour gains and ccm. ccm for\n> colour space transform and cg for the white balance. Both can be\n> described using calibration and a colour temperature but are otherwise\n> completely independent. As these cameras tend to have no ct estimation\n> at all and only do grey world model white balance, this is also not\n> needed. So I'm used to loading a ccm for a given light source (in our\n> case by specifying a CT) and then adjust the cg to reproduce grays as\n> grays. But that is quit theoretical. \n> (See also https://docs.baslerweb.com/light-source-preset )\n\nI see where that comes from, but it sounds like you'd like to have two\nmutually exclusive behaviours:\n\n- A greyworld, machine vision behaviour where there's not CT, and where\n CG and CCM are set manually and separately.\n\n- An automatic model where CCM is computed by AWB based on CT, and where\n you want AWB to estimate CT from CG.\n\nAsking for AWB to estimate CT from CG and set CCM accordingly, while at\nthe same asking for CCM not to be overridden, seems contradicting.\n\n> > > But that can be worked around from user side, by supplying a manual CCM\n> > > in the request. Which brings me to the next point. The CCM is the only\n> > > parameter here, that is basically impossible to handcraft in an\n> > > application. So there might be use cases where I'd like to set a CCM\n> > > based on a color temperature and then hand tweak the CG.\n> > \n> > To do so, you would need to set the CG, wait until it gets applied,\n> > retrieved the CCM from the metadata, and then change the CG while\n> > specifying the CCM in the same request. Is that what you have in mind ?\n> \n> Yes, that would be the workaround, but I don't see a strong argument why\n> we should cut the CT, CG -> CG, CCM(CT) case. \n\nI suppose we could. It's going to be fun to document, but we can give it\na try.\n\n> > > So I would be fine if we say CG -> CG, CCM(CT(CG)). That is new to me\n> > > and I haven't look into the implementation effort yet.\n> > >\n> > > CG, CT -> CG, CCM(CT) is something I'd like to have, but I don't know if\n> > > we need to force it (@David: would it be much effort on your side, just\n> > > for completeness sake?)\n> > \n> > How would we document that ? We can't say anymore that CG takes\n> > precedence over CT. This would require documenting precisely how the\n> > different values are computed from each other in all cases, and would\n> > limit options for IPA modules. I fear we may corner ourselves.\n> \n> This overlaps with the other mails of the thread and is answered there.\n> I'm going to post a proposal for that.\n> \n> > > CCM, CT -> CG(CT), CCM also feels natural to me, but without any known\n> > > usecase. (Maybe in calibration, where you want to force a unit ccm\n> > > and check the correctness of the gains)\n> > \n> > I'm fine with this, we can document that CCM, when specified, overrides\n> > any computed value.\n> > \n> > > CT, CG, CCM -> CG, CCM I think we can agree on that one. It is\n> > > overspecified, but the lower level controls should win.\n> > \n> > Yes, this is fine.\n> > \n> > > > > Does that help or just confuse further?\n> > > > \n> > > > Let's see how the discussion progresses :-) Thank you for your input.\n> > > > \n> > > > > > > > Do you have a nice sentence for that?\n> > > > > > >\n> > > > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > > > appropriate. Sorry :-)\n> > > > > >\n> > > > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > > > same time, ColourGains takes precedence. The same applies to\n> > > > > > ColourCorrectionMatrix.\"?\n> > > > > >\n> > > > > > We can discuss the final wording after feedback from David.\n> > > > > >\n> > > > > > > > > > +\n> > > > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > > > + available, even if set manually.\n> > > > > > > > >\n> > > > > > > > > I'm not sure to understand this.\n> > > > > > > >\n> > > > > > > > This is based on the comment from Kieran:\n> > > > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > > > >\n> > > > > > > This means that whether or not the estimated colour temperature will be\n> > > > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > > > makes writing portable applications much more difficult.\n> > > > > > >\n> > > > > > > My other concern is that metadata is supposed to report the setting\n> > > > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > > > having multiple controls that ultimately set the same values is also\n> > > > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > > > higher-level controls that get translated to lower-level controls are\n> > > > > > > never reported in metadata ?\n> > > > > > >\n> > > > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > > > like it much. Should we have two different controls ?\n> > > > > >\n> > > > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > > > contains \"something\" as the statistics are always available. But in the\n> > > > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > > > valid colour temperature.\n> > > > \n> > > > If the algorithm failed to estimate the colour temperature, that should\n> > > > be indicated by the absence of an estimated colour temperature in\n> > > > metadata.\n> > > > \n> > > > David, why do you (if I understand correctly) stop estimating the colour\n> > > > temperature from the statistics in manual AWB mode ?\n> > > > \n> > > > > > > > > > +\n> > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > + \\sa ColourGains\n> > > > > > > > > >\n> > > > > > > > > > - Saturation:\n> > > > > > > > > > type: float\n> > > > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > > > order in an array of 9 floating point values.\n> > > > > > > > > >\n> > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > size: [3,3]\n> > > > > > > > > >\n> > > > > > > > > > - ScalerCrop:","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 B2428C3304\n\tfor ;\n\tWed, 18 Dec 2024 03:51:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ECC626807B;\n\tWed, 18 Dec 2024 04:51:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 20F9361898\n\tfor ;\n\tWed, 18 Dec 2024 04:51:18 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F034514;\n\tWed, 18 Dec 2024 04:50:40 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YCDl380t\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734493840;\n\tbh=BWZ+Zqeeo57EOLsc/zgAADzrgztqgiVFGMWs0A+gIUA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YCDl380tgUVX6EBg1gpjXskxvzh3DxCSmpe3ONAsu/qRdYoW+xAwbbABiKVHcAlaF\n\tYCSi+cAG80ONKL2sQwtbgX0GJjWR2f5lX/ie3shRXus3iG9Zo2I5jlWGHjEnZl5ux2\n\tVlCzajqhmHNguWGkdGW6YAqzeFs8IlyRzcYEPS/s=","Date":"Wed, 18 Dec 2024 05:51:15 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"David Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20241218035115.GB23470@pendragon.ideasonboard.com>","References":"<20240813084451.44099-2-stefan.klug@ideasonboard.com>\n\t<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t\n\t<20241209014729.GA31303@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":32865,"web_url":"https://patchwork.libcamera.org/comment/32865/","msgid":"<20241218040951.GC23470@pendragon.ideasonboard.com>","date":"2024-12-18T04:09:51","subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Dec 10, 2024 at 05:14:30PM +0100, Stefan Klug wrote:\n> Hi Laurent,\n> \n> maybe it's easiest to post the updated documentation diff before posting\n> the v6. I believe the rest is easy...\n> \n> diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> index d45cf8e56187..2317515d6e8d 100644\n> --- a/src/libcamera/control_ids_core.yaml\n> +++ b/src/libcamera/control_ids_core.yaml\n> @@ -283,7 +283,19 @@ controls:\n> description: |\n> Enable or disable the AWB.\n> \n> + When AWB is enabled, the algorithm estimates the colour temperature of\n> + the scene and computes colour gains and the colour correction matrix\n> + automatically. The computed colour temperature, gains and correction\n> + matrix are reported in metadata. The corresponding controls are ignored\n> + if set in a request.\n> +\n> + When AWB is disabled, the colour temperature, gains and correction\n> + matrix are not updated automatically and can be set manually in\n> + requests.\n> +\n> + \\sa ColourCorrectionMatrix\n> \\sa ColourGains\n> + \\sa ColourTemperature\n> \n> # AwbMode needs further attention:\n> # - Auto-generate max enum value.\n> @@ -338,17 +350,34 @@ controls:\n> Pair of gain values for the Red and Blue colour channels, in that\n> order.\n> \n> - ColourGains can only be applied in a Request when the AWB is disabled.\n> + ColourGains can only be applied in a Request when the AWB is disabled.\n> + If ColourGains is set in a request but ColourTemperature is not, the\n> + implementation shall calculate and set the ColourTemperature based on\n> + the ColourGains.\n> \n> \\sa AwbEnable\n> + \\sa ColourTemperature\n> size: [2]\n> \n> - ColourTemperature:\n> type: int32_t\n> description: |\n> - Report the estimate of the colour temperature for the frame, in kelvin.\n> + ColourTemperature of the frame, in kelvin.\n> +\n> + ColourTemperature can only be applied in a Request when the AWB is\n> + disabled.\n> +\n> + If ColourTemperature is set in a request but ColourGains is not, the\n> + implementation shall calculate and set the ColourGains based on the\n> + given ColourTemperature. If ColourTemperature is set but\n> + ColourCorrectionMarix is not, the ColourCorrectionMarix is updated based\n> + on the ColourTemperature.\n\n If ColourTemperature is set (either directly, or indirectly by setting\n\tColourGains) but ColourCorrectionMatrix is not, the\n\tColourCorrectionMatrix is updated based on the ColourTemperature.\n\n> +\n> + The ColourTemperature used to process the frame is reported in metadata.\n> \n> - The ColourTemperature control can only be returned in metadata.\n> + \\sa AwbEnable\n> + \\sa ColourCorrectionMatrix\n> + \\sa ColourGains\n> \n> - Saturation:\n> type: float\n> @@ -405,6 +434,11 @@ controls:\n> stored in conventional reading order in an array of 9 floating point\n> values.\n> \n> + ColourCorrectionMatrix can only be applied in a Request when the AWB is\n> + disabled.\n> +\n> + \\sa AwbEnable\n> + \\sa ColourTemperature\n\nWe can give this a try.\n\n> size: [3,3]\n> \n> - ScalerCrop:\n> \n> On Tue, Dec 10, 2024 at 04:34:11PM +0100, Stefan Klug wrote:\n> > On Mon, Dec 09, 2024 at 03:02:09AM +0200, Laurent Pinchart wrote:\n> > > On Mon, Sep 02, 2024 at 08:28:24AM +0100, Naushir Patuck wrote:\n> > > > On Sat, 31 Aug 2024 at 16:16, Laurent Pinchart wrote:\n> > > > > On Fri, Aug 30, 2024 at 04:31:50PM +0100, David Plowman wrote:\n> > > > > > On Fri, 30 Aug 2024 at 12:21, Stefan Klug wrote:\n> > > > > > > On Fri, Aug 30, 2024 at 01:15:55PM +0300, Laurent Pinchart wrote:\n> > > > > > > > On Fri, Aug 30, 2024 at 07:58:31AM +0200, Stefan Klug wrote:\n> > > > > > > > > On Fri, Aug 30, 2024 at 02:01:19AM +0300, Laurent Pinchart wrote:\n> > > > > > > > > > On Tue, Aug 13, 2024 at 10:44:18AM +0200, Stefan Klug wrote:\n> > > > > > > > > > > For manual control it is helpful to be able to specify a fixed colour\n> > > > > > > > > > > temperature. It also provides an easy way to apply the temperature\n> > > > > > > > > > > specific CCMs and colour gains that are contained in the tuning files.\n> > > > > > > > > > >\n> > > > > > > > > > > Document this and update the control dependencies.\n> > > > > > > > > > >\n> > > > > > > > > > > Signed-off-by: Stefan Klug \n> > > > > > > > > > > ---\n> > > > > > > > > > > src/libcamera/control_ids_core.yaml | 24 ++++++++++++++++++++----\n> > > > > > > > > > > 1 file changed, 20 insertions(+), 4 deletions(-)\n> > > > > > > > > > >\n> > > > > > > > > > > diff --git a/src/libcamera/control_ids_core.yaml b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > > index cf40771d3896..04dcc4af67fc 100644\n> > > > > > > > > > > --- a/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > > +++ b/src/libcamera/control_ids_core.yaml\n> > > > > > > > > > > @@ -252,9 +252,12 @@ controls:\n> > > > > > > > > > > - AwbEnable:\n> > > > > > > > > > > type: bool\n> > > > > > > > > > > description: |\n> > > > > > > > > > > - Enable or disable the AWB.\n> > > > > > > > > > > + Enable or disable the AWB. Disabling AWB stops updates to the\n> > > > > > > > > > > + ColourGains and to the ColourCorrectionMatrix.\n> > > > > > > > > >\n> > > > > > > > > > Please split this in two paragraphs. The first paragraph is translated\n> > > > > > > > > > to a \\brief doxygen command, and should be a single sentence. Same\n> > > > > > > > > > below.\n> > > > > > > > > >\n> > > > > > > > > > I would like to clarify this a bit. Here's an attempt, is it what you\n> > > > > > > > > > mean ?\n> > > > > > > > > >\n> > > > > > > > > > Enable or disable the AWB.\n> > > > > > > > > >\n> > > > > > > > > > When AWB is enabled, the algorithm estimates the colour temperature of\n> > > > > > > > > > the scene, and computes colour gains and the colour correction matrix\n> > > > > > > > > > automatically. The computed colour temperate, gains and correction\n> > > > > > > > > > matrix are reported in metadata. The corresponding controls are ignored\n> > > > > > > > > > if set in a request.\n> > > > > > > > > >\n> > > > > > > > > > When AWB is disabled, the colour temperature, gains and correction\n> > > > > > > > > > matrix are not updated automatically and can be set manually in\n> > > > > > > > > > requests.\n> > > > > > > > >\n> > > > > > > > > Looks good, I'll apply that.\n> > > > > > > > >\n> > > > > > > > > > >\n> > > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > > \\sa ColourGains\n> > > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > >\n> > > > > > > > > > > # AwbMode needs further attention:\n> > > > > > > > > > > # - Auto-generate max enum value.\n> > > > > > > > > > > @@ -309,13 +312,24 @@ controls:\n> > > > > > > > > > > disabled.\n> > > > > > > > > > >\n> > > > > > > > > > > \\sa AwbEnable\n> > > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > > size: [2]\n> > > > > > > > > > >\n> > > > > > > > > > > - ColourTemperature:\n> > > > > > > > > > > type: int32_t\n> > > > > > > > > > > - description: Report the current estimate of the colour temperature, in\n> > > > > > > > > > > - kelvin, for this frame. The ColourTemperature control can only be\n> > > > > > > > > > > - returned in metadata.\n> > > > > > > > > > > + description: |\n> > > > > > > > > > > + Report the current estimate of the colour temperature, in kelvin, for\n> > > > > > > > > > > + this frame. An implementation may also allow this control to be set when\n> > > > > > > > > >\n> > > > > > > > > > s/also //\n> > > > > > > > > >\n> > > > > > > > > > > + AWB is disabled. In that case ColourGains and the ColourCorrectionMatrix\n> > > > > > > > > > > + get set accordingly. If either ColourGains or ColourCorrectionMatrix are\n> > > > > > > > > > > + specified at the same time, they take precedence.\n> > > > > > > > > >\n> > > > > > > > > > maybe \"they take precedence, and the ColourTemperature is ignored\" ? Or,\n> > > > > > > > > > if an application sets ColourTemperature and ColourGains but not\n> > > > > > > > > > ColourCorrectionMatrix, do you expect the implementation to apply the\n> > > > > > > > > > requested ColourGains and set ColourCorrectionMatrix based on the\n> > > > > > > > > > temperature ?\n> > > > > > > > >\n> > > > > > > > > The latter is exactly what happens. The following table lists the\n> > > > > > > > > possible options:\n> > > > > > > > >\n> > > > > > > > > CT -> CG(CT), CCM(CT)\n> > > > > > > > > CT, CG -> CG, CCM(CT)\n> > > > > > > > > CT, CCM -> CG(CT), CCM\n> > > > > > > > > CT, CG, CCM -> CG, CCM\n> > > > > > > >\n> > > > > > > > David, is this the behaviour you would also expect for Raspberry Pi ?\n> > > > > >\n> > > > > > I'm guessing the question here is really what to do if an application\n> > > > > > sets both CT and CG together? The other cases seem fairly\n> > > > > > uncontroversial (?).\n> > > > >\n> > > > > As far as I understand, for the rkisp1, the colour gains and the CCM are\n> > > > > both calculated based on the colour temperature. The case where the\n> > > > > application sets CT and CG is conceptually as problematic as the case\n> > > > > where it sets CT and CCM. Stefan, is that correct ?\n> > > > >\n> > > > > > For us, I'm not sure it really makes much sense to supply both CT and\n> > > > > > CG at once. When someone sets CG, the algorithm estimates the CT and\n> > > > > > passes this to other algorithms, like lens shading and CCM. (So we\n> > > > > > actually have a \"CG -> CG, CT(CG), CCM(CT)\" kind of case going on.)\n> > > > >\n> > > > > When an application sets CG, does the algorithm estimate CT from the\n> > > > > stats, or from the CG alone for RPi ? Stefan, how about your plans for\n> > > > > rkisp1 ?\n> > > > \n> > > > When an application sets manual CGs, the algorithm estimates the CT\n> > > > from the CT-curve in the tuning directly.\n> > > \n> > > I'll try to recap what we have (hopefully) agreed on so far.\n> > > \n> > > OK, so in manual AWB mode, CT and CG are interchangeable, one will be\n> > > calculated from the other based on the tuning data, without taking\n> > > statistics into account. As this is a pure software implementation, if\n> > > both controls are specified, we can freely specify which one takes\n> > > precedence, and this can be applied to all platforms. I have a\n> > > preference for making the gains take precedence (as Stefan did in v5) as\n> > > they provide more precise control.\n> > \n> > As written in the next paragraph, I propose for no precedence between CG\n> > and CT at all as we would unnecessarily limit our options.\n> > \n> > > In auto AWB mode, CG and CT are both calculated by the AWB algorithm and\n> > > any value set through controls is ignored.\n> > > \n> > > So far, so good ?\n> > > \n> > > The next question is how to handle CCM. Auto mode is easy, CCM is\n> > > computed automatically, and any value set through controls is ignored.\n> > > In manual mode, if CCM is set in a request, its value should be applied.\n> > > What if a request contains CG or CT and no CCM ? Would CCM be computed\n> > > by the AWB algorithm (in manual mode), or its previous value retained ?\n> > \n> > I prepared a new update to the controls documentation, that I'll post\n> > shortly. The proposal there is (for manual mode only):\n> > \n> > - CG updates CT if CT is not set.\n> > - CT updates CG if CG is not set.\n> > - CT updates CCM if CCM is not set (Which could be triggered by the CG ->\n> > CT -> CCM chain)\n> > \n> > That seems to be easiest to explain at the moment captures most use\n> > cases.\n> > \n> > > > > > When I add this new control, it will calculate CG from the calibrated\n> > > > > > colour temperature curve in the camera tuning and use those, and pass\n> > > > > > the CT to those other algorithms (so like the first row in the table).\n> > > > >\n> > > > > That's the main use, and I think we all agree on the behaviour. It seems\n> > > > > quite uncontroversial.\n> > > > >\n> > > > > > I don't think I particularly want to implement cases where it takes a\n> > > > > > CG value, uses them, and also a random CT which it then passes on?\n> > > > > > Possibly I'd rather leave these cases (where we have both CT and CG on\n> > > > > > the LHS) as either \"implementation dependent\" or just \"undefined\", and\n> > > > > > recommend setting one or other.\n> > > > >\n> > > > > I dislike implementation-dependent behaviours if we can avoid them, at\n> > > > > least in cases that we consider valid use cases. Is there a valid use\n> > > > > case for setting CT along with CG or CCM ? It sounds to me like there\n> > > > > wouldn't be one for RPi. Stefan, how about you ?\n> > > > \n> > > > I don't think RPi has a use case that requires the CT and CGs to be both set.\n> > > > \n> > > > > > Does that help or just confuse further?\n> > > > >\n> > > > > Let's see how the discussion progresses :-) Thank you for your input.\n> > > > >\n> > > > > > > > > Do you have a nice sentence for that?\n> > > > > > > >\n> > > > > > > > /me checks... I have a set of French proverbs, but none of the seem very\n> > > > > > > > appropriate. Sorry :-)\n> > > > > > >\n> > > > > > > What about \"If ColourGains and ColourTemperature are specified at the\n> > > > > > > same time, ColourGains takes precedence. The same applies to\n> > > > > > > ColourCorrectionMatrix.\"?\n> > > > > > >\n> > > > > > > We can discuss the final wording after feedback from David.\n> > > > > > >\n> > > > > > > > > > > +\n> > > > > > > > > > > + The metadata will only report measured colour temperature values when\n> > > > > > > > > > > + available, even if set manually.\n> > > > > > > > > >\n> > > > > > > > > > I'm not sure to understand this.\n> > > > > > > > >\n> > > > > > > > > This is based on the comment from Kieran:\n> > > > > > > > > https://patchwork.libcamera.org/patch/20771/#30590 He wanted to prepare\n> > > > > > > > > for cases (RPi) where no temperature gets estimated. Only measurements\n> > > > > > > > > are returned in metadata. Manually set temperature is not reflected in\n> > > > > > > > > the metadata. This has the nice side effect, that you can set\n> > > > > > > > > AwbEnable=false, and still get temperature estimations in the metadata.\n> > > > > > > >\n> > > > > > > > This means that whether or not the estimated colour temperature will be\n> > > > > > > > returned in metadata will be platform-dependent. Can we avoid that ? It\n> > > > > > > > makes writing portable applications much more difficult.\n> > > > > > > >\n> > > > > > > > My other concern is that metadata is supposed to report the setting\n> > > > > > > > applied to a frame. The general rule is that, if a control can be set,\n> > > > > > > > the value that has been set for a frame is reported in metadata. There\n> > > > > > > > are exception for trigger-like controls. As this example clearly shows,\n> > > > > > > > having multiple controls that ultimately set the same values is also\n> > > > > > > > problematic from this point of view. Do we need to set a rule that\n> > > > > > > > higher-level controls that get translated to lower-level controls are\n> > > > > > > > never reported in metadata ?\n> > > > > > > >\n> > > > > > > > If we do that, then we'll have ColourTemperature as a control being\n> > > > > > > > defined differently from ColourTemperature as metadata. I'm not sure I\n> > > > > > > > like it much. Should we have two different controls ?\n> > > > > > >\n> > > > > > > We could split that to \"MeasuredColourTemperature\" and\n> > > > > > > \"AppliedColorTemperature\". But there are always cases where one of them\n> > > > > > > (or both) is not available (as discussed on Patch 3). I think on rkisp\n> > > > > > > we could ensure that MeasuredColourTemperature is always available and\n> > > > > > > contains \"something\" as the statistics are always available. But in the\n> > > > > > > end as a user I'd prefer to know when the algorithm failed to deduce a\n> > > > > > > valid colour temperature.\n> > > > >\n> > > > > If the algorithm failed to estimate the colour temperature, that should\n> > > > > be indicated by the absence of an estimated colour temperature in\n> > > > > metadata.\n> > > > >\n> > > > > David, why do you (if I understand correctly) stop estimating the colour\n> > > > > temperature from the statistics in manual AWB mode ?\n> > > > \n> > > > I think it's to keep things consistent. The CT-curve in the tuning\n> > > > file is taken as the canonical truth. So if manual CGs are applied,\n> > > > and assuming the users know what they are doing(!), we return a simple\n> > > > lookup without going through all the computations again. Of course,\n> > > > this is simple enough to change in our code if the need arises.\n> > > \n> > > I'm fine with that behaviour, but I would like to keep the metadata\n> > > behaviour consistent between platforms.\n> > > \n> > > For auto-mode, I think we all agree that the CG, CT and CCM metadata\n> > > will report the values computed by the AWB algorithm (please wave if\n> > > you disagree).\n> > > \n> > > For manual mode, we've discussed two different behaviours that apply to\n> > > CG, CT and CCM:\n> > > \n> > > - Report the controls that are applied to the device. This matches the\n> > > standard libcamera metadata behaviour.\n> > > \n> > > - Let the AWB algorithm continue to process statistics, and report the\n> > > estimated values. As far as I understand, this was requested for CT\n> > > estimation only, not for CG or CCM estimation.\n> > > \n> > > While I understand that continuous CT estimation in manual mode can be\n> > > tempting, I think it would require more discussions to specify it\n> > > unambiguously. Stefan, Kieran, do you have a use case for this now, or\n> > > is this something you considered as a nice-to-have feature ?\n> > \n> > The discussion roughly went as follows:\n> > \n> > There are valid cases for both colour temperatures. The one from the\n> > statistics engine (measuredTemp) would be very useful to do any manual\n> > regulation (outside of libcamera). The one used in the ISP for\n> > processing the frame at hand (usedTemp) is useful to store in image\n> > metadata or similar things. But for none of them there is a hard use\n> > case.\n> > \n> > The issue with usedTemp is that it is not well defined in manual mode.\n> > At the time this was discussed we also had no way on rkisp1 to map from\n> > CG to CT. As you lean towards that value I propose the following:\n> > \n> > CT: usedTemp = CT, correct\n> > CG: usedTemp = CT(CG), correct (now possible on rkisp1 also)\n> > CT,CG: usedTemp = CT, incorrect because CG not taken into account but best\n> > effort\n> > CCM: usedTemp is unchanged, incorrect but best effort\n> > \n> > I think we can live with that incorrectness as it only affects corner\n> > cases.\n> > \n> > For the measuredTemp we can easily introduce a MeasuredColourTemperature\n> > metadata at a later stage.\n> > \n> > > > > > > > > > > +\n> > > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > > + \\sa ColourCorrectionMatrix\n> > > > > > > > > > > + \\sa ColourGains\n> > > > > > > > > > >\n> > > > > > > > > > > - Saturation:\n> > > > > > > > > > > type: float\n> > > > > > > > > > > @@ -365,6 +379,8 @@ controls:\n> > > > > > > > > > > transformation. The 3x3 matrix is stored in conventional reading\n> > > > > > > > > > > order in an array of 9 floating point values.\n> > > > > > > > > > >\n> > > > > > > > > > > + \\sa AwbEnable\n> > > > > > > > > > > + \\sa ColourTemperature\n> > > > > > > > > > > size: [3,3]\n> > > > > > > > > > >\n> > > > > > > > > > > - ScalerCrop:","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 60AA9C3301\n\tfor ;\n\tWed, 18 Dec 2024 04:09:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8EEF86807B;\n\tWed, 18 Dec 2024 05:09:55 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9E00861898\n\tfor ;\n\tWed, 18 Dec 2024 05:09:54 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B026F670;\n\tWed, 18 Dec 2024 05:09:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hL7soA2y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1734494956;\n\tbh=qun6PTE2T7pk9H3cd3raadqPP2t253MB6CRSDFJg/MY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hL7soA2yKGDXKijIqMbAclJ1SH7neFozz10rjsCSqKGEaCbWQ6C25jCW6QAdycISc\n\tlelFyQ9X5XtDfLhbGaib0EkfTrt0GiBNp1mC2JHYXKDexsYVax8B9n/nMyIfOxHX5z\n\thTil+39JMfIdmp1/bk7JISFyDg6gjf3v8Hemb2V8=","Date":"Wed, 18 Dec 2024 06:09:51 +0200","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"Naushir Patuck ,\n\tDavid Plowman ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v4 1/6] libcamera: controls: Update the ColourTemperature\n\tcontrol to be writable","Message-ID":"<20241218040951.GC23470@pendragon.ideasonboard.com>","References":"<20240829230119.GA25163@pendragon.ideasonboard.com>\n\t\n\t<20240830101555.GG25163@pendragon.ideasonboard.com>\n\t\n\t\n\t<20240831151610.GX3811@pendragon.ideasonboard.com>\n\t\n\t<20241209010209.GD6230@pendragon.ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]