[{"id":18006,"web_url":"https://patchwork.libcamera.org/comment/18006/","msgid":"<4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com>","date":"2021-07-07T09:33:02","subject":"Re: [libcamera-devel] [PATCH] camera_sensor: Remove redundant\n\taspect-ratio check","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Umang,\n\nOn 06/07/2021 11:29, Umang Jain wrote:\n> While trying to find the best sensor resolution,\n> CameraSensor::getFormat() tracks best aspect-ratio of the sensor-format\n> it has seen beforehand, among other things. If a better aspect-ratio is\n> found, the code shall proceed to check other best-fit parameters.\n> However, the check for aspect-ratio is occuring twice, eliminate one of\n> them.\n> \n> This commit does not introduces any functional changes.\n> \n> Signed-off-by: Umang Jain \n> ---\n> src/libcamera/camera_sensor.cpp | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n> index cde431cc..1bf42acf 100644\n> --- a/src/libcamera/camera_sensor.cpp\n> +++ b/src/libcamera/camera_sensor.cpp\n> @@ -572,7 +572,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n> \t\t\tif (ratioDiff > bestRatio)\n> \t\t\t\tcontinue;\n> \n> -\t\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> +\t\t\tif (areaDiff < bestArea) {\n> \t\t\t\tbestRatio = ratioDiff;\n> \t\t\t\tbestArea = areaDiff;\n> \t\t\t\tbestSize = &sz;\n> \n\n\nI don't think this change is doing what you expect I'm afraid.\n\nPreviously if the ratioDiff < bestRatio /or/ areaDiff < bestArea, - we\nwould enter this context and set the new bestRatio .. granted the check\nabove means that we know ratioDiff < (or =) bestRatio ...\n\nBut now - with your change it will /only/ do so if areaDiff < bestArea.\n\nIt is true that the ratioDiff > bestRatio check will mean that the code\npath can only flow down here if the ratioDiff is improved, but now, we\nare cutting off the actual setting of those values unless areaDiff <\nbestArea as well.\n\nSo I'm not sure this is quite correct. It is essentially turning the\nline from\n\n> -\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\nto\n> +\tif (ratioDiff <= bestRatio && areaDiff < bestArea) {\n\nWhich is looks like a functional change to me, because crucially,\npreviously to this patch the values could be updated if the bestRatio\nimproved, regardless of the bestArea check.\n\n\n--\nKieran","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 4994CBD794\n\tfor ;\n\tWed, 7 Jul 2021 09:33:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8069768506;\n\tWed, 7 Jul 2021 11:33:06 +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 515E568506\n\tfor ;\n\tWed, 7 Jul 2021 11:33:05 +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 D755D2E4;\n\tWed, 7 Jul 2021 11:33:04 +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=\"bZrsLP+b\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625650384;\n\tbh=6slHXpO2ISlWFDrbJpr51zneZVxYhmNCQJaP55X2D0o=;\n\th=To:References:From:Subject:Date:In-Reply-To:From;\n\tb=bZrsLP+bkrpU59MwSKWhQISUP4Aoep8Et+9AsJkn1s+XXc4B+V+HyVX4FDSKGA4So\n\t+DmlmrmkccullfPTA4PxNI/Yovg6bocwpG/Q1UnM7Z9ljDpBDkar6imE4VBOabEsZX\n\tnrZmA2dgnK8R4vhNFsmngE3BiLYYqerXyI01iZL0=","To":"Umang Jain ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210706102918.682536-1-umang.jain@ideasonboard.com>","From":"Kieran Bingham ","Message-ID":"<4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com>","Date":"Wed, 7 Jul 2021 10:33:02 +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":"<20210706102918.682536-1-umang.jain@ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH] camera_sensor: Remove redundant\n\taspect-ratio check","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":18007,"web_url":"https://patchwork.libcamera.org/comment/18007/","msgid":"","date":"2021-07-07T10:09:08","subject":"Re: [libcamera-devel] [PATCH] camera_sensor: Remove redundant\n\taspect-ratio check","submitter":{"id":86,"url":"https://patchwork.libcamera.org/api/people/86/","name":"Umang Jain","email":"umang.jain@ideasonboard.com"},"content":"hi Kieran,\n\nOn 7/7/21 3:03 PM, Kieran Bingham wrote:\n> Hi Umang,\n>\n> On 06/07/2021 11:29, Umang Jain wrote:\n>> While trying to find the best sensor resolution,\n>> CameraSensor::getFormat() tracks best aspect-ratio of the sensor-format\n>> it has seen beforehand, among other things. If a better aspect-ratio is\n>> found, the code shall proceed to check other best-fit parameters.\n>> However, the check for aspect-ratio is occuring twice, eliminate one of\n>> them.\n>>\n>> This commit does not introduces any functional changes.\n>>\n>> Signed-off-by: Umang Jain \n>> ---\n>> src/libcamera/camera_sensor.cpp | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp\n>> index cde431cc..1bf42acf 100644\n>> --- a/src/libcamera/camera_sensor.cpp\n>> +++ b/src/libcamera/camera_sensor.cpp\n>> @@ -572,7 +572,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector &mbu\n>> \t\t\tif (ratioDiff > bestRatio)\n>> \t\t\t\tcontinue;\n>> \n>> -\t\t\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n>> +\t\t\tif (areaDiff < bestArea) {\n>> \t\t\t\tbestRatio = ratioDiff;\n>> \t\t\t\tbestArea = areaDiff;\n>> \t\t\t\tbestSize = &sz;\n>>\n>\n> I don't think this change is doing what you expect I'm afraid.\n>\n> Previously if the ratioDiff < bestRatio /or/ areaDiff < bestArea, - we\n> would enter this context and set the new bestRatio .. granted the check\n> above means that we know ratioDiff < (or =) bestRatio ...\n>\n> But now - with your change it will /only/ do so if areaDiff < bestArea.\n>\n> It is true that the ratioDiff > bestRatio check will mean that the code\n> path can only flow down here if the ratioDiff is improved, but now, we\n> are cutting off the actual setting of those values unless areaDiff <\n> bestArea as well.\n>\n> So I'm not sure this is quite correct. It is essentially turning the\n> line from\n>\n>> -\tif (ratioDiff < bestRatio || areaDiff < bestArea) {\n> to\n>> +\tif (ratioDiff <= bestRatio && areaDiff < bestArea) {\n> Which is looks like a functional change to me, because crucially,\n> previously to this patch the values could be updated if the bestRatio\n> improved, regardless of the bestArea check.\n\nOk, so I agree, this is a functional change and I should remove the \nclaim that it's not, from the commit message.\n\nSpeaking on the change itself, you'll find \"[RFC PATCH] camera_sensor: \nDo not always prioritize aspect-ratios\" interesting, with respect to\n\n if (ratioDiff <= bestRatio && areaDiff < bestArea) {\n\nIt tries to look at both ratioDiff and areaDiff, and see if a current \nsensor resolution configuration seems more apt or not, rather than \nignoring the areaDiff(s), well in most cases :-)\n\n\n>\n>\n> --\n> Kieran","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id BBDFBC3224\n\tfor ;\n\tWed, 7 Jul 2021 10:09:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 445936028E;\n\tWed, 7 Jul 2021 12:09:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C85260284\n\tfor ;\n\tWed, 7 Jul 2021 12:09:14 +0200 (CEST)","from [192.168.0.107] (unknown [103.251.226.59])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B1DA62E4;\n\tWed, 7 Jul 2021 12:09:13 +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=\"KK3//8pS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625652554;\n\tbh=bruvuN6NI3J7NHVG8In69RiHXyahdldLO9f9JxR685o=;\n\th=Subject:To:References:From:Date:In-Reply-To:From;\n\tb=KK3//8pShtluxrZEsFT7vJKZPdujbAtEQaMmbCO+kI9cdiNkIIcF98K/iOjjW9PLu\n\tBu2qA8go7nkpTZ6eu/aF1qVNqxau4qcAhA0bnsNAvnFhFdixURhZDR+JNHVaSCE9Bn\n\teThK8Gl2ddgrSlNM2/+1NwJDmmLJXt0qbZOAVp6A=","To":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20210706102918.682536-1-umang.jain@ideasonboard.com>\n\t<4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com>","From":"Umang Jain ","Message-ID":"","Date":"Wed, 7 Jul 2021 15:39:08 +0530","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101\n\tThunderbird/78.10.2","MIME-Version":"1.0","In-Reply-To":"<4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com>","Content-Type":"multipart/alternative;\n\tboundary=\"------------87DFF9DA6C1E8C8C83CC489D\"","Content-Language":"en-US","Subject":"Re: [libcamera-devel] [PATCH] camera_sensor: Remove redundant\n\taspect-ratio check","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\" "}}]