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

[Jacopo Mondi]

Hello Umang, Paul

On Tue, Jun 08, 2021 at 03:24:58PM +0900, paul.elder at ideasonboard.com wrote:
> Hi Umang,
>
> On Tue, Jun 08, 2021 at 11:08:00AM +0530, Umang Jain wrote:
> > 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 ("android: Make FRAME_DURATION result key available in static
> > metadata")
>
> I'm pretty sure that section corresponds to [1], so it should indeed be
> fine without declaring the key.
>
> [1] https://git.linuxtv.org/libcamera.git/tree/src/android/camera_device.cpp#n1215

More than that, ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT
(and the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_INPUT
couterpart) are flags that are expected to be used to populate the 4th
field of each stream confiuration reported as available through
ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS, which aims to report
the stream direction (output == produced by the camera, input ==
provided by application for reprocessing).

It should indeed not be added to the list of available characteristics
keys.

Thanks
   j

>
>
> Paul
>
> >
> >
> > 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.
> >
> >
> >
> >                         ANDROID_SCALER_CROPPING_TYPE,
> >                         ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES,
> >                         ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,
> >         --
> >         2.31.0
> >
> >