AutomatedTestWidgetsFlutterBinding.pump should match engine frame precision #112610

Activity

1. danagbemava-nc

   added

   in triagePresently being triaged by the triage teamPresently being triaged by the triage team

   a: tests"flutter test", flutter_test, or one of our tests"flutter test", flutter_test, or one of our tests

   frameworkflutter/packages/flutter repository. See also f: labels.flutter/packages/flutter repository. See also f: labels.

   and removed

   in triagePresently being triaged by the triage teamPresently being triaged by the triage team

   on Sep 29, 2022
2. pdblasi-google commented on Sep 30, 2022

   

   pdblasi-google

   on Sep 30, 2022

   Contributor

   More actions

   @fzyzcjy

   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?
3. fzyzcjy commented on Sep 30, 2022

   

   fzyzcjy

   on Sep 30, 2022

   ContributorAuthor

   More actions

   @pdblasi-google

   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 ;)
4. fzyzcjy commented on Sep 30, 2022

   

   fzyzcjy

   on Sep 30, 2022

   ContributorAuthor

   More actions

   @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
5. fzyzcjy commented on Sep 30, 2022

   

   fzyzcjy

   on Sep 30, 2022

   ContributorAuthor

   More actions

   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.
6. pdblasi-google commented on Sep 30, 2022

   

   pdblasi-google

   on Sep 30, 2022

   Contributor

   More actions

   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.
7. pdblasi-google commented on Sep 30, 2022

   

   pdblasi-google

   on Sep 30, 2022

   Contributor

   More actions

   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?
8. fzyzcjy commented on Sep 30, 2022

   

   fzyzcjy

   on Sep 30, 2022

   Last edited by fzyzcjy

   ContributorAuthor

   More actions

   @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:
9. fzyzcjy commented on Sep 30, 2022

   

   fzyzcjy

   on Sep 30, 2022

   Last edited by fzyzcjy

   ContributorAuthor

   More actions

   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 _beginFrame hook is called. Then indeed call the begin frame we are all familiar with.
10. pdblasi-google commented on Sep 30, 2022

    

    pdblasi-google

    on Sep 30, 2022

    Contributor

    More actions

    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 VsyncWaiter class 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 kSingleFrameInterval as 1/60th of a second here
    • The [TimePoint] class used to define frame_target_time and kSingleFrameInterval has nanosecond precision
    • Therefore we should support microsecond precision in the test binding to get as close to the expected precision as possible

    😄React with 😄1Reacted by fzyzcjy
11. pdblasi-google commented on Sep 30, 2022

    

    pdblasi-google

    on Sep 30, 2022

    Contributor

    More actions

    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!
12. fzyzcjy commented on Sep 30, 2022

    

    fzyzcjy

    on Sep 30, 2022

    ContributorAuthor

    More actions

    Sure and you are welcome! :)
13. 7 remaining items

    Load more

14. fzyzcjy commented on Nov 22, 2022

    

    fzyzcjy

    on Nov 22, 2022

    ContributorAuthor

    More actions

    @pdblasi-google Take your time, thanks for the updates, and looking forward to the progress!
15. flutter-triage-bot

    added

    team-frameworkOwned by Framework teamOwned by Framework team

    triaged-frameworkTriaged by Framework teamTriaged by Framework team

    and removed

    triaged-frameworkTriaged by Framework teamTriaged by Framework team

    on Jul 8, 2023
16. flutter-triage-bot commented on Jul 9, 2023

    

    flutter-triage-botbot

    on Jul 9, 2023

    More actions

    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!
17. fzyzcjy commented on Jul 9, 2023

    

    fzyzcjy

    on Jul 9, 2023

    Last edited by fzyzcjy

    ContributorAuthor

    More actions

    👀 Looking forward to the fix!
18. flutter-triage-bot

    added

    ⏳Bot is counting down the days until it unassigns the issueBot is counting down the days until it unassigns the issue

    on Jul 30, 2023
19. pdblasi-google commented on Aug 11, 2023

    

    pdblasi-google

    on Aug 11, 2023

    Contributor

    More actions

    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.

    😄React with 😄1Reacted by fzyzcjy
20. flutter-triage-bot

    removed

    ⏳Bot is counting down the days until it unassigns the issueBot is counting down the days until it unassigns the issue

    on Aug 12, 2023
21. auto-submit

    closed this as completedin #132401on Aug 14, 2023
22. auto-submit

    added a commit that references this issue on Aug 14, 2023

    Updates `AutomatedTestWidgetsFlutterBinding.pump` to support microsec…

    ...

    This issue will close once commit e1ec358 is merged into the 'master' branch.

    e1ec358
23. github-actions commented on Aug 29, 2023

    

    github-actionsbot

    on Aug 29, 2023

    More actions

    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 -v and a minimal reproduction of the issue.
24. github-actions

    locked as resolved and limited conversation to collaborators on Aug 29, 2023