<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p>Hi Paul,</p>
<p>Thanks for pitching in.<br>
</p>
<div class="moz-cite-prefix">On 6/7/21 11:54 AM,
<a class="moz-txt-link-abbreviated" href="mailto:paul.elder@ideasonboard.com">paul.elder@ideasonboard.com</a> wrote:<br>
</div>
<blockquote type="cite"
cite="mid:20210607062432.GG156622@pyrite.rasen.tech">
<pre class="moz-quote-pre" wrap="">Hi Umang,
On Fri, Jun 04, 2021 at 03:56:06PM +0530, Umang Jain wrote:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">Signed-off-by: Umang Jain <a class="moz-txt-link-rfc2396E" href="mailto:umang.jain@ideasonboard.com"><umang.jain@ideasonboard.com></a>
---
This patch is for reference and discussion purposes only.
Please DO NOT MERGE. If anything, there will be follow up
separate patches for merge separately.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
Thank you for the investigation and starting the discussion.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
Problem:
On nautilus, 26 tests were failing because of "Fail to open camera",
but more specifically from adb logcat:
```
E Camera2-Parameters: generated preview size list is empty!!
E Camera2Client: initializeImpl: Camera 0: unable to build defaults: Invalid argument (-22)
E CameraService: connectHelper: Could not initialize client from HAL.
I Camera2Client: Camera 0: Closed
```
was found to be root of the problem. The checks triggered are here:
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""><a class="moz-txt-link-freetext" href="Parameters::getFilteredSizes()">Parameters::getFilteredSizes()</a>
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap=""> <a class="moz-txt-link-freetext" href="https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976">https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976</a>
nautilus reports higher frame-duration to start with: 34100000
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
iirc this was the minimum frame duration. This converts to 29.32 fps.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">(in <a class="moz-txt-link-freetext" href="CameraDevice::getStaticMetadata())">CameraDevice::getStaticMetadata())</a>.
minFrameDurationNsec is meant to be re-adjusted down the line
before round-up, *if* the difference is < 500 useconds.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
And the minimum frame duration gets compared to what android requires as
the maximum value of the minimum frame duration, which is 33366700ns =
29.97 fps.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
On nautilus, since the difference was much larger than 500 useconds,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
What this adjustment does is if the minimum frame duration (= max fps)
supported by the sensor is greater than the maximum allowed value (= the
supported max fps is lower than the max requirement), but it's within
500us, we can adjust it.
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">the re-adjustment failed and libcamera reported a much higher
frame-duration to upper-layers/libcameraservice.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
(Yes, it was rounded down instead to 29 fps and the corresponding
34482758ns, which is less/greater than 29.97fps / 33366700ns.)
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">This led to the problem in <a class="moz-txt-link-freetext" href="Parameters::getFilteredSizes()">Parameters::getFilteredSizes()</a>,
where all potential streams for preview are skipped to be added,
due to high minFrameDuration.
To force this re-adjustment, the scope of difference was increased
to 1200 useconds as done in the patch.
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
So by allowing 1200us, what we're saying is that we can still reasonably
say that we support a minimum frame duration of 33366700ns when the real
minimum that we support is 34566700ns. It's a whole 1.2ms off per frame.
Is this a reasonable assertion?</pre>
</blockquote>
Indeed, that's what is in question. I plan to discuss this in
today's weekly.<br>
<blockquote type="cite"
cite="mid:20210607062432.GG156622@pyrite.rasen.tech">
<pre class="moz-quote-pre" wrap="">
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap="">
CTS Results on nautilus after applying the changes:
Total Run time: 8m 42s
1/1 modules completed
Total Tests : 221
PASSED : 221
FAILED : 0
---
src/android/camera_device.cpp | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index fe332ec3..d0676a7f 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -877,13 +877,13 @@ const camera_metadata_t *<a class="moz-txt-link-freetext" href="CameraDevice::getStaticMetadata()">CameraDevice::getStaticMetadata()</a>
* (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service
* implementation).
*
- * If we're close enough (+ 500 useconds) to that value, round
+ * If we're close enough (+ 1200 useconds) to that value, round
* the minimum frame duration of the camera to an accepted
* value.
*/
static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_NS = 1e9 / 29.97;
if (minFrameDurationNsec > MAX_PREVIEW_RECORD_DURATION_NS &&
- minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 500000)
+ minFrameDurationNsec < MAX_PREVIEW_RECORD_DURATION_NS + 1200000)
minFrameDurationNsec = MAX_PREVIEW_RECORD_DURATION_NS - 1000;
/*
@@ -1335,6 +1335,7 @@ const camera_metadata_t *<a class="moz-txt-link-freetext" href="CameraDevice::getStaticMetadata()">CameraDevice::getStaticMetadata()</a>
ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
+ ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
</pre>
</blockquote>
<pre class="moz-quote-pre" wrap="">
What's this for?</pre>
</blockquote>
<p><br>
</p>
<p>The
<a class="moz-txt-link-freetext" href="https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976">https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976</a><br>
check , reads in
ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT key, and I
realized we don't report it hence, I thought it's a similar case<br>
as <a
href="https://git.linuxtv.org/libcamera.git/commit/?id=51c09236c4e8bec2b0a272ebecaa33f32432208a">51c09236c4e8</a>
("android: Make FRAME_DURATION result key available in static
metadata")</p>
<p><br>
</p>
<p>However this morning, I ran CTS without<br>
<font face="monospace">>
+ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT<br>
</font>and it ran just fine. Maybe I don't understand keys-related
magic not well-enough.<br>
</p>
<p><br>
</p>
<blockquote type="cite"
cite="mid:20210607062432.GG156622@pyrite.rasen.tech">
<pre class="moz-quote-pre" wrap="">
Thanks,
Paul
</pre>
<blockquote type="cite">
<pre class="moz-quote-pre" wrap=""> ANDROID_SCALER_CROPPING_TYPE,
ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
--
2.31.0
</pre>
</blockquote>
</blockquote>
</body>
</html>