Message ID | 20210422094102.371772-4-paul.elder@ideasonboard.com |
---|---|
State | Accepted |
Delegated to: | Paul Elder |
Headers | show |
Series |
|
Related | show |
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
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; + /* * Register to the camera service {min, max} and {max, max} * intervals as requested by the metadata documentation.