[RUM-2258] Improve vital support for higher refresh rate devices by plousada · Pull Request #1806 · DataDog/dd-sdk-android

plousada · 2024-01-03T17:52:00Z

What does this PR do?

CPU Vitals start to take into account refresh rate of device obtained either through OnFrameMetricsAvailableListener or Display.getRefreshRate().

Motivation

What inspired you to submit this pull request?

Additional Notes

Anything else we should know when reviewing?

Review checklist (to be filled by reviewers)

Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
Make sure you discussed the feature or bugfix with the maintaining team in an Issue
Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

codecov-commenter · 2024-01-03T18:11:25Z

Codecov Report

Merging #1806 (8873500) into develop (8145782) will decrease coverage by 0.16%.
Report is 10 commits behind head on develop.
The diff coverage is 34.48%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1806      +/-   ##
===========================================
- Coverage    83.49%   83.33%   -0.16%     
===========================================
  Files          467      467              
  Lines        16491    16517      +26     
  Branches      2483     2492       +9     
===========================================
- Hits         13769    13764       -5     
- Misses        2043     2068      +25     
- Partials       679      685       +6

Files	Coverage Δ
...ernal/vitals/JankStatsActivityLifecycleListener.kt	`71.28% <34.48%> (-17.13%)`	⬇️

... and 27 files with indirect coverage changes

0xnm · 2024-01-04T08:09:14Z

+    interface DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {
+        override fun onFrameMetricsAvailable(
+            window: Window,
+            frameMetrics: FrameMetrics,
+            dropCountSinceLastInvocation: Int
+        )
+
+        var frameDeadline: Long


I believe this one can be removed

Suggested change

interface DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {

override fun onFrameMetricsAvailable(

window: Window,

frameMetrics: FrameMetrics,

dropCountSinceLastInvocation: Int

)

var frameDeadline: Long

interface DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {

var frameDeadline: Long

0xnm · 2024-01-04T08:22:23Z

+    private fun registerMetricListener(window: Window, activity: Activity) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+            val handler = Handler(Looper.getMainLooper())
+            window.addOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT, handler)
+        } else {
+        // Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have
+        // Frame Metrics callbacks available
+        if (Build.VERSION.SDK_INT == Build.VERSION_CODES.R) {
+            val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager
+            display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)
+        }
+    }
+}


latest version of ktlint we are using have indent formatting turned off (for a while, we have a ticket for it), so it is better to format code with Android Studio to fix the indent.

Suggested change

private fun registerMetricListener(window: Window, activity: Activity) {

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {

val handler = Handler(Looper.getMainLooper())

window.addOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT, handler)

} else {

// Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have

// Frame Metrics callbacks available

if (Build.VERSION.SDK_INT == Build.VERSION_CODES.R) {

val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager

display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)

}

}

}

private fun registerMetricListener(window: Window, activity: Activity) {

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {

val handler = Handler(Looper.getMainLooper())

window.addOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT, handler)

} else if (Build.VERSION.SDK_INT == Build.VERSION_CODES.R && display == null) {

// Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have

// Frame Metrics callbacks available

val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager

display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)

}

}

Also I think we may set display only once (if DisplayManager doesn't depend on a particular UI context though).

I think not, although I didn't look through AOSP to see if on foldables the default screen changes depending on fold state.

0xnm · 2024-01-04T08:23:36Z

+    private fun unregisterMetricListener(window: Window) {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {
+                window.removeOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT)
+        }
+    }


formatting:

Suggested change

private fun unregisterMetricListener(window: Window) {

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {

window.removeOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT)

}

}

private fun unregisterMetricListener(window: Window) {

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) {

window.removeOnFrameMetricsAvailableListener(DDFrameMetricsListener.DEFAULT)

}

}

0xnm · 2024-01-04T08:27:48Z

+            frameRate *= (SIXTY_FPS / screenRefreshRate)
+
+            // If normalized frame rate is still at over 60fps it means the frame rendered
+            // quickly enough for the devices refresh rate.
+            if (frameRate > MAX_FPS) {
+                frameRate = MAX_FPS
+            }


very minor, just looks shorter

Suggested change

frameRate *= (SIXTY_FPS / screenRefreshRate)

// If normalized frame rate is still at over 60fps it means the frame rendered

// quickly enough for the devices refresh rate.

if (frameRate > MAX_FPS) {

frameRate = MAX_FPS

}

