[Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` #46392

matanlurey · 2023-09-29T00:51:45Z

I plan to add tests before submitting once #46433 lands, probably something around glDebugMessageControl being emitted once per context, since that's what we need to keep working for Pixel phones.

Output

Screenshot of app running locally, showing lack of off-screen MSAA

$ fl run --enable-impeller --local-engine=$ENGINE/out/android_debug_unopt_arm64 --local-engine-host=$ENGINE/out/host_debug_unopt_arm64

Launching lib/main.dart on Pixel 5 in debug mode...
Running Gradle task 'assembleDebug'...                             33.7s
✓  Built build/app/outputs/flutter-apk/app-debug.apk.
Installing build/app/outputs/flutter-apk/app-debug.apk...          17.8s
E/flutter (17749): [ERROR:flutter/shell/platform/android/android_context_gl_impeller.cc(80)] Using the Impeller rendering backend (OpenGLES).
Syncing files to device Pixel 5...                                  70ms

Flutter run key commands.
r Hot reload. 🔥🔥🔥
R Hot restart.
h List all available interactive commands.
d Detach (terminate "flutter run" but leave application running).
c Clear the screen
q Quit (terminate the application on the device).

A Dart VM Service on Pixel 5 is available at: http://127.0.0.1:57181/CrsakIgCHFY=/
I/cle_opacity_foo(17749): Compiler allocated 4413KB to compile void android.view.ViewRootImpl.performTraversals()
The Flutter DevTools debugger and profiler on Pixel 5 is available at:
http://127.0.0.1:9101?uri=http://127.0.0.1:57181/CrsakIgCHFY=/

flutter-dashboard · 2023-09-29T00:51:47Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

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.

impeller/renderer/backend/gles/description_gles.h

impeller/renderer/backend/gles/proc_table_gles.cc

…ebugGroupKHR-crashFix

impeller/renderer/backend/gles/proc_table_gles.cc

impeller/renderer/backend/gles/reactor_gles.cc

…ebugGroupKHR-crashFix

chinmaygarde · 2023-10-03T20:03:50Z

impeller/renderer/backend/gles/reactor_gles.cc

 bool ReactorGLES::FlushOps() {
  TRACE_EVENT0("impeller", __FUNCTION__);
+
+#ifdef IMPELLER_DEBUG


How about setting the proc tables entries to be no-ops in case IMPELLER_DEBUG is set? Prevents having to litter the code with ifdef IMPELLER_DEBUG.

We do this for extensions in the proc table. For instance, get the proc but reset it if the extension is absent. You can do the same for debug extensions, discard ifndef IMPELLER_DEBUG.

chinmaygarde · 2023-10-03T20:05:02Z

impeller/renderer/backend/gles/blit_pass_gles.cc

 #include "flutter/fml/trace_event.h"
-#include "impeller/base/config.h"
-#include "impeller/base/validation.h"
+#include "fml/closure.h"


The fml bits use flutter/fml. See the include right above.

…ugGroup` (flutter/engine#46392)

…135938) flutter/engine@8ab7c88...eef2449 2023-10-03 [email protected] Roll Skia from c66c89c56549 to 4cc56b6b3453 (1 revision) (flutter/engine#46525) 2023-10-03 [email protected] [Impeller] Implement a `MockGLES`, that provides trampolines for `ProcGLESTable` (flutter/engine#46433) 2023-10-03 [email protected] [Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` (flutter/engine#46392) 2023-10-03 [email protected] Roll Dart SDK from 318c46832196 to 817f77ceba40 (1 revision) (flutter/engine#46520) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…lutter#135938) flutter/engine@8ab7c88...eef2449 2023-10-03 [email protected] Roll Skia from c66c89c56549 to 4cc56b6b3453 (1 revision) (flutter/engine#46525) 2023-10-03 [email protected] [Impeller] Implement a `MockGLES`, that provides trampolines for `ProcGLESTable` (flutter/engine#46433) 2023-10-03 [email protected] [Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` (flutter/engine#46392) 2023-10-03 [email protected] Roll Dart SDK from 318c46832196 to 817f77ceba40 (1 revision) (flutter/engine#46520) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

…46392) Closes flutter/flutter#135715. I plan to add tests before submitting once #46433 lands, probably something around `glDebugMessageControl` being emitted once per context, since that's what we need to keep working for Pixel phones. ## Output <details> <summary>Screenshot of app running locally, showing lack of off-screen MSAA</summary> ![flutter_01](https://github.com/flutter/engine/assets/168174/23642582-ce4c-47dc-8edf-7e6cfd12a074) </details> ```shell $ fl run --enable-impeller --local-engine=$ENGINE/out/android_debug_unopt_arm64 --local-engine-host=$ENGINE/out/host_debug_unopt_arm64 Launching lib/main.dart on Pixel 5 in debug mode... Running Gradle task 'assembleDebug'... 33.7s ✓ Built build/app/outputs/flutter-apk/app-debug.apk. Installing build/app/outputs/flutter-apk/app-debug.apk... 17.8s E/flutter (17749): [ERROR:flutter/shell/platform/android/android_context_gl_impeller.cc(80)] Using the Impeller rendering backend (OpenGLES). Syncing files to device Pixel 5... 70ms Flutter run key commands. r Hot reload. 🔥🔥🔥 R Hot restart. h List all available interactive commands. d Detach (terminate "flutter run" but leave application running). c Clear the screen q Quit (terminate the application on the device). A Dart VM Service on Pixel 5 is available at: http://127.0.0.1:57181/CrsakIgCHFY=/ I/cle_opacity_foo(17749): Compiler allocated 4413KB to compile void android.view.ViewRootImpl.performTraversals() The Flutter DevTools debugger and profiler on Pixel 5 is available at: http://127.0.0.1:9101?uri=http://127.0.0.1:57181/CrsakIgCHFY=/ ```

Invoke glDebugMessageControl before glPushDebugGroup.

1105e01

matanlurey requested a review from jason-simmons September 29, 2023 00:51

github-actions bot added the e: impeller label Sep 29, 2023

matanlurey changed the title ~~Invoke glDebugMessageControl before glPushDebugGroup~~ [Impeller] Invoke glDebugMessageControl before glPushDebugGroup Sep 29, 2023

jason-simmons reviewed Sep 29, 2023

View reviewed changes

impeller/renderer/backend/gles/description_gles.h Outdated Show resolved Hide resolved

impeller/renderer/backend/gles/proc_table_gles.cc Outdated Show resolved Hide resolved

matanlurey added 3 commits September 29, 2023 14:03

Update feedback, fix some imports.

9a86d3a

Merge remote-tracking branch 'upstream/main' into impeller-openGLES-d…

10be196

…ebugGroupKHR-crashFix

Fix imports.

647e399

matanlurey requested review from jason-simmons and jonahwilliams September 30, 2023 03:00

jason-simmons reviewed Sep 30, 2023

View reviewed changes

impeller/renderer/backend/gles/proc_table_gles.cc Outdated Show resolved Hide resolved

impeller/renderer/backend/gles/reactor_gles.cc Outdated Show resolved Hide resolved

matanlurey added 3 commits October 3, 2023 09:58

Merge remote-tracking branch 'upstream/main' into impeller-openGLES-d…

74b096e

…ebugGroupKHR-crashFix

Revert changes to PushDebugGroup.

bd62d30

Remove comment.

b981b06

matanlurey requested a review from jason-simmons October 3, 2023 19:21

chinmaygarde assigned matanlurey Oct 3, 2023

chinmaygarde approved these changes Oct 3, 2023

View reviewed changes

jason-simmons approved these changes Oct 3, 2023

View reviewed changes

matanlurey merged commit d29369f into flutter:main Oct 3, 2023

matanlurey deleted the impeller-openGLES-debugGroupKHR-crashFix branch October 3, 2023 22:14

engine-flutter-autoroll mentioned this pull request Oct 3, 2023

Roll Flutter Engine from 8ab7c8841ee5 to eef2449ff80b (4 revisions) flutter/flutter#135938

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 3, 2023

d29369f87 [Impeller] Invoke glDebugMessageControl before `glPushDeb…

116714c

…ugGroup` (flutter/engine#46392)

matanlurey mentioned this pull request Oct 9, 2023

[Impeller] OpenGL: Support offscreen MSAA flutter/flutter#130045

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` #46392

[Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` #46392

Uh oh!

matanlurey commented Sep 29, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Sep 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chinmaygarde Oct 3, 2023

Uh oh!

chinmaygarde Oct 3, 2023

Uh oh!

chinmaygarde Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Impeller] Invoke glDebugMessageControl before glPushDebugGroup #46392

[Impeller] Invoke glDebugMessageControl before glPushDebugGroup #46392

Uh oh!

Conversation

matanlurey commented Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Output

Uh oh!

flutter-dashboard bot commented Sep 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chinmaygarde Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

chinmaygarde Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` #46392

[Impeller] Invoke `glDebugMessageControl` before `glPushDebugGroup` #46392

matanlurey commented Sep 29, 2023 •

edited

Loading