Repository navigation
AutomatedTestWidgetsFlutterBinding.pump should match engine frame precision #112610
Description
Activity
Could you help me understand why this is considered a bug? Is there something you're using the AutomatedTestWidgetsFlutterBinding for that requires the microsecond precision? What issues would be resolved by changing the resolution of that duration?
Is there something you're using the AutomatedTestWidgetsFlutterBinding for that requires the microsecond precision?
Yes, https://github.com/fzyzcjy/flutter_smooth
What issues would be resolved by changing the resolution of that duration?
The testing of https://github.com/fzyzcjy/flutter_smooth ;)
@pdblasi-google Btw that package is WIP, but has discussed a lot in flutter Discord "hackers-framework" already. see the links in the README of the repo
For more details if you are interested:
The flutter_smooth package treat the timeStamp as vsync data, which is exactly a multiple of 16.67ms (plus a few microsecond shift) from C++ engine, and it works well for real devices. But in testing env, it originally receive only 16.00 ms (and its multiples), so then it errors.
Anyway, milliseconds mean it lose accuracy while microsecond is the correct data. IMHO we should provide the correct data to the users.
The flag is there to maintain compatibility for old code relying on the inprecise timestamp data.
I've seen the discord discussions, so I thought this might be related. Thanks for confirming.
My concern is that there isn't any documentation whether the millisecond limited precision is expected behavior or not. It could be a mistake that was made, or it could be that the millisecond precision is there on purpose.
Well shoot, beat me to the details. My followup was going to be checking what precision the engine was at and making sure we're lined up with that here. Do you have a link to where you found the 16.67ms figure, or was that just from your local testing?
@pdblasi-google Thanks :)
Do you have a link to where you found the 16.67ms figure, or was that just from your local testing?
From my local testing, if you need I can provide some sample data (spoiler: very dumb, just time, time+16.667ms, time+2x16.667ms etc, no special things).
That is also confirmed from source code. Details are in the next post:
Details showing why begin frame has argument that increase by 16.667ms - because it is a real vsync signal data.
- VsyncWaiter::FireCallback has "frame_target_time" (the vsync data, 16.667ms interval), save to FrameTimingsRecorder::RecordVsync
- the FireCallback indeed fires Animator::BeginFrame. The Animator::BeginFrame calls FrameTimingsRecorder::GetVsyncTargetTime, to read that frame_target_time we just store.
- Then call the delegate_.OnAnimatorBeginFrame with frame_target_time.
- that OnAnimatorBeginFrame calls Shell::OnAnimatorBeginFrame, indeed calls Engine::BeginFrame, indeed RuntimeController::BeginFrame, indeed PlatformConfiguration::BeginFrame, indeed
tonic::DartInvoke(begin_frame_.Get()) - Thus the Dart's
_beginFramehook is called. Then indeed call the begin frame we are all familiar with.
Perfect. Between that and my own findings, I'm pretty confident you're correct that we should be supporting all of the precision we can with this binding. Here're my findings as well for anyone who stumbles across this issue:
- The
VsyncWaiterclass is abstract and intended to be implemented by the various platforms - There is a fallback implemented (
VsyncWaiterFallback) that is available in case there is no platform implementation available- It can be assumed that the fallback would fulfill at least the minimal precision expectation
- This fallback defines and uses
kSingleFrameIntervalas 1/60th of a second here - The [
TimePoint] class used to defineframe_target_timeandkSingleFrameIntervalhas nanosecond precision - Therefore we should support microsecond precision in the test binding to get as close to the expected precision as possible
Reacted by fzyzcjy- The
I'm going to edit the title of your issue here for discoverability, but other than that I think we're good to go on the issue side. I'll follow up on the actual PR shortly.
Thanks for finding this and helping confirm and document the issue!
Sure and you are welcome! :)
7 remaining items
@pdblasi-google Take your time, thanks for the updates, and looking forward to the progress!
This issue is assigned but has had no recent status updates. Please consider unassigning this issue if it is not going to be addressed in the near future. This allows people to have a clearer picture of what work is actually planned. Thanks!
👀 Looking forward to the fix!
I'm going to see if I can push this through the G3 issues we saw last time without having to go through the heavier breaking change and deprecation process I originally planned. Now that I've got a better idea of how the system works, I think that's the better option.
Reacted by fzyzcjy- added a commit that references this issue
on Aug 14, 2023 This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of
flutter doctor -vand a minimal reproduction of the issue.- locked as resolved and limited conversation to collaborators
on Aug 29, 2023
Companion issue fixed by #112609. Please see PR for description :)
Copied from PR
The fix is just one line:
I have
git blameand find the bug exist since the first version 7yr ago, and no special comments about why this is introduced so I guess it is but not feature.IMHO the bug may be written like, the programmer wants to convert DateTime (the
clock.now()) into a Duration. But then it is forgotten that both are microseconds precision instead of milliseconds precision, and the milliseconds approach is used.