Message ID | 20210604102606.218464-1-umang.jain@ideasonboard.com |
---|---|
State | Not Applicable |
Headers | show |
Series |
|
Related | show |
Hi Umang, On Fri, Jun 04, 2021 at 03:56:06PM +0530, Umang Jain wrote: > Signed-off-by: Umang Jain <umang.jain@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? > > 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? Thanks, Paul > ANDROID_SCALER_CROPPING_TYPE, > ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES, > ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE, > -- > 2.31.0 >
Hi Paul, Thanks for pitching in. On 6/7/21 11:54 AM, paul.elder@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@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 >>
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@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@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 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 > >
Hello Umang, Paul On Tue, Jun 08, 2021 at 03:24:58PM +0900, paul.elder@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@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@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 > > > >
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, ANDROID_SCALER_CROPPING_TYPE, ANDROID_SENSOR_AVAILABLE_TEST_PATTERN_MODES, ANDROID_SENSOR_INFO_ACTIVE_ARRAY_SIZE,