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
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cyanglaz
commented
Nov 13, 2019
Contributor
Author
cyanglaz
left a comment
There was a problem hiding this comment.
Adding comments to all the additional code after the reverted renderer context switch PR.
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
There was a problem hiding this comment.
Refactored out these methods so we can set the gl context for each of them. @chinmaygarde
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)); | ||
| } |
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
There was a problem hiding this comment.
The MakeRasterSnapshot test to prevent regression from typo like flutter/flutter#31355 (comment)
@liyuqian
liyuqian
reviewed
Nov 14, 2019
shell/common/shell_unittests.cc
Outdated
|
|
||
| RunEngine(shell.get(), std::move(configuration)); | ||
|
|
||
| std::shared_ptr<fml::AutoResetWaitableEvent> latch = |
Contributor
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 = |
shell/common/shell_unittests.cc
Outdated
| PumpOneFrame(shell.get()); | ||
|
|
||
| fml::TaskRunner::RunNowOrPostTask( | ||
| shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { |
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)); | ||
| } |
liyuqian
reviewed
Nov 14, 2019
Contributor
liyuqian
left a comment
There was a problem hiding this comment.
LGTM on the test with tiny nits!
chinmaygarde
approved these changes
Nov 14, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 14, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 14, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 14, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
This was referenced Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Nov 15, 2019
flar
pushed a commit
to flutter/flutter
that referenced
this pull request
Nov 15, 2019
* 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)
Contributor
|
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
Nov 18, 2019
)" This reverts commit f456423.
tvolkert
added a commit
that referenced
this pull request
Nov 19, 2019
)" (#13906) This reverts commit f456423. This is being reverted because it caused flutter/flutter#45098 (images don't load on iOS).
Partizann
pushed a commit
to Partizann/engine
that referenced
this pull request
Mar 23, 2020
Partizann
pushed a commit
to Partizann/engine
that referenced
this pull request
Mar 23, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a rework for the RendererContextSwitching.
This PR added:
0. A complete revert of #13788.
ScreenshotLastLayerTreeto make GLContext available for screenshots.All the changes addition to the previous context switch(1 - 3 above) commit will be commented in the code.