<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>hi Kieran,<br>
</p>
<div class="moz-cite-prefix">On 7/7/21 3:03 PM, Kieran Bingham
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com">
<pre class="moz-quote-pre" wrap="">Hi Umang,
On 06/07/2021 11:29, Umang Jain wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">While trying to find the best sensor resolution,
<a class="moz-txt-link-freetext" href="CameraSensor::getFormat()">CameraSensor::getFormat()</a> tracks best aspect-ratio of the sensor-format
it has seen beforehand, among other things. If a better aspect-ratio is
found, the code shall proceed to check other best-fit parameters.
However, the check for aspect-ratio is occuring twice, eliminate one of
them.
This commit does not introduces any functional changes.
Signed-off-by: Umang Jain <a class="moz-txt-link-rfc2396E" href="mailto:umang.jain@ideasonboard.com"><umang.jain@ideasonboard.com></a>
---
src/libcamera/camera_sensor.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index cde431cc..1bf42acf 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -572,7 +572,7 @@ V4L2SubdeviceFormat <a class="moz-txt-link-freetext" href="CameraSensor::getFormat(const">CameraSensor::getFormat(const</a> <a class="moz-txt-link-freetext" href="std::vector">std::vector</a><unsigned int> &mbu
if (ratioDiff > bestRatio)
continue;
- if (ratioDiff < bestRatio || areaDiff < bestArea) {
+ if (areaDiff < bestArea) {
bestRatio = ratioDiff;
bestArea = areaDiff;
bestSize = &sz;
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
I don't think this change is doing what you expect I'm afraid.
Previously if the ratioDiff < bestRatio /or/ areaDiff < bestArea, - we
would enter this context and set the new bestRatio .. granted the check
above means that we know ratioDiff < (or =) bestRatio ...
But now - with your change it will /only/ do so if areaDiff < bestArea.
It is true that the ratioDiff > bestRatio check will mean that the code
path can only flow down here if the ratioDiff is improved, but now, we
are cutting off the actual setting of those values unless areaDiff <
bestArea as well.
So I'm not sure this is quite correct. It is essentially turning the
line from
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">- if (ratioDiff < bestRatio || areaDiff < bestArea) {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">to
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">+ if (ratioDiff <= bestRatio && areaDiff < bestArea) {
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Which is looks like a functional change to me, because crucially,
previously to this patch the values could be updated if the bestRatio
improved, regardless of the bestArea check.</pre>
</blockquote>
<p>Ok, so I agree, this is a functional change and I should remove
the claim that it's not, from the commit message.</p>
<p>Speaking on the change itself, you'll find "[RFC PATCH]
camera_sensor: Do not always prioritize aspect-ratios"
interesting, with respect to<br>
</p>
<blockquote>
<p> <font face="monospace">if (ratioDiff <= bestRatio
&& areaDiff < bestArea) {</font></p>
</blockquote>
<p>It tries to look at both ratioDiff and areaDiff, and see if a
current sensor resolution configuration seems more apt or not,
rather than ignoring the areaDiff(s), well in most cases :-) <br>
</p>
<p><br>
<font face="monospace"></font></p>
<blockquote type="cite"
cite="mid:4f14f0d0-7a0b-3235-b024-49aa9e299ed1@ideasonboard.com">
<pre class="moz-quote-pre" wrap="">
--
Kieran
</pre>
</blockquote>
</body>
</html>