From 6007ec373ce9b18da27839f04216abf1ea451981 Mon Sep 17 00:00:00 2001 From: George Wright Date: Tue, 19 Nov 2019 13:34:03 -0800 Subject: [PATCH 1/9] Ensure we use the base CompositorContext's AcquireFrame method when screenshotting. This ensures we rasterize into the canvas passed in as subclasses may reimplement AcquireFrame in different ways that don't utilize the canvas object passed in (such as Fuchsia's flutter_runner::CompositorContext). --- shell/common/rasterizer.cc | 6 +-- shell/common/shell_unittests.cc | 65 +++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index d61e1ff86ddc3..a98ab1ba689b0 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -399,9 +399,9 @@ static sk_sp ScreenshotLayerTreeAsImage( SkMatrix root_surface_transformation; root_surface_transformation.reset(); - auto frame = compositor_context.AcquireFrame(surface_context, canvas, nullptr, - root_surface_transformation, - false, nullptr); + auto frame = compositor_context.flutter::CompositorContext::AcquireFrame( + surface_context, canvas, nullptr, root_surface_transformation, false, + nullptr); canvas->clear(SK_ColorTRANSPARENT); frame->Raster(*tree, true); canvas->flush(); diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 0146619cce4cf..4d1d3eb2085bc 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -10,6 +10,7 @@ #include #include "flutter/flow/layers/layer_tree.h" +#include "flutter/flow/layers/picture_layer.h" #include "flutter/flow/layers/transform_layer.h" #include "flutter/fml/command_line.h" #include "flutter/fml/make_copyable.h" @@ -952,5 +953,69 @@ TEST_F(ShellTest, IsolateCanAccessPersistentIsolateData) { DestroyShell(std::move(shell), std::move(task_runners)); } +TEST_F(ShellTest, Screenshot) { + auto settings = CreateSettingsForFixture(); + std::unique_ptr shell = CreateShell(settings); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + LayerTreeBuilder builder = [&](std::shared_ptr root) { + SkPictureRecorder recorder; + SkCanvas* recording_canvas = + recorder.beginRecording(SkRect::MakeXYWH(0, 0, 80, 80)); + recording_canvas->drawRect(SkRect::MakeXYWH(0, 0, 80, 80), + SkPaint(SkColor4f::FromColor(SK_ColorRED))); + auto sk_picture = recorder.finishRecordingAsPicture(); + fml::RefPtr queue = fml::MakeRefCounted( + this->GetCurrentTaskRunner(), fml::TimeDelta::FromSeconds(0)); + auto picture_layer = std::make_shared( + SkPoint::Make(10, 10), + flutter::SkiaGPUObject({sk_picture, queue}), false, false); + root->Add(picture_layer); + }; + + PumpOneFrame(shell.get(), 100, 100, builder); + + fml::Status result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_TRUE(result.ok()); + + std::shared_ptr latch = + std::make_shared(); + + Rasterizer::Screenshot screenshot; + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { + auto rasterizer = shell->GetRasterizer(); + screenshot = rasterizer->ScreenshotLastLayerTree( + Rasterizer::ScreenshotType::CompressedImage, true); + latch->Signal(); + }); + latch->Wait(); + + const char* reference_png = + "iVBORw0KGgoAAAANSUhEUgAAAGQAAABkCAYAAABw4pVUAAAAAXNSR0IArs4c6QAAAARzQklU" + "CAgICHwIZIgAAADpSURBVHic7dHBCcAwDARBOf337FQQSB6GJcy89Ti0MwAAAEDHenu4Z/" + "bJIX+" + "3Xv76Oj2EbwSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJEQQAAADg0Q0wHwKgbFGDCQAAAABJRU5ErkJggg=="; + // Use MakeWithoutCopy instead of MakeWithCString because we don't want to + // encode the null sentinel + sk_sp reference_data = + SkData::MakeWithoutCopy(reference_png, strlen(reference_png)); + + ASSERT_TRUE(reference_data->equals(screenshot.data.get())); + + DestroyShell(std::move(shell)); +} + } // namespace testing } // namespace flutter From 54df916e3cee6773caef3c45a54c8485f45986e4 Mon Sep 17 00:00:00 2001 From: George Wright Date: Tue, 19 Nov 2019 17:03:29 -0800 Subject: [PATCH 2/9] Review updates --- flow/BUILD.gn | 2 - flow/flow_run_all_unittests.cc | 2 +- .../performance_overlay_layer_unittests.cc | 2 +- shell/common/shell_unittests.cc | 48 +++++++++--------- testing/BUILD.gn | 2 + testing/resources/screenshot_gold.png | Bin 0 -> 319 bytes testing/run_all_unittests.cc | 5 ++ testing/run_tests.py | 7 ++- .../test_utils.cc | 0 .../flow_test_utils.h => testing/test_utils.h | 0 10 files changed, 39 insertions(+), 29 deletions(-) create mode 100644 testing/resources/screenshot_gold.png rename flow/flow_test_utils.cc => testing/test_utils.cc (100%) rename flow/flow_test_utils.h => testing/test_utils.h (100%) diff --git a/flow/BUILD.gn b/flow/BUILD.gn index c2f0c98415a3e..7c4cfb5259993 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -107,8 +107,6 @@ executable("flow_unittests") { sources = [ "flow_run_all_unittests.cc", - "flow_test_utils.cc", - "flow_test_utils.h", "layers/performance_overlay_layer_unittests.cc", "layers/physical_shape_layer_unittests.cc", "matrix_decomposition_unittests.cc", diff --git a/flow/flow_run_all_unittests.cc b/flow/flow_run_all_unittests.cc index 4cf0ba3d7fcdf..c6065bb1762f2 100644 --- a/flow/flow_run_all_unittests.cc +++ b/flow/flow_run_all_unittests.cc @@ -18,7 +18,7 @@ #include "flutter/fml/logging.h" #include "gtest/gtest.h" -#include "flow_test_utils.h" +#include "flutter/testing/test_utils.h" int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); diff --git a/flow/layers/performance_overlay_layer_unittests.cc b/flow/layers/performance_overlay_layer_unittests.cc index 605717c870ee3..301f8d67461fc 100644 --- a/flow/layers/performance_overlay_layer_unittests.cc +++ b/flow/layers/performance_overlay_layer_unittests.cc @@ -2,10 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -#include "flutter/flow/flow_test_utils.h" #include "flutter/flow/layers/performance_overlay_layer.h" #include "flutter/flow/raster_cache.h" #include "flutter/fml/build_config.h" +#include "flutter/testing/test_utils.h" #include "third_party/skia/include/core/SkSurface.h" #include "third_party/skia/include/utils/SkBase64.h" diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 4d1d3eb2085bc..b360ffabe423e 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -23,6 +23,7 @@ #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/switches.h" #include "flutter/shell/common/thread_host.h" +#include "flutter/testing/test_utils.h" #include "flutter/testing/testing.h" #include "third_party/tonic/converter/dart_converter.h" @@ -954,7 +955,16 @@ TEST_F(ShellTest, IsolateCanAccessPersistentIsolateData) { } TEST_F(ShellTest, Screenshot) { +#if !OS_LINUX + // TODO(gw280): Enable this on Fuchsia when shell_unittests runs on Fuchsia + GTEST_SKIP() << "Skipping golden tests on non-Linux OSes"; +#endif // OS_LINUX + auto settings = CreateSettingsForFixture(); + fml::AutoResetWaitableEvent firstFrameLatch; + settings.frame_rasterized_callback = + [&firstFrameLatch](const FrameTiming& t) { firstFrameLatch.Signal(); }; + std::unique_ptr shell = CreateShell(settings); // Create the surface needed by rasterizer @@ -981,38 +991,28 @@ TEST_F(ShellTest, Screenshot) { }; PumpOneFrame(shell.get(), 100, 100, builder); + firstFrameLatch.Wait(); - fml::Status result = - shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); - ASSERT_TRUE(result.ok()); + std::promise screenshot_promise; + auto screenshot_future = screenshot_promise.get_future(); - std::shared_ptr latch = - std::make_shared(); - - Rasterizer::Screenshot screenshot; fml::TaskRunner::RunNowOrPostTask( - shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { + shell->GetTaskRunners().GetGPUTaskRunner(), + [&screenshot_promise, &shell]() { auto rasterizer = shell->GetRasterizer(); - screenshot = rasterizer->ScreenshotLastLayerTree( - Rasterizer::ScreenshotType::CompressedImage, true); - latch->Signal(); + screenshot_promise.set_value(rasterizer->ScreenshotLastLayerTree( + Rasterizer::ScreenshotType::CompressedImage, false)); }); - latch->Wait(); - const char* reference_png = - "iVBORw0KGgoAAAANSUhEUgAAAGQAAABkCAYAAABw4pVUAAAAAXNSR0IArs4c6QAAAARzQklU" - "CAgICHwIZIgAAADpSURBVHic7dHBCcAwDARBOf337FQQSB6GJcy89Ti0MwAAAEDHenu4Z/" - "bJIX+" - "3Xv76Oj2EbwSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJEQQAAADg0Q0wHwKgbFGDCQAAAABJRU5ErkJggg=="; - // Use MakeWithoutCopy instead of MakeWithCString because we don't want to - // encode the null sentinel + std::stringstream golden_file_path; + // This unit test should only be run on Linux (not even on Mac since it's a + // golden test). Hence we don't have to worry about the "/" vs. "\". + golden_file_path << flutter::GetGoldenDir() << "/" + << "screenshot_gold.png"; sk_sp reference_data = - SkData::MakeWithoutCopy(reference_png, strlen(reference_png)); + SkData::MakeFromFileName(golden_file_path.str().c_str()); - ASSERT_TRUE(reference_data->equals(screenshot.data.get())); + ASSERT_TRUE(reference_data->equals(screenshot_future.get().data.get())); DestroyShell(std::move(shell)); } diff --git a/testing/BUILD.gn b/testing/BUILD.gn index a8db2229e29f5..2f22eba9c2694 100644 --- a/testing/BUILD.gn +++ b/testing/BUILD.gn @@ -9,6 +9,8 @@ source_set("testing_lib") { "$flutter_root/testing/assertions.h", "$flutter_root/testing/testing.cc", "$flutter_root/testing/testing.h", + "$flutter_root/testing/test_utils.cc", + "$flutter_root/testing/test_utils.h", "$flutter_root/testing/thread_test.cc", "$flutter_root/testing/thread_test.h", ] diff --git a/testing/resources/screenshot_gold.png b/testing/resources/screenshot_gold.png new file mode 100644 index 0000000000000000000000000000000000000000..3ad7d7e01512a3167df9bd041df566959284441a GIT binary patch literal 319 zcmeAS@N?(olHy`uVBq!ia0vp^DImT| xJ?`HxE8CWQmQFzxw+RxCNkf9c0`$X!i@XN%Obc=Xn>m3z22WQ%mvv4FO#rLISzQ1C literal 0 HcmV?d00001 diff --git a/testing/run_all_unittests.cc b/testing/run_all_unittests.cc index 8c47c30a087c2..d0ae12a986098 100644 --- a/testing/run_all_unittests.cc +++ b/testing/run_all_unittests.cc @@ -3,6 +3,8 @@ // found in the LICENSE file. #include "flutter/fml/build_config.h" +#include "flutter/fml/command_line.h" +#include "flutter/testing/test_utils.h" #include "gtest/gtest.h" #ifdef OS_IOS @@ -18,5 +20,8 @@ int main(int argc, char** argv) { #endif // OS_IOS ::testing::InitGoogleTest(&argc, argv); + fml::CommandLine cmd = fml::CommandLineFromArgcArgv(argc, argv); + flutter::SetGoldenDir( + cmd.GetOptionValueWithDefault("golden-dir", "flutter/testing/resources")); return RUN_ALL_TESTS(); } diff --git a/testing/run_tests.py b/testing/run_tests.py index d345bb887c0ac..8b79c2e4380bf 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -102,9 +102,14 @@ def RunCCTests(build_dir, filter): RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) + shell_flags = [] + if IsLinux(): + shell_flags = [ + '--golden-dir=%s' % golden_dir, + ] # https://github.com/flutter/flutter/issues/36295 if not IsWindows(): - RunEngineExecutable(build_dir, 'shell_unittests', filter, shuffle_flags) + RunEngineExecutable(build_dir, 'shell_unittests', filter, shell_flags + shuffle_flags) RunEngineExecutable(build_dir, 'ui_unittests', filter, shuffle_flags) diff --git a/flow/flow_test_utils.cc b/testing/test_utils.cc similarity index 100% rename from flow/flow_test_utils.cc rename to testing/test_utils.cc diff --git a/flow/flow_test_utils.h b/testing/test_utils.h similarity index 100% rename from flow/flow_test_utils.h rename to testing/test_utils.h From ca3df5ac531edbf480f59ce1a830c84e6fe86ba9 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 10:33:17 -0800 Subject: [PATCH 3/9] Revert "Review updates" This reverts commit 54df916e3cee6773caef3c45a54c8485f45986e4. --- flow/BUILD.gn | 2 + flow/flow_run_all_unittests.cc | 2 +- .../test_utils.cc => flow/flow_test_utils.cc | 0 .../test_utils.h => flow/flow_test_utils.h | 0 .../performance_overlay_layer_unittests.cc | 2 +- shell/common/shell_unittests.cc | 48 +++++++++--------- testing/BUILD.gn | 2 - testing/resources/screenshot_gold.png | Bin 319 -> 0 bytes testing/run_all_unittests.cc | 5 -- testing/run_tests.py | 7 +-- 10 files changed, 29 insertions(+), 39 deletions(-) rename testing/test_utils.cc => flow/flow_test_utils.cc (100%) rename testing/test_utils.h => flow/flow_test_utils.h (100%) delete mode 100644 testing/resources/screenshot_gold.png diff --git a/flow/BUILD.gn b/flow/BUILD.gn index 7c4cfb5259993..c2f0c98415a3e 100644 --- a/flow/BUILD.gn +++ b/flow/BUILD.gn @@ -107,6 +107,8 @@ executable("flow_unittests") { sources = [ "flow_run_all_unittests.cc", + "flow_test_utils.cc", + "flow_test_utils.h", "layers/performance_overlay_layer_unittests.cc", "layers/physical_shape_layer_unittests.cc", "matrix_decomposition_unittests.cc", diff --git a/flow/flow_run_all_unittests.cc b/flow/flow_run_all_unittests.cc index c6065bb1762f2..4cf0ba3d7fcdf 100644 --- a/flow/flow_run_all_unittests.cc +++ b/flow/flow_run_all_unittests.cc @@ -18,7 +18,7 @@ #include "flutter/fml/logging.h" #include "gtest/gtest.h" -#include "flutter/testing/test_utils.h" +#include "flow_test_utils.h" int main(int argc, char** argv) { testing::InitGoogleTest(&argc, argv); diff --git a/testing/test_utils.cc b/flow/flow_test_utils.cc similarity index 100% rename from testing/test_utils.cc rename to flow/flow_test_utils.cc diff --git a/testing/test_utils.h b/flow/flow_test_utils.h similarity index 100% rename from testing/test_utils.h rename to flow/flow_test_utils.h diff --git a/flow/layers/performance_overlay_layer_unittests.cc b/flow/layers/performance_overlay_layer_unittests.cc index 301f8d67461fc..605717c870ee3 100644 --- a/flow/layers/performance_overlay_layer_unittests.cc +++ b/flow/layers/performance_overlay_layer_unittests.cc @@ -2,10 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include "flutter/flow/flow_test_utils.h" #include "flutter/flow/layers/performance_overlay_layer.h" #include "flutter/flow/raster_cache.h" #include "flutter/fml/build_config.h" -#include "flutter/testing/test_utils.h" #include "third_party/skia/include/core/SkSurface.h" #include "third_party/skia/include/utils/SkBase64.h" diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index b360ffabe423e..4d1d3eb2085bc 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -23,7 +23,6 @@ #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/switches.h" #include "flutter/shell/common/thread_host.h" -#include "flutter/testing/test_utils.h" #include "flutter/testing/testing.h" #include "third_party/tonic/converter/dart_converter.h" @@ -955,16 +954,7 @@ TEST_F(ShellTest, IsolateCanAccessPersistentIsolateData) { } TEST_F(ShellTest, Screenshot) { -#if !OS_LINUX - // TODO(gw280): Enable this on Fuchsia when shell_unittests runs on Fuchsia - GTEST_SKIP() << "Skipping golden tests on non-Linux OSes"; -#endif // OS_LINUX - auto settings = CreateSettingsForFixture(); - fml::AutoResetWaitableEvent firstFrameLatch; - settings.frame_rasterized_callback = - [&firstFrameLatch](const FrameTiming& t) { firstFrameLatch.Signal(); }; - std::unique_ptr shell = CreateShell(settings); // Create the surface needed by rasterizer @@ -991,28 +981,38 @@ TEST_F(ShellTest, Screenshot) { }; PumpOneFrame(shell.get(), 100, 100, builder); - firstFrameLatch.Wait(); - std::promise screenshot_promise; - auto screenshot_future = screenshot_promise.get_future(); + fml::Status result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_TRUE(result.ok()); + std::shared_ptr latch = + std::make_shared(); + + Rasterizer::Screenshot screenshot; fml::TaskRunner::RunNowOrPostTask( - shell->GetTaskRunners().GetGPUTaskRunner(), - [&screenshot_promise, &shell]() { + shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { auto rasterizer = shell->GetRasterizer(); - screenshot_promise.set_value(rasterizer->ScreenshotLastLayerTree( - Rasterizer::ScreenshotType::CompressedImage, false)); + screenshot = rasterizer->ScreenshotLastLayerTree( + Rasterizer::ScreenshotType::CompressedImage, true); + latch->Signal(); }); + latch->Wait(); - std::stringstream golden_file_path; - // This unit test should only be run on Linux (not even on Mac since it's a - // golden test). Hence we don't have to worry about the "/" vs. "\". - golden_file_path << flutter::GetGoldenDir() << "/" - << "screenshot_gold.png"; + const char* reference_png = + "iVBORw0KGgoAAAANSUhEUgAAAGQAAABkCAYAAABw4pVUAAAAAXNSR0IArs4c6QAAAARzQklU" + "CAgICHwIZIgAAADpSURBVHic7dHBCcAwDARBOf337FQQSB6GJcy89Ti0MwAAAEDHenu4Z/" + "bJIX+" + "3Xv76Oj2EbwSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" + "JESRGkBhBYgSJESRGkBhBYgSJEQQAAADg0Q0wHwKgbFGDCQAAAABJRU5ErkJggg=="; + // Use MakeWithoutCopy instead of MakeWithCString because we don't want to + // encode the null sentinel sk_sp reference_data = - SkData::MakeFromFileName(golden_file_path.str().c_str()); + SkData::MakeWithoutCopy(reference_png, strlen(reference_png)); - ASSERT_TRUE(reference_data->equals(screenshot_future.get().data.get())); + ASSERT_TRUE(reference_data->equals(screenshot.data.get())); DestroyShell(std::move(shell)); } diff --git a/testing/BUILD.gn b/testing/BUILD.gn index 2f22eba9c2694..a8db2229e29f5 100644 --- a/testing/BUILD.gn +++ b/testing/BUILD.gn @@ -9,8 +9,6 @@ source_set("testing_lib") { "$flutter_root/testing/assertions.h", "$flutter_root/testing/testing.cc", "$flutter_root/testing/testing.h", - "$flutter_root/testing/test_utils.cc", - "$flutter_root/testing/test_utils.h", "$flutter_root/testing/thread_test.cc", "$flutter_root/testing/thread_test.h", ] diff --git a/testing/resources/screenshot_gold.png b/testing/resources/screenshot_gold.png deleted file mode 100644 index 3ad7d7e01512a3167df9bd041df566959284441a..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 319 zcmeAS@N?(olHy`uVBq!ia0vp^DImT| xJ?`HxE8CWQmQFzxw+RxCNkf9c0`$X!i@XN%Obc=Xn>m3z22WQ%mvv4FO#rLISzQ1C diff --git a/testing/run_all_unittests.cc b/testing/run_all_unittests.cc index d0ae12a986098..8c47c30a087c2 100644 --- a/testing/run_all_unittests.cc +++ b/testing/run_all_unittests.cc @@ -3,8 +3,6 @@ // found in the LICENSE file. #include "flutter/fml/build_config.h" -#include "flutter/fml/command_line.h" -#include "flutter/testing/test_utils.h" #include "gtest/gtest.h" #ifdef OS_IOS @@ -20,8 +18,5 @@ int main(int argc, char** argv) { #endif // OS_IOS ::testing::InitGoogleTest(&argc, argv); - fml::CommandLine cmd = fml::CommandLineFromArgcArgv(argc, argv); - flutter::SetGoldenDir( - cmd.GetOptionValueWithDefault("golden-dir", "flutter/testing/resources")); return RUN_ALL_TESTS(); } diff --git a/testing/run_tests.py b/testing/run_tests.py index 8b79c2e4380bf..d345bb887c0ac 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -102,14 +102,9 @@ def RunCCTests(build_dir, filter): RunEngineExecutable(build_dir, 'runtime_unittests', filter, shuffle_flags) - shell_flags = [] - if IsLinux(): - shell_flags = [ - '--golden-dir=%s' % golden_dir, - ] # https://github.com/flutter/flutter/issues/36295 if not IsWindows(): - RunEngineExecutable(build_dir, 'shell_unittests', filter, shell_flags + shuffle_flags) + RunEngineExecutable(build_dir, 'shell_unittests', filter, shuffle_flags) RunEngineExecutable(build_dir, 'ui_unittests', filter, shuffle_flags) From d7420f85876224a7d7bf7bb6804af1d8ffca0df2 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 11:24:53 -0800 Subject: [PATCH 4/9] Use test fixtures --- shell/common/BUILD.gn | 2 ++ shell/common/fixtures/shelltest_screenshot.png | Bin 0 -> 319 bytes shell/common/shell_unittests.cc | 17 +++++++---------- 3 files changed, 9 insertions(+), 10 deletions(-) create mode 100644 shell/common/fixtures/shelltest_screenshot.png diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index f93bca63478f0..b02c4626c8c37 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -153,6 +153,8 @@ if (current_toolchain == host_toolchain) { test_fixtures("shell_unittests_fixtures") { dart_main = "fixtures/shell_test.dart" + + fixtures = [ "fixtures/shelltest_screenshot.png", ] } shell_host_executable("shell_unittests") { diff --git a/shell/common/fixtures/shelltest_screenshot.png b/shell/common/fixtures/shelltest_screenshot.png new file mode 100644 index 0000000000000000000000000000000000000000..3ad7d7e01512a3167df9bd041df566959284441a GIT binary patch literal 319 zcmeAS@N?(olHy`uVBq!ia0vp^DImT| xJ?`HxE8CWQmQFzxw+RxCNkf9c0`$X!i@XN%Obc=Xn>m3z22WQ%mvv4FO#rLISzQ1C literal 0 HcmV?d00001 diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 4d1d3eb2085bc..ffa3e655e9236 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -994,23 +994,20 @@ TEST_F(ShellTest, Screenshot) { shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { auto rasterizer = shell->GetRasterizer(); screenshot = rasterizer->ScreenshotLastLayerTree( - Rasterizer::ScreenshotType::CompressedImage, true); + Rasterizer::ScreenshotType::CompressedImage, false); latch->Signal(); }); latch->Wait(); - const char* reference_png = - "iVBORw0KGgoAAAANSUhEUgAAAGQAAABkCAYAAABw4pVUAAAAAXNSR0IArs4c6QAAAARzQklU" - "CAgICHwIZIgAAADpSURBVHic7dHBCcAwDARBOf337FQQSB6GJcy89Ti0MwAAAEDHenu4Z/" - "bJIX+" - "3Xv76Oj2EbwSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgSJESRGkBhBYgS" - "JESRGkBhBYgSJESRGkBhBYgSJEQQAAADg0Q0wHwKgbFGDCQAAAABJRU5ErkJggg=="; + auto fixtures_dir = fml::OpenDirectory(GetFixturesPath(), false, fml::FilePermission::kRead); + + auto reference_png = + fml::FileMapping::CreateReadOnly(fixtures_dir, "shelltest_screenshot.png"); + // Use MakeWithoutCopy instead of MakeWithCString because we don't want to // encode the null sentinel sk_sp reference_data = - SkData::MakeWithoutCopy(reference_png, strlen(reference_png)); + SkData::MakeWithoutCopy(reference_png->GetMapping(), reference_png->GetSize()); ASSERT_TRUE(reference_data->equals(screenshot.data.get())); From 18594ccbd88399aa93deb72c40eb63ef073e5006 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 12:28:41 -0800 Subject: [PATCH 5/9] review updates - use test fixtures --- shell/common/shell_unittests.cc | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index ffa3e655e9236..c23831a555ad7 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -955,6 +955,10 @@ TEST_F(ShellTest, IsolateCanAccessPersistentIsolateData) { TEST_F(ShellTest, Screenshot) { auto settings = CreateSettingsForFixture(); + fml::AutoResetWaitableEvent firstFrameLatch; + settings.frame_rasterized_callback = + [&firstFrameLatch](const FrameTiming& t) { firstFrameLatch.Signal(); }; + std::unique_ptr shell = CreateShell(settings); // Create the surface needed by rasterizer @@ -981,23 +985,17 @@ TEST_F(ShellTest, Screenshot) { }; PumpOneFrame(shell.get(), 100, 100, builder); + firstFrameLatch.Wait(); - fml::Status result = - shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); - ASSERT_TRUE(result.ok()); + std::promise screenshot_promise; + auto screenshot_future = screenshot_promise.get_future(); - std::shared_ptr latch = - std::make_shared(); - - Rasterizer::Screenshot screenshot; fml::TaskRunner::RunNowOrPostTask( - shell->GetTaskRunners().GetGPUTaskRunner(), [&]() { + shell->GetTaskRunners().GetGPUTaskRunner(), [&screenshot_promise, &shell]() { auto rasterizer = shell->GetRasterizer(); - screenshot = rasterizer->ScreenshotLastLayerTree( - Rasterizer::ScreenshotType::CompressedImage, false); - latch->Signal(); + screenshot_promise.set_value(rasterizer->ScreenshotLastLayerTree( + Rasterizer::ScreenshotType::CompressedImage, false)); }); - latch->Wait(); auto fixtures_dir = fml::OpenDirectory(GetFixturesPath(), false, fml::FilePermission::kRead); @@ -1009,7 +1007,7 @@ TEST_F(ShellTest, Screenshot) { sk_sp reference_data = SkData::MakeWithoutCopy(reference_png->GetMapping(), reference_png->GetSize()); - ASSERT_TRUE(reference_data->equals(screenshot.data.get())); + ASSERT_TRUE(reference_data->equals(screenshot_future.get().data.get())); DestroyShell(std::move(shell)); } From 03e94d5edbc49280ae12a6c0f9d0f248a863fec4 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 12:29:51 -0800 Subject: [PATCH 6/9] formatting --- shell/common/shell_unittests.cc | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index c23831a555ad7..b69bf226b5c61 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -991,21 +991,23 @@ TEST_F(ShellTest, Screenshot) { auto screenshot_future = screenshot_promise.get_future(); fml::TaskRunner::RunNowOrPostTask( - shell->GetTaskRunners().GetGPUTaskRunner(), [&screenshot_promise, &shell]() { + shell->GetTaskRunners().GetGPUTaskRunner(), + [&screenshot_promise, &shell]() { auto rasterizer = shell->GetRasterizer(); screenshot_promise.set_value(rasterizer->ScreenshotLastLayerTree( Rasterizer::ScreenshotType::CompressedImage, false)); }); - auto fixtures_dir = fml::OpenDirectory(GetFixturesPath(), false, fml::FilePermission::kRead); + auto fixtures_dir = + fml::OpenDirectory(GetFixturesPath(), false, fml::FilePermission::kRead); - auto reference_png = - fml::FileMapping::CreateReadOnly(fixtures_dir, "shelltest_screenshot.png"); + auto reference_png = fml::FileMapping::CreateReadOnly( + fixtures_dir, "shelltest_screenshot.png"); // Use MakeWithoutCopy instead of MakeWithCString because we don't want to // encode the null sentinel - sk_sp reference_data = - SkData::MakeWithoutCopy(reference_png->GetMapping(), reference_png->GetSize()); + sk_sp reference_data = SkData::MakeWithoutCopy( + reference_png->GetMapping(), reference_png->GetSize()); ASSERT_TRUE(reference_data->equals(screenshot_future.get().data.get())); From 88216cc3e4938063ac2fa6990cf208ea67fed0fc Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 12:33:50 -0800 Subject: [PATCH 7/9] review update - comment on why CompositorContext is being called explicitly --- shell/common/rasterizer.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index a98ab1ba689b0..7e85d75d4491b 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -399,6 +399,10 @@ static sk_sp ScreenshotLayerTreeAsImage( SkMatrix root_surface_transformation; root_surface_transformation.reset(); + // We want to ensure we call the base method for CompositorContext::AcquireFrame + // instead of the platform-specific method. Specifically, Fuchsia's CompositorContext + // handles the rendering surface itself which means that we will still continue to render + // to the onscreen surface if we don't call the base method. auto frame = compositor_context.flutter::CompositorContext::AcquireFrame( surface_context, canvas, nullptr, root_surface_transformation, false, nullptr); From 4ceef764095349b95ba4f7333484425f6a15e3f6 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 12:47:20 -0800 Subject: [PATCH 8/9] formatting --- shell/common/BUILD.gn | 2 +- shell/common/rasterizer.cc | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/shell/common/BUILD.gn b/shell/common/BUILD.gn index b02c4626c8c37..9d57ee5ff5ecf 100644 --- a/shell/common/BUILD.gn +++ b/shell/common/BUILD.gn @@ -154,7 +154,7 @@ if (current_toolchain == host_toolchain) { test_fixtures("shell_unittests_fixtures") { dart_main = "fixtures/shell_test.dart" - fixtures = [ "fixtures/shelltest_screenshot.png", ] + fixtures = [ "fixtures/shelltest_screenshot.png" ] } shell_host_executable("shell_unittests") { diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 7e85d75d4491b..f40be319848bb 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -399,10 +399,11 @@ static sk_sp ScreenshotLayerTreeAsImage( SkMatrix root_surface_transformation; root_surface_transformation.reset(); - // We want to ensure we call the base method for CompositorContext::AcquireFrame - // instead of the platform-specific method. Specifically, Fuchsia's CompositorContext - // handles the rendering surface itself which means that we will still continue to render - // to the onscreen surface if we don't call the base method. + // We want to ensure we call the base method for + // CompositorContext::AcquireFrame instead of the platform-specific method. + // Specifically, Fuchsia's CompositorContext handles the rendering surface + // itself which means that we will still continue to render to the onscreen + // surface if we don't call the base method. auto frame = compositor_context.flutter::CompositorContext::AcquireFrame( surface_context, canvas, nullptr, root_surface_transformation, false, nullptr); From fd5c0311e92645716fdd5d9294416823268e6218 Mon Sep 17 00:00:00 2001 From: George Wright Date: Wed, 20 Nov 2019 13:39:58 -0800 Subject: [PATCH 9/9] update licence file --- ci/licenses_golden/licenses_flutter | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index fe19287614df0..33569a6ced33a 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -503,6 +503,7 @@ FILE: ../../../flutter/shell/common/canvas_spy_unittests.cc FILE: ../../../flutter/shell/common/engine.cc FILE: ../../../flutter/shell/common/engine.h FILE: ../../../flutter/shell/common/fixtures/shell_test.dart +FILE: ../../../flutter/shell/common/fixtures/shelltest_screenshot.png FILE: ../../../flutter/shell/common/input_events_unittests.cc FILE: ../../../flutter/shell/common/isolate_configuration.cc FILE: ../../../flutter/shell/common/isolate_configuration.h