Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RUMM-3222 Use targetTimestamp as reference to calculate FPS #1272

Merged
merged 8 commits into from
May 5, 2023

Conversation

ganeshnj
Copy link
Contributor

@ganeshnj ganeshnj commented May 3, 2023

What and why?

Currently, to find out the FPS of a view, the SDK look for the frame duration and inverses it to get the FPS.

Eg. if the frame duration is 16ms (0.016s), the FPS is 1/0.016 = 60FPS (rounded).

This generally works but when comes to variable refresh rate displays, it becomes an issue. For example, let's say OS has decided to increase the frame duration to 32ms to save battery, the FPS will be 30FPS but that doesn't mean the app is having a poor UI performance. It can very well be that the app is doing nothing.

How?

CADisplayLink has a property called targetTimestamp which is the time when the next frame is scheduled to be displayed. This is the time when the frame is actually displayed on the screen. Hence, the SDK uses this property to calculate the expected frame duration and then inverses it to get the FPS.

At the same time, SDK continues to use the old logic to calculate the FPS using the timestamp property.

Finally, the SDK normalizes the FPS to 60FPS as Datadog backend only supports 60FPS rate for now described here.

The logic for constant refresh rate displays has not changed.

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests
  • Run integration tests
  • Run smoke tests

@ganeshnj ganeshnj marked this pull request as ready for review May 3, 2023 13:05
@ganeshnj ganeshnj requested a review from a team as a code owner May 3, 2023 13:05
@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented May 3, 2023

Datadog Report

Branch report: ganeshnj/fix/RUMM-3222-promotion-fps
Commit report: 8a90022

dd-sdk-ios: 0 Failed, 0 New Flaky, 116 Passed, 0 Skipped, 3m 10.2s Wall Time

Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

One tiny suggestion, and one question about expanding the support for 60+ FPS on the backend.

Sources/Datadog/RUM/RUMVitals/VitalRefreshRateReader.swift Outdated Show resolved Hide resolved
// ProMotion displays (e.g. iPad Pro and newer iPhone Pro) can have refresh rate higher than 60 FPS.
if let expectedCurrentFrameDuration = self.nextFrameDuration, provider.maximumDeviceFramesPerSecond > 60 {
let expectedFPS = 1.0 / expectedCurrentFrameDuration
fps = currentFPS * (60.0 / expectedFPS)
Copy link
Member

@maciejburda maciejburda May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quote from the blog:

Since devices can have [different maximum refresh rates](https://developer.android.com/guide/topics/media/frame-rate), Mobile Vitals scales this metric to a value between zero and 60, allowing you to track your application’s refresh rate regardless of hardware-specific restrictions.

I wonder if normalisation makes sense though. The FPS value is completely wrong in that case (although gives the sense of performance). I don't fully understand why we couldn't send both current FPS and the baseline to the backend.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, in ideal world, we would want to have a graph with two values on a time scale.

Expected frame rate and real frame rate but we are not designed for this (as of now).

My plan to start a conversion with backed to allow sending metrics that have such type of measurement scale.

Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and it reads very well 💪👍. I left few suggestions and (perhaps), flakiness alert ❄️ ⚠️.

ganeshnj added 5 commits May 4, 2023 16:38
### What and why?

Currently, to find out the FPS of a view, the SDK look for the frame duration and inverses it to get the FPS.

Eg. if the frame duration is 16ms (0.016s), the FPS is 1/0.016 = 60FPS (rounded).

This generally works but when comes to variable refresh rate displays, it becomes an issue. For example, let's say OS has decided to increase the frame duration to 32ms to save battery, the FPS will be 30FPS but that doesn't mean the app is having a poor UI performance. It can very well be that the app is doing nothing.

### How?

`CADisplayLink` has a property called `targetTimestamp` which is the time when the next frame is scheduled to be displayed. This is the time when the frame is actually displayed on the screen. Hence, the SDK uses this property to calculate the expected frame duration and then inverses it to get the FPS.

At the same time, SDK continues to use the old logic to calculate the FPS using the `timestamp` property.

Finally, the SDK normalizes the FPS to 60FPS as Datadog backend only supports 60FPS rate for now described [here](https://www.datadoghq.com/blog/monitor-mobile-vitals-datadog/#slow-rendering).
@ganeshnj ganeshnj force-pushed the ganeshnj/fix/RUMM-3222-promotion-fps branch from 0e201fc to d88e9bf Compare May 4, 2023 14:45
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

Thanks for the CHANGELOG, tho it needs an extra line for linking the PR. I also left a minor suggestion, but it's good to merge as is 👍

CHANGELOG.md Show resolved Hide resolved
Comment on lines +61 to +85
var fps: Double? = nil

if let lastFrameTimestamp = self.lastFrameTimestamp {
let currentFrameDuration = provider.currentFrameTimestamp - lastFrameTimestamp
guard currentFrameDuration > 0 else {
return nil
}
let currentFPS = 1.0 / currentFrameDuration

// ProMotion displays (e.g. iPad Pro and newer iPhone Pro) can have refresh rate higher than 60 FPS.
if let expectedCurrentFrameDuration = self.nextFrameDuration, provider.adaptiveFrameRateSupported {
guard expectedCurrentFrameDuration > 0 else {
return nil
}
let expectedFPS = 1.0 / expectedCurrentFrameDuration
fps = currentFPS * (Self.backendSupportedFrameRate / expectedFPS)
} else {
fps = currentFPS
}
}

self.lastFrameTimestamp = provider.currentFrameTimestamp
self.nextFrameDuration = provider.nextFrameTimestamp - provider.currentFrameTimestamp

return fps
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/nit/suggestion

The nested statements make it difficult to catch, maybe we could flatten these by using a defer, e.g:

defer {
    self.lastFrameTimestamp = provider.currentFrameTimestamp
    self.nextFrameDuration = provider.nextFrameTimestamp - provider.currentFrameTimestamp
}

guard let lastFrameTimestamp = self.lastFrameTimestamp else {
    return nil
}

let currentFrameDuration = provider.currentFrameTimestamp - lastFrameTimestamp
guard currentFrameDuration > 0 else {
    return nil
}

let currentFPS = 1.0 / currentFrameDuration

// ProMotion displays (e.g. iPad Pro and newer iPhone Pro) can have refresh rate higher than 60 FPS.
guard provider.adaptiveFrameRateSupported, let expectedCurrentFrameDuration = self.nextFrameDuration else {
    return currentFPS
}

guard expectedCurrentFrameDuration > 0 else {
    return nil
}

let expectedFPS = 1.0 / expectedCurrentFrameDuration
return currentFPS * (Self.backendSupportedFrameRate / expectedFPS)

If I got it right :)

@ganeshnj ganeshnj merged commit b72076e into develop May 5, 2023
@ganeshnj ganeshnj deleted the ganeshnj/fix/RUMM-3222-promotion-fps branch May 5, 2023 15:22
@ncreated ncreated mentioned this pull request May 24, 2023
6 tasks
@maxep maxep mentioned this pull request May 30, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants