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

Added assert for opengles thread safety #56585

Merged
merged 2 commits into from
Nov 15, 2024
Merged

Added assert for opengles thread safety #56585

merged 2 commits into from
Nov 15, 2024

Conversation

gaaclarke
Copy link
Member

This assert would have blocked the following bug: flutter/flutter#158535

This shouldn't be landed until #56573 lands.

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.

@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!).

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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group.

jonahwilliams
jonahwilliams reviewed Nov 14, 2024
View reviewed changes
impeller/renderer/backend/gles/proc_table_gles.h
@@ -99,6 +101,14 @@ struct GLProc {
///
bool log_calls = false;

//----------------------------------------------------------------------------
/// Whether the OpenGL call asserts it is only used from / one thread in
/// IMPELLER_DEBUG builds.
Copy link
Member

Choose a reason for hiding this comment

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

IMPELLER_DEBUG is debug/profile mode. !defined(NDEBUG) means unopt local engine IIRC, that should be fine.

jonahwilliams
jonahwilliams suggested changes Nov 14, 2024
View reviewed changes
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

I don't think this is correct. We can call blitFramebuffer from the raster thread as part of rendering and still separately use blitFramebuffer from the iio thread for image resize.

@gaaclarke
Copy link
Member Author

I don't think this is correct. We can call blitFramebuffer from the raster thread as part of rendering and still separately use blitFramebuffer from the iio thread for image resize.

The only symbol that is set up to enforce this is glUseProgram. Under no circumstances today is it valid to use glUseProgram from any thread but the raster thread. No meaningful drawing can be done without glUseProgram, blitting and uploading texture data is fine.

@jonahwilliams
Copy link
Member

oh duh, kk

jonahwilliams
jonahwilliams approved these changes Nov 14, 2024
View reviewed changes
Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

@gaaclarke
Copy link
Member Author

It would be awesome if this failed on CI without jasons patch. I doubt that's going to happen though since it requires a bit of usage of the app before the foul happens.

@gaaclarke
Copy link
Member Author

It might break benchmarks on device lab though, so that's good.

@jonahwilliams
Copy link
Member

It won't because those don't run unopt builds.

@gaaclarke
Copy link
Member Author

It won't because those don't run unopt builds.

Hmm, that sounds like we should potentially move this to just #if IMPELLER_DEBUG then, no?

@jonahwilliams
Copy link
Member

We should have expensive debug checks active. We've already found that adding too much overhead via validation layers negatively impacts developer experience.

@jonahwilliams
Copy link
Member

should = shouldn't 🤦

@gaaclarke gaaclarke merged commit 79fe6b4 into flutter:main Nov 15, 2024
28 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
a0cf243
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
6677c01
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
cad6fb9
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
f32562a
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
33c8857
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 15, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 15, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
f577eda
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Nov 16, 2024
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Nov 16, 2024
@engine-flutter-autoroll
79fe6b466 Added assert for opengles thread safety (flutter/engine#56585)
9765ce2
github-merge-queue bot pushed a commit to flutter/flutter that referenced this pull request Nov 16, 2024
@engine-flutter-autoroll
Roll Flutter Engine from 619804c0fbb7 to f649330affa8 (47 revisions) (#…
1ef8d51 
…159015)

Roll Flutter Engine from 619804c0fbb7 to f649330affa8 (47 revisions)

flutter/engine@619804c...f649330

2024-11-16 [email protected] Roll Skia from 1ef3b910e064 to
6b0f264bde33 (6 revisions) (flutter/engine#56635)
2024-11-16 [email protected] Revert "Remove
`android_jit_release_x86`." (flutter/engine#56634)
2024-11-15 [email protected] Move SetRenderTargetType to
EmbedderTestCompositor (flutter/engine#56626)
2024-11-15 [email protected] Roll Dart SDK from
b62dadb14d82 to eccb15756020 (2 revisions) (flutter/engine#56630)
2024-11-15 [email protected] [TextInput] Add
TextInputType.webSearch (#15762) (flutter/engine#56428)
2024-11-15 [email protected] [web] Send the correct view ID with
semantics actions (flutter/engine#56595)
2024-11-15 [email protected] [macos] early
return to suppress clang-tidy warning (flutter/engine#56588)
2024-11-15 [email protected] [Impeller] libImpeller: Tinker on the
README to cross reference the SDK and the example.
(flutter/engine#56623)
2024-11-15 [email protected] [fuchsia] MaybeRunInitialVsyncCallback
should only called once (flutter/engine#56429)
2024-11-15 [email protected] Move mac clangd build from x64 to arm
(flutter/engine#56567)
2024-11-15 [email protected] Added assert for
opengles thread safety (flutter/engine#56585)
2024-11-15 [email protected] [Impeller] enable OpenGL fallback on
Android. (flutter/engine#56591)
2024-11-15 [email protected] [Impeller] Maintain
separate queues of GLES operations for each thread in the reactor
(flutter/engine#56573)
2024-11-15 [email protected] Roll Dart SDK from
abe86ca4eb01 to b62dadb14d82 (2 revisions) (flutter/engine#56615)
2024-11-15 [email protected] Roll Skia from b730eb340852 to
1ef3b910e064 (4 revisions) (flutter/engine#56613)
2024-11-15 [email protected] Roll Fuchsia Linux SDK from
JkpuAsLzcmYLzf1iX... to UpSQzyXGUhMfedYIh... (flutter/engine#56611)
2024-11-15 [email protected] Roll Dart SDK from
bf0eeb9d929c to abe86ca4eb01 (1 revision) (flutter/engine#56610)
2024-11-15 [email protected] Roll Skia from e5fda8472b21 to
b730eb340852 (1 revision) (flutter/engine#56608)
2024-11-15 [email protected] Roll Skia from 8ff65da4b8bf to
e5fda8472b21 (1 revision) (flutter/engine#56603)
2024-11-15 [email protected] Move TestMetalContext/Surface to testing
namespace (flutter/engine#56602)
2024-11-15 [email protected] Roll Dart SDK from
343b9fc6eafe to bf0eeb9d929c (1 revision) (flutter/engine#56604)
2024-11-15 [email protected] [Impeller] Don't enable KHR_debug on
Angle. (flutter/engine#56601)
2024-11-15 [email protected] [Impeller] libImpeller: Reset the GL
state when transitioning control back to the embedder.
(flutter/engine#56597)
2024-11-15 [email protected] Roll Skia from 8946ea477b42 to
8ff65da4b8bf (2 revisions) (flutter/engine#56600)
2024-11-14 [email protected] [Impeller] Put test components in the
testing namespace. (flutter/engine#56594)
2024-11-14 [email protected] [Impeller] libImpeller: Add a warning
about OpenGL state being trampled. (flutter/engine#56599)
2024-11-14 [email protected] Embedder: Refactor EmbedderConfigBuilder by
backend (flutter/engine#56598)
2024-11-14 [email protected] Migrate Metal translation units to Obj-C++
(flutter/engine#56592)
2024-11-14 [email protected] Roll Dart SDK from
db4d7d52db48 to 343b9fc6eafe (1 revision) (flutter/engine#56590)
2024-11-14 [email protected] iOS,macOS: Build/test
test_metal_surface_unittests (flutter/engine#56586)
2024-11-14 [email protected] Roll Skia from 3222456e63dc to
8946ea477b42 (2 revisions) (flutter/engine#56587)
2024-11-14 [email protected] Roll Skia from 16178bf63de7 to
3222456e63dc (3 revisions) (flutter/engine#56584)
2024-11-14 [email protected] [Android] Fix Slider semantics double
tap behaviors (flutter/engine#56452)
2024-11-14 [email protected] Roll Dart SDK from
a3b6652100c8 to db4d7d52db48 (1 revision) (flutter/engine#56581)
2024-11-14 [email protected] Move Vulkan context to
EmbedderTestContextVulkan (flutter/engine#56571)
2024-11-14 [email protected] Roll Skia from a076435073fc to
16178bf63de7 (1 revision) (flutter/engine#56579)
2024-11-14 [email protected] Roll Skia from 70553189e4e8 to
a076435073fc (1 revision) (flutter/engine#56577)
2024-11-14 [email protected] Roll Skia from fa52f2c1ddba to
70553189e4e8 (1 revision) (flutter/engine#56576)
2024-11-14 [email protected] Roll Dart SDK from
3f05b3540cb2 to a3b6652100c8 (3 revisions) (flutter/engine#56575)
2024-11-14 [email protected] Roll Skia from ddbd8d1cb8d8 to
fa52f2c1ddba (3 revisions) (flutter/engine#56574)
2024-11-14 [email protected] [Android] choose 24 bit depth
buffer. (flutter/engine#56553)
2024-11-14 [email protected] Roll Skia from 3750b8939c7f to
ddbd8d1cb8d8 (1 revision) (flutter/engine#56562)
2024-11-14 [email protected] Move FlMouseCursorHandler from
FlView to FlEngine (flutter/engine#56026)
2024-11-14 [email protected] [Impeller] force software resize
usage for GLES backend. (flutter/engine#56511)
2024-11-14 [email protected] Roll Dart SDK from
66ab1774bfe9 to 3f05b3540cb2 (2 revisions) (flutter/engine#56564)
2024-11-14 [email protected] Embedder: Refactor EmbedderTestContext
creation (flutter/engine#56563)
...
jason-simmons added a commit to jason-simmons/flutter_engine that referenced this pull request Nov 20, 2024
@jason-simmons
Revert "Added assert for opengles thread safety (flutter#56585)"
29bbff0 
This reverts commit 79fe6b4.

The implementation of the assert assumes that there is a single raster thread ID that will remain constant throughout the lifetime of the process.  That is not true for scenarios like recreating the engine after suspending and resuming an Android app, or instantiating multiple engines within one process.
auto-submit bot pushed a commit that referenced this pull request Nov 20, 2024
@jason-simmons
Revert "Added assert for opengles thread safety (#56585)" (#56730)
2557027 
This reverts commit 79fe6b4.

The implementation of the assert assumes that there is a single raster thread ID that will remain constant throughout the lifetime of the process.  That is not true for scenarios like recreating the engine after suspending and resuming an Android app, or instantiating multiple engines within one process.
nick9822 pushed a commit to nick9822/flutter that referenced this pull request Dec 18, 2024
@gaaclarke
Added assert for opengles thread safety (flutter/engine#56585)
11123f1 
This assert would have blocked the following bug:
flutter#158535

This shouldn't be landed until
flutter/engine#56573 lands.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide] and the [C++,
Objective-C, Java style guides].
- [x] 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.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I signed the [CLA].
- [x] 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
@doodlezucc doodlezucc mentioned this pull request Dec 28, 2024
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonahwilliams jonahwilliams jonahwilliams approved these changes

@jason-simmons jason-simmons Awaiting requested review from jason-simmons

Assignees
No one assigned
Labels
e: impeller
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gaaclarke @jonahwilliams