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

RendererContextSwitch guard flutter's gl context rework.#13812

Merged

cyanglaz merged 6 commits intoflutter:masterfrom

cyanglaz:gl_rework

Nov 14, 2019

Contributor

cyanglaz commented Nov 13, 2019

This is a rework for the RendererContextSwitching.
This PR added:
0. A complete revert of #13788.

Refactored the Rasterizer.cc around ScreenshotLastLayerTree to make GLContext available for screenshots.
shell/RasterizerScreenshot tests which prevents a previous framework screenshot related test failure.
shell/RasterizerMakeRasterSnapshot to prevent a regression from a typo, mentioned in Crash in ShaderWarmUp.execute() flutter#31355 (comment)

All the changes addition to the previous context switch(1 - 3 above) commit will be commented in the code.

Chris Yang added 3 commits

November 12, 2019 17:56


          add the orignal gl context switch code

f02a59e


          unit tests and fix inverted context result check

5e0943f


          more shell test to catch regression

8546c90

googlebot added the cla: yes label

cyanglaz commented

View reviewed changes

Contributor Author

cyanglaz left a comment

Adding comments to all the additional code after the reverted renderer context switch PR.

shell/common/rasterizer.cc

Comment on lines +440 to +480

+              sk_sp<SkImage> Rasterizer::MakeImageSnapshot(
+                  sk_sp<SkSurface> snapshot_surface) {
+                std::unique_ptr<RendererContextSwitchManager::RendererContextSwitch>
+                    context_switch = surface_->MakeRenderContextCurrent();
+                if (!context_switch->GetSwitchResult()) {
+                  return nullptr;
+                }
+                auto potentially_gpu_snapshot = snapshot_surface->makeImageSnapshot();
+                if (!potentially_gpu_snapshot) {
+                  FML_LOG(ERROR) << "Screenshot: unable to make image screenshot";
+                  return nullptr;
+                }
+                return potentially_gpu_snapshot;
+              }
+              sk_sp<SkImage> Rasterizer::MakeRasterImage(
+                  sk_sp<SkImage> potentially_gpu_snapshot) {
+                std::unique_ptr<RendererContextSwitchManager::RendererContextSwitch>
+                    context_switch = surface_->MakeRenderContextCurrent();
+                if (!context_switch->GetSwitchResult()) {
+                  return nullptr;
+                }
+                auto cpu_snapshot = potentially_gpu_snapshot->makeRasterImage();
+                if (!cpu_snapshot) {
+                  FML_LOG(ERROR) << "Screenshot: unable to make raster image";
+                  return nullptr;
+                }
+                return cpu_snapshot;
+              }
+              void Rasterizer::ScreenshotFlushCanvas(SkCanvas& canvas) {
+                std::unique_ptr<RendererContextSwitchManager::RendererContextSwitch>
+                    context_switch = surface_->MakeRenderContextCurrent();
+                if (!context_switch->GetSwitchResult()) {
+                  FML_LOG(ERROR)
+                      << "Screenshot: unable to switch gl context to flutter's context";
+                  return;
+                }
+                canvas.flush();
+              }

Contributor Author

cyanglaz Nov 13, 2019

Refactored out these methods so we can set the gl context for each of them. @chinmaygarde

shell/common/shell_unittests.cc

Comment on lines +955 to +985

+              TEST_F(ShellTest, RasterizerScreenshot) {
+                Settings settings = CreateSettingsForFixture();
+                auto configuration = RunConfiguration::InferFromSettings(settings);
+                auto task_runner = CreateNewThread();
+                TaskRunners task_runners("test", task_runner, task_runner, task_runner,
+                                         task_runner);
+                std::unique_ptr<Shell> shell =
+                    CreateShell(std::move(settings), std::move(task_runners));
+                ASSERT_TRUE(ValidateShell(shell.get()));
+                PlatformViewNotifyCreated(shell.get());
+                RunEngine(shell.get(), std::move(configuration));
+                std::shared_ptr<fml::AutoResetWaitableEvent> latch =
+                    std::make_shared<fml::AutoResetWaitableEvent>();
+                PumpOneFrame(shell.get());
+                fml::TaskRunner::RunNowOrPostTask(
+                    shell->GetTaskRunners().GetGPUTaskRunner(), [&]() {
+                      Rasterizer::Screenshot screenshot =
+                          shell->GetRasterizer()->ScreenshotLastLayerTree(
+                              Rasterizer::ScreenshotType::CompressedImage, true);
+                      EXPECT_EQ(screenshot.data != nullptr, true);
+                      latch->Signal();
+                    });
+                latch->Wait();
+                DestroyShell(std::move(shell), std::move(task_runners));
+              }

Contributor Author

cyanglaz Nov 13, 2019

Screenshot related test @chinmaygarde

shell/common/shell_unittests.cc

Comment on lines +987 to +1021

+              TEST_F(ShellTest, RasterizerMakeRasterSnapshot) {
+                Settings settings = CreateSettingsForFixture();
+                auto configuration = RunConfiguration::InferFromSettings(settings);
+                auto task_runner = CreateNewThread();
+                TaskRunners task_runners("test", task_runner, task_runner, task_runner,
+                                         task_runner);
+                std::unique_ptr<Shell> shell =
+                    CreateShell(std::move(settings), std::move(task_runners));
+                ASSERT_TRUE(ValidateShell(shell.get()));
+                PlatformViewNotifyCreated(shell.get());
+                RunEngine(shell.get(), std::move(configuration));
+                std::shared_ptr<fml::AutoResetWaitableEvent> latch =
+                    std::make_shared<fml::AutoResetWaitableEvent>();
+                PumpOneFrame(shell.get());
+                //  sk_sp<SkPicture> placeHolderPicture = SkPicture::MakePlaceholder({0, 0,
+                //  50, 50}); SkISize size = SkISize::Make(50, 50);
+                fml::TaskRunner::RunNowOrPostTask(
+                    shell->GetTaskRunners().GetGPUTaskRunner(), [&]() {
+                      SnapshotDelegate* delegate =
+                          reinterpret_cast<Rasterizer*>(shell->GetRasterizer().get());
+                      sk_sp<SkImage> image = delegate->MakeRasterSnapshot(
+                          SkPicture::MakePlaceholder({0, 0, 50, 50}), SkISize::Make(50, 50));
+                      EXPECT_EQ(image != nullptr, true);
+                      latch->Signal();
+                    });
+                latch->Wait();
+                DestroyShell(std::move(shell), std::move(task_runners));
+              }

Contributor Author

cyanglaz Nov 13, 2019

The MakeRasterSnapshot test to prevent regression from typo like flutter/flutter#31355 (comment)
@liyuqian

Contributor

liyuqian Nov 14, 2019

Great!

cyanglaz requested review from chinmaygarde and liyuqian

November 13, 2019 04:57

Chris Yang and others added 2 commits

November 12, 2019 20:58


          remove commented out code

d6a3750


          use NE

d337c00

cyanglaz added the CQ+1 label

liyuqian reviewed

View reviewed changes

shell/common/shell_unittests.cc Outdated


		RunEngine(shell.get(), std::move(configuration));

		std::shared_ptr<fml::AutoResetWaitableEvent> latch =

Contributor

liyuqian Nov 13, 2019

nit: auto latch = ... is good enough here. Our C++ doesn't have lint rule to enforce something like final LongDartClassName x = LongDartClassName();

shell/common/shell_unittests.cc Outdated

+                PumpOneFrame(shell.get());
+                fml::TaskRunner::RunNowOrPostTask(
+                    shell->GetTaskRunners().GetGPUTaskRunner(), [&]() {

Contributor

liyuqian Nov 14, 2019

nit: explicitly capture shell, latch instead of capturing everything :)

shell/common/shell_unittests.cc Outdated


		RunEngine(shell.get(), std::move(configuration));

		std::shared_ptr<fml::AutoResetWaitableEvent> latch =

Contributor

liyuqian Nov 14, 2019

ditto.

shell/common/shell_unittests.cc Outdated

+                PumpOneFrame(shell.get());
+                fml::TaskRunner::RunNowOrPostTask(
+                    shell->GetTaskRunners().GetGPUTaskRunner(), [&]() {

Contributor

liyuqian Nov 14, 2019

ditto.

shell/common/shell_unittests.cc

Comment on lines +987 to +1021

+              TEST_F(ShellTest, RasterizerMakeRasterSnapshot) {
+                Settings settings = CreateSettingsForFixture();
+                auto configuration = RunConfiguration::InferFromSettings(settings);
+                auto task_runner = CreateNewThread();
+                TaskRunners task_runners("test", task_runner, task_runner, task_runner,
+                                         task_runner);
+                std::unique_ptr<Shell> shell =
+                    CreateShell(std::move(settings), std::move(task_runners));
+                ASSERT_TRUE(ValidateShell(shell.get()));
+                PlatformViewNotifyCreated(shell.get());
+                RunEngine(shell.get(), std::move(configuration));
+                std::shared_ptr<fml::AutoResetWaitableEvent> latch =
+                    std::make_shared<fml::AutoResetWaitableEvent>();
+                PumpOneFrame(shell.get());
+                //  sk_sp<SkPicture> placeHolderPicture = SkPicture::MakePlaceholder({0, 0,
+                //  50, 50}); SkISize size = SkISize::Make(50, 50);
+                fml::TaskRunner::RunNowOrPostTask(
+                    shell->GetTaskRunners().GetGPUTaskRunner(), [&]() {
+                      SnapshotDelegate* delegate =
+                          reinterpret_cast<Rasterizer*>(shell->GetRasterizer().get());
+                      sk_sp<SkImage> image = delegate->MakeRasterSnapshot(
+                          SkPicture::MakePlaceholder({0, 0, 50, 50}), SkISize::Make(50, 50));
+                      EXPECT_EQ(image != nullptr, true);
+                      latch->Signal();
+                    });
+                latch->Wait();
+                DestroyShell(std::move(shell), std::move(task_runners));
+              }

Contributor

liyuqian Nov 14, 2019

Great!

liyuqian reviewed

View reviewed changes

Contributor

liyuqian left a comment

LGTM on the test with tiny nits!

chinmaygarde approved these changes

View reviewed changes


          review fixess

74faafd

cyanglaz merged commit f456423 into flutter:master

cyanglaz deleted the gl_rework branch

November 14, 2019 19:50

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

ef4fbff

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..f456423cfb82 (4 commits) flutter/flutter#44943

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

58bd941

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..5b10fa35b169 (6 commits) flutter/flutter#44945

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

82aa78e

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..5b10fa35b169 (6 commits) flutter/flutter#44948

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

…tter/engine#13812)

This was referenced Nov 15, 2019

Roll engine 77c3512ec85e..97df087ee678 (7 commits) flutter/flutter#44949

Closed

Roll engine 77c3512ec85e..97df087ee678 (7 commits) flutter/flutter#44951

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

264d510

…tter/engine#13812)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

f52ffc1

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..141dc785dec9 (9 commits) flutter/flutter#44952

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

70a00b3

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..141dc785dec9 (9 commits) flutter/flutter#44954

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

a80d6ff

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..141dc785dec9 (9 commits) flutter/flutter#44956

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

d6ee073

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..141dc785dec9 (9 commits) flutter/flutter#44958

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request


          f456423 RendererContextSwitch guard flutter's gl context rework. (flu…

179243f

…tter/engine#13812)

engine-flutter-autoroll mentioned this pull request

Roll engine 77c3512ec85e..90a6054a878d (10 commits) flutter/flutter#44960

Merged

flar pushed a commit to flutter/flutter that referenced this pull request


          Roll engine 77c3512..90a6054 (10 commits) (#44960)

6268a7f

* 174e0e9 Roll src/third_party/dart dc35290111..dc808f3fcb (5 commits) (flutter/engine#13859)

* 33d997c Roll fuchsia/sdk/core/mac-amd64 from 7XOyl... to VMTIz... (flutter/engine#13861)

* b4899d9 Roll src/third_party/skia d860a78fd60c..e57ca4931952 (44 commits) (flutter/engine#13862)

* f456423 RendererContextSwitch guard flutter's gl context rework. (flutter/engine#13812)

* 6bab64e Fix test to account for pixel ratio transformations being framework responsibility. (flutter/engine#13850)

* 5b10fa3 Guard against orphaned semantic objects from referencing dead accessibility bridge on iOS (flutter/engine#13857)

* 97df087 [fuchsia] Package flutter_frontend_server snapshot for fuchsia (flutter/engine#13865)

* 0832dfd [flow][fuchsia] Add more tracing to layers and Fuchsia surface pool (flutter/engine#13864)

* 141dc785d Roll src/third_party/skia e57ca4931952..c1c4634dcb07 (15 commits) (flutter/engine#13866)

* 90a6054 Revert "Roll src/third_party/dart dc35290111..dc808f3fcb (5 commits) (#13859)" (flutter/engine#13867)

cyanglaz mentioned this pull request

Make sure platform views are isolated from the engine's GL context flutter/flutter#41620

Closed

tvolkert mentioned this pull request

Just upgrade flutter and all ImageView can not be shown on iOS 13.2 iPhoneX flutter/flutter#45098

Closed

Contributor

tvolkert commented Nov 18, 2019

This has been implicated via bisect to have introduced flutter/flutter#45098. I'm going to revert.

tvolkert added a commit that referenced this pull request


          Revert "RendererContextSwitch guard flutter's gl context rework. (#13812

745c23e

)"

This reverts commit f456423.

tvolkert mentioned this pull request

Revert "RendererContextSwitch guard flutter's gl context rework." #13906

Merged

tvolkert added a commit that referenced this pull request


          Revert "RendererContextSwitch guard flutter's gl context rework. (#13812

762294c

)" (#13906)

This reverts commit f456423.

This is being reverted because it caused flutter/flutter#45098
(images don't load on iOS).

cyanglaz restored the gl_rework branch

November 19, 2019 18:42

cyanglaz mentioned this pull request

Reland "RendererContextSwitch guard flutter's gl context rework. #13812" #13946

Closed

shorben07 mentioned this pull request

Purge caches on iOS can happen when unexpected GL context is active flutter/flutter#48182

Closed

liyuqian mentioned this pull request

Crash in ShaderWarmUp.execute() flutter/flutter#31355

Closed

Partizann pushed a commit to Partizann/engine that referenced this pull request


          Fixes after 'Reland 'RendererContextSwitch guard flutter's gl context…

c3d097a

… rework. flutter#13812' flutter#13946'

Partizann pushed a commit to Partizann/engine that referenced this pull request


          Fixes after 'Reland 'RendererContextSwitch guard flutter's gl context…

e2102de

… rework. flutter#13812' flutter#13946'

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

Labels