| Message ID | 20220209071917.559993-10-hanlinchen@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Han-Lin, Thank you for the patch. I'm CC'ing Jacopo and Umang as they've both spent a large amount of time solving issues related to frame durations. On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > It's notice that Chrome Camera App filters out the resolutions which cannot > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower > within 2%. Unless I'm mistaken this patch doesn't depend on the rest of the series, so we can review and merge it separately. > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > --- > src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- > 1 file changed, 20 insertions(+), 11 deletions(-) > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > index 55d651f3..e10ab036 100644 > --- a/src/android/camera_capabilities.cpp > +++ b/src/android/camera_capabilities.cpp > @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() > int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; > > /* > - * Cap min frame duration to 30 FPS with 1% tolerance. > + * Cap min frame duration to 30 FPS with 1% tolerance, > + * and elevate min frame duration to 30 FPS with 2% > + * tolerance. > * > * 30 frames per second has been validated as the most > * opportune frame rate for quality tuning, and power > @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() > * to the in-development configuration API rework. > */ > int64_t minFrameDurationCap = 1e9 / 30.0; > - if (minFrameDuration < minFrameDurationCap) { > - float tolerance = > - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > + float tolerance = > + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > - /* > - * If the tolerance is less than 1%, do not cap > - * the frame duration. > - */ > - if (tolerance > 1.0) > - minFrameDuration = minFrameDurationCap; > - } > + /* > + * If the tolerance is less than 1%, do not cap > + * the frame duration. > + */ > + if (tolerance > 1.0) > + minFrameDuration = minFrameDurationCap; > + > + /* > + * Some applications, ex: Chrome Camera App filters out > + * the resolutions which cannot achieve 30 FPS. Elevate > + * the minimum frame duration to 30 FPS if FPS is lower > + * within 2%. > + */ > + if (tolerance < 0 && tolerance > -2.0) > + minFrameDuration = minFrameDurationCap; > > streamConfigurations_.push_back({ > res, androidFormat, minFrameDuration, maxFrameDuration,
On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > Hi Han-Lin, > > Thank you for the patch. > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of time > solving issues related to frame durations. And now with Umang's correct address. > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > It's notice that Chrome Camera App filters out the resolutions which cannot > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower > > within 2%. > > Unless I'm mistaken this patch doesn't depend on the rest of the series, > so we can review and merge it separately. > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > --- > > src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > index 55d651f3..e10ab036 100644 > > --- a/src/android/camera_capabilities.cpp > > +++ b/src/android/camera_capabilities.cpp > > @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() > > int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; > > > > /* > > - * Cap min frame duration to 30 FPS with 1% tolerance. > > + * Cap min frame duration to 30 FPS with 1% tolerance, > > + * and elevate min frame duration to 30 FPS with 2% > > + * tolerance. > > * > > * 30 frames per second has been validated as the most > > * opportune frame rate for quality tuning, and power > > @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() > > * to the in-development configuration API rework. > > */ > > int64_t minFrameDurationCap = 1e9 / 30.0; > > - if (minFrameDuration < minFrameDurationCap) { > > - float tolerance = > > - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > + float tolerance = > > + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > - /* > > - * If the tolerance is less than 1%, do not cap > > - * the frame duration. > > - */ > > - if (tolerance > 1.0) > > - minFrameDuration = minFrameDurationCap; > > - } > > + /* > > + * If the tolerance is less than 1%, do not cap > > + * the frame duration. > > + */ > > + if (tolerance > 1.0) > > + minFrameDuration = minFrameDurationCap; > > + > > + /* > > + * Some applications, ex: Chrome Camera App filters out > > + * the resolutions which cannot achieve 30 FPS. Elevate > > + * the minimum frame duration to 30 FPS if FPS is lower > > + * within 2%. > > + */ > > + if (tolerance < 0 && tolerance > -2.0) > > + minFrameDuration = minFrameDurationCap; > > > > streamConfigurations_.push_back({ > > res, androidFormat, minFrameDuration, maxFrameDuration,
On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > Hi Han-Lin, > > > > Thank you for the patch. > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of time > > solving issues related to frame durations. > > And now with Umang's correct address. Jacopo, Umang, any feedback on this ? > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > It's notice that Chrome Camera App filters out the resolutions which cannot > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower > > > within 2%. > > > > Unless I'm mistaken this patch doesn't depend on the rest of the series, > > so we can review and merge it separately. > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > --- > > > src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > index 55d651f3..e10ab036 100644 > > > --- a/src/android/camera_capabilities.cpp > > > +++ b/src/android/camera_capabilities.cpp > > > @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() > > > int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; > > > > > > /* > > > - * Cap min frame duration to 30 FPS with 1% tolerance. > > > + * Cap min frame duration to 30 FPS with 1% tolerance, > > > + * and elevate min frame duration to 30 FPS with 2% > > > + * tolerance. > > > * > > > * 30 frames per second has been validated as the most > > > * opportune frame rate for quality tuning, and power > > > @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() > > > * to the in-development configuration API rework. > > > */ > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > - if (minFrameDuration < minFrameDurationCap) { > > > - float tolerance = > > > - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > + float tolerance = > > > + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > - /* > > > - * If the tolerance is less than 1%, do not cap > > > - * the frame duration. > > > - */ > > > - if (tolerance > 1.0) > > > - minFrameDuration = minFrameDurationCap; > > > - } > > > + /* > > > + * If the tolerance is less than 1%, do not cap > > > + * the frame duration. > > > + */ > > > + if (tolerance > 1.0) > > > + minFrameDuration = minFrameDurationCap; > > > + > > > + /* > > > + * Some applications, ex: Chrome Camera App filters out > > > + * the resolutions which cannot achieve 30 FPS. Elevate > > > + * the minimum frame duration to 30 FPS if FPS is lower > > > + * within 2%. > > > + */ > > > + if (tolerance < 0 && tolerance > -2.0) > > > + minFrameDuration = minFrameDurationCap; > > > > > > streamConfigurations_.push_back({ > > > res, androidFormat, minFrameDuration, maxFrameDuration,
Hello, sorry for being late On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > Hi Han-Lin, > > > > > > Thank you for the patch. > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of time > > > solving issues related to frame durations. > > > > And now with Umang's correct address. > > Jacopo, Umang, any feedback on this ? > Let me recap a bit first. We currently adjust all durations > 33333333*101/100 to 33333333. The idea was to avoid reporting any frame duration < 10^9/30 c7ae5a50c10de1e790855d66842192c766da4dd3 to bring the HAL in par with what the Intel HAL does. Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the situation to allow slightly faster durations to be reported, to please CTS which would have otherwise complained if the actual duration was smaller than the reported on. Hence, so far, we have been dealing with -faster- cameras than the fixed 10^9/30 limit. Umang: do you recall how did we pick the 1% positive tolerance ? The issue here is instead that some -slower- cameras might need to be adjusted and report a smaller frame duration than what they actually produce. We had a lengthy discussion in the past [1] about how to report correctly the durations of Nautilus which was reporting a duration of 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was not complaining. We ended up fixing the driver in "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka instead, if I'm not mistaken, resolutions that produce < 30 FPS are not reported as part of the preview streams list (so only resolutions up to 1080p are reported there). So I would like first to know what cameras and what resolutions is this patch trying to please. Also, reading again the thread in [1] I realized CTS allows a 1.5% tolerance " Frame duration must be in the range of [33333333, 33333333], value 34387000 is out of range [32833332, 33833332])" How was 2% picked here ? Sorry for making things difficult, but I'm always a bit concerned that moving a tiny thing here would make some CTS test failures creep in unnoticed. Thanks j [1] [libcamera-internal] Question on camera stream durations > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > It's notice that Chrome Camera App filters out the resolutions which cannot > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower > > > > within 2%. > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the series, > > > so we can review and merge it separately. > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > --- > > > > src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp > > > > index 55d651f3..e10ab036 100644 > > > > --- a/src/android/camera_capabilities.cpp > > > > +++ b/src/android/camera_capabilities.cpp > > > > @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() > > > > int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > /* > > > > - * Cap min frame duration to 30 FPS with 1% tolerance. > > > > + * Cap min frame duration to 30 FPS with 1% tolerance, > > > > + * and elevate min frame duration to 30 FPS with 2% > > > > + * tolerance. > > > > * > > > > * 30 frames per second has been validated as the most > > > > * opportune frame rate for quality tuning, and power > > > > @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() > > > > * to the in-development configuration API rework. > > > > */ > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > - if (minFrameDuration < minFrameDurationCap) { > > > > - float tolerance = > > > > - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > + float tolerance = > > > > + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > - /* > > > > - * If the tolerance is less than 1%, do not cap > > > > - * the frame duration. > > > > - */ > > > > - if (tolerance > 1.0) > > > > - minFrameDuration = minFrameDurationCap; > > > > - } > > > > + /* > > > > + * If the tolerance is less than 1%, do not cap > > > > + * the frame duration. > > > > + */ > > > > + if (tolerance > 1.0) > > > > + minFrameDuration = minFrameDurationCap; > > > > + > > > > + /* > > > > + * Some applications, ex: Chrome Camera App filters out > > > > + * the resolutions which cannot achieve 30 FPS. Elevate > > > > + * the minimum frame duration to 30 FPS if FPS is lower > > > > + * within 2%. > > > > + */ > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > + minFrameDuration = minFrameDurationCap; > > > > > > > > streamConfigurations_.push_back({ > > > > res, androidFormat, minFrameDuration, maxFrameDuration, > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, Sorry for the late reply. On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > Hello, > sorry for being late > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > Hi Han-Lin, > > > > > > > > Thank you for the patch. > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > time > > > > solving issues related to frame durations. > > > > > > And now with Umang's correct address. > > > > Jacopo, Umang, any feedback on this ? > > > > Let me recap a bit first. > > We currently adjust all durations > 33333333*101/100 to 33333333. > The idea was to avoid reporting any frame duration < 10^9/30 > c7ae5a50c10de1e790855d66842192c766da4dd3 > to bring the HAL in par with what the Intel HAL does. > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > situation to allow slightly faster durations to be reported, to please > CTS which would have otherwise complained if the actual duration was > smaller than the reported on. > > Hence, so far, we have been dealing with -faster- cameras than the > fixed 10^9/30 limit. > > Umang: do you recall how did we pick the 1% positive tolerance ? > > The issue here is instead that some -slower- cameras might need to be > adjusted and report a smaller frame duration than what they actually > produce. We had a lengthy discussion in the past [1] about how to report > correctly the durations of Nautilus which was reporting a duration of > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > not complaining. We ended up fixing the driver in > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > not reported as part of the preview streams list (so only resolutions > up to 1080p are reported there). > > So I would like first to know what cameras and what resolutions > is this patch trying to please. > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > tolerance > > " Frame duration must be in the range of [33333333, 33333333], value > 34387000 is out of range [32833332, 33833332])" > > How was 2% picked here ? > > Sorry for making things difficult, but I'm always a bit concerned that > moving a tiny thing here would make some CTS test failures creep in > unnoticed. > > Thanks > j > > [1] [libcamera-internal] Question on camera stream durations > For the front camera of soraka, thus OV5685, which reports frame duration 33336000 for resolution larger than 720p. When calculating FPS, we did a slight elevation it due to CTS in f78f714b4486dbfd62bd62d7a479abc1d98d7495: // unsigned int fps = static_cast<unsigned int> (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); It gets floor(29.997600192 + 0.5) = 30, and passes the bar. However, we still report the minFrameDuration as 33336000. I checked with the chrome team about their logic: Chrome would only try the frame rate listed in ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, which is elevated to 30 in the case, and filter out the stream with frame duration with 33336000, since its calculated FPS would be 29.97. As a result, chrome rejects resolution > 720p for the front camera. On the other hand, the back camera reports 33389000 for the full size. The 2% is from the value, which is about 1.6% from 33333333. After a rethink, maybe I should elevate the min frame duration together with where we elevate the FPS, instead of a 2% cap. Or try to centralize all adjustments on FPS/frame duration to an earlier stage. > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > It's notice that Chrome Camera App filters out the resolutions > which cannot > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > FPS is lower > > > > > within 2%. > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > series, > > > > so we can review and merge it separately. > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > --- > > > > > src/android/camera_capabilities.cpp | 31 > +++++++++++++++++++---------- > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > b/src/android/camera_capabilities.cpp > > > > > index 55d651f3..e10ab036 100644 > > > > > --- a/src/android/camera_capabilities.cpp > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > @@ -655,7 +655,9 @@ int > CameraCapabilities::initializeStreamConfigurations() > > > > > int64_t maxFrameDuration = > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > /* > > > > > - * Cap min frame duration to 30 FPS with > 1% tolerance. > > > > > + * Cap min frame duration to 30 FPS with > 1% tolerance, > > > > > + * and elevate min frame duration to 30 > FPS with 2% > > > > > + * tolerance. > > > > > * > > > > > * 30 frames per second has been validated > as the most > > > > > * opportune frame rate for quality > tuning, and power > > > > > @@ -675,17 +677,24 @@ int > CameraCapabilities::initializeStreamConfigurations() > > > > > * to the in-development configuration API > rework. > > > > > */ > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > - if (minFrameDuration < > minFrameDurationCap) { > > > > > - float tolerance = > > > > > - (minFrameDurationCap - > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > + float tolerance = > > > > > + (minFrameDurationCap - > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > - /* > > > > > - * If the tolerance is less than > 1%, do not cap > > > > > - * the frame duration. > > > > > - */ > > > > > - if (tolerance > 1.0) > > > > > - minFrameDuration = > minFrameDurationCap; > > > > > - } > > > > > + /* > > > > > + * If the tolerance is less than 1%, do > not cap > > > > > + * the frame duration. > > > > > + */ > > > > > + if (tolerance > 1.0) > > > > > + minFrameDuration = > minFrameDurationCap; > > > > > + > > > > > + /* > > > > > + * Some applications, ex: Chrome Camera > App filters out > > > > > + * the resolutions which cannot achieve 30 > FPS. Elevate > > > > > + * the minimum frame duration to 30 FPS if > FPS is lower > > > > > + * within 2%. > > > > > + */ > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > + minFrameDuration = > minFrameDurationCap; > > > > > > > > > > streamConfigurations_.push_back({ > > > > > res, androidFormat, > minFrameDuration, maxFrameDuration, > > > > -- > > Regards, > > > > Laurent Pinchart >
Hello, On 4/6/22 12:39, Jacopo Mondi wrote: > Hello, > sorry for being late > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: >> On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: >>> On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: >>>> Hi Han-Lin, >>>> >>>> Thank you for the patch. >>>> >>>> I'm CC'ing Jacopo and Umang as they've both spent a large amount of time >>>> solving issues related to frame durations. >>> And now with Umang's correct address. >> Jacopo, Umang, any feedback on this ? >> > Let me recap a bit first. > > We currently adjust all durations > 33333333*101/100 to 33333333. > The idea was to avoid reporting any frame duration < 10^9/30 > c7ae5a50c10de1e790855d66842192c766da4dd3 > to bring the HAL in par with what the Intel HAL does. > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > situation to allow slightly faster durations to be reported, to please > CTS which would have otherwise complained if the actual duration was > smaller than the reported on. > > Hence, so far, we have been dealing with -faster- cameras than the > fixed 10^9/30 limit. > > Umang: do you recall how did we pick the 1% positive tolerance ? The 1% duration was picked as per nautilus' use-case (i.e. numerical values of minFrameDuration reported on nautilus). I didn't need more than anything more than 1% hence, we agreed that it's a good tolerance % that can go in. > > The issue here is instead that some -slower- cameras might need to be > adjusted and report a smaller frame duration than what they actually > produce. We had a lengthy discussion in the past [1] about how to report > correctly the durations of Nautilus which was reporting a duration of > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > not complaining. We ended up fixing the driver in > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > not reported as part of the preview streams list (so only resolutions > up to 1080p are reported there). > > So I would like first to know what cameras and what resolutions > is this patch trying to please. > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > tolerance > > " Frame duration must be in the range of [33333333, 33333333], value > 34387000 is out of range [32833332, 33833332])" > > How was 2% picked here ? > > Sorry for making things difficult, but I'm always a bit concerned that > moving a tiny thing here would make some CTS test failures creep in > unnoticed. > > Thanks > j > > [1] [libcamera-internal] Question on camera stream durations > >>>> On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: >>>>> It's notice that Chrome Camera App filters out the resolutions which cannot >>>>> achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower >>>>> within 2%. >>>> Unless I'm mistaken this patch doesn't depend on the rest of the series, >>>> so we can review and merge it separately. >>>> >>>>> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> >>>>> --- >>>>> src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- >>>>> 1 file changed, 20 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp >>>>> index 55d651f3..e10ab036 100644 >>>>> --- a/src/android/camera_capabilities.cpp >>>>> +++ b/src/android/camera_capabilities.cpp >>>>> @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() >>>>> int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; >>>>> >>>>> /* >>>>> - * Cap min frame duration to 30 FPS with 1% tolerance. >>>>> + * Cap min frame duration to 30 FPS with 1% tolerance, >>>>> + * and elevate min frame duration to 30 FPS with 2% >>>>> + * tolerance. >>>>> * >>>>> * 30 frames per second has been validated as the most >>>>> * opportune frame rate for quality tuning, and power >>>>> @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() >>>>> * to the in-development configuration API rework. >>>>> */ >>>>> int64_t minFrameDurationCap = 1e9 / 30.0; >>>>> - if (minFrameDuration < minFrameDurationCap) { >>>>> - float tolerance = >>>>> - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; >>>>> + float tolerance = >>>>> + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; >>>>> >>>>> - /* >>>>> - * If the tolerance is less than 1%, do not cap >>>>> - * the frame duration. >>>>> - */ >>>>> - if (tolerance > 1.0) >>>>> - minFrameDuration = minFrameDurationCap; >>>>> - } >>>>> + /* >>>>> + * If the tolerance is less than 1%, do not cap >>>>> + * the frame duration. >>>>> + */ >>>>> + if (tolerance > 1.0) >>>>> + minFrameDuration = minFrameDurationCap; >>>>> + >>>>> + /* >>>>> + * Some applications, ex: Chrome Camera App filters out >>>>> + * the resolutions which cannot achieve 30 FPS. Elevate >>>>> + * the minimum frame duration to 30 FPS if FPS is lower >>>>> + * within 2%. >>>>> + */ >>>>> + if (tolerance < 0 && tolerance > -2.0) >>>>> + minFrameDuration = minFrameDurationCap; >>>>> >>>>> streamConfigurations_.push_back({ >>>>> res, androidFormat, minFrameDuration, maxFrameDuration, >> -- >> Regards, >> >> Laurent Pinchart
Hi Han-Lin, On Thu, Apr 14, 2022 at 08:34:56PM +0800, Hanlin Chen wrote: > Hi Jacopo, > Sorry for the late reply. > > On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > Hello, > > sorry for being late > > > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > > Hi Han-Lin, > > > > > > > > > > Thank you for the patch. > > > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > > time > > > > > solving issues related to frame durations. > > > > > > > > And now with Umang's correct address. > > > > > > Jacopo, Umang, any feedback on this ? > > > > > > > Let me recap a bit first. > > > > We currently adjust all durations > 33333333*101/100 to 33333333. > > The idea was to avoid reporting any frame duration < 10^9/30 > > c7ae5a50c10de1e790855d66842192c766da4dd3 > > to bring the HAL in par with what the Intel HAL does. > > > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > > situation to allow slightly faster durations to be reported, to please > > CTS which would have otherwise complained if the actual duration was > > smaller than the reported on. > > > > Hence, so far, we have been dealing with -faster- cameras than the > > fixed 10^9/30 limit. > > > > Umang: do you recall how did we pick the 1% positive tolerance ? > > > > The issue here is instead that some -slower- cameras might need to be > > adjusted and report a smaller frame duration than what they actually > > produce. We had a lengthy discussion in the past [1] about how to report > > correctly the durations of Nautilus which was reporting a duration of > > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > > not complaining. We ended up fixing the driver in > > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > > not reported as part of the preview streams list (so only resolutions > > up to 1080p are reported there). > > > > So I would like first to know what cameras and what resolutions > > is this patch trying to please. > > > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > > tolerance > > > > " Frame duration must be in the range of [33333333, 33333333], value > > 34387000 is out of range [32833332, 33833332])" > > > > How was 2% picked here ? > > > > Sorry for making things difficult, but I'm always a bit concerned that > > moving a tiny thing here would make some CTS test failures creep in > > unnoticed. > > > > Thanks > > j > > > > [1] [libcamera-internal] Question on camera stream durations > > > > For the front camera of soraka, thus OV5685, which reports frame duration > 33336000 for resolution larger than 720p. > When calculating FPS, we did a slight elevation it due to CTS in > f78f714b4486dbfd62bd62d7a479abc1d98d7495: > > // unsigned int fps = static_cast<unsigned int> (floor(1e9 / > entry.minFrameDurationNsec + 0.05f)); > > It gets floor(29.997600192 + 0.5) = 30, and passes the bar. > However, we still report the minFrameDuration as 33336000. > > I checked with the chrome team about their logic: > Chrome would only try the frame rate listed in > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > which is elevated to 30 in the case, and filter out the stream with frame > duration with 33336000, since its calculated FPS would be 29.97. As a > result, chrome rejects resolution > 720p for the front camera. Is this something new ? I recall we had 1080p for preview on Soraka. Am I mistaken ? > On the other hand, the back camera reports 33389000 for the full size. The > 2% is from the value, which is about 1.6% from 33333333. Full sizes are in facts not reported as 30-FPS capable streams, if I recall correctly ? > > After a rethink, maybe I should elevate the min frame duration together > with where we elevate the FPS, instead of a 2% cap. > Or try to centralize all adjustments on FPS/frame duration to an earlier > stage. > It would be indeed better to centralize all the adjustments, probably at CameraCapabilities::initializeStreamConfigurations() time. The way we handle minFrameDurations is CameraCapabilities::initializeStreamConfigurations() { int64_t minFrameDuration = frameDurations->second.min().get<int64_t>() * 1000; .. int64_t minFrameDurationCap = 1e9 / 30.0; if (minFrameDuration < minFrameDurationCap) { float tolerance = (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; /* * If the tolerance is less than 1%, do not cap * the frame duration. */ if (tolerance > 1.0) minFrameDuration = minFrameDurationCap; } streamConfigurations_.push_back({ res, androidFormat, minFrameDuration, maxFrameDuration, }); So here we adjust faster cameras to report max 30 FPS +- 1% This was due to get in par with the Intel HAL, but might cause errors as some streams are -faster- than what we report here. When populating the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT camera property, as you noticed, we add a 0.05 to the calculated FPS to accept as 30-fps-capable streams that can do > 29.95. unsigned int fps = static_cast<unsigned int> (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) continue; And we use the adjusted FPS to later populate AE_AVAILABLE_TARGET_FPS_RANGE /* * Collect the FPS of the maximum YUV output size to populate * AE_AVAILABLE_TARGET_FPS_RANGE */ if (entry.androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888 && entry.resolution > maxYUVSize) { maxYUVSize = entry.resolution; maxYUVFps = fps; } int32_t availableAeFpsTarget[] = { minFps, maxYUVFps, maxYUVFps, maxYUVFps, }; staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, availableAeFpsTarget); Then we populate ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS with the entry.minFrameDurationNsec value, without adjusting it. /* Per-stream durations. */ minFrameDurations.push_back(entry.androidFormat); minFrameDurations.push_back(entry.resolution.width); minFrameDurations.push_back(entry.resolution.height); minFrameDurations.push_back(entry.minFrameDurationNsec); staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, minFrameDurations); So yes, we report a faster FPS in AE_AVAILABLE_TARGET_FPS_RANGE than the min frame duration reported in ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS. Now, a 2% tolerance seems a lot, but CTS has 1.5%, so it might be acceptable. I'm just afraid that if we adjust too much, then CTS will detect that we report a min duration that doesn't match the actual measured one and complains because a stream slower than expected is detected. Anyway, yes, we should adjust frame durations in a single place to avoid reporting inconsistent information in the camera capabilities. The question is how much to adjust ? :) } > > > > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > > It's notice that Chrome Camera App filters out the resolutions > > which cannot > > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > > FPS is lower > > > > > > within 2%. > > > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > > series, > > > > > so we can review and merge it separately. > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > --- > > > > > > src/android/camera_capabilities.cpp | 31 > > +++++++++++++++++++---------- > > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > > b/src/android/camera_capabilities.cpp > > > > > > index 55d651f3..e10ab036 100644 > > > > > > --- a/src/android/camera_capabilities.cpp > > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > > @@ -655,7 +655,9 @@ int > > CameraCapabilities::initializeStreamConfigurations() > > > > > > int64_t maxFrameDuration = > > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > > > /* > > > > > > - * Cap min frame duration to 30 FPS with > > 1% tolerance. > > > > > > + * Cap min frame duration to 30 FPS with > > 1% tolerance, > > > > > > + * and elevate min frame duration to 30 > > FPS with 2% > > > > > > + * tolerance. > > > > > > * > > > > > > * 30 frames per second has been validated > > as the most > > > > > > * opportune frame rate for quality > > tuning, and power > > > > > > @@ -675,17 +677,24 @@ int > > CameraCapabilities::initializeStreamConfigurations() > > > > > > * to the in-development configuration API > > rework. > > > > > > */ > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > > - if (minFrameDuration < > > minFrameDurationCap) { > > > > > > - float tolerance = > > > > > > - (minFrameDurationCap - > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > + float tolerance = > > > > > > + (minFrameDurationCap - > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > > > - /* > > > > > > - * If the tolerance is less than > > 1%, do not cap > > > > > > - * the frame duration. > > > > > > - */ > > > > > > - if (tolerance > 1.0) > > > > > > - minFrameDuration = > > minFrameDurationCap; > > > > > > - } > > > > > > + /* > > > > > > + * If the tolerance is less than 1%, do > > not cap > > > > > > + * the frame duration. > > > > > > + */ > > > > > > + if (tolerance > 1.0) > > > > > > + minFrameDuration = > > minFrameDurationCap; > > > > > > + > > > > > > + /* > > > > > > + * Some applications, ex: Chrome Camera > > App filters out > > > > > > + * the resolutions which cannot achieve 30 > > FPS. Elevate > > > > > > + * the minimum frame duration to 30 FPS if > > FPS is lower > > > > > > + * within 2%. > > > > > > + */ > > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > > + minFrameDuration = > > minFrameDurationCap; > > > > > > > > > > > > streamConfigurations_.push_back({ > > > > > > res, androidFormat, > > minFrameDuration, maxFrameDuration, > > > > > > -- > > > Regards, > > > > > > Laurent Pinchart > >
Hi Jacopo, On Fri, Apr 15, 2022 at 4:29 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Han-Lin, > > On Thu, Apr 14, 2022 at 08:34:56PM +0800, Hanlin Chen wrote: > > Hi Jacopo, > > Sorry for the late reply. > > > > On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Hello, > > > sorry for being late > > > > > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > > > Hi Han-Lin, > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > > > time > > > > > > solving issues related to frame durations. > > > > > > > > > > And now with Umang's correct address. > > > > > > > > Jacopo, Umang, any feedback on this ? > > > > > > > > > > Let me recap a bit first. > > > > > > We currently adjust all durations > 33333333*101/100 to 33333333. > > > The idea was to avoid reporting any frame duration < 10^9/30 > > > c7ae5a50c10de1e790855d66842192c766da4dd3 > > > to bring the HAL in par with what the Intel HAL does. > > > > > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > > > situation to allow slightly faster durations to be reported, to please > > > CTS which would have otherwise complained if the actual duration was > > > smaller than the reported on. > > > > > > Hence, so far, we have been dealing with -faster- cameras than the > > > fixed 10^9/30 limit. > > > > > > Umang: do you recall how did we pick the 1% positive tolerance ? > > > > > > The issue here is instead that some -slower- cameras might need to be > > > adjusted and report a smaller frame duration than what they actually > > > produce. We had a lengthy discussion in the past [1] about how to report > > > correctly the durations of Nautilus which was reporting a duration of > > > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > > > not complaining. We ended up fixing the driver in > > > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > > > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > > > not reported as part of the preview streams list (so only resolutions > > > up to 1080p are reported there). > > > > > > So I would like first to know what cameras and what resolutions > > > is this patch trying to please. > > > > > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > > > tolerance > > > > > > " Frame duration must be in the range of [33333333, 33333333], value > > > 34387000 is out of range [32833332, 33833332])" > > > > > > How was 2% picked here ? > > > > > > Sorry for making things difficult, but I'm always a bit concerned that > > > moving a tiny thing here would make some CTS test failures creep in > > > unnoticed. > > > > > > Thanks > > > j > > > > > > [1] [libcamera-internal] Question on camera stream durations > > > > > > > For the front camera of soraka, thus OV5685, which reports frame duration > > 33336000 for resolution larger than 720p. > > When calculating FPS, we did a slight elevation it due to CTS in > > f78f714b4486dbfd62bd62d7a479abc1d98d7495: > > > > // unsigned int fps = static_cast<unsigned int> (floor(1e9 / > > entry.minFrameDurationNsec + 0.05f)); > > > > It gets floor(29.997600192 + 0.5) = 30, and passes the bar. > > However, we still report the minFrameDuration as 33336000. > > > > I checked with the chrome team about their logic: > > Chrome would only try the frame rate listed in > > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > which is elevated to 30 in the case, and filter out the stream with frame > > duration with 33336000, since its calculated FPS would be 29.97. As a > > result, chrome rejects resolution > 720p for the front camera. > > Is this something new ? I recall we had 1080p for preview on Soraka. > Am I mistaken ? CCA will accept 1080p but silently fall back to 480p. I notice it by checking its expert mode accidentally. > > > On the other hand, the back camera reports 33389000 for the full size. The > > 2% is from the value, which is about 1.6% from 33333333. > > Full sizes are in facts not reported as 30-FPS capable streams, if I > recall correctly ? > > > > > After a rethink, maybe I should elevate the min frame duration together > > with where we elevate the FPS, instead of a 2% cap. > > Or try to centralize all adjustments on FPS/frame duration to an earlier > > stage. > > > > It would be indeed better to centralize all the adjustments, probably > at CameraCapabilities::initializeStreamConfigurations() time. > > The way we handle minFrameDurations is > > CameraCapabilities::initializeStreamConfigurations() { > int64_t minFrameDuration = frameDurations->second.min().get<int64_t>() * 1000; > .. > > int64_t minFrameDurationCap = 1e9 / 30.0; > if (minFrameDuration < minFrameDurationCap) { > float tolerance = > (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > /* > * If the tolerance is less than 1%, do not cap > * the frame duration. > */ > if (tolerance > 1.0) > minFrameDuration = minFrameDurationCap; > } > > streamConfigurations_.push_back({ > res, androidFormat, minFrameDuration, maxFrameDuration, > }); > > So here we adjust faster cameras to report max 30 FPS +- 1% > This was due to get in par with the Intel HAL, but might cause errors > as some streams are -faster- than what we report here. > > When populating the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT > camera property, as you noticed, we add a 0.05 to the calculated FPS to > accept as 30-fps-capable streams that can do > 29.95. > > unsigned int fps = static_cast<unsigned int> > (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > continue; > > And we use the adjusted FPS to later populate > AE_AVAILABLE_TARGET_FPS_RANGE > > /* > * Collect the FPS of the maximum YUV output size to populate > * AE_AVAILABLE_TARGET_FPS_RANGE > */ > if (entry.androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888 && > entry.resolution > maxYUVSize) { > maxYUVSize = entry.resolution; > maxYUVFps = fps; > } > > int32_t availableAeFpsTarget[] = { > minFps, maxYUVFps, maxYUVFps, maxYUVFps, > }; > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > availableAeFpsTarget); > > > Then we populate ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS with the > entry.minFrameDurationNsec value, without adjusting it. > > /* Per-stream durations. */ > minFrameDurations.push_back(entry.androidFormat); > minFrameDurations.push_back(entry.resolution.width); > minFrameDurations.push_back(entry.resolution.height); > minFrameDurations.push_back(entry.minFrameDurationNsec); > > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > minFrameDurations); > > So yes, we report a faster FPS in AE_AVAILABLE_TARGET_FPS_RANGE than the min > frame duration reported in ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS. > > Now, a 2% tolerance seems a lot, but CTS has 1.5%, so it might be > acceptable. > > I'm just afraid that if we adjust too much, then CTS will > detect that we report a min duration that doesn't match the actual > measured one and complains because a stream slower than expected is > detected. > > Anyway, yes, we should adjust frame durations in a single place to > avoid reporting inconsistent information in the camera capabilities. > The question is how much to adjust ? :) Yes, I was thinking of elevating it to where we elevate the FPS. Maybe: unsigned int fps = static_cast<unsigned int> (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); /* * Since CTS calculate FPS with additional 0.05f before taking * floor. Adjust the reported min frame duration accordingly. */ int64_t adjustedMinFrameDurationNsec = 1e9 / fps; if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) continue; And then we can abandon the 2% in the patch. > > } > > > > > > > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > > > It's notice that Chrome Camera App filters out the resolutions > > > which cannot > > > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > > > FPS is lower > > > > > > > within 2%. > > > > > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > > > series, > > > > > > so we can review and merge it separately. > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > --- > > > > > > > src/android/camera_capabilities.cpp | 31 > > > +++++++++++++++++++---------- > > > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > > > b/src/android/camera_capabilities.cpp > > > > > > > index 55d651f3..e10ab036 100644 > > > > > > > --- a/src/android/camera_capabilities.cpp > > > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > > > @@ -655,7 +655,9 @@ int > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > int64_t maxFrameDuration = > > > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > > > > > /* > > > > > > > - * Cap min frame duration to 30 FPS with > > > 1% tolerance. > > > > > > > + * Cap min frame duration to 30 FPS with > > > 1% tolerance, > > > > > > > + * and elevate min frame duration to 30 > > > FPS with 2% > > > > > > > + * tolerance. > > > > > > > * > > > > > > > * 30 frames per second has been validated > > > as the most > > > > > > > * opportune frame rate for quality > > > tuning, and power > > > > > > > @@ -675,17 +677,24 @@ int > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > * to the in-development configuration API > > > rework. > > > > > > > */ > > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > > > - if (minFrameDuration < > > > minFrameDurationCap) { > > > > > > > - float tolerance = > > > > > > > - (minFrameDurationCap - > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > + float tolerance = > > > > > > > + (minFrameDurationCap - > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > > > > > - /* > > > > > > > - * If the tolerance is less than > > > 1%, do not cap > > > > > > > - * the frame duration. > > > > > > > - */ > > > > > > > - if (tolerance > 1.0) > > > > > > > - minFrameDuration = > > > minFrameDurationCap; > > > > > > > - } > > > > > > > + /* > > > > > > > + * If the tolerance is less than 1%, do > > > not cap > > > > > > > + * the frame duration. > > > > > > > + */ > > > > > > > + if (tolerance > 1.0) > > > > > > > + minFrameDuration = > > > minFrameDurationCap; > > > > > > > + > > > > > > > + /* > > > > > > > + * Some applications, ex: Chrome Camera > > > App filters out > > > > > > > + * the resolutions which cannot achieve 30 > > > FPS. Elevate > > > > > > > + * the minimum frame duration to 30 FPS if > > > FPS is lower > > > > > > > + * within 2%. > > > > > > > + */ > > > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > > > + minFrameDuration = > > > minFrameDurationCap; > > > > > > > > > > > > > > streamConfigurations_.push_back({ > > > > > > > res, androidFormat, > > > minFrameDuration, maxFrameDuration, > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart > > >
Hi Jacopo, Sorry for the separate email. On Fri, Apr 15, 2022 at 4:29 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Han-Lin, > > On Thu, Apr 14, 2022 at 08:34:56PM +0800, Hanlin Chen wrote: > > Hi Jacopo, > > Sorry for the late reply. > > > > On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > Hello, > > > sorry for being late > > > > > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > > > Hi Han-Lin, > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > > > time > > > > > > solving issues related to frame durations. > > > > > > > > > > And now with Umang's correct address. > > > > > > > > Jacopo, Umang, any feedback on this ? > > > > > > > > > > Let me recap a bit first. > > > > > > We currently adjust all durations > 33333333*101/100 to 33333333. > > > The idea was to avoid reporting any frame duration < 10^9/30 > > > c7ae5a50c10de1e790855d66842192c766da4dd3 > > > to bring the HAL in par with what the Intel HAL does. > > > > > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > > > situation to allow slightly faster durations to be reported, to please > > > CTS which would have otherwise complained if the actual duration was > > > smaller than the reported on. > > > > > > Hence, so far, we have been dealing with -faster- cameras than the > > > fixed 10^9/30 limit. > > > > > > Umang: do you recall how did we pick the 1% positive tolerance ? > > > > > > The issue here is instead that some -slower- cameras might need to be > > > adjusted and report a smaller frame duration than what they actually > > > produce. We had a lengthy discussion in the past [1] about how to report > > > correctly the durations of Nautilus which was reporting a duration of > > > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > > > not complaining. We ended up fixing the driver in > > > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > > > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > > > not reported as part of the preview streams list (so only resolutions > > > up to 1080p are reported there). > > > > > > So I would like first to know what cameras and what resolutions > > > is this patch trying to please. > > > > > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > > > tolerance > > > > > > " Frame duration must be in the range of [33333333, 33333333], value > > > 34387000 is out of range [32833332, 33833332])" > > > > > > How was 2% picked here ? > > > > > > Sorry for making things difficult, but I'm always a bit concerned that > > > moving a tiny thing here would make some CTS test failures creep in > > > unnoticed. > > > > > > Thanks > > > j > > > > > > [1] [libcamera-internal] Question on camera stream durations > > > > > > > For the front camera of soraka, thus OV5685, which reports frame duration > > 33336000 for resolution larger than 720p. > > When calculating FPS, we did a slight elevation it due to CTS in > > f78f714b4486dbfd62bd62d7a479abc1d98d7495: > > > > // unsigned int fps = static_cast<unsigned int> (floor(1e9 / > > entry.minFrameDurationNsec + 0.05f)); > > > > It gets floor(29.997600192 + 0.5) = 30, and passes the bar. > > However, we still report the minFrameDuration as 33336000. > > > > I checked with the chrome team about their logic: > > Chrome would only try the frame rate listed in > > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > which is elevated to 30 in the case, and filter out the stream with frame > > duration with 33336000, since its calculated FPS would be 29.97. As a > > result, chrome rejects resolution > 720p for the front camera. > > Is this something new ? I recall we had 1080p for preview on Soraka. > Am I mistaken ? > > > On the other hand, the back camera reports 33389000 for the full size. The > > 2% is from the value, which is about 1.6% from 33333333. > > Full sizes are in facts not reported as 30-FPS capable streams, if I > recall correctly ? Another reason is due to the configuration file. The config tuned by Intel always sets the sensor to full size. Since we calculate the FrameDurationLimit by sensor size. The min duration would always be 33389000. I suppose this is why shipped soraka does not encounter the 1% faster problem in CTS, since it's actually slower. It's a little awkward, since 1e9 / 33389000 = 29.49983, slightly less than the bar 29.5. =.= The only way I can think of is to change the FrameDurationLimit calculation in close sourced IPA. > > > > > After a rethink, maybe I should elevate the min frame duration together > > with where we elevate the FPS, instead of a 2% cap. > > Or try to centralize all adjustments on FPS/frame duration to an earlier > > stage. > > > > It would be indeed better to centralize all the adjustments, probably > at CameraCapabilities::initializeStreamConfigurations() time. > > The way we handle minFrameDurations is > > CameraCapabilities::initializeStreamConfigurations() { > int64_t minFrameDuration = frameDurations->second.min().get<int64_t>() * 1000; > .. > > int64_t minFrameDurationCap = 1e9 / 30.0; > if (minFrameDuration < minFrameDurationCap) { > float tolerance = > (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > /* > * If the tolerance is less than 1%, do not cap > * the frame duration. > */ > if (tolerance > 1.0) > minFrameDuration = minFrameDurationCap; > } > > streamConfigurations_.push_back({ > res, androidFormat, minFrameDuration, maxFrameDuration, > }); > > So here we adjust faster cameras to report max 30 FPS +- 1% > This was due to get in par with the Intel HAL, but might cause errors > as some streams are -faster- than what we report here. > > When populating the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT > camera property, as you noticed, we add a 0.05 to the calculated FPS to > accept as 30-fps-capable streams that can do > 29.95. > > unsigned int fps = static_cast<unsigned int> > (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > continue; > > And we use the adjusted FPS to later populate > AE_AVAILABLE_TARGET_FPS_RANGE > > /* > * Collect the FPS of the maximum YUV output size to populate > * AE_AVAILABLE_TARGET_FPS_RANGE > */ > if (entry.androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888 && > entry.resolution > maxYUVSize) { > maxYUVSize = entry.resolution; > maxYUVFps = fps; > } > > int32_t availableAeFpsTarget[] = { > minFps, maxYUVFps, maxYUVFps, maxYUVFps, > }; > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > availableAeFpsTarget); > > > Then we populate ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS with the > entry.minFrameDurationNsec value, without adjusting it. > > /* Per-stream durations. */ > minFrameDurations.push_back(entry.androidFormat); > minFrameDurations.push_back(entry.resolution.width); > minFrameDurations.push_back(entry.resolution.height); > minFrameDurations.push_back(entry.minFrameDurationNsec); > > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > minFrameDurations); > > So yes, we report a faster FPS in AE_AVAILABLE_TARGET_FPS_RANGE than the min > frame duration reported in ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS. > > Now, a 2% tolerance seems a lot, but CTS has 1.5%, so it might be > acceptable. > > I'm just afraid that if we adjust too much, then CTS will > detect that we report a min duration that doesn't match the actual > measured one and complains because a stream slower than expected is > detected. > > Anyway, yes, we should adjust frame durations in a single place to > avoid reporting inconsistent information in the camera capabilities. > The question is how much to adjust ? :) > > } > > > > > > > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > > > It's notice that Chrome Camera App filters out the resolutions > > > which cannot > > > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > > > FPS is lower > > > > > > > within 2%. > > > > > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > > > series, > > > > > > so we can review and merge it separately. > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > --- > > > > > > > src/android/camera_capabilities.cpp | 31 > > > +++++++++++++++++++---------- > > > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > > > b/src/android/camera_capabilities.cpp > > > > > > > index 55d651f3..e10ab036 100644 > > > > > > > --- a/src/android/camera_capabilities.cpp > > > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > > > @@ -655,7 +655,9 @@ int > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > int64_t maxFrameDuration = > > > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > > > > > /* > > > > > > > - * Cap min frame duration to 30 FPS with > > > 1% tolerance. > > > > > > > + * Cap min frame duration to 30 FPS with > > > 1% tolerance, > > > > > > > + * and elevate min frame duration to 30 > > > FPS with 2% > > > > > > > + * tolerance. > > > > > > > * > > > > > > > * 30 frames per second has been validated > > > as the most > > > > > > > * opportune frame rate for quality > > > tuning, and power > > > > > > > @@ -675,17 +677,24 @@ int > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > * to the in-development configuration API > > > rework. > > > > > > > */ > > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > > > - if (minFrameDuration < > > > minFrameDurationCap) { > > > > > > > - float tolerance = > > > > > > > - (minFrameDurationCap - > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > + float tolerance = > > > > > > > + (minFrameDurationCap - > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > > > > > - /* > > > > > > > - * If the tolerance is less than > > > 1%, do not cap > > > > > > > - * the frame duration. > > > > > > > - */ > > > > > > > - if (tolerance > 1.0) > > > > > > > - minFrameDuration = > > > minFrameDurationCap; > > > > > > > - } > > > > > > > + /* > > > > > > > + * If the tolerance is less than 1%, do > > > not cap > > > > > > > + * the frame duration. > > > > > > > + */ > > > > > > > + if (tolerance > 1.0) > > > > > > > + minFrameDuration = > > > minFrameDurationCap; > > > > > > > + > > > > > > > + /* > > > > > > > + * Some applications, ex: Chrome Camera > > > App filters out > > > > > > > + * the resolutions which cannot achieve 30 > > > FPS. Elevate > > > > > > > + * the minimum frame duration to 30 FPS if > > > FPS is lower > > > > > > > + * within 2%. > > > > > > > + */ > > > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > > > + minFrameDuration = > > > minFrameDurationCap; > > > > > > > > > > > > > > streamConfigurations_.push_back({ > > > > > > > res, androidFormat, > > > minFrameDuration, maxFrameDuration, > > > > > > > > -- > > > > Regards, > > > > > > > > Laurent Pinchart > > >
Hi Han-Lin, On Fri, Apr 15, 2022 at 12:51:45PM +0800, Hanlin Chen wrote: > Hi Jacopo, > Sorry for the separate email. > > On Fri, Apr 15, 2022 at 4:29 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Han-Lin, > > > > On Thu, Apr 14, 2022 at 08:34:56PM +0800, Hanlin Chen wrote: > > > Hi Jacopo, > > > Sorry for the late reply. > > > > > > On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > Hello, > > > > sorry for being late > > > > > > > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > > > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > > > > Hi Han-Lin, > > > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > > > > time > > > > > > > solving issues related to frame durations. > > > > > > > > > > > > And now with Umang's correct address. > > > > > > > > > > Jacopo, Umang, any feedback on this ? > > > > > > > > > > > > > Let me recap a bit first. > > > > > > > > We currently adjust all durations > 33333333*101/100 to 33333333. > > > > The idea was to avoid reporting any frame duration < 10^9/30 > > > > c7ae5a50c10de1e790855d66842192c766da4dd3 > > > > to bring the HAL in par with what the Intel HAL does. > > > > > > > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > > > > situation to allow slightly faster durations to be reported, to please > > > > CTS which would have otherwise complained if the actual duration was > > > > smaller than the reported on. > > > > > > > > Hence, so far, we have been dealing with -faster- cameras than the > > > > fixed 10^9/30 limit. > > > > > > > > Umang: do you recall how did we pick the 1% positive tolerance ? > > > > > > > > The issue here is instead that some -slower- cameras might need to be > > > > adjusted and report a smaller frame duration than what they actually > > > > produce. We had a lengthy discussion in the past [1] about how to report > > > > correctly the durations of Nautilus which was reporting a duration of > > > > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > > > > not complaining. We ended up fixing the driver in > > > > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > > > > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > > > > not reported as part of the preview streams list (so only resolutions > > > > up to 1080p are reported there). > > > > > > > > So I would like first to know what cameras and what resolutions > > > > is this patch trying to please. > > > > > > > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > > > > tolerance > > > > > > > > " Frame duration must be in the range of [33333333, 33333333], value > > > > 34387000 is out of range [32833332, 33833332])" > > > > > > > > How was 2% picked here ? > > > > > > > > Sorry for making things difficult, but I'm always a bit concerned that > > > > moving a tiny thing here would make some CTS test failures creep in > > > > unnoticed. > > > > > > > > Thanks > > > > j > > > > > > > > [1] [libcamera-internal] Question on camera stream durations > > > > > > > > > > For the front camera of soraka, thus OV5685, which reports frame duration > > > 33336000 for resolution larger than 720p. > > > When calculating FPS, we did a slight elevation it due to CTS in > > > f78f714b4486dbfd62bd62d7a479abc1d98d7495: > > > > > > // unsigned int fps = static_cast<unsigned int> (floor(1e9 / > > > entry.minFrameDurationNsec + 0.05f)); > > > > > > It gets floor(29.997600192 + 0.5) = 30, and passes the bar. > > > However, we still report the minFrameDuration as 33336000. > > > > > > I checked with the chrome team about their logic: > > > Chrome would only try the frame rate listed in > > > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > > which is elevated to 30 in the case, and filter out the stream with frame > > > duration with 33336000, since its calculated FPS would be 29.97. As a > > > result, chrome rejects resolution > 720p for the front camera. > > > > Is this something new ? I recall we had 1080p for preview on Soraka. > > Am I mistaken ? > > > > > On the other hand, the back camera reports 33389000 for the full size. The > > > 2% is from the value, which is about 1.6% from 33333333. > > > > Full sizes are in facts not reported as 30-FPS capable streams, if I > > recall correctly ? > > Another reason is due to the configuration file. The config tuned by > Intel always sets the sensor to full size. Almost. In the revision I have here for ov13858 9 modes gets generated from smaller sensor resolutions. It only happens to small-ish modes though [1] > Since we calculate the FrameDurationLimit by sensor size. The min > duration would always be 33389000. > I suppose this is why shipped soraka does not encounter the 1% faster > problem in CTS, since it's actually slower. > It's a little awkward, since 1e9 / 33389000 = 29.49983, slightly less > than the bar 29.5. =.= Be aware that the IPU pipeline handler tries to use a sensor resolution as close as possible to the maximum requested size src/libcamera/pipeline/ipu3/ipu3.cpp: /* * Generate raw configuration from CIO2. * * The output YUV streams will be limited in size to the maximum frame * size requested for the RAW stream, if present. * * If no raw stream is requested generate a size as large as the maximum * requested YUV size aligned to the ImgU constraints and bound by the * sensor's maximum resolution. See * https://bugs.libcamera.org/show_bug.cgi?id=32 */ if (rawSize.isNull()) rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, ImgUDevice::kIFMaxCropHeight }) .grownBy({ ImgUDevice::kOutputMarginWidth, ImgUDevice::kOutputMarginHeight }) .boundedTo(data_->cio2_.sensor()->resolution()); And looking at the ov13858 driver supported modes static const struct ov13858_mode supported_modes[] = { { .width = 4224, .height = 3136, .vts_def = OV13858_VTS_30FPS, .vts_min = OV13858_VTS_30FPS, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_4224x3136_regs), .regs = mode_4224x3136_regs, }, .link_freq_index = OV13858_LINK_FREQ_INDEX_0, }, { .width = 2112, .height = 1568, .vts_def = OV13858_VTS_30FPS, .vts_min = 1608, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_2112x1568_regs), .regs = mode_2112x1568_regs, }, .link_freq_index = OV13858_LINK_FREQ_INDEX_1, }, { .width = 2112, .height = 1188, .vts_def = OV13858_VTS_30FPS, .vts_min = 1608, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_2112x1188_regs), .regs = mode_2112x1188_regs, }, .link_freq_index = OV13858_LINK_FREQ_INDEX_1, }, { .width = 1056, .height = 784, .vts_def = OV13858_VTS_30FPS, .vts_min = 804, .reg_list = { .num_of_regs = ARRAY_SIZE(mode_1056x784_regs), .regs = mode_1056x784_regs, }, .link_freq_index = OV13858_LINK_FREQ_INDEX_1, } }; We might get to use full size only for full-resolution mode. (not verified, only by looking at the code) Clarifying what sensor mode gets selected for what output mode, and what is the reported frame durations for each sensor mode, might help clarify what actually happens. Looking at numbers from "Question on camera stream durations" [2] ov13858 can do 60fps@1080p (which is likely produced by 2112x1568 sensor mode), and can do 29@full-res. Hence I'm missing why 1080p doesn't get registered as 30-FPS-capable in you case. For the Soraka ov5670 front camera that instead can do 33336000 for all modes > 720p so far we got away with the 0.05 rounding, but as you said we miss adjusting the AE_AVAILABLE_TARGET_FPS_RANGE. > The only way I can think of is to change the FrameDurationLimit > calculation in close sourced IPA. As a general consideration, this is an android specific requirement and I see it better placed in the HAL. However the closed source IPA serves only for Android so far... What I would do is: - Move the 0.05 adjustment to when minFrameDuration is calculated, so that we keep AE_AVAILABLE_TARGET_FPS_RANGE and ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT in sync. The adjustment can be expressed as a percentage, and since CTS accepts a 1.5% tolerance we could do the same (which means adjusting to 30FPS streams that can only do 29.55 which seems -a lot- of adjustment to me). - Clarify how 1080p gets generated on Soraka ov13858, from which sensor mode and what durations are reported. From the calculations in [2] it should be capable of 60 FPS (capped at 30), but you see CCA falling back to 480p. Apart from this, if sensor full resolution can do 29.94 I think a tiny 0.2% adjustment is fine as we can register full resolution as 30-FPS capable if CTS doesn't complain. Be aware though that full resolution modes for both the front and back camera are reported as being 24FPS in the Intel camera HAL as per the discussion in [2] (this is said to be because of the ImgU processing bandwidth limitations iirc) so if we want to get in-par we should really only care about 1080p being @30FPS. Thanks j > > > > > > > > > After a rethink, maybe I should elevate the min frame duration together > > > with where we elevate the FPS, instead of a 2% cap. > > > Or try to centralize all adjustments on FPS/frame duration to an earlier > > > stage. > > > [1] cat ./media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml | grep -A3 \<input | grep -v 4224 | grep \<input -A3 <input width="2112" height="1568" format="CIO2" /> <vf width="960" height="1280" format="NV12" peer="vf"/> <main width="960" height="1280" format="NV12" peer="main"/> -- <input width="2112" height="1568" format="CIO2" /> <vf width="720" height="1280" format="NV12" peer="vf"/> <main width="960" height="1280" format="NV12" peer="main"/> -- <input width="2112" height="1568" format="CIO2" /> <vf width="480" height="640" format="NV12" peer="vf"/> <main width="960" height="1280" format="NV12" peer="main"/> -- <input width="2112" height="1568" format="CIO2" /> <vf width="480" height="640" format="NV12" peer="vf"/> <main width="720" height="1280" format="NV12" peer="main"/> -- <input width="1056" height="784" format="CIO2" /> <vf width="480" height="640" format="NV12" peer="vf"/> <main width="480" height="640" format="NV12" peer="main"/> -- <input width="2112" height="1568" format="CIO2" /> <vf width="240" height="320" format="NV12" peer="vf"/> <main width="960" height="1280" format="NV12" peer="main"/> -- <input width="2112" height="1568" format="CIO2" /> <vf width="240" height="320" format="NV12" peer="vf"/> <main width="720" height="1280" format="NV12" peer="main"/> -- <input width="1056" height="784" format="CIO2" /> <vf width="240" height="320" format="NV12" peer="vf"/> <main width="480" height="640" format="NV12" peer="main"/> -- <input width="1056" height="784" format="CIO2" /> <vf width="240" height="320" format="NV12" peer="vf"/> <main width="240" height="320" format="NV12" peer="main"/> [2] - ov5670 (320x240)[16149000]@61 (640x480)[16149000]@61 (1280x720)[33336000]@30 (1280x960)[33336000]@30 (1920x1080)[33336000]@30 (2560x1920)[33336000]@30 - ov13858 (320x240)[8352000]@119 (640x480)[8352000]@119 (1280x720)[16705000]@59 (1920x1080)[16705000]@59 (4160x3104)[33389000]@29 - imx258 (320x240)[17644000]@56 (640x480)[17644000]@56 (1280x720)[33282000]@30 (1920x1080)[34100000]@29 (4160x3104)[34100000]@29 Which can be summarized as: - ov5670 can do 30FPS at max res and 1080p - ov13858 can do 29.94 FPS at max res and 60 in 1080p - imx258 can do 29.3 FPS at max res and 1080p > > > > It would be indeed better to centralize all the adjustments, probably > > at CameraCapabilities::initializeStreamConfigurations() time. > > > > The way we handle minFrameDurations is > > > > CameraCapabilities::initializeStreamConfigurations() { > > int64_t minFrameDuration = frameDurations->second.min().get<int64_t>() * 1000; > > .. > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > if (minFrameDuration < minFrameDurationCap) { > > float tolerance = > > (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > /* > > * If the tolerance is less than 1%, do not cap > > * the frame duration. > > */ > > if (tolerance > 1.0) > > minFrameDuration = minFrameDurationCap; > > } > > > > streamConfigurations_.push_back({ > > res, androidFormat, minFrameDuration, maxFrameDuration, > > }); > > > > So here we adjust faster cameras to report max 30 FPS +- 1% > > This was due to get in par with the Intel HAL, but might cause errors > > as some streams are -faster- than what we report here. > > > > When populating the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT > > camera property, as you noticed, we add a 0.05 to the calculated FPS to > > accept as 30-fps-capable streams that can do > 29.95. > > > > unsigned int fps = static_cast<unsigned int> > > (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > > continue; > > > > And we use the adjusted FPS to later populate > > AE_AVAILABLE_TARGET_FPS_RANGE > > > > /* > > * Collect the FPS of the maximum YUV output size to populate > > * AE_AVAILABLE_TARGET_FPS_RANGE > > */ > > if (entry.androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888 && > > entry.resolution > maxYUVSize) { > > maxYUVSize = entry.resolution; > > maxYUVFps = fps; > > } > > > > int32_t availableAeFpsTarget[] = { > > minFps, maxYUVFps, maxYUVFps, maxYUVFps, > > }; > > > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > availableAeFpsTarget); > > > > > > Then we populate ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS with the > > entry.minFrameDurationNsec value, without adjusting it. > > > > /* Per-stream durations. */ > > minFrameDurations.push_back(entry.androidFormat); > > minFrameDurations.push_back(entry.resolution.width); > > minFrameDurations.push_back(entry.resolution.height); > > minFrameDurations.push_back(entry.minFrameDurationNsec); > > > > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > > minFrameDurations); > > > > So yes, we report a faster FPS in AE_AVAILABLE_TARGET_FPS_RANGE than the min > > frame duration reported in ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS. > > > > Now, a 2% tolerance seems a lot, but CTS has 1.5%, so it might be > > acceptable. > > > > I'm just afraid that if we adjust too much, then CTS will > > detect that we report a min duration that doesn't match the actual > > measured one and complains because a stream slower than expected is > > detected. > > > > Anyway, yes, we should adjust frame durations in a single place to > > avoid reporting inconsistent information in the camera capabilities. > > The question is how much to adjust ? :) > > > > } > > > > > > > > > > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > > > > It's notice that Chrome Camera App filters out the resolutions > > > > which cannot > > > > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > > > > FPS is lower > > > > > > > > within 2%. > > > > > > > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > > > > series, > > > > > > > so we can review and merge it separately. > > > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > --- > > > > > > > > src/android/camera_capabilities.cpp | 31 > > > > +++++++++++++++++++---------- > > > > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > > > > b/src/android/camera_capabilities.cpp > > > > > > > > index 55d651f3..e10ab036 100644 > > > > > > > > --- a/src/android/camera_capabilities.cpp > > > > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > > > > @@ -655,7 +655,9 @@ int > > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > > int64_t maxFrameDuration = > > > > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > > > > > > > /* > > > > > > > > - * Cap min frame duration to 30 FPS with > > > > 1% tolerance. > > > > > > > > + * Cap min frame duration to 30 FPS with > > > > 1% tolerance, > > > > > > > > + * and elevate min frame duration to 30 > > > > FPS with 2% > > > > > > > > + * tolerance. > > > > > > > > * > > > > > > > > * 30 frames per second has been validated > > > > as the most > > > > > > > > * opportune frame rate for quality > > > > tuning, and power > > > > > > > > @@ -675,17 +677,24 @@ int > > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > > * to the in-development configuration API > > > > rework. > > > > > > > > */ > > > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > > > > - if (minFrameDuration < > > > > minFrameDurationCap) { > > > > > > > > - float tolerance = > > > > > > > > - (minFrameDurationCap - > > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > + float tolerance = > > > > > > > > + (minFrameDurationCap - > > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > > > > > > > - /* > > > > > > > > - * If the tolerance is less than > > > > 1%, do not cap > > > > > > > > - * the frame duration. > > > > > > > > - */ > > > > > > > > - if (tolerance > 1.0) > > > > > > > > - minFrameDuration = > > > > minFrameDurationCap; > > > > > > > > - } > > > > > > > > + /* > > > > > > > > + * If the tolerance is less than 1%, do > > > > not cap > > > > > > > > + * the frame duration. > > > > > > > > + */ > > > > > > > > + if (tolerance > 1.0) > > > > > > > > + minFrameDuration = > > > > minFrameDurationCap; > > > > > > > > + > > > > > > > > + /* > > > > > > > > + * Some applications, ex: Chrome Camera > > > > App filters out > > > > > > > > + * the resolutions which cannot achieve 30 > > > > FPS. Elevate > > > > > > > > + * the minimum frame duration to 30 FPS if > > > > FPS is lower > > > > > > > > + * within 2%. > > > > > > > > + */ > > > > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > > > > + minFrameDuration = > > > > minFrameDurationCap; > > > > > > > > > > > > > > > > streamConfigurations_.push_back({ > > > > > > > > res, androidFormat, > > > > minFrameDuration, maxFrameDuration, > > > > > > > > > > -- > > > > > Regards, > > > > > > > > > > Laurent Pinchart > > > >
Hi Jacopo, Thanks for the detailed reply. On Fri, Apr 15, 2022 at 3:49 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Han-Lin, > > On Fri, Apr 15, 2022 at 12:51:45PM +0800, Hanlin Chen wrote: > > Hi Jacopo, > > Sorry for the separate email. > > > > On Fri, Apr 15, 2022 at 4:29 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Han-Lin, > > > > > > On Thu, Apr 14, 2022 at 08:34:56PM +0800, Hanlin Chen wrote: > > > > Hi Jacopo, > > > > Sorry for the late reply. > > > > > > > > On Wed, Apr 6, 2022 at 3:09 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > Hello, > > > > > sorry for being late > > > > > > > > > > On Wed, Apr 06, 2022 at 04:38:53AM +0300, Laurent Pinchart wrote: > > > > > > On Wed, Feb 16, 2022 at 03:01:13AM +0200, Laurent Pinchart wrote: > > > > > > > On Wed, Feb 16, 2022 at 03:00:43AM +0200, Laurent Pinchart wrote: > > > > > > > > Hi Han-Lin, > > > > > > > > > > > > > > > > Thank you for the patch. > > > > > > > > > > > > > > > > I'm CC'ing Jacopo and Umang as they've both spent a large amount of > > > > > time > > > > > > > > solving issues related to frame durations. > > > > > > > > > > > > > > And now with Umang's correct address. > > > > > > > > > > > > Jacopo, Umang, any feedback on this ? > > > > > > > > > > > > > > > > Let me recap a bit first. > > > > > > > > > > We currently adjust all durations > 33333333*101/100 to 33333333. > > > > > The idea was to avoid reporting any frame duration < 10^9/30 > > > > > c7ae5a50c10de1e790855d66842192c766da4dd3 > > > > > to bring the HAL in par with what the Intel HAL does. > > > > > > > > > > Umang's 37c41aa6a69985f99f9540dc89d094df61770fdc adjusted the > > > > > situation to allow slightly faster durations to be reported, to please > > > > > CTS which would have otherwise complained if the actual duration was > > > > > smaller than the reported on. > > > > > > > > > > Hence, so far, we have been dealing with -faster- cameras than the > > > > > fixed 10^9/30 limit. > > > > > > > > > > Umang: do you recall how did we pick the 1% positive tolerance ? > > > > > > > > > > The issue here is instead that some -slower- cameras might need to be > > > > > adjusted and report a smaller frame duration than what they actually > > > > > produce. We had a lengthy discussion in the past [1] about how to report > > > > > correctly the durations of Nautilus which was reporting a duration of > > > > > 29.3 msec while the Intel HAL happily hard-coded 33.333 and CST was > > > > > not complaining. We ended up fixing the driver in > > > > > "[libcamera-devel] [PATCH 0/2] IMX258 driver fixes". For Soraka > > > > > instead, if I'm not mistaken, resolutions that produce < 30 FPS are > > > > > not reported as part of the preview streams list (so only resolutions > > > > > up to 1080p are reported there). > > > > > > > > > > So I would like first to know what cameras and what resolutions > > > > > is this patch trying to please. > > > > > > > > > > Also, reading again the thread in [1] I realized CTS allows a 1.5% > > > > > tolerance > > > > > > > > > > " Frame duration must be in the range of [33333333, 33333333], value > > > > > 34387000 is out of range [32833332, 33833332])" > > > > > > > > > > How was 2% picked here ? > > > > > > > > > > Sorry for making things difficult, but I'm always a bit concerned that > > > > > moving a tiny thing here would make some CTS test failures creep in > > > > > unnoticed. > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > [1] [libcamera-internal] Question on camera stream durations > > > > > > > > > > > > > For the front camera of soraka, thus OV5685, which reports frame duration > > > > 33336000 for resolution larger than 720p. > > > > When calculating FPS, we did a slight elevation it due to CTS in > > > > f78f714b4486dbfd62bd62d7a479abc1d98d7495: > > > > > > > > // unsigned int fps = static_cast<unsigned int> (floor(1e9 / > > > > entry.minFrameDurationNsec + 0.05f)); > > > > > > > > It gets floor(29.997600192 + 0.5) = 30, and passes the bar. > > > > However, we still report the minFrameDuration as 33336000. > > > > > > > > I checked with the chrome team about their logic: > > > > Chrome would only try the frame rate listed in > > > > ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > > > which is elevated to 30 in the case, and filter out the stream with frame > > > > duration with 33336000, since its calculated FPS would be 29.97. As a > > > > result, chrome rejects resolution > 720p for the front camera. > > > > > > Is this something new ? I recall we had 1080p for preview on Soraka. > > > Am I mistaken ? > > > > > > > On the other hand, the back camera reports 33389000 for the full size. The > > > > 2% is from the value, which is about 1.6% from 33333333. > > > > > > Full sizes are in facts not reported as 30-FPS capable streams, if I > > > recall correctly ? > > > > Another reason is due to the configuration file. The config tuned by > > Intel always sets the sensor to full size. > > Almost. In the revision I have here for ov13858 9 modes gets generated > from smaller sensor resolutions. It only happens to small-ish modes > though [1] > > > Since we calculate the FrameDurationLimit by sensor size. The min > > duration would always be 33389000. > > I suppose this is why shipped soraka does not encounter the 1% faster > > problem in CTS, since it's actually slower. > > It's a little awkward, since 1e9 / 33389000 = 29.49983, slightly less > > than the bar 29.5. =.= > > Be aware that the IPU pipeline handler tries to use a sensor > resolution as close as possible to the maximum requested size > > src/libcamera/pipeline/ipu3/ipu3.cpp: > > /* > * Generate raw configuration from CIO2. > * > * The output YUV streams will be limited in size to the maximum frame > * size requested for the RAW stream, if present. > * > * If no raw stream is requested generate a size as large as the maximum > * requested YUV size aligned to the ImgU constraints and bound by the > * sensor's maximum resolution. See > * https://bugs.libcamera.org/show_bug.cgi?id=32 > */ > if (rawSize.isNull()) > rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth, > ImgUDevice::kIFMaxCropHeight }) > .grownBy({ ImgUDevice::kOutputMarginWidth, > ImgUDevice::kOutputMarginHeight }) > .boundedTo(data_->cio2_.sensor()->resolution()); > > And looking at the ov13858 driver supported modes > > static const struct ov13858_mode supported_modes[] = { > { > .width = 4224, > .height = 3136, > .vts_def = OV13858_VTS_30FPS, > .vts_min = OV13858_VTS_30FPS, > .reg_list = { > .num_of_regs = ARRAY_SIZE(mode_4224x3136_regs), > .regs = mode_4224x3136_regs, > }, > .link_freq_index = OV13858_LINK_FREQ_INDEX_0, > }, > { > .width = 2112, > .height = 1568, > .vts_def = OV13858_VTS_30FPS, > .vts_min = 1608, > .reg_list = { > .num_of_regs = ARRAY_SIZE(mode_2112x1568_regs), > .regs = mode_2112x1568_regs, > }, > .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > }, > { > .width = 2112, > .height = 1188, > .vts_def = OV13858_VTS_30FPS, > .vts_min = 1608, > .reg_list = { > .num_of_regs = ARRAY_SIZE(mode_2112x1188_regs), > .regs = mode_2112x1188_regs, > }, > .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > }, > { > .width = 1056, > .height = 784, > .vts_def = OV13858_VTS_30FPS, > .vts_min = 804, > .reg_list = { > .num_of_regs = ARRAY_SIZE(mode_1056x784_regs), > .regs = mode_1056x784_regs, > }, > .link_freq_index = OV13858_LINK_FREQ_INDEX_1, > } > }; > > We might get to use full size only for full-resolution mode. > (not verified, only by looking at the code) > > Clarifying what sensor mode gets selected for what output mode, and > what is the reported frame durations for each sensor mode, might help > clarify what actually happens. > > Looking at numbers from "Question on camera stream durations" [2] ov13858 > can do 60fps@1080p (which is likely produced by 2112x1568 sensor mode), > and can do 29@full-res. Hence I'm missing why 1080p doesn't get registered > as 30-FPS-capable in you case. > > For the Soraka ov5670 front camera that instead can do 33336000 for > all modes > 720p so far we got away with the 0.05 rounding, but as you > said we miss adjusting the AE_AVAILABLE_TARGET_FPS_RANGE. > > > The only way I can think of is to change the FrameDurationLimit > > calculation in close sourced IPA. > > As a general consideration, this is an android specific requirement > and I see it better placed in the HAL. However the closed source IPA > serves only for Android so far... > > What I would do is: > > - Move the 0.05 adjustment to when minFrameDuration is calculated, so > that we keep AE_AVAILABLE_TARGET_FPS_RANGE and > ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT in sync. > > The adjustment can be expressed as a percentage, and since CTS > accepts a 1.5% tolerance we could do the same (which means adjusting > to 30FPS streams that can only do 29.55 which seems -a lot- of > adjustment to me). > > - Clarify how 1080p gets generated on Soraka ov13858, from which sensor mode > and what durations are reported. From the calculations in [2] it > should be capable of 60 FPS (capped at 30), but you see CCA falling > back to 480p. > > Apart from this, if sensor full resolution can do 29.94 I think a > tiny 0.2% adjustment is fine as we can register full resolution as > 30-FPS capable if CTS doesn't complain. Be aware though that full > resolution modes for both the front and back camera are reported as > being 24FPS in the Intel camera HAL as per the discussion in [2] > (this is said to be because of the ImgU processing bandwidth > limitations iirc) so if we want to get in-par we should really only > care about 1080p being @30FPS. I think there was a mix of two scenarios and I created some confusion here. I'm sorry about that. There are two scenarios: Case [A] Without configuration: 1. 1080p for ov13858 works as you mentioned. 2. Front camera needs adjustment for its min frame duration: 33336000 when resolution > 720p This follows the calculation in [2]. The tolerance is less than 1%. All is good. Case [B] With the configuration file: As in [1], although it lists resolutions with half and smaller raw frames, the use case is only for width < height. You can see the similar lines: <main width="960" height="1280" format="NV12" peer="main"/> In Intel HAL, those cases are not used. In fact, they shouldn't exist from the beginning. It's bad. If we filter out the case where width < height. You can see all configurations are using full size raw frames. In result, it only left us the case. (4160x3104)[33389000]@29 for all resolutions, i.e., even if the user only requests 1080p. 333389000 is awkward, since its tolerance is 1.67%. This is where 2% comes from. And it's why the patch is sent with the configuration file series. All of these are bad. I think we agree that we should adjust the min frame duration for case [A]. For [B], it's more tricky, because it's not that the sensor is not capable. It's more like a problem caused by the pipeline configuration file. It makes me think [B] should be fixed by overriding the frame duration by the pipeline configuration file. If so, we tolerate 1% for the general solution and leave the quirk to the pipeline configuration file. Would it be better? > > Thanks > j > > > > > > > > > > > > > > After a rethink, maybe I should elevate the min frame duration together > > > > with where we elevate the FPS, instead of a 2% cap. > > > > Or try to centralize all adjustments on FPS/frame duration to an earlier > > > > stage. > > > > > > [1] > > cat ./media-libs/cros-camera-hal-configs-poppy/files/gcss/graph_settings_ov13858.xml | grep -A3 \<input | grep -v 4224 | grep \<input -A3 > > <input width="2112" height="1568" format="CIO2" /> > <vf width="960" height="1280" format="NV12" peer="vf"/> > <main width="960" height="1280" format="NV12" peer="main"/> > > -- > <input width="2112" height="1568" format="CIO2" /> > <vf width="720" height="1280" format="NV12" peer="vf"/> > <main width="960" height="1280" format="NV12" peer="main"/> > > -- > <input width="2112" height="1568" format="CIO2" /> > <vf width="480" height="640" format="NV12" peer="vf"/> > <main width="960" height="1280" format="NV12" peer="main"/> > > -- > <input width="2112" height="1568" format="CIO2" /> > <vf width="480" height="640" format="NV12" peer="vf"/> > <main width="720" height="1280" format="NV12" peer="main"/> > > -- > <input width="1056" height="784" format="CIO2" /> > <vf width="480" height="640" format="NV12" peer="vf"/> > <main width="480" height="640" format="NV12" peer="main"/> > > -- > <input width="2112" height="1568" format="CIO2" /> > <vf width="240" height="320" format="NV12" peer="vf"/> > <main width="960" height="1280" format="NV12" peer="main"/> > > -- > <input width="2112" height="1568" format="CIO2" /> > <vf width="240" height="320" format="NV12" peer="vf"/> > <main width="720" height="1280" format="NV12" peer="main"/> > > -- > <input width="1056" height="784" format="CIO2" /> > <vf width="240" height="320" format="NV12" peer="vf"/> > <main width="480" height="640" format="NV12" peer="main"/> > > -- > <input width="1056" height="784" format="CIO2" /> > <vf width="240" height="320" format="NV12" peer="vf"/> > <main width="240" height="320" format="NV12" peer="main"/> > > [2] > - ov5670 > (320x240)[16149000]@61 > (640x480)[16149000]@61 > (1280x720)[33336000]@30 > (1280x960)[33336000]@30 > (1920x1080)[33336000]@30 > (2560x1920)[33336000]@30 > > - ov13858 > (320x240)[8352000]@119 > (640x480)[8352000]@119 > (1280x720)[16705000]@59 > (1920x1080)[16705000]@59 > (4160x3104)[33389000]@29 > > - imx258 > (320x240)[17644000]@56 > (640x480)[17644000]@56 > (1280x720)[33282000]@30 > (1920x1080)[34100000]@29 > (4160x3104)[34100000]@29 > > Which can be summarized as: > - ov5670 can do 30FPS at max res and 1080p > - ov13858 can do 29.94 FPS at max res and 60 in 1080p > - imx258 can do 29.3 FPS at max res and 1080p > > > > > > > It would be indeed better to centralize all the adjustments, probably > > > at CameraCapabilities::initializeStreamConfigurations() time. > > > > > > The way we handle minFrameDurations is > > > > > > CameraCapabilities::initializeStreamConfigurations() { > > > int64_t minFrameDuration = frameDurations->second.min().get<int64_t>() * 1000; > > > .. > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > if (minFrameDuration < minFrameDurationCap) { > > > float tolerance = > > > (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > /* > > > * If the tolerance is less than 1%, do not cap > > > * the frame duration. > > > */ > > > if (tolerance > 1.0) > > > minFrameDuration = minFrameDurationCap; > > > } > > > > > > streamConfigurations_.push_back({ > > > res, androidFormat, minFrameDuration, maxFrameDuration, > > > }); > > > > > > So here we adjust faster cameras to report max 30 FPS +- 1% > > > This was due to get in par with the Intel HAL, but might cause errors > > > as some streams are -faster- than what we report here. > > > > > > When populating the ANDROID_SCALER_AVAILABLE_STREAM_CONFIGURATIONS_OUTPUT > > > camera property, as you noticed, we add a 0.05 to the calculated FPS to > > > accept as 30-fps-capable streams that can do > 29.95. > > > > > > unsigned int fps = static_cast<unsigned int> > > > (floor(1e9 / entry.minFrameDurationNsec + 0.05f)); > > > if (entry.androidFormat != HAL_PIXEL_FORMAT_BLOB && fps < 30) > > > continue; > > > > > > And we use the adjusted FPS to later populate > > > AE_AVAILABLE_TARGET_FPS_RANGE > > > > > > /* > > > * Collect the FPS of the maximum YUV output size to populate > > > * AE_AVAILABLE_TARGET_FPS_RANGE > > > */ > > > if (entry.androidFormat == HAL_PIXEL_FORMAT_YCbCr_420_888 && > > > entry.resolution > maxYUVSize) { > > > maxYUVSize = entry.resolution; > > > maxYUVFps = fps; > > > } > > > > > > int32_t availableAeFpsTarget[] = { > > > minFps, maxYUVFps, maxYUVFps, maxYUVFps, > > > }; > > > > > > staticMetadata_->addEntry(ANDROID_CONTROL_AE_AVAILABLE_TARGET_FPS_RANGES, > > > availableAeFpsTarget); > > > > > > > > > Then we populate ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS with the > > > entry.minFrameDurationNsec value, without adjusting it. > > > > > > /* Per-stream durations. */ > > > minFrameDurations.push_back(entry.androidFormat); > > > minFrameDurations.push_back(entry.resolution.width); > > > minFrameDurations.push_back(entry.resolution.height); > > > minFrameDurations.push_back(entry.minFrameDurationNsec); > > > > > > staticMetadata_->addEntry(ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS, > > > minFrameDurations); > > > > > > So yes, we report a faster FPS in AE_AVAILABLE_TARGET_FPS_RANGE than the min > > > frame duration reported in ANDROID_SCALER_AVAILABLE_MIN_FRAME_DURATIONS. > > > > > > Now, a 2% tolerance seems a lot, but CTS has 1.5%, so it might be > > > acceptable. > > > > > > I'm just afraid that if we adjust too much, then CTS will > > > detect that we report a min duration that doesn't match the actual > > > measured one and complains because a stream slower than expected is > > > detected. > > > > > > Anyway, yes, we should adjust frame durations in a single place to > > > avoid reporting inconsistent information in the camera capabilities. > > > The question is how much to adjust ? :) > > > > > > } > > > > > > > > > > > > > > > > > On Wed, Feb 09, 2022 at 03:19:17PM +0800, Han-Lin Chen wrote: > > > > > > > > > It's notice that Chrome Camera App filters out the resolutions > > > > > which cannot > > > > > > > > > achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if > > > > > FPS is lower > > > > > > > > > within 2%. > > > > > > > > > > > > > > > > Unless I'm mistaken this patch doesn't depend on the rest of the > > > > > series, > > > > > > > > so we can review and merge it separately. > > > > > > > > > > > > > > > > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> > > > > > > > > > --- > > > > > > > > > src/android/camera_capabilities.cpp | 31 > > > > > +++++++++++++++++++---------- > > > > > > > > > 1 file changed, 20 insertions(+), 11 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/src/android/camera_capabilities.cpp > > > > > b/src/android/camera_capabilities.cpp > > > > > > > > > index 55d651f3..e10ab036 100644 > > > > > > > > > --- a/src/android/camera_capabilities.cpp > > > > > > > > > +++ b/src/android/camera_capabilities.cpp > > > > > > > > > @@ -655,7 +655,9 @@ int > > > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > > > int64_t maxFrameDuration = > > > > > frameDurations->second.max().get<int64_t>() * 1000; > > > > > > > > > > > > > > > > > > /* > > > > > > > > > - * Cap min frame duration to 30 FPS with > > > > > 1% tolerance. > > > > > > > > > + * Cap min frame duration to 30 FPS with > > > > > 1% tolerance, > > > > > > > > > + * and elevate min frame duration to 30 > > > > > FPS with 2% > > > > > > > > > + * tolerance. > > > > > > > > > * > > > > > > > > > * 30 frames per second has been validated > > > > > as the most > > > > > > > > > * opportune frame rate for quality > > > > > tuning, and power > > > > > > > > > @@ -675,17 +677,24 @@ int > > > > > CameraCapabilities::initializeStreamConfigurations() > > > > > > > > > * to the in-development configuration API > > > > > rework. > > > > > > > > > */ > > > > > > > > > int64_t minFrameDurationCap = 1e9 / 30.0; > > > > > > > > > - if (minFrameDuration < > > > > > minFrameDurationCap) { > > > > > > > > > - float tolerance = > > > > > > > > > - (minFrameDurationCap - > > > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > + float tolerance = > > > > > > > > > + (minFrameDurationCap - > > > > > minFrameDuration) * 100.0 / minFrameDurationCap; > > > > > > > > > > > > > > > > > > - /* > > > > > > > > > - * If the tolerance is less than > > > > > 1%, do not cap > > > > > > > > > - * the frame duration. > > > > > > > > > - */ > > > > > > > > > - if (tolerance > 1.0) > > > > > > > > > - minFrameDuration = > > > > > minFrameDurationCap; > > > > > > > > > - } > > > > > > > > > + /* > > > > > > > > > + * If the tolerance is less than 1%, do > > > > > not cap > > > > > > > > > + * the frame duration. > > > > > > > > > + */ > > > > > > > > > + if (tolerance > 1.0) > > > > > > > > > + minFrameDuration = > > > > > minFrameDurationCap; > > > > > > > > > + > > > > > > > > > + /* > > > > > > > > > + * Some applications, ex: Chrome Camera > > > > > App filters out > > > > > > > > > + * the resolutions which cannot achieve 30 > > > > > FPS. Elevate > > > > > > > > > + * the minimum frame duration to 30 FPS if > > > > > FPS is lower > > > > > > > > > + * within 2%. > > > > > > > > > + */ > > > > > > > > > + if (tolerance < 0 && tolerance > -2.0) > > > > > > > > > + minFrameDuration = > > > > > minFrameDurationCap; > > > > > > > > > > > > > > > > > > streamConfigurations_.push_back({ > > > > > > > > > res, androidFormat, > > > > > minFrameDuration, maxFrameDuration, > > > > > > > > > > > > -- > > > > > > Regards, > > > > > > > > > > > > Laurent Pinchart > > > > >
diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp index 55d651f3..e10ab036 100644 --- a/src/android/camera_capabilities.cpp +++ b/src/android/camera_capabilities.cpp @@ -655,7 +655,9 @@ int CameraCapabilities::initializeStreamConfigurations() int64_t maxFrameDuration = frameDurations->second.max().get<int64_t>() * 1000; /* - * Cap min frame duration to 30 FPS with 1% tolerance. + * Cap min frame duration to 30 FPS with 1% tolerance, + * and elevate min frame duration to 30 FPS with 2% + * tolerance. * * 30 frames per second has been validated as the most * opportune frame rate for quality tuning, and power @@ -675,17 +677,24 @@ int CameraCapabilities::initializeStreamConfigurations() * to the in-development configuration API rework. */ int64_t minFrameDurationCap = 1e9 / 30.0; - if (minFrameDuration < minFrameDurationCap) { - float tolerance = - (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; + float tolerance = + (minFrameDurationCap - minFrameDuration) * 100.0 / minFrameDurationCap; - /* - * If the tolerance is less than 1%, do not cap - * the frame duration. - */ - if (tolerance > 1.0) - minFrameDuration = minFrameDurationCap; - } + /* + * If the tolerance is less than 1%, do not cap + * the frame duration. + */ + if (tolerance > 1.0) + minFrameDuration = minFrameDurationCap; + + /* + * Some applications, ex: Chrome Camera App filters out + * the resolutions which cannot achieve 30 FPS. Elevate + * the minimum frame duration to 30 FPS if FPS is lower + * within 2%. + */ + if (tolerance < 0 && tolerance > -2.0) + minFrameDuration = minFrameDurationCap; streamConfigurations_.push_back({ res, androidFormat, minFrameDuration, maxFrameDuration,
It's notice that Chrome Camera App filters out the resolutions which cannot achieve 30 FPS. Elevate the minimum frame duration to 30 FPS if FPS is lower within 2%. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org> --- src/android/camera_capabilities.cpp | 31 +++++++++++++++++++---------- 1 file changed, 20 insertions(+), 11 deletions(-)