[libcamera-devel,v2,4/6] fixup! android: camera_device: Compute frame durations
diff mbox series

Message ID 20210126173008.446321-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • android: Report frame durations
Related show

Commit Message

Jacopo Mondi Jan. 26, 2021, 5:30 p.m. UTC 
---
 src/android/camera_device.cpp | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Comments

Laurent Pinchart Feb. 9, 2021, 1:50 a.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:
> ---
>  src/android/camera_device.cpp | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index e3d43bea4700..5a8072a8a007 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		minFrameDurationUsec = frameDurationsInfo->second.min().get<int64_t>();
>  		maxFrameDurationUsec = frameDurationsInfo->second.max().get<int64_t>();
>  
> +		/*
> +		 * Adjust the minimum frame duration to comply with Android
> +		 * requirements. The camera service mandates all preview/record
> +		 * streams to have a minimum frame duration < 33,366 milliseconds
> +		 * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service
> +		 * implementation).
> +		 *
> +		 * If we're close enough (+- 500 useconds) to that value round
> +		 * the minimum frame duration of the camera to an accepted
> +		 * value.
> +		 */
> +		static constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;
> +		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;

You could write

		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;

as MIN_PREVIEW_RECORD_FPS is only used here.

> +		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {
> +			double frameDurationDelta = minFrameDurationUsec -
> +						    MAX_PREVIEW_RECORD_DURATION_US;
> +			if (frameDurationDelta < 500)
> +				minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
> +		}

Is the following more readable ?

		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&
		    minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {
			minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;

This is of course a horrible hack, but if we have no choice...

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

What's the maximum FPS we otherwise report ? Is it really that the
sensor can't reach 30fps, or is there an issue somewhere else ?

>  		/*
>  		 * The AE routine frame rate limits are computed using the frame
>  		 * duration limits, as libcamera clips the AE routine to the
Laurent Pinchart March 8, 2021, 11:47 p.m. UTC | #2 
Hi Jacopo,

On Tue, Feb 09, 2021 at 03:50:55AM +0200, Laurent Pinchart wrote:
> On Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:
> > ---
> >  src/android/camera_device.cpp | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index e3d43bea4700..5a8072a8a007 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> >  		minFrameDurationUsec = frameDurationsInfo->second.min().get<int64_t>();
> >  		maxFrameDurationUsec = frameDurationsInfo->second.max().get<int64_t>();
> >  
> > +		/*
> > +		 * Adjust the minimum frame duration to comply with Android
> > +		 * requirements. The camera service mandates all preview/record
> > +		 * streams to have a minimum frame duration < 33,366 milliseconds
> > +		 * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service
> > +		 * implementation).
> > +		 *
> > +		 * If we're close enough (+- 500 useconds) to that value round
> > +		 * the minimum frame duration of the camera to an accepted
> > +		 * value.
> > +		 */
> > +		static constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;
> > +		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;
> 
> You could write
> 
> 		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;
> 
> as MIN_PREVIEW_RECORD_FPS is only used here.
> 
> > +		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {
> > +			double frameDurationDelta = minFrameDurationUsec -
> > +						    MAX_PREVIEW_RECORD_DURATION_US;
> > +			if (frameDurationDelta < 500)
> > +				minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
> > +		}
> 
> Is the following more readable ?
> 
> 		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&
> 		    minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {
> 			minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
> 
> This is of course a horrible hack, but if we have no choice...
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> What's the maximum FPS we otherwise report ? Is it really that the
> sensor can't reach 30fps, or is there an issue somewhere else ?

Do you have any data regarding this question, why can't we achieve 30fps
without cheating ? I'm also curious, how does the Intel IPU3 HAL handle
this issue ?

> >  		/*
> >  		 * The AE routine frame rate limits are computed using the frame
> >  		 * duration limits, as libcamera clips the AE routine to the
Jacopo Mondi March 9, 2021, 7:30 a.m. UTC | #3 
Hi Laurent,

On Tue, Mar 09, 2021 at 01:47:01AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Tue, Feb 09, 2021 at 03:50:55AM +0200, Laurent Pinchart wrote:
> > On Tue, Jan 26, 2021 at 06:30:06PM +0100, Jacopo Mondi wrote:
> > > ---
> > >  src/android/camera_device.cpp | 20 ++++++++++++++++++++
> > >  1 file changed, 20 insertions(+)
> > >
> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > > index e3d43bea4700..5a8072a8a007 100644
> > > --- a/src/android/camera_device.cpp
> > > +++ b/src/android/camera_device.cpp
> > > @@ -768,6 +768,26 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
> > >  		minFrameDurationUsec = frameDurationsInfo->second.min().get<int64_t>();
> > >  		maxFrameDurationUsec = frameDurationsInfo->second.max().get<int64_t>();
> > >
> > > +		/*
> > > +		 * Adjust the minimum frame duration to comply with Android
> > > +		 * requirements. The camera service mandates all preview/record
> > > +		 * streams to have a minimum frame duration < 33,366 milliseconds
> > > +		 * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service
> > > +		 * implementation).
> > > +		 *
> > > +		 * If we're close enough (+- 500 useconds) to that value round
> > > +		 * the minimum frame duration of the camera to an accepted
> > > +		 * value.
> > > +		 */
> > > +		static constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;
> > > +		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;
> >
> > You could write
> >
> > 		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / 29.97;
> >
> > as MIN_PREVIEW_RECORD_FPS is only used here.
> >
> > > +		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {
> > > +			double frameDurationDelta = minFrameDurationUsec -
> > > +						    MAX_PREVIEW_RECORD_DURATION_US;
> > > +			if (frameDurationDelta < 500)
> > > +				minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
> > > +		}
> >
> > Is the following more readable ?
> >
> > 		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US &&
> > 		    minFrameDurationUsec < MAX_PREVIEW_RECORD_DURATION_US + 500) {
> > 			minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
> >
> > This is of course a horrible hack, but if we have no choice...
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > What's the maximum FPS we otherwise report ? Is it really that the
> > sensor can't reach 30fps, or is there an issue somewhere else ?
>
> Do you have any data regarding this question, why can't we achieve 30fps

I thought I reported it already, sorry about that. The sensor can
achieve 29.94 FPS while android wants 29.97

> without cheating ? I'm also curious, how does the Intel IPU3 HAL handle
> this issue ?
>

Afaict from looking at the static metadata they report a fram duration
of 33.33 milliseconds unconditionally (30.00.. FPS)


> > >  		/*
> > >  		 * The AE routine frame rate limits are computed using the frame
> > >  		 * duration limits, as libcamera clips the AE routine to the
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index e3d43bea4700..5a8072a8a007 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -768,6 +768,26 @@  const camera_metadata_t *CameraDevice::getStaticMetadata()
 		minFrameDurationUsec = frameDurationsInfo->second.min().get<int64_t>();
 		maxFrameDurationUsec = frameDurationsInfo->second.max().get<int64_t>();
 
+		/*
+		 * Adjust the minimum frame duration to comply with Android
+		 * requirements. The camera service mandates all preview/record
+		 * streams to have a minimum frame duration < 33,366 milliseconds
+		 * (see MAX_PREVIEW_RECORD_DURATION_NS in the camera service
+		 * implementation).
+		 *
+		 * If we're close enough (+- 500 useconds) to that value round
+		 * the minimum frame duration of the camera to an accepted
+		 * value.
+		 */
+		static constexpr double MIN_PREVIEW_RECORD_FPS = 29.97;
+		static constexpr int64_t MAX_PREVIEW_RECORD_DURATION_US = 1e6 / MIN_PREVIEW_RECORD_FPS;
+		if (minFrameDurationUsec > MAX_PREVIEW_RECORD_DURATION_US) {
+			double frameDurationDelta = minFrameDurationUsec -
+						    MAX_PREVIEW_RECORD_DURATION_US;
+			if (frameDurationDelta < 500)
+				minFrameDurationUsec = MAX_PREVIEW_RECORD_DURATION_US - 1;
+		}
+
 		/*
 		 * The AE routine frame rate limits are computed using the frame
 		 * duration limits, as libcamera clips the AE routine to the