[libcamera-devel,RFC,v2,03/12] android: CameraDevice: Report proper min and max frame durations

12070 diff mbox series

The HAL layer was getting the min and max frame durations from from the
camera, then rounding it to fps to report as available fps ranges. The
same min and max frame durations were then being reported as min and max
frame durations. Since the fps are integer values while the frame
durations are in ns, this caused a rounding error making it seem like we
were reporting an available max fps that was higher than was was allowed
by the minimum frame duration.

An example is if the minimum frame duration is reported as 33366700ns.
The HAL layer would then convert it to fps, which is 29.97, but it would
be rounded and reported as 30 fps. When 30 fps is converted to a frame
duration it is 33333333ns, which is less than the minimum frame duration
that we report. Thus the minimum frame duration that we report
contradicts the fps rage that we report.

Fix this by recalculating the frame durations based on the rounded fps
values.

This allows the following CTS test to pass:
- android.hardware.camera2.cts.SurfaceViewPreviewTest#testPreviewFpsRange

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 4 ++++
 1 file changed, 4 insertions(+)

Hi Paul,

Thank you for the patch.

On Thu, Apr 22, 2021 at 06:40:53PM +0900, Paul Elder wrote:
> The HAL layer was getting the min and max frame durations from from the

s/from from/from/

> camera, then rounding it to fps to report as available fps ranges. The
> same min and max frame durations were then being reported as min and max
> frame durations. Since the fps are integer values while the frame
> durations are in ns, this caused a rounding error making it seem like we
> were reporting an available max fps that was higher than was was allowed
> by the minimum frame duration.
> 
> An example is if the minimum frame duration is reported as 33366700ns.
> The HAL layer would then convert it to fps, which is 29.97, but it would
> be rounded and reported as 30 fps. When 30 fps is converted to a frame
> duration it is 33333333ns, which is less than the minimum frame duration
> that we report. Thus the minimum frame duration that we report
> contradicts the fps rage that we report.

s/rage/range/

> Fix this by recalculating the frame durations based on the rounded fps
> values.
> 
> This allows the following CTS test to pass:
> - android.hardware.camera2.cts.SurfaceViewPreviewTest#testPreviewFpsRange
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 76863877..a11ad848 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -900,6 +900,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		int32_t minFps = std::round(1e9 / maxFrameDurationNsec);
>  		minFps = std::max(1, minFps);
>  
> +		/* Avoid rounding errors when we reuse these variables later */

Isn't it the other way around, aren't you're forcing rounding errors ?
The change could be fine (although it seems a bit weird that CTS would
choke on this, as it's obvious that rounding errors would occur), but
the comment, and possibly the commit message, should be updated.

> +		minFrameDurationNsec = 1e9 / maxFps;
> +		maxFrameDurationNsec = 1e9 / minFps;
> +
>  		/*
>  		 * Register to the camera service {min, max} and {max, max}
>  		 * intervals as requested by the metadata documentation.

Hi Paul,

On Thu, Apr 22, 2021 at 06:40:53PM +0900, Paul Elder wrote:
> The HAL layer was getting the min and max frame durations from from the

from from

> camera, then rounding it to fps to report as available fps ranges. The
> same min and max frame durations were then being reported as min and max
> frame durations. Since the fps are integer values while the frame
> durations are in ns, this caused a rounding error making it seem like we
> were reporting an available max fps that was higher than was was allowed

s/was was/what was/

> by the minimum frame duration.
>
> An example is if the minimum frame duration is reported as 33366700ns.
> The HAL layer would then convert it to fps, which is 29.97, but it would
> be rounded and reported as 30 fps. When 30 fps is converted to a frame
> duration it is 33333333ns, which is less than the minimum frame duration
> that we report. Thus the minimum frame duration that we report
> contradicts the fps rage that we report.

Not easy to explain :)

>
> Fix this by recalculating the frame durations based on the rounded fps
> values.
>
> This allows the following CTS test to pass:
> - android.hardware.camera2.cts.SurfaceViewPreviewTest#testPreviewFpsRange
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 76863877..a11ad848 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -900,6 +900,10 @@ const camera_metadata_t *CameraDevice::getStaticMetadata()
>  		int32_t minFps = std::round(1e9 / maxFrameDurationNsec);
>  		minFps = std::max(1, minFps);
>
> +		/* Avoid rounding errors when we reuse these variables later */
> +		minFrameDurationNsec = 1e9 / maxFps;
> +		maxFrameDurationNsec = 1e9 / minFps;
> +

Agreed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>  		/*
>  		 * Register to the camera service {min, max} and {max, max}
>  		 * intervals as requested by the metadata documentation.
> --
> 2.27.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel