[libcamera-devel] [PATCH] [DNI]: Fixes for CTS on nautilus(LIMITED)

[Umang Jain]

Hi Paul,

Thanks for pitching in.

On 6/7/21 11:54 AM, paul.elder at ideasonboard.com wrote:
> Hi Umang,
>
> On Fri, Jun 04, 2021 at 03:56:06PM +0530, Umang Jain wrote:
>> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
>> ---
>>
>> 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.
> Thank you for the investigation and starting the discussion.
>
>> 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:
>>> Parameters::getFilteredSizes()
>>    https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976
>>
>> nautilus reports higher frame-duration to start with: 34100000
> iirc this was the minimum frame duration. This converts to 29.32 fps.
>
>> (in CameraDevice::getStaticMetadata()).
>> minFrameDurationNsec is meant to be re-adjusted down the line
>> before round-up, *if* the difference is < 500 useconds.
> 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.
>
>> On nautilus, since the difference was much larger than 500 useconds,
> 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.
>
>> the re-adjustment failed and libcamera reported a much higher
>> frame-duration to upper-layers/libcameraservice.
> (Yes, it was rounded down instead to 29 fps and the corresponding
> 34482758ns, which is less/greater than 29.97fps / 33366700ns.)
>
>> This led to the problem in Parameters::getFilteredSizes(),
>> 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.
> 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?
Indeed, that's what is in question. I plan to discuss this in today's 
weekly.
>
>> 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 *CameraDevice::getStaticMetadata()
>>   		 * (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 *CameraDevice::getStaticMetadata()
>>   		ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS,
>>   		ANDROID_SCALER_AVAILABLE_STALL_DURATIONS,
>>   		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS,
>> +		ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT,
> What's this for?


The 
https://android.googlesource.com/platform/frameworks/av/+/refs/heads/master/services/camera/libcameraservice/api1/client2/Parameters.cpp#2976
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
as 51c09236c4e8 
<https://git.linuxtv.org/libcamera.git/commit/?id=51c09236c4e8bec2b0a272ebecaa33f32432208a> 
("android: Make FRAME_DURATION result key available in static metadata")


However this morning, I ran CTS without
 > +ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT
and it ran just fine. Maybe I don't understand keys-related magic not 
well-enough.


>
>
> Thanks,
>
> Paul
>
>>   		ANDROID_SCALER_CROPPING_TYPE,
>>   		ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
>>   		ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
>> -- 
>> 2.31.0
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210608/08f5bb05/attachment-0001.htm>