-
Couldn't load subscription status.
- Fork 1.7k
Fix frame rate metrics for dynamic refresh rates #10220 #15446
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
base: main
Are you sure you want to change the base?
Conversation
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant improvement by adapting the frame rate metrics to handle dynamic refresh rates, which is crucial for modern Apple devices. The approach of using CADisplayLink's targetTimestamp and timestamp to determine the actual frame duration is solid and correctly addresses the scenarios outlined in the description. The logic for identifying slow and frozen frames is now much more accurate across different display frequencies.
I have a couple of minor suggestions to clean up the code: one regarding an unused instance variable that was introduced, and another about a comment's formatting. Overall, this is a great change that will improve the accuracy of performance monitoring.
FirebasePerformance/Sources/AppActivity/FPRScreenTraceTracker.m
Outdated
Show resolved
Hide resolved
FirebasePerformance/Sources/AppActivity/FPRScreenTraceTracker.m
Outdated
Show resolved
Hide resolved
FirebasePerformance/Sources/AppActivity/FPRScreenTraceTracker.m
Outdated
Show resolved
Hide resolved
FirebasePerformance/Sources/AppActivity/FPRScreenTraceTracker.m
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses issues with frame rate metrics on devices with dynamic or non-60Hz refresh rates. The core change is to move away from static thresholds for slow and frozen frames to dynamic ones based on the actual frame duration provided by CADisplayLink. This is a solid approach that correctly handles variable refresh rates as described in the problem statement. The introduction of kFPRFrozenFrameMultiplier to define a frozen frame relative to the current refresh rate is a clever way to maintain perceptual consistency across different devices. Additionally, pausing the CADisplayLink when the app is backgrounded is a nice performance and battery-saving improvement. The changes are well-implemented and the code is clear. I have one minor suggestion regarding comment formatting.
FirebasePerformance/Sources/AppActivity/FPRScreenTraceTracker.m
Outdated
Show resolved
Hide resolved
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
@visumickey All of my commits ready for review, just continuing with my testing atm |
Fix frame rate metrics for dynamic refresh rates (#10220)
Discussion
Fix slow rendering false positives mentioned in #10220 (Work In Progress Ongoing testing!!!)
User Statements:
What this fixes? (My possible reproduction ideas on why this is happening):
Case 1 - tvOS 50Hz Display (Europe)
Case 2 - ProMotion iPhone, iPad and MacBook Dynamic Rates
Case 3 - External Displays with Custom Refresh Rates
Case 4 - Adaptive Sync Displays
Testing
API Changes