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

[Laurent Pinchart]

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 at ideasonboard.com>
> Reviewed-by: Hirokazu Honda <hiroh at 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.

-- 
Regards,

Laurent Pinchart