feat: add Support for High-FPS Readings in Flashlight for Devices Over 60 FPS #316
Conversation
✅ Deploy Preview for flashlight-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -12,7 +12,11 @@ Time taken to run the test. | |||
| Can be helpful to measure Time To Interactive of your app, if the test is checking app start for instance. | |||
| Average FPS | |||
| 60 FPS | |||
| Frame Per Second. Your app should display 60 Frames Per Second to give an impression of fluidity. This number should be close to 60, otherwise it will seem laggy. | |||
| Frame Per Second. Your app should display | |||
| 60 | |||
There was a problem hiding this comment.
Head scratching moment why this change happened
There was a problem hiding this comment.
did the snapshot update come from this commit specifically?
There was a problem hiding this comment.
Hi @MalcolmTomisin !
First of all, apologies for the delay answering and thanks so much for submitting this great PR!! ❤️
This has been an open issue for some time now, and it's so cool to see someone spending a lot of time to fix it! 🙏
I've added some comments here and there, but I have to ask: what is the reasoning behind not adding the refresh rate in the results object?
It seems to me that adding the device specs directly there would simplify the code.
Also flashlight relies in a lot of place on a simple json file being the source of truth for a report. For instance, flashlight test outputs a json that can be used by flashlight report, and flashlight measure can also output one. flashlight cloud also has a similar mechanism.
So putting the device specs out of it might make it tricky to have for instance:
- flashlight test recognizing 120FPS and outputting results in json file
- flashlight report displaying the report for the json file, taking into account that the json file was a result for a 120FPS phone
I'll be away again for 10 days without access to computer, but I'm very happy to discuss this by message over Discord/Twitter/... if you want to get this in as soon as possible 🤞
| @@ -12,7 +12,11 @@ Time taken to run the test. | |||
| Can be helpful to measure Time To Interactive of your app, if the test is checking app start for instance. | |||
| Average FPS | |||
| 60 FPS | |||
| Frame Per Second. Your app should display 60 Frames Per Second to give an impression of fluidity. This number should be close to 60, otherwise it will seem laggy. | |||
| Frame Per Second. Your app should display | |||
| 60 | |||
There was a problem hiding this comment.
did the snapshot update come from this commit specifically?
packages/platforms/android/src/commands/__tests__/detectCurrentDeviceRefreshRate.test.ts
Show resolved
Hide resolved
packages/platforms/android/src/commands/__tests__/detectCurrentDeviceRefreshRate.test.ts
Show resolved
Hide resolved
I’ve considered adding the Additionally, I’m thinking about including device specifications in the reports, which I believe would be a significant benefit for transparency when reporting performance data. |
|
We could add a We could make the field accept an undefined value for backwards compatibility. What do you think? |
Sounds fine to me. |
MalcolmTomisin
force-pushed
the
main
branch
from
cb6c536
to
7d5b3fa
Compare
Use regular expression to find all matches of fps details from the adb command response. A contrived algorithm was used previously, relying on different regex constructs to isolate sections of the adb response strings such as strings that had the following format - "refreshRate %d", "fps=%d". The "refreshRate" string was dropped for the more common "fps". chore(profiler): update base types with new method Adding detectDeviceRefreshRate method to the base and sub classes. An implementation templation that can be adopted across multiple platforms to provide GPU refresh rate of the devices being profiled
… event in a socket
chore: use device specs type A new device spec has been created for use to model device spec relevant to performance test: update snapshots and test cases
…ADB command refactor: remove unnecessary catch block
Some devices come with the capabilities of switching between 60,90, 120 fps. The renderFrameRate reports the currently configured refresh rate.
MalcolmTomisin
force-pushed
the
main
branch
from
0571bef
to
792c999
Compare
For consistency, refresh rate is added to the DeviceSpecs property of the TestCaseResult and AveragedTestCaseResult chore(test): update snapshots refactor: remove unneeded changes refactor: use logger for error
MalcolmTomisin
force-pushed
the
main
branch
from
792c999
to
c6d4ded
Compare
Flashlight Automated Report 🔦 |
|
Thanks @MalcolmTomisin for the updates! I'll probably get a second look and we should release this early next week! 🥳 |
Almouro
changed the title
Description
This PR introduces a new feature to Flashlight that extends support for devices capable of displaying frame rates (FPS) higher than 60Hz. It addresses this issue
Key Changes:
High-FPS Support: Adjustments have been made to ensure that FPS readings above 60Hz are correctly captured and displayed by the flashlight tool.
Data-model: A
DeviceSpecstype has been created, including the refresh rate into theResultstype didn't seem like an optimal way to model the results of profiling.Testing: Added unit tests to verify the functionality of the
adbcommand for fps information.Minor: Extracting socket events into enums
Challenges
There's a lot of prop drilling going on, perhaps, including the refresh rate into the results type could have avoided all the prop drilling, looking forward to comments on that.
Additional Notes
DeviceSpecstype. Adding device specs of the device being profiled could be helpful metadata to add to reports.Screenshots