Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reland 3: Multiview pipeline #49950

Merged
merged 34 commits into from
Feb 23, 2024
Merged

Conversation

dkwingsmt
Copy link
Contributor

@dkwingsmt dkwingsmt commented Jan 22, 2024

This is the 3rd attempt to land multiview pipeline, following #47239.

The pipeline now properly implements the required logic for scheduleWarmUpFrame to work in a multi-view setup, following the preparation in flutter/flutter#143290 and #50570.

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 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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@dkwingsmt dkwingsmt force-pushed the reland-3-mv-pipeline branch from d8afe9d to 01fee45 Compare January 22, 2024 22:53
@dkwingsmt dkwingsmt marked this pull request as ready for review February 3, 2024 06:45
@dkwingsmt dkwingsmt requested a review from loic-sharma February 5, 2024 04:53
@dkwingsmt dkwingsmt force-pushed the reland-3-mv-pipeline branch 4 times, most recently from fe0c92a to 4d8e42f Compare February 11, 2024 22:41
@dkwingsmt dkwingsmt force-pushed the reland-3-mv-pipeline branch from 4d8e42f to f67fe66 Compare February 12, 2024 04:59
@github-actions github-actions bot added the platform-web Code specifically for the web engine label Feb 12, 2024
@dkwingsmt dkwingsmt force-pushed the reland-3-mv-pipeline branch from f67fe66 to bc69ddc Compare February 12, 2024 05:14
@dkwingsmt dkwingsmt marked this pull request as draft February 12, 2024 19:04
@dkwingsmt dkwingsmt mentioned this pull request Feb 14, 2024
8 tasks
dkwingsmt added a commit that referenced this pull request Feb 22, 2024
This PR adds `PlatformDispatcher.scheduleWarmUpFrame`.

This PR is needed for the follow up changes:
* The framework will switch to using this function to render warmup
frames in flutter/flutter#143290.
* Then the engine will finally be able to switch to multiview pipeline
with no regression on startup timing in
#49950.

For why the warm up frame must involve the engine to render, see
flutter/flutter#142851.


## 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 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.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
@dkwingsmt dkwingsmt merged commit 9409b75 into flutter:main Feb 23, 2024
24 checks passed
} else if (!result.is_first_item) {
// Do nothing. It has been successfully pushed to the pipeline but not as
// the first item. Eventually the 'Rasterizer' will consume it, so we
// don't need to notify the delegate.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm suspicious that this branch might be cuasing the redness on CI.

dkwingsmt added a commit that referenced this pull request Feb 23, 2024
@dkwingsmt dkwingsmt added the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
Copy link
Contributor

auto-submit bot commented Feb 23, 2024

A reason for requesting a revert of flutter/engine/49950 could
not be found or the reason was not properly formatted. Begin a comment with 'Reason for revert:' to tell the bot why
this issue is being reverted.

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
@dkwingsmt
Copy link
Contributor Author

Reason for revert: Head redness

java.lang.RuntimeException: Timeout waiting for firstFrameLatch to signal
	at dev.flutter.scenarios.ExternalTextureFlutterActivity.waitUntilFlutterRendered(ExternalTextureFlutterActivity.java:98)
	at dev.flutter.scenariosui.ScreenshotUtil.capture(ScreenshotUtil.java:122)

@dkwingsmt dkwingsmt added the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
auto-submit bot pushed a commit that referenced this pull request Feb 23, 2024
@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
@dkwingsmt dkwingsmt added the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
Copy link
Contributor

auto-submit bot commented Feb 23, 2024

Unable to create the revert pull request due to ProcessException: Standard error
Pushing to https://github.com/flutter/engine.git
To https://github.com/flutter/engine.git
! [rejected] revert_9409b75e8f35c53e6937a8e7e98b101372a95096 -> revert_9409b75e8f35c53e6937a8e7e98b101372a95096 (non-fast-forward)
error: failed to push some refs to 'https://github.com/flutter/engine.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
Command: git push --verbose origin revert_9409b75e8f35c53e6937a8e7e98b101372a95096

@auto-submit auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Feb 23, 2024
dkwingsmt pushed a commit that referenced this pull request Feb 23, 2024
Reverts #49950

Initiated by: dkwingsmt

Reason for reverting: Head redness 
```
java.lang.RuntimeException: Timeout waiting for firstFrameLatch to signal
	at dev.flutter.scenarios.ExternalTextureFlutterActivity.waitUntilFlutterRendered(ExternalTextureFlutterActivity.java:98)
	at dev.flutter.scenariosui.ScreenshotUtil.capture(ScreenshotUtil.java:122)
```

Original PR Author: dkwingsmt

Reviewed By: {loic-sharma}

This change reverts the following previous change:
Original Description:
This is the 3rd attempt to land multiview pipeline, following
#47239.

The pipeline now properly implements the required logic for
`scheduleWarmUpFrame` to work in a multi-view setup, following the
preparation in flutter/flutter#143290 and
#50570.

## 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 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.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat

Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 23, 2024
dkwingsmt added a commit to dkwingsmt/engine that referenced this pull request Feb 23, 2024
@dkwingsmt dkwingsmt mentioned this pull request Feb 23, 2024
8 tasks
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Feb 24, 2024
…144061)

flutter/engine@733163c...9409b75

2024-02-23 dkwingsmt@users.noreply.github.com Reland 3: Multiview pipeline  (flutter/engine#49950)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC jimgraham@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
dkwingsmt added a commit that referenced this pull request Feb 24, 2024
This relands #49950.

Nothing is changed. The error turns out a flake (it passes once),
although this PR might have made the flake more flaky.

## 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 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.

If you need help, consider asking for advice on the #hackers-new channel
on [Discord].

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#overview
[Tree Hygiene]: https://github.com/flutter/flutter/wiki/Tree-hygiene
[test-exempt]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo
[C++, Objective-C, Java style guides]:
https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
[testing the engine]:
https://github.com/flutter/flutter/wiki/Testing-the-engine
[CLA]: https://cla.developers.google.com/
[flutter/tests]: https://github.com/flutter/tests
[breaking change policy]:
https://github.com/flutter/flutter/wiki/Tree-hygiene#handling-breaking-changes
[Discord]: https://github.com/flutter/flutter/wiki/Chat
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-web Code specifically for the web engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants