Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Conversation

@ColdPaleLight
Copy link
Member

@ColdPaleLight ColdPaleLight commented Jan 21, 2022

Fix issue:
flutter/flutter#45131
flutter/flutter#91717

Detail: flutter/flutter#45131 (comment)

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
    writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the [CLA].
  • All existing and new tests are passing.

@ColdPaleLight ColdPaleLight force-pushed the simplify-schedule-frame branch from 963bad7 to 61683c2 Compare January 21, 2022 15:57
@dnfield
Copy link
Contributor

dnfield commented Jan 21, 2022

This seems like it means we'll call Dart_NotifyIdle more often, right? In that case, we could end up doing more GC work than we really ought to be when the application is backgrounded.

@ColdPaleLight
Copy link
Member Author

ColdPaleLight commented Jan 24, 2022

This seems like it means we'll call Dart_NotifyIdle more often, right? In that case, we could end up doing more GC work than we really ought to be when the application is backgrounded.

@dnfield
When the app is in the background, AppLifecycleState is AppLifecycleState.paused. So the SchedulerBinding._framesEnabled has been set to false.
https://github.com/flutter/flutter/blob/ab0a33597328772fe27dbc0b217ac930373ef5f0/packages/flutter/lib/src/scheduler/binding.dart#L348-L361

  void handleAppLifecycleStateChanged(AppLifecycleState state) {
    assert(state != null);
    _lifecycleState = state;
    switch (state) {
      case AppLifecycleState.resumed:
      case AppLifecycleState.inactive:
        _setFramesEnabledState(true);
        break;
      case AppLifecycleState.paused:
      case AppLifecycleState.detached:
        _setFramesEnabledState(false);
        break;
    }
  }

Therefore, after the App enters the background,SchedulerBinding.scheduleFrame will return early here. So Animator::RequestFrame and Dart_NotifyIdle will not be called more often than before.
https://github.com/flutter/flutter/blob/ab0a33597328772fe27dbc0b217ac930373ef5f0/packages/flutter/lib/src/scheduler/binding.dart#L781-L783

  void scheduleFrame() {
    if (_hasScheduledFrame || !framesEnabled)
      return;

This PR is mainly to allow scheduleForcedFrame to work correct in the future.
https://github.com/flutter/flutter/blob/ab0a33597328772fe27dbc0b217ac930373ef5f0/packages/flutter/lib/src/scheduler/binding.dart#L814-L818

  void scheduleForcedFrame() {
    // TODO(chunhtai): Removes the if case once the issue is fixed
    // https://github.com/flutter/flutter/issues/45131
    if (!framesEnabled)
      return;

Simplifying the logic of Animator::RequestFrame will also benefit the maintenance of the engine.

@dnfield dnfield requested a review from iskakaushik January 24, 2022 17:29
@dnfield
Copy link
Contributor

dnfield commented Jan 24, 2022

I'm a big fan of this in that case.

I'd appreciate input from @chinmaygarde and @iskakaushik (and of course @gaaclarke and @chunhtai if they have any input)

engine->OnOutputSurfaceCreated();
// this implies we can re-use the last
// frame to trigger begin frame rather
// than re-generating the layer tree.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this since the framework ought to handle frame scheduling now. If it is redundant, we can get rid of the RunNowOrPostTask call completely. We can do this in a later patch if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@ColdPaleLight feel free to add the waiting for tree to go green label if you think this is good to go, I'm holding off on adding it in case you plan to address this now)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll do this in a later patch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants