[{"id":21022,"web_url":"https://patchwork.libcamera.org/comment/21022/","msgid":"<163732026902.1089182.9908600671592660146@Monstersaurus>","date":"2021-11-19T11:11:09","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n> We can have the case where the gain is very close to 1, as coded in\n> double, and after gainCode() returns, the value as an uint32_t is 0. In\n> order to avoid that, always round the value halfway cases away from zero\n> before returning.\n> \n> Signed-off-by: Jean-Michel Hautbois \n\nThis seems to make sense.\n\nAre there any issues with other values being rounded? I presume not.\nIs there any requirement to further protect or warn if gainCode is\nreturned as zero? or can that still be valid in some situations?\n\n\nReviewed-by: Kieran Bingham \n\n> ---\n> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> index 0b0eb503..67cf6913 100644\n> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> @@ -7,6 +7,8 @@\n> */\n> #include \"camera_sensor_helper.h\"\n> \n> +#include \n> +\n> #include \n> \n> /**\n> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> \n> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n> }\n> \n> /**\n> -- \n> 2.32.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 A4961BDB13\n\tfor ;\n\tFri, 19 Nov 2021 11:11:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 587B560378;\n\tFri, 19 Nov 2021 12:11:13 +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 640B9600B5\n\tfor ;\n\tFri, 19 Nov 2021 12:11:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 15CD01974;\n\tFri, 19 Nov 2021 12:11:11 +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=\"I7uk7CtV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637320271;\n\tbh=yvsDVjkEd8sJWN+/B3b4+HoTEwQgxaQW6BUjCHBLWUY=;\n\th=In-Reply-To:References:Subject:From:To:Date:From;\n\tb=I7uk7CtVrL9ls/akkfzxChUTHWNzLn3ISy24q8ez5Oc1ZmUEVdKigJ7283n9FitLw\n\tyKFndfuwmq458pkAbYOmy3nOv4LhgwUF7P8fl44MHwwnrrrmxxRyEB3hWx6JAe7nEg\n\t9K369+/TO9/XFs/aqIua6lNJR/4AYH4LrDyMyKCU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>","From":"Kieran Bingham ","To":"Jean-Michel Hautbois ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 19 Nov 2021 11:11:09 +0000","Message-ID":"<163732026902.1089182.9908600671592660146@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","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":21023,"web_url":"https://patchwork.libcamera.org/comment/21023/","msgid":"<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>","date":"2021-11-19T11:14:20","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 19/11/2021 12:11, Kieran Bingham wrote:\n> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n>> We can have the case where the gain is very close to 1, as coded in\n>> double, and after gainCode() returns, the value as an uint32_t is 0. In\n>> order to avoid that, always round the value halfway cases away from zero\n>> before returning.\n>>\n>> Signed-off-by: Jean-Michel Hautbois \n> \n> This seems to make sense.\n> \n> Are there any issues with other values being rounded? I presume not.\n> Is there any requirement to further protect or warn if gainCode is\n> returned as zero? or can that still be valid in some situations?\n\nThat is a tough question... Can we have a gainCode set to 0 in a linear \nanalogue gain controlled sensor... I am not sure of that at all...\n\nBTW, investigating it (before introducing std::round) I found that using \nfloat instead of double made it return 1...\n\n> \n> \n> Reviewed-by: Kieran Bingham \n> \n>> ---\n>> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>> index 0b0eb503..67cf6913 100644\n>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>> @@ -7,6 +7,8 @@\n>> */\n>> #include \"camera_sensor_helper.h\"\n>> \n>> +#include \n>> +\n>> #include \n>> \n>> /**\n>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n>> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n>> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>> \n>> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n>> }\n>> \n>> /**\n>> -- \n>> 2.32.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 76F1DBF415\n\tfor ;\n\tFri, 19 Nov 2021 11:14:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 60D8D60371;\n\tFri, 19 Nov 2021 12:14:25 +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 9D958600B5\n\tfor ;\n\tFri, 19 Nov 2021 12:14:23 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:db30:8e54:a96:9838] (unknown\n\t[IPv6:2a01:e0a:169:7140:db30:8e54:a96:9838])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4588B1C19;\n\tFri, 19 Nov 2021 12:14:23 +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=\"bY8OHRZ0\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637320463;\n\tbh=YufUGWqdV/YdGlf50JzUUu/T4UQaV2VljQz4tvpvsCU=;\n\th=Date:Subject:To:References:From:In-Reply-To:From;\n\tb=bY8OHRZ04Bz1QU+AuOn7avh64as7PCNLBbeURjxQ3zijTQe1wVyn7ReK2CdiZPjKi\n\tDWSRaAZCmPdIYTIxybE14RPn910tMngFMRMUHZTHe5zLShsXDOla5l9H5FQxjI8bZh\n\tRusMFb+BzzN4THF3h5RyigIdKChSw8eiFjUlnrIE=","Message-ID":"<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>","Date":"Fri, 19 Nov 2021 12:14:20 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163732026902.1089182.9908600671592660146@Monstersaurus>","From":"Jean-Michel Hautbois ","In-Reply-To":"<163732026902.1089182.9908600671592660146@Monstersaurus>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","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":21037,"web_url":"https://patchwork.libcamera.org/comment/21037/","msgid":"","date":"2021-11-19T12:47:26","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nThank you for the patch.\n\nOn Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:\n> On 19/11/2021 12:11, Kieran Bingham wrote:\n> > Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n> >> We can have the case where the gain is very close to 1, as coded in\n> >> double, and after gainCode() returns, the value as an uint32_t is 0. In\n> >> order to avoid that, always round the value halfway cases away from zero\n> >> before returning.\n\nWhy do we want to avoid that, why is rounding to the closest integer\nbetter than rounding down ?\n\n> >> Signed-off-by: Jean-Michel Hautbois \n> > \n> > This seems to make sense.\n> > \n> > Are there any issues with other values being rounded? I presume not.\n> > Is there any requirement to further protect or warn if gainCode is\n> > returned as zero? or can that still be valid in some situations?\n> \n> That is a tough question... Can we have a gainCode set to 0 in a linear \n> analogue gain controlled sensor... I am not sure of that at all...\n\nIt may depends on the constaints of the linear model. For IMX219, which\nuses and inverse gain model (gain = 256 / (256 - gain code)), a gain\ncode set to 0 is valid and produces a gain of 1.0.\n\n> BTW, investigating it (before introducing std::round) I found that using \n> float instead of double made it return 1...\n> \n> > Reviewed-by: Kieran Bingham \n> >\n> >> ---\n> >> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n> >> 1 file changed, 4 insertions(+), 2 deletions(-)\n> >>\n> >> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> index 0b0eb503..67cf6913 100644\n> >> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> >> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >> @@ -7,6 +7,8 @@\n> >> */\n> >> #include \"camera_sensor_helper.h\"\n> >> \n> >> +#include \n> >> +\n> >> #include \n> >> \n> >> /**\n> >> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> >> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> >> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> >> \n> >> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> >> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> >> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> >> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n> >> }\n> >> \n> >> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 69E11BDB13\n\tfor ;\n\tFri, 19 Nov 2021 12:47:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8071760378;\n\tFri, 19 Nov 2021 13:47:51 +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 963B0600B5\n\tfor ;\n\tFri, 19 Nov 2021 13:47:49 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E4EE91959;\n\tFri, 19 Nov 2021 13:47:48 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"eqrsg9hy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637326069;\n\tbh=i5xTa7BZw5Xk245OivWSWiZJrUIcKhhYw9cgbSVNSvM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=eqrsg9hy31UsTPbv0I5hSV2ljyComZJYeQ8BJEx85t4BEpXsFpL6KbhP4TFi2Nldq\n\tQ6/cQzGW39zMeBxzEaPSTCmVgzoHhikjvgH8OUn5pJRNHnYwjPQOvCM7FffrraaiWi\n\t9NwjytC5DqA2MJC8vsxP8eHWvA7/zxq0SvEjh53o=","Date":"Fri, 19 Nov 2021 14:47:26 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163732026902.1089182.9908600671592660146@Monstersaurus>\n\t<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21042,"web_url":"https://patchwork.libcamera.org/comment/21042/","msgid":"<011912c1-76a1-17a8-e764-1d7521d4a410@ideasonboard.com>","date":"2021-11-19T13:22:23","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 19/11/2021 13:47, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> Thank you for the patch.\n> \n> On Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:\n>> On 19/11/2021 12:11, Kieran Bingham wrote:\n>>> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n>>>> We can have the case where the gain is very close to 1, as coded in\n>>>> double, and after gainCode() returns, the value as an uint32_t is 0. In\n>>>> order to avoid that, always round the value halfway cases away from zero\n>>>> before returning.\n> \n> Why do we want to avoid that, why is rounding to the closest integer\n> better than rounding down ?\n> \n>>>> Signed-off-by: Jean-Michel Hautbois \n>>>\n>>> This seems to make sense.\n>>>\n>>> Are there any issues with other values being rounded? I presume not.\n>>> Is there any requirement to further protect or warn if gainCode is\n>>> returned as zero? or can that still be valid in some situations?\n>>\n>> That is a tough question... Can we have a gainCode set to 0 in a linear\n>> analogue gain controlled sensor... I am not sure of that at all...\n> \n> It may depends on the constaints of the linear model. For IMX219, which\n> uses and inverse gain model (gain = 256 / (256 - gain code)), a gain\n> code set to 0 is valid and produces a gain of 1.0.\n\nThe minimum gainCode is 1, which produces a gain of 1.00392.\nWhen this gain is applied the other way around, gainCode returns 0 and \nnot 1 as expected.\n\n> \n>> BTW, investigating it (before introducing std::round) I found that using\n>> float instead of double made it return 1...\n>>\n>>> Reviewed-by: Kieran Bingham \n>>>\n>>>> ---\n>>>> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n>>>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>>>\n>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> index 0b0eb503..67cf6913 100644\n>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>> @@ -7,6 +7,8 @@\n>>>> */\n>>>> #include \"camera_sensor_helper.h\"\n>>>> \n>>>> +#include \n>>>> +\n>>>> #include \n>>>> \n>>>> /**\n>>>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n>>>> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n>>>> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>>> \n>>>> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>>>> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>>>> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>>>> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n>>>> }\n>>>> \n>>>> /**\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5042ABF415\n\tfor ;\n\tFri, 19 Nov 2021 13:22:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B5FCE60371;\n\tFri, 19 Nov 2021 14:22: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 D0D78600B5\n\tfor ;\n\tFri, 19 Nov 2021 14:22:25 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:db30:8e54:a96:9838] (unknown\n\t[IPv6:2a01:e0a:169:7140:db30:8e54:a96:9838])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 659C01959;\n\tFri, 19 Nov 2021 14:22:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Nlv4h1Np\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637328145;\n\tbh=kYWtiqYPVimIobiAt+h9zlaju74jkov9YMhKp9ujtFA=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=Nlv4h1NpWZ1ChHqyWffVwfFzCmCkwU4h2lV64Fb5tPmuiBTNXHf4StcaHCXShg7fp\n\tRmpZm43xf8Z8qiioNO1Kpfu2fInOexEhEcM/Z8jGzZT9WCg8XEbHo3sh8qo8UwwOrb\n\thHFCwGNUIe92lS0XyC3vjSBFH5FqGYSzqinSgKXg=","Message-ID":"<011912c1-76a1-17a8-e764-1d7521d4a410@ideasonboard.com>","Date":"Fri, 19 Nov 2021 14:22:23 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163732026902.1089182.9908600671592660146@Monstersaurus>\n\t<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21044,"web_url":"https://patchwork.libcamera.org/comment/21044/","msgid":"","date":"2021-11-19T13:27:31","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jean-Michel,\n\nOn Fri, Nov 19, 2021 at 02:22:23PM +0100, Jean-Michel Hautbois wrote:\n> On 19/11/2021 13:47, Laurent Pinchart wrote:\n> > On Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:\n> >> On 19/11/2021 12:11, Kieran Bingham wrote:\n> >>> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n> >>>> We can have the case where the gain is very close to 1, as coded in\n> >>>> double, and after gainCode() returns, the value as an uint32_t is 0. In\n> >>>> order to avoid that, always round the value halfway cases away from zero\n> >>>> before returning.\n> > \n> > Why do we want to avoid that, why is rounding to the closest integer\n> > better than rounding down ?\n> > \n> >>>> Signed-off-by: Jean-Michel Hautbois \n> >>>\n> >>> This seems to make sense.\n> >>>\n> >>> Are there any issues with other values being rounded? I presume not.\n> >>> Is there any requirement to further protect or warn if gainCode is\n> >>> returned as zero? or can that still be valid in some situations?\n> >>\n> >> That is a tough question... Can we have a gainCode set to 0 in a linear\n> >> analogue gain controlled sensor... I am not sure of that at all...\n> > \n> > It may depends on the constaints of the linear model. For IMX219, which\n> > uses and inverse gain model (gain = 256 / (256 - gain code)), a gain\n> > code set to 0 is valid and produces a gain of 1.0.\n> \n> The minimum gainCode is 1, which produces a gain of 1.00392.\n> When this gain is applied the other way around, gainCode returns 0 and \n> not 1 as expected.\n\nWhy is the minimum gain code 1 ? The sensor seems to support 0, at least\naccording to the datasheet.\n\n> >> BTW, investigating it (before introducing std::round) I found that using\n> >> float instead of double made it return 1...\n> >>\n> >>> Reviewed-by: Kieran Bingham \n> >>>\n> >>>> ---\n> >>>> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n> >>>> 1 file changed, 4 insertions(+), 2 deletions(-)\n> >>>>\n> >>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> index 0b0eb503..67cf6913 100644\n> >>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n> >>>> @@ -7,6 +7,8 @@\n> >>>> */\n> >>>> #include \"camera_sensor_helper.h\"\n> >>>> \n> >>>> +#include \n> >>>> +\n> >>>> #include \n> >>>> \n> >>>> /**\n> >>>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n> >>>> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n> >>>> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n> >>>> \n> >>>> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> >>>> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n> >>>> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n> >>>> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n> >>>> }\n> >>>> \n> >>>> /**","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4F5DBDB13\n\tfor ;\n\tFri, 19 Nov 2021 13:27:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FB7A60378;\n\tFri, 19 Nov 2021 14:27:56 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E25D9600B5\n\tfor ;\n\tFri, 19 Nov 2021 14:27:53 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 708C21959;\n\tFri, 19 Nov 2021 14:27:53 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"TWCdPAvU\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637328473;\n\tbh=xHHWUeFEfQuAdlatYoukgl0zfxSHrLHNTfc5SkNS7JY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TWCdPAvUwV2T6qTTQ+VUWeEuwz5d0CxvN6BFzdfrK08NYB7zzOpfcywIWZWp5lGuU\n\t/Z1nxw25F+93osLQCSlDVB5BDEhceNBbowjD+mvnewKgl9qp2Frm83y4VZVxuulMkB\n\t3Q77m7Ek4fqal17zRXcmVb+YCdUV2JshYkTdLJ1A=","Date":"Fri, 19 Nov 2021 15:27:31 +0200","From":"Laurent Pinchart ","To":"Jean-Michel Hautbois ","Message-ID":"","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163732026902.1089182.9908600671592660146@Monstersaurus>\n\t<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>\n\t\n\t<011912c1-76a1-17a8-e764-1d7521d4a410@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<011912c1-76a1-17a8-e764-1d7521d4a410@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":21045,"web_url":"https://patchwork.libcamera.org/comment/21045/","msgid":"","date":"2021-11-19T13:36:20","subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","submitter":{"id":75,"url":"https://patchwork.libcamera.org/api/people/75/","name":"Jean-Michel Hautbois","email":"jeanmichel.hautbois@ideasonboard.com"},"content":"On 19/11/2021 14:27, Laurent Pinchart wrote:\n> Hi Jean-Michel,\n> \n> On Fri, Nov 19, 2021 at 02:22:23PM +0100, Jean-Michel Hautbois wrote:\n>> On 19/11/2021 13:47, Laurent Pinchart wrote:\n>>> On Fri, Nov 19, 2021 at 12:14:20PM +0100, Jean-Michel Hautbois wrote:\n>>>> On 19/11/2021 12:11, Kieran Bingham wrote:\n>>>>> Quoting Jean-Michel Hautbois (2021-11-19 10:25:58)\n>>>>>> We can have the case where the gain is very close to 1, as coded in\n>>>>>> double, and after gainCode() returns, the value as an uint32_t is 0. In\n>>>>>> order to avoid that, always round the value halfway cases away from zero\n>>>>>> before returning.\n>>>\n>>> Why do we want to avoid that, why is rounding to the closest integer\n>>> better than rounding down ?\n>>>\n>>>>>> Signed-off-by: Jean-Michel Hautbois \n>>>>>\n>>>>> This seems to make sense.\n>>>>>\n>>>>> Are there any issues with other values being rounded? I presume not.\n>>>>> Is there any requirement to further protect or warn if gainCode is\n>>>>> returned as zero? or can that still be valid in some situations?\n>>>>\n>>>> That is a tough question... Can we have a gainCode set to 0 in a linear\n>>>> analogue gain controlled sensor... I am not sure of that at all...\n>>>\n>>> It may depends on the constaints of the linear model. For IMX219, which\n>>> uses and inverse gain model (gain = 256 / (256 - gain code)), a gain\n>>> code set to 0 is valid and produces a gain of 1.0.\n>>\n>> The minimum gainCode is 1, which produces a gain of 1.00392.\n>> When this gain is applied the other way around, gainCode returns 0 and\n>> not 1 as expected.\n> \n> Why is the minimum gain code 1 ? The sensor seems to support 0, at least\n> according to the datasheet.\n> \n\nThe driver sets it to 0...\nThe real issue being in configure():\nminGain_ = std::max(itGain->second.min().get(), 1);\n\nI will remove this patch :-) and have another one instead, thanks !\n\n>>>> BTW, investigating it (before introducing std::round) I found that using\n>>>> float instead of double made it return 1...\n>>>>\n>>>>> Reviewed-by: Kieran Bingham \n>>>>>\n>>>>>> ---\n>>>>>> src/ipa/libipa/camera_sensor_helper.cpp | 6 ++++--\n>>>>>> 1 file changed, 4 insertions(+), 2 deletions(-)\n>>>>>>\n>>>>>> diff --git a/src/ipa/libipa/camera_sensor_helper.cpp b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> index 0b0eb503..67cf6913 100644\n>>>>>> --- a/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> +++ b/src/ipa/libipa/camera_sensor_helper.cpp\n>>>>>> @@ -7,6 +7,8 @@\n>>>>>> */\n>>>>>> #include \"camera_sensor_helper.h\"\n>>>>>> \n>>>>>> +#include \n>>>>>> +\n>>>>>> #include \n>>>>>> \n>>>>>> /**\n>>>>>> @@ -61,8 +63,8 @@ uint32_t CameraSensorHelper::gainCode(double gain) const\n>>>>>> ASSERT(analogueGainConstants_.m0 == 0 || analogueGainConstants_.m1 == 0);\n>>>>>> ASSERT(analogueGainConstants_.type == AnalogueGainLinear);\n>>>>>> \n>>>>>> - return (analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>>>>>> - (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0);\n>>>>>> + return std::round((analogueGainConstants_.c0 - analogueGainConstants_.c1 * gain) /\n>>>>>> + (analogueGainConstants_.m1 * gain - analogueGainConstants_.m0));\n>>>>>> }\n>>>>>> \n>>>>>> /**\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5B934BF415\n\tfor ;\n\tFri, 19 Nov 2021 13:36:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C444260233;\n\tFri, 19 Nov 2021 14:36:25 +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 849FD600B5\n\tfor ;\n\tFri, 19 Nov 2021 14:36:23 +0100 (CET)","from [IPV6:2a01:e0a:169:7140:db30:8e54:a96:9838] (unknown\n\t[IPv6:2a01:e0a:169:7140:db30:8e54:a96:9838])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 195521959;\n\tFri, 19 Nov 2021 14:36:23 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vrhW+mGw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1637328983;\n\tbh=lhd1706FBPdLAKfdQ1Z6gWAW+Lpzcv45O1TME6iltKo=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=vrhW+mGwz5JjkwWbXkL1F4gVJ3XmTXTIHukvHdsfzkCPufR0a3p+MWFTw8kH5zRad\n\tb7yGTLAA8ItQdi/2eU+s0/fH+T5C5pH79eutTvMC2vzttNTIx9k/XcDLS+6Yx6i3QL\n\tU7wI+872UJsXQMvHbgUuqX3ctUXCnL4iAFPIlFDI=","Message-ID":"","Date":"Fri, 19 Nov 2021 14:36:20 +0100","MIME-Version":"1.0","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101\n\tThunderbird/91.3.1","Content-Language":"en-US","To":"Laurent Pinchart ","References":"<20211119102559.56522-1-jeanmichel.hautbois@ideasonboard.com>\n\t<20211119102559.56522-2-jeanmichel.hautbois@ideasonboard.com>\n\t<163732026902.1089182.9908600671592660146@Monstersaurus>\n\t<12b06ac6-d245-9211-628b-d406e5bc52ae@ideasonboard.com>\n\t\n\t<011912c1-76a1-17a8-e764-1d7521d4a410@ideasonboard.com>\n\t","From":"Jean-Michel Hautbois ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH 1/2] libipa: Round gain code before\n\treturning the value","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]