[libcamera-devel] [PATCH 9/9] android: Elevate min duration to 30 FPS if it's lower within 2%
Hanlin Chen
hanlinchen at chromium.org
Fri Apr 15 17:29:21 CEST 2022
Hi Jacopo,
Thanks for the detailed reply.
On Fri, Apr 15, 2022 at 3:49 PM Jacopo Mondi <jacopo at 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 at 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 at 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 at 1080p (which is likely produced by 2112x1568 sensor mode),
> and can do 29 at 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 at 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
> > > > >
More information about the libcamera-devel
mailing list