IntervalStates time range is incorrect [213245198]

Fixed

Bug

Status Update

No update yet.

Description

cc...@google.com

created issue #1

Jan 5, 2022 01:06AM

JankStats#logFrameData does the following:

var frameStates =
            metricsStateHolder.state?.getIntervalStates(startTime, startTime + actualDuration)
            ?: emptyList()

Which is incorrect on API 24+, since actualDuration will include renderthread time that may overlap the next frame. Instead, that function should receive an additional arg to use here:

var frameStates =
            metricsStateHolder.state?.getIntervalStates(startTime, startTime + **uiThreadDuration**)
            ?: emptyList()

So it only looks at UI thread time.

Similarly, I'm not sure if startTime is the right value - I think it should be max(lastFrameUiEndTime, thisFrameIntendedVsync) so that in considers code that runs before the frame, but doesn't overlap with the previous frame.

Comments

ch...@google.com <ch...@google.com> #2Jan 6, 2022 10:45PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ch...@google.com <ch...@google.com> #3Mar 15, 2022 10:02PM

The first problem in #1 is already fixed. in JankStatsImplApi24.getFrameData(), we first retrieve the UI duration, then use that to compute the states in that interval:

val uiDuration = frameMetrics.getMetric(FrameMetrics.UNKNOWN_DELAY_DURATION) +
frameMetrics.getMetric(FrameMetrics.INPUT_HANDLING_DURATION) +
frameMetrics.getMetric(FrameMetrics.ANIMATION_DURATION) +
frameMetrics.getMetric(FrameMetrics.LAYOUT_MEASURE_DURATION) +
frameMetrics.getMetric(FrameMetrics.DRAW_DURATION) +
frameMetrics.getMetric(FrameMetrics.SYNC_DURATION)
val frameStates =
metricsStateHolder.state?.getIntervalStates(startTime, startTime + uiDuration)
?: emptyList()

ch...@google.com <ch...@google.com> #4Mar 16, 2022 12:04AM

Fix for second problem (as suggested, taking into account both INTENDED_VSYNC_TIMESTAMP as well as the previous frame end time) in review.

ap...@google.com <ap...@google.com> #5Mar 29, 2022 05:30PM

Project: platform/frameworks/support
Branch: androidx-main

commit f069d601844f0b3f02ad5be2a491cc22d62456e2
Author: Chet Haase <chet@google.com>
Date: Tue Mar 15 16:51:14 2022

Refine startTime value

Two minor fixes to refine the time used to make the start time
of a frame:
- INTENDED_VSYNC_TIMESTAMP: On API 26+, this time more accurately
represents when the frame should have started than the previous
VSYNC_TIMESTAMP. They are usually the same, but in janky times,
the INTENDED time can capture more detail about when things should
have occurred, and how long they actually took.
- current vs previous frames: The use of INTENDED_VSYNC_TIMESTAMP
means that some frames can appear to overlap (because the system
tried to start one frame while the previous was still being processed).
This can lead to errors in state captures for those frames since they
appear in multiple frames for the same point in time. To avoid that
problem, the system now caches the previous "end" time of a frame,
and avoids using a start time less than that value.

Bug: 213245198
Test: Passed all JankStats tests on multiple API versions
Change-Id: I23fcc474f48619be9e0a996d1d0ac4d8e168e9e1

M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi31Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi24Impl.kt
M metrics/metrics-performance/src/main/java/androidx/metrics/performance/JankStatsApi26Impl.kt

https://android-review.googlesource.com/2027704

ch...@google.com <ch...@google.com> Apr 20, 2022 06:24PM

Marked as fixed.

Issue 213245198