[{"id":19050,"web_url":"https://patchwork.libcamera.org/comment/19050/","msgid":"<20210825101226.vjgtpxztykwc5bct@uno.localdomain>","date":"2021-08-25T10:12:26","subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipu3: cio2: Tweak sensor size\n\tselection policy","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"On Tue, Aug 10, 2021 at 01:28:54PM +0530, Umang Jain wrote:\n> Do not compare higher precision of the ratios, as it might lead to\n> absurd selection of sensor size for a relatively low requested\n> resolution size.\n\nJust \"lead to size selection mismatches\" maybe ?\nThe example below is explicative enough\n\n>\n> For example:\n> The imx258 driver supports the following sensor resolutions:\n>\n> - 4208x3118 = 1.349583066\n> - 2104x1560 = 1.348717949\n> - 1048x780 = 1.343589744\n>\n> It can be inferred that, that the aspect ratio only differs by a small\n\ns/that, that/that/\n\n> factor with each other. It does not makes sense to select a 4208x3118\n> for a requested size of say 640x480 or 1280x720, which is what is\n> happening currently.\n> ($) cam -c1 -swidth=640,height=480,role=raw\n> - CIO2 configuration: 4208x3118-SGRBG10_IPU3\n>\n> In order to address this constraint, only compare the ratio with single\n> precision to make a better decision on the sensor resolution policy\n> selection.\n>\n> ($) cam -c1 -srole=raw,width=640,height=480\n> - CIO2 configuration: 1048x780-SGRBG10_IPU3\n>\n> Signed-off-by: Umang Jain \n> Tested-by: Umang Jain \n> Tested-by: Jacopo Mondi \n\nI recall I asked about if we want rounding or truncating is fine. I\nthink you confirmed truncating is fine, which I agree.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> ---\n> src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++\n> 1 file changed, 5 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n> index 3c9331e3..6ccef301 100644\n> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n> @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector\n> \t\t\t\tcontinue;\n>\n> \t\t\tfloat ratio = static_cast(sz.width) / sz.height;\n> +\t\t\t/*\n> +\t\t\t * Comparing ratios with a single precision digit\n> +\t\t\t * is enough.\n> +\t\t\t */\n> +\t\t\tratio = static_cast(ratio * 10) / 10.0;\n> \t\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n> \t\t\tunsigned int area = sz.width * sz.height;\n> \t\t\tunsigned int areaDiff = area - desiredArea;\n> --\n> 2.31.1\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 A009BBD87D\n\tfor ;\n\tWed, 25 Aug 2021 10:11:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0916568893;\n\tWed, 25 Aug 2021 12:11:40 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A1CE860259\n\tfor ;\n\tWed, 25 Aug 2021 12:11:38 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id E7302C0003;\n\tWed, 25 Aug 2021 10:11:37 +0000 (UTC)"],"Date":"Wed, 25 Aug 2021 12:12:26 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20210825101226.vjgtpxztykwc5bct@uno.localdomain>","References":"<20210810075854.86191-1-umang.jain@ideasonboard.com>\n\t<20210810075854.86191-5-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210810075854.86191-5-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipu3: cio2: Tweak sensor size\n\tselection policy","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":19147,"web_url":"https://patchwork.libcamera.org/comment/19147/","msgid":"<6bb38704-a8ff-6a31-5e63-555ae5e4a1fd@ideasonboard.com>","date":"2021-08-27T12:52:40","subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipu3: cio2: Tweak sensor size\n\tselection policy","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"On 25/08/2021 11:12, Jacopo Mondi wrote:\n> On Tue, Aug 10, 2021 at 01:28:54PM +0530, Umang Jain wrote:\n>> Do not compare higher precision of the ratios, as it might lead to\n>> absurd selection of sensor size for a relatively low requested\n>> resolution size.\n> \n> Just \"lead to size selection mismatches\" maybe ?\n> The example below is explicative enough\n> \n>>\n>> For example:\n>> The imx258 driver supports the following sensor resolutions:\n>>\n>> - 4208x3118 = 1.349583066\n>> - 2104x1560 = 1.348717949\n>> - 1048x780 = 1.343589744\n>>\n>> It can be inferred that, that the aspect ratio only differs by a small\n> \n> s/that, that/that/\n> \n>> factor with each other. It does not makes sense to select a 4208x3118\n>> for a requested size of say 640x480 or 1280x720, which is what is\n>> happening currently.\n>> ($) cam -c1 -swidth=640,height=480,role=raw\n>> - CIO2 configuration: 4208x3118-SGRBG10_IPU3\n>>\n>> In order to address this constraint, only compare the ratio with single\n>> precision to make a better decision on the sensor resolution policy\n>> selection.\n>>\n>> ($) cam -c1 -srole=raw,width=640,height=480\n>> - CIO2 configuration: 1048x780-SGRBG10_IPU3\n>>\n>> Signed-off-by: Umang Jain \n>> Tested-by: Umang Jain \n>> Tested-by: Jacopo Mondi \n> \n> I recall I asked about if we want rounding or truncating is fine. I\n> think you confirmed truncating is fine, which I agree.\n> \n> Reviewed-by: Jacopo Mondi \n> \n> Thanks\n> j\n> \n>> ---\n>> src/libcamera/pipeline/ipu3/cio2.cpp | 5 +++++\n>> 1 file changed, 5 insertions(+)\n>>\n>> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp\n>> index 3c9331e3..6ccef301 100644\n>> --- a/src/libcamera/pipeline/ipu3/cio2.cpp\n>> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp\n>> @@ -290,6 +290,11 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector\n>> \t\t\t\tcontinue;\n>>\n>> \t\t\tfloat ratio = static_cast(sz.width) / sz.height;\n>> +\t\t\t/*\n>> +\t\t\t * Comparing ratios with a single precision digit\n>> +\t\t\t * is enough.\n>> +\t\t\t */\n\nIt might be worth expanding this to say why it's enough, or rather what\nhappens if greater precision is used.\n\nIt's covered by the commit message, but that might not be seen by\nsomeone trying to work out 'why' single precision is used rather than\nthe full float.\n\nBut either way, I can see how this is a direct advantage.\n\nReviewed-by: Kieran Bingham \n\n\n\n>> +\t\t\tratio = static_cast(ratio * 10) / 10.0;\n>> \t\t\tfloat ratioDiff = fabsf(ratio - desiredRatio);\n>> \t\t\tunsigned int area = sz.width * sz.height;\n>> \t\t\tunsigned int areaDiff = area - desiredArea;\n>> --\n>> 2.31.1\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 9325DBD87C\n\tfor ;\n\tFri, 27 Aug 2021 12:52:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FEEA68932;\n\tFri, 27 Aug 2021 14:52:45 +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 C49F760256\n\tfor ;\n\tFri, 27 Aug 2021 14:52:43 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 183BA5A1;\n\tFri, 27 Aug 2021 14:52:43 +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=\"V8CM1epW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1630068763;\n\tbh=Eyv4aQ1o79As6BS/00jWURSL1gEKdvjYk1I+rF/rDg8=;\n\th=Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=V8CM1epW6xOOX1zMfhZWjaemMW62uJ0NO36IJJcGFlAFN7yuXDbOJNhXxQFIGZnhl\n\tGMwx3BBfaoHxM88bbx4I7YTUXuyf5Ou+o3mxq/V+Zf6pGRU9gGQwZ5/N5C/n+Xv4VL\n\tXfy15RQYpw2cvq5oqTzpZwsx+Xx28keD0m3TJteA=","To":"Jacopo Mondi ,\n\tUmang Jain ","References":"<20210810075854.86191-1-umang.jain@ideasonboard.com>\n\t<20210810075854.86191-5-umang.jain@ideasonboard.com>\n\t<20210825101226.vjgtpxztykwc5bct@uno.localdomain>","From":"Kieran Bingham ","Message-ID":"<6bb38704-a8ff-6a31-5e63-555ae5e4a1fd@ideasonboard.com>","Date":"Fri, 27 Aug 2021 13:52:40 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.11.0","MIME-Version":"1.0","In-Reply-To":"<20210825101226.vjgtpxztykwc5bct@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"7bit","Subject":"Re: [libcamera-devel] [PATCH v2 4/4] ipu3: cio2: Tweak sensor size\n\tselection policy","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\" "}}]