[libcamera-devel,v2,11/17] android: Filter preview streams on FPS
diff mbox series

Message ID 20210907194107.803730-12-jacopo@jmondi.org
State Superseded
Headers show
Series
  • IPU3 control info update and HAL frame durations
Related show

Commit Message

Jacopo Mondi Sept. 7, 2021, 7:41 p.m. UTC 
Register as preview streams only streams capable of producing at least
30 FPS.

This requirement comes from inspecting the existing HAL implementation
on Intel IPU3 platform.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_capabilities.cpp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Laurent Pinchart Oct. 6, 2021, 1:45 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Sep 07, 2021 at 09:41:01PM +0200, Jacopo Mondi wrote:
> Register as preview streams only streams capable of producing at least
> 30 FPS.
> 
> This requirement comes from inspecting the existing HAL implementation
> on Intel IPU3 platform.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_capabilities.cpp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> index c6791b9d6238..e5cfe67f9b73 100644
> --- a/src/android/camera_capabilities.cpp
> +++ b/src/android/camera_capabilities.cpp
> @@ -1259,6 +1259,21 @@ int CameraCapabilities::initializeStaticMetadata()
>  	std::vector<uint32_t> availableStreamConfigurations;
>  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> +		/*
> +		 * Filter out YUV streams not capable of running at 30 FPS.
> +		 *
> +		 * This requirement comes from inspecting the Intel IPU3
> +		 * HAL implementation but no reference has been found in the
> +		 * metadata documentation.

Does the requirement also come from CTS ? If so, could you please point
to the corresponding test(s) ? If not, why do we need this ?

> +		 *
> +		 * Calculate FPS as CTS does: see
> +		 * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration()
> +		 */
> +		unsigned int fps = static_cast<unsigned int>
> +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> +			continue;
> +
>  		availableStreamConfigurations.push_back(entry.androidFormat);
>  		availableStreamConfigurations.push_back(entry.resolution.width);
>  		availableStreamConfigurations.push_back(entry.resolution.height);
> @@ -1271,6 +1286,11 @@ int CameraCapabilities::initializeStaticMetadata()
>  	std::vector<int64_t> minFrameDurations;
>  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
>  	for (const auto &entry : streamConfigurations_) {
> +		unsigned int fps = static_cast<unsigned int>
> +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> +			continue;
> +
>  		minFrameDurations.push_back(entry.androidFormat);
>  		minFrameDurations.push_back(entry.resolution.width);
>  		minFrameDurations.push_back(entry.resolution.height);
Jacopo Mondi Oct. 6, 2021, 8:23 a.m. UTC | #2 
Hi Laurent,

On Wed, Oct 06, 2021 at 04:45:25AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Sep 07, 2021 at 09:41:01PM +0200, Jacopo Mondi wrote:
> > Register as preview streams only streams capable of producing at least
> > 30 FPS.
> >
> > This requirement comes from inspecting the existing HAL implementation
> > on Intel IPU3 platform.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > ---
> >  src/android/camera_capabilities.cpp | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > index c6791b9d6238..e5cfe67f9b73 100644
> > --- a/src/android/camera_capabilities.cpp
> > +++ b/src/android/camera_capabilities.cpp
> > @@ -1259,6 +1259,21 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	std::vector<uint32_t> availableStreamConfigurations;
> >  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
> >  	for (const auto &entry : streamConfigurations_) {
> > +		/*
> > +		 * Filter out YUV streams not capable of running at 30 FPS.
> > +		 *
> > +		 * This requirement comes from inspecting the Intel IPU3
> > +		 * HAL implementation but no reference has been found in the
> > +		 * metadata documentation.
>
> Does the requirement also come from CTS ? If so, could you please point
> to the corresponding test(s) ? If not, why do we need this ?
>

I keep failing to find any reference to this requirement in
documentation, and the way we populate the YUV stream combinations has
an impact on the AE_AVAILABLE_TARGET_FPS_RANGE content as

android.control.aeAvailableTargetFpsRanges:
For devices at the LIMITED level or above:
For YUV_420_888 burst capture use case, this list will always include
(min, max) and (max, max) where min <= 15 and max = the maximum output
frame rate of the maximum YUV_420_888 output size.

I think I do now populate AE_AVAILABLE_TARGET_FPS_RANGE correctly
since "android: capabilties: Fix ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES"
in this series.

I'll try to revert this patch and add here a reference to which tests
is not pleased

Thanks
  j

> > +		 *
> > +		 * Calculate FPS as CTS does: see
> > +		 * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration()
> > +		 */
> > +		unsigned int fps = static_cast<unsigned int>
> > +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> > +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> > +			continue;
> > +
> >  		availableStreamConfigurations.push_back(entry.androidFormat);
> >  		availableStreamConfigurations.push_back(entry.resolution.width);
> >  		availableStreamConfigurations.push_back(entry.resolution.height);
> > @@ -1271,6 +1286,11 @@ int CameraCapabilities::initializeStaticMetadata()
> >  	std::vector<int64_t> minFrameDurations;
> >  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
> >  	for (const auto &entry : streamConfigurations_) {
> > +		unsigned int fps = static_cast<unsigned int>
> > +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> > +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> > +			continue;
> > +
> >  		minFrameDurations.push_back(entry.androidFormat);
> >  		minFrameDurations.push_back(entry.resolution.width);
> >  		minFrameDurations.push_back(entry.resolution.height);
>
> --
> Regards,
>
> Laurent Pinchart
Jacopo Mondi Oct. 11, 2021, 1:49 p.m. UTC | #3 
Hi Laurent,

On Wed, Oct 06, 2021 at 10:23:00AM +0200, Jacopo Mondi wrote:
> Hi Laurent,
>
> On Wed, Oct 06, 2021 at 04:45:25AM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Tue, Sep 07, 2021 at 09:41:01PM +0200, Jacopo Mondi wrote:
> > > Register as preview streams only streams capable of producing at least
> > > 30 FPS.
> > >
> > > This requirement comes from inspecting the existing HAL implementation
> > > on Intel IPU3 platform.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
> > > ---
> > >  src/android/camera_capabilities.cpp | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
> > > index c6791b9d6238..e5cfe67f9b73 100644
> > > --- a/src/android/camera_capabilities.cpp
> > > +++ b/src/android/camera_capabilities.cpp
> > > @@ -1259,6 +1259,21 @@ int CameraCapabilities::initializeStaticMetadata()
> > >  	std::vector<uint32_t> availableStreamConfigurations;
> > >  	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
> > >  	for (const auto &entry : streamConfigurations_) {
> > > +		/*
> > > +		 * Filter out YUV streams not capable of running at 30 FPS.
> > > +		 *
> > > +		 * This requirement comes from inspecting the Intel IPU3
> > > +		 * HAL implementation but no reference has been found in the
> > > +		 * metadata documentation.
> >
> > Does the requirement also come from CTS ? If so, could you please point
> > to the corresponding test(s) ? If not, why do we need this ?
> >
>
> I keep failing to find any reference to this requirement in
> documentation, and the way we populate the YUV stream combinations has
> an impact on the AE_AVAILABLE_TARGET_FPS_RANGE content as
>
> android.control.aeAvailableTargetFpsRanges:
> For devices at the LIMITED level or above:
> For YUV_420_888 burst capture use case, this list will always include
> (min, max) and (max, max) where min <= 15 and max = the maximum output
> frame rate of the maximum YUV_420_888 output size.
>
> I think I do now populate AE_AVAILABLE_TARGET_FPS_RANGE correctly
> since "android: capabilties: Fix ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES"
> in this series.
>
> I'll try to revert this patch and add here a reference to which tests
> is not pleased

10-11 15:29:33 I/ModuleListener: [75/231] android.hardware.camera2.cts.RecordingTest#testBasicRecording fail:
junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

10-11 15:30:16 I/ModuleListener: [81/231] android.hardware.camera2.cts.RecordingTest#testRecordingFramerateLowToHigh fail:
junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

10-11 15:30:18 I/ModuleListener: [82/231] android.hardware.camera2.cts.RecordingTest#testRecordingFromPersistentSurface fail:
junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

10-11 15:44:27 I/ConsoleReporter: [118/231 x86_64 CtsCameraTestCases 192.168.1.141:22] android.hardware.camera2.cts.RecordingTest#testRecordingFramerateLowToHigh fail: junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

10-11 15:44:27 I/ConsoleReporter: [129/231 x86_64 CtsCameraTestCases 192.168.1.141:22] android.hardware.camera2.cts.RecordingTest#testRecordingFromPersistentSurface fail: junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

10-11 15:44:27 I/ConsoleReporter: [162/231 x86_64 CtsCameraTestCases 192.168.1.141:22] android.hardware.camera2.cts.RecordingTest#testBasicRecording fail: junit.framework.AssertionFailedError: Frame rate range [30, 30] (for profile ID 1) must be one of the camera device available FPS range!

So it's basically all the recording tests ? This might be related to
the requirement specified in the camcoder profile (which I read about
but I'm not sure how is it defined in Android).

I'll add that "android.hardware.camera2.cts.RecordingTest#*" requires
viewfinder streams to be at 30FPS.

Thanks
   j

>
> Thanks
>   j
>
> > > +		 *
> > > +		 * Calculate FPS as CTS does: see
> > > +		 * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration()
> > > +		 */
> > > +		unsigned int fps = static_cast<unsigned int>
> > > +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> > > +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> > > +			continue;
> > > +
> > >  		availableStreamConfigurations.push_back(entry.androidFormat);
> > >  		availableStreamConfigurations.push_back(entry.resolution.width);
> > >  		availableStreamConfigurations.push_back(entry.resolution.height);
> > > @@ -1271,6 +1286,11 @@ int CameraCapabilities::initializeStaticMetadata()
> > >  	std::vector<int64_t> minFrameDurations;
> > >  	minFrameDurations.reserve(streamConfigurations_.size() * 4);
> > >  	for (const auto &entry : streamConfigurations_) {
> > > +		unsigned int fps = static_cast<unsigned int>
> > > +				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
> > > +		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
> > > +			continue;
> > > +
> > >  		minFrameDurations.push_back(entry.androidFormat);
> > >  		minFrameDurations.push_back(entry.resolution.width);
> > >  		minFrameDurations.push_back(entry.resolution.height);
> >
> > --
> > Regards,
> >
> > Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp
index c6791b9d6238..e5cfe67f9b73 100644
--- a/src/android/camera_capabilities.cpp
+++ b/src/android/camera_capabilities.cpp
@@ -1259,6 +1259,21 @@  int CameraCapabilities::initializeStaticMetadata()
 	std::vector<uint32_t> availableStreamConfigurations;
 	availableStreamConfigurations.reserve(streamConfigurations_.size() * 4);
 	for (const auto &entry : streamConfigurations_) {
+		/*
+		 * Filter out YUV streams not capable of running at 30 FPS.
+		 *
+		 * This requirement comes from inspecting the Intel IPU3
+		 * HAL implementation but no reference has been found in the
+		 * metadata documentation.
+		 *
+		 * Calculate FPS as CTS does: see
+		 * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration()
+		 */
+		unsigned int fps = static_cast<unsigned int>
+				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
+		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
+			continue;
+
 		availableStreamConfigurations.push_back(entry.androidFormat);
 		availableStreamConfigurations.push_back(entry.resolution.width);
 		availableStreamConfigurations.push_back(entry.resolution.height);
@@ -1271,6 +1286,11 @@  int CameraCapabilities::initializeStaticMetadata()
 	std::vector<int64_t> minFrameDurations;
 	minFrameDurations.reserve(streamConfigurations_.size() * 4);
 	for (const auto &entry : streamConfigurations_) {
+		unsigned int fps = static_cast<unsigned int>
+				   (floor(1e9 / entry.minFrameDurationNsec + 0.05f));
+		if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30)
+			continue;
+
 		minFrameDurations.push_back(entry.androidFormat);
 		minFrameDurations.push_back(entry.resolution.width);
 		minFrameDurations.push_back(entry.resolution.height);