| Message ID | 20220427151334.609152-1-hanlinchen@chromium.org |
|---|---|
| State | Accepted |
| Commit | 49e85fbe9c3bf59c99f7d764abc9a7294e8b5e91 |
| Headers | show |
| Series |
|
| Related | show |
Hi On 4/27/22 20:43, Han-Lin Chen via libcamera-devel wrote: > CTS calculates FPS with a rounding formula: See > Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() > > fps = floor(1e9 / minFrameDuration + 0.05f) It seems(to me at-least) that the placement of the + 0.05f adjustment is changed as per this patch. > > The android adapter reports it as the AE target FPS. The patch adjusts the > reported minimum frame duration to match the reported FPS. Hence we are not introducing any 'new adjustments' rather changing the placement of the adjustment. Given my inference is correct (of course I might be missing the entire point here, please do correct me then), I would re-pharse this commit message to reflect that this patch doesn't introduce any functional changes. android: camera_capabilities: Move minimum frame duration adjustment CTS calculates FPS with a rounding formula: See Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() fps = floor(1e9 / minFrameDuration + 0.05f) The android adapter reports it as the AE target FPS. The patch moves this adjustment while populating the stream configuration's minimum frame duration itself. What do you think ? > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/android/camera_capabilities.cpp | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 55d651f3..ee17cdc7 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -687,6 +687,14 @@ int CameraCapabilities::initializeStreamConfigurations() > minFrameDuration = minFrameDurationCap; > } > > + /* > + * Calculate FPS as CTS does and adjust the minimum > + * frame duration accordingly: see > + * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() > + */ > + minFrameDuration = > + 1e9 / static_cast<unsigned int>(floor(1e9 / minFrameDuration + 0.05f)); > + > streamConfigurations_.push_back({ > res, androidFormat, minFrameDuration, maxFrameDuration, > }); > @@ -1287,12 +1295,10 @@ int CameraCapabilities::initializeStaticMetadata() > * recording profile. Inspecting the Intel IPU3 HAL > * implementation confirms this 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)); > + unsigned int fps = > + static_cast<unsigned int>(floor(1e9 / entry.minFrameDurationNsec)); > + > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > continue; >
Hi Han-Lin, On 4/28/22 00:40, Umang Jain via libcamera-devel wrote: > Hi > > On 4/27/22 20:43, Han-Lin Chen via libcamera-devel wrote: >> CTS calculates FPS with a rounding formula: See >> Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() >> >> fps = floor(1e9 / minFrameDuration + 0.05f) > > > It seems(to me at-least) that the placement of the + 0.05f adjustment > is changed as per this patch. > >> >> The android adapter reports it as the AE target FPS. The patch >> adjusts the >> reported minimum frame duration to match the reported FPS. > > > Hence we are not introducing any 'new adjustments' rather changing the > placement of the adjustment. Being clarified by Jacopo on IRC, that the patch makes the FPS and minFrameDuration in-sync, such that the adjustment is now reflected for both. That makes perfect sense now that I am looking, hence, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > > Given my inference is correct (of course I might be missing the entire > point here, please do correct me > then), I would re-pharse this commit message to reflect that this > patch doesn't introduce any functional > changes. > > android: camera_capabilities: Move minimum frame duration adjustment > > CTS calculates FPS with a rounding formula: See > Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() > > fps = floor(1e9 / minFrameDuration + 0.05f) > > The android adapter reports it as the AE target FPS. The patch > moves this adjustment while populating the stream configuration's > minimum frame duration itself. > > What do you think ? > >> >> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> >> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> >> --- >> src/android/camera_capabilities.cpp | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/src/android/camera_capabilities.cpp >> b/src/android/camera_capabilities.cpp >> index 55d651f3..ee17cdc7 100644 >> --- a/src/android/camera_capabilities.cpp >> +++ b/src/android/camera_capabilities.cpp >> @@ -687,6 +687,14 @@ int >> CameraCapabilities::initializeStreamConfigurations() >> minFrameDuration = minFrameDurationCap; >> } >> + /* >> + * Calculate FPS as CTS does and adjust the minimum >> + * frame duration accordingly: see >> + * >> Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() >> + */ >> + minFrameDuration = >> + 1e9 / static_cast<unsigned int>(floor(1e9 / >> minFrameDuration + 0.05f)); >> + >> streamConfigurations_.push_back({ >> res, androidFormat, minFrameDuration, >> maxFrameDuration, >> }); >> @@ -1287,12 +1295,10 @@ int >> CameraCapabilities::initializeStaticMetadata() >> * recording profile. Inspecting the Intel IPU3 HAL >> * implementation confirms this 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)); >> + unsigned int fps = >> + static_cast<unsigned int>(floor(1e9 / >> entry.minFrameDurationNsec)); >> + >> if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) >> continue;
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index 55d651f3..ee17cdc7 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -687,6 +687,14 @@ int CameraCapabilities::initializeStreamConfigurations() minFrameDuration = minFrameDurationCap; } + /* + * Calculate FPS as CTS does and adjust the minimum + * frame duration accordingly: see + * Camera2SurfaceViewTestCase.java:getSuitableFpsRangeForDuration() + */ + minFrameDuration = + 1e9 / static_cast<unsigned int>(floor(1e9 / minFrameDuration + 0.05f)); + streamConfigurations_.push_back({ res, androidFormat, minFrameDuration, maxFrameDuration, }); @@ -1287,12 +1295,10 @@ int CameraCapabilities::initializeStaticMetadata() * recording profile. Inspecting the Intel IPU3 HAL * implementation confirms this 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)); + unsigned int fps = + static_cast<unsigned int>(floor(1e9 / entry.minFrameDurationNsec)); + if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) continue;