// If normalized frame rate is still at over 60fps it means the frame rendered

// quickly enough for the devices refresh rate.

frameRate *= (SIXTY_FPS / screenRefreshRate).coerceAtMost(MAX_FPS)

0xnm · 2024-01-04T08:45:37Z

    }

+    @RequiresApi(Build.VERSION_CODES.N)
+    interface DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {


we should move it to the bottom of the file, just before the companion, because the ordering is propeties > methods > inner classes/interfaces

0xnm · 2024-01-04T08:48:00Z

+    private var screenRefreshRate: Double = 60.0
 ) : ActivityLifecycleCallbacks, JankStats.OnFrameListener {



We can remove it from the constructor, I don't see any call sites where we pass custom screenRefreshRate at the JankStatsActivityLifecycleListener construction time

Suggested change

private var screenRefreshRate: Double = 60.0

) : ActivityLifecycleCallbacks, JankStats.OnFrameListener {

) : ActivityLifecycleCallbacks, JankStats.OnFrameListener {

private var screenRefreshRate: Double = 60.0

It is used in the test 𝕄 adjust sample value to refresh rate 𝕎 doFrame() {refresh rate over 60hz}

0xnm · 2024-01-04T08:51:06Z

    }

+    @Test
+    fun `𝕄 adjust sample value to refresh rate 𝕎 doFrame() {refresh rate over 60hz}`(


ideally, it would be nice to see the tests for different Android API versions (since there are different code paths in the class under test depending on API version), but this may explode the number of tests, so it is not a blocker for me.

We just need to test pre-R, R and post-R.
I can include those.

mariusc83 · 2024-01-04T08:53:16Z

@@ -65,6 +101,8 @@ internal class JankStatsActivityLifecycleListener(
                activeWindowsListener[window] = jankStats


Not related with this PR but maybe we should add a task here to not use anymore the window as a key here. See the previous problems we had with using the activity as a key for view scopes.

mariusc83 · 2024-01-04T08:55:09Z

    }

+    @RequiresApi(Build.VERSION_CODES.N)
+    interface DDFrameMetricsListener : Window.OnFrameMetricsAvailableListener {


any particular reason you need an interface here ? I think just a simple class would be simpler.

Originally it was meant to be provided as parameter to aid in mocking, but ultimately didn't need to. I'll change it.

mariusc83 · 2024-01-04T08:56:06Z


    internal val activeWindowsListener = WeakHashMap<Window, JankStats>()
    internal val activeActivities = WeakHashMap<Window, MutableList<WeakReference<Activity>>>()
+    internal var display: Display? = null


we should not keep the Display as a reference here as it can create memory leaks.

if it is not bound to the particular UI-context and there is just a single instance for the whole app, it should be fine I guess.

That was my assumption, I can replace with a weak reference if you prefer.

if you are certain that it will be the main window display instance then it will be fine.

jonathanmos · 2024-01-08T09:08:56Z

        const val ONE_SECOND_NS: Long = 1000L * 1000L * 1000L
        const val TEN_SECOND_NS: Long = 10L * ONE_SECOND_NS
        const val ONE_MINUTE_NS: Long = 60L * ONE_SECOND_NS
+        const val MIN_FPS: Double = 1.0


minor: If we in the future modify min/max fps we would have to change this in two places. We could make the class variables internal to share with the test

I think it's unlikely to change, but I can follow this pattern if we're using it elsewhere.

mariusc83

I am going to change to Comment from Request Changes to not block you while I am in PTO

0xnm

I've added some comments/thoughts, let me know what you think. But overall the change is lgtm.

0xnm · 2024-01-24T09:42:17Z

+        } else {
+        // Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have
+        // Frame Metrics callbacks available
+        if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) {
+            val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager
+            display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)
+        }
+    }
+}


seems formatting is a bit off here (ktlint in the current state has indent rule disabled, so it doesn't fix/verify it):

Suggested change

} else {

// Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have

// Frame Metrics callbacks available

if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) {

val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager

display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)

}

}

}

} else {

// Fallback - Android 30 allows apps to not run at a fixed 60hz, but didn't yet have

// Frame Metrics callbacks available

if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) {

val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager

display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)

}

}

}

better to re-format this file with Android Studio.

0xnm · 2024-01-24T09:44:46Z

        activeActivities[window] = list
    }

+    @RequiresApi(Build.VERSION_CODES.N)


It seems that it should be Build.VERSION_CODES.R here? because I can see 2 logical branches here: one which is executed only for Build.VERSION_CODES.S and above and another for Build.VERSION_CODES.R.

Or we just rely on the lowest API required from the methods description?

I was relying on the API required for the method itself, just to be on the safe side.
But I don't have a strong opinion here, I can change it to .R if we feel it's better.

0xnm · 2024-01-24T10:00:08Z

+        if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) {
+            val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager
+            display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)
+        }


maybe we can pull this out from this method? it may be confusing, because there is no listener registration in this block. Also by pulling it out we will have a symmetry at the calls site between registerMetricListener and unregisterMetricListener calls in terms of the build version required, because currently the former is guarded by Build.VERSION.SDK_INT >= Build.VERSION_CODES.R and the latter Build.VERSION.SDK_INT >= Build.VERSION_CODES.S - this raises the obvious question initially: we don't unregister listener registered if Build.VERSION.SDK_INT == Build.VERSION_CODES.R?

After this change, the following:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { registerMetricListener(window, activity) }

will become:

if (buildSdkVersionProvider.version() >= Build.VERSION_CODES.S) { registerMetricListener(window, activity) } else if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) { val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager display = displayManager.getDisplay(Display.DEFAULT_DISPLAY) }

which is cleaner to me

0xnm · 2024-01-24T10:09:18Z

+        // Frame Metrics callbacks available
+        if (display == null && buildSdkVersionProvider.version() == Build.VERSION_CODES.R) {
+            val displayManager = activity.getSystemService(Context.DISPLAY_SERVICE) as DisplayManager
+            display = displayManager.getDisplay(Display.DEFAULT_DISPLAY)


I believe holding Display object shouldn't cause memory leaks by looking on its structure https://cs.android.com/android/platform/superproject/main/+/main:frameworks/base/core/java/android/view/Display.java;l=1?q=Display.java&sq= - it seems it doesn't have references to the UI-related context (Activity), the only concern is Resources object which may hold such reference. So maybe it is worth to check this change for the leaks.

Didn't see anything on my local profiles.
Also did a quick analysis on the activity allocation/deallocation pattern in the sample app with and without these changes and found no difference.

plousada requested review from a team as code owners January 3, 2024 17:52

0xnm reviewed Jan 4, 2024

View reviewed changes

mariusc83 requested changes Jan 4, 2024

View reviewed changes

jonathanmos reviewed Jan 8, 2024

View reviewed changes

mariusc83 reviewed Jan 19, 2024

View reviewed changes

mariusc83 approved these changes Jan 19, 2024

View reviewed changes

plousada added 3 commits January 22, 2024 17:03

[RUM-2258] Fix frame rate vital for variable refresh rate displays

c63258e

Small refactor to frame listener

dcdb95c

Address PR comments.

f41093c

plousada force-pushed the plousada/RUM-2258/fix_over_60fps_vital branch 4 times, most recently from 676e934 to 81b6565 Compare January 22, 2024 19:48

plousada requested review from 0xnm, jonathanmos and mariusc83 January 23, 2024 18:04

0xnm reviewed Jan 24, 2024

View reviewed changes

Add tests to cover S and R versions

8873500

plousada force-pushed the plousada/RUM-2258/fix_over_60fps_vital branch from 81b6565 to 8873500 Compare January 25, 2024 22:12

xgouchet approved these changes Jan 26, 2024

View reviewed changes

0xnm approved these changes Jan 26, 2024

View reviewed changes

plousada merged commit c925c52 into develop Jan 26, 2024

plousada deleted the plousada/RUM-2258/fix_over_60fps_vital branch January 26, 2024 10:56

xgouchet added this to the 2.6.0 milestone Feb 19, 2024

		private var screenRefreshRate: Double = 60.0
		) : ActivityLifecycleCallbacks, JankStats.OnFrameListener {

		@@ -65,6 +101,8 @@ internal class JankStatsActivityLifecycleListener(
		activeWindowsListener[window] = jankStats

Uh oh!

Conversation

plousada commented Jan 3, 2024

What does this PR do?

Motivation

Additional Notes

Review checklist (to be filled by reviewers)

Uh oh!

codecov-commenter commented Jan 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

plousada Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariusc83 left a comment

Choose a reason for hiding this comment

Uh oh!

0xnm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Jan 3, 2024 •

edited

Loading

plousada Jan 19, 2024 •

edited

Loading