Fix wrong VSYNC event
#36775
Fix wrong VSYNC event
#36775
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
cc @dnfield |
shell/common/vsync_waiter.cc
Outdated
| @@ -109,6 +109,17 @@ void VsyncWaiter::FireCallback(fml::TimePoint frame_start_time, | |||
| return; | |||
| } | |||
|
|
|||
| // The event named "VSYNC" is special in `chrome://tracing` tool - it will | |||
There was a problem hiding this comment.
Why here?
This means the event won't get emitted on vsyncs where there's no callback, right?
I think making this more accurate is a good idea, but probably should fire unconditionally
There was a problem hiding this comment.
You are right! I have not thought about this problem before. Let me change it (probably within hours)
|
@iskakaushik can you provide a secondary review here? |
iskakaushik
added
the
autosubmit
This reverts commit 9ccaa33.
Firstly, we know
VSYNCevent in timeline is special -chrome://tracingtool will show "zebra" colors (gray and white) at every VSYNC. Therefore, it is critical to let this event have correct timing, otherwise every user is doing reasoning with the wrong vsync time.In the image below, the "a" shows the new VSYNC with this PR, while the "b" shows the old VSYNC interval before this PR. As we can see, the left side of "a" and "b" does not coincide. In other words, before this PR (where we have "b" as the VSYNC and there is no "a"), we consider the wrong time as the vsync time.
The cause is quite simple: Before this PR, the left edge of "VSYNC" event is (for example) the call time of
VsyncWaiterAndroid::OnVsyncFromJava. However, there exist "frame delay" (e.g.frameDelayNanosargument inOnVsyncFromJava), so the real vsync time should minus that delay.As a side remark, in the image below the difference is not very much, but in real scenarios, I have seen once in a while it has large differences. Then you know, the visualization goes wild, and it took me some time before I realized, it is not a bug in code anywhere, but a bug of the VSYNC event time.
Close #113475
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.