From 99dd2c814a48aec1293ed96fbba703683c6df179 Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Mon, 26 Feb 2024 13:09:18 -0500 Subject: [PATCH 01/21] Roll Fuchsia Linux SDK from kLvCWEgbL1VTRW69e... to JCdhkDSFXzHyPuP4I... (#50970) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/fuchsia-linux-sdk-flutter-engine Please CC jimgraham@google.com,rmistry@google.com,zra@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index c2ae8d71643da..09609aad8c8d1 100644 --- a/DEPS +++ b/DEPS @@ -1000,7 +1000,7 @@ deps = { 'packages': [ { 'package': 'fuchsia/sdk/core/linux-amd64', - 'version': 'kLvCWEgbL1VTRW69e1re3ERw4CUAUrJ-DJdxXyXem7gC' + 'version': 'JCdhkDSFXzHyPuP4IPh4GFybBKbqhSej208gJoZj1poC' } ], 'condition': 'host_os == "linux" and not download_fuchsia_sdk', From add0f5109c4307b6e90875952c7b7434e26d1fb6 Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Mon, 26 Feb 2024 10:52:56 -0800 Subject: [PATCH 02/21] Correctly offset the cull rect of the opacity layer. (#50928) This fixes https://github.com/flutter/flutter/issues/140999. Previously, the cull rect would be misplaced causing many elements to not render at all. --- lib/web_ui/lib/src/engine/layers.dart | 1 + lib/web_ui/test/ui/scene_builder_test.dart | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/web_ui/lib/src/engine/layers.dart b/lib/web_ui/lib/src/engine/layers.dart index e5b52f762779a..7d6940be76be5 100644 --- a/lib/web_ui/lib/src/engine/layers.dart +++ b/lib/web_ui/lib/src/engine/layers.dart @@ -283,6 +283,7 @@ class OpacityOperation implements LayerOperation { if (offset != ui.Offset.zero) { canvas.save(); canvas.translate(offset.dx, offset.dy); + cullRect = cullRect.shift(-offset); } canvas.saveLayer( cullRect, diff --git a/lib/web_ui/test/ui/scene_builder_test.dart b/lib/web_ui/test/ui/scene_builder_test.dart index 7bd8463c5ced0..fee4e87e6b74a 100644 --- a/lib/web_ui/test/ui/scene_builder_test.dart +++ b/lib/web_ui/test/ui/scene_builder_test.dart @@ -124,16 +124,16 @@ Future testMain() async { ); })); - sceneBuilder.pushOpacity(0x7F); + sceneBuilder.pushOpacity(0x7F, offset: const ui.Offset(150, 150)); sceneBuilder.addPicture(ui.Offset.zero, drawPicture((ui.Canvas canvas) { final ui.Paint paint = ui.Paint()..color = const ui.Color(0xFFFF0000); canvas.drawCircle( - const ui.Offset(125, 150), + const ui.Offset(-25, 0), 50, paint ); canvas.drawCircle( - const ui.Offset(175, 150), + const ui.Offset(25, 0), 50, paint ); From a04fba4a23a90908555f1941054f130af8e21bad Mon Sep 17 00:00:00 2001 From: Jackson Gardner Date: Mon, 26 Feb 2024 10:52:59 -0800 Subject: [PATCH 03/21] Make sure to call `setHeightOverride` as well on TextStyle and StrutStyle (#50920) This fixes https://github.com/flutter/flutter/issues/143877 We apparently need to call `setHeightOverride(true)` on `TextStyle` and `StrutStyle` objects in order to properly apply the height. --- lib/web_ui/skwasm/text/strut_style.cpp | 1 + lib/web_ui/skwasm/text/text_style.cpp | 1 + lib/web_ui/test/ui/text_golden_test.dart | 5 +++++ 3 files changed, 7 insertions(+) diff --git a/lib/web_ui/skwasm/text/strut_style.cpp b/lib/web_ui/skwasm/text/strut_style.cpp index 41f9edf52e7fd..f3c7ea0d2fa33 100644 --- a/lib/web_ui/skwasm/text/strut_style.cpp +++ b/lib/web_ui/skwasm/text/strut_style.cpp @@ -33,6 +33,7 @@ SKWASM_EXPORT void strutStyle_setFontSize(StrutStyle* style, SKWASM_EXPORT void strutStyle_setHeight(StrutStyle* style, SkScalar height) { style->setHeight(height); + style->setHeightOverride(true); } SKWASM_EXPORT void strutStyle_setHalfLeading(StrutStyle* style, diff --git a/lib/web_ui/skwasm/text/text_style.cpp b/lib/web_ui/skwasm/text/text_style.cpp index b520f52a5424d..c0a50451fb10e 100644 --- a/lib/web_ui/skwasm/text/text_style.cpp +++ b/lib/web_ui/skwasm/text/text_style.cpp @@ -96,6 +96,7 @@ SKWASM_EXPORT void textStyle_setWordSpacing(TextStyle* style, SKWASM_EXPORT void textStyle_setHeight(TextStyle* style, SkScalar height) { style->setHeight(height); + style->setHeightOverride(true); } SKWASM_EXPORT void textStyle_setHalfLeading(TextStyle* style, diff --git a/lib/web_ui/test/ui/text_golden_test.dart b/lib/web_ui/test/ui/text_golden_test.dart index 60b956647875f..66813ff44c418 100644 --- a/lib/web_ui/test/ui/text_golden_test.dart +++ b/lib/web_ui/test/ui/text_golden_test.dart @@ -68,6 +68,11 @@ Future testMain() async { layoutWidth: 50, paragraphHeight: 1.5); }); + test('text styles - text style height overriding paragraph height', () async { + await testTextStyle('text style height and paragraph style height', + layoutWidth: 50, paragraphHeight: 1.5, height: 2.0); + }); + test('text styles - paragraph text height behavior', () async { await testTextStyle('paragraph text height behavior', layoutWidth: 50, From b28cfbbb4d900d370cc4d7967ce8b0243e476f84 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 26 Feb 2024 11:36:10 -0800 Subject: [PATCH 04/21] Better filtering for Android `scenario_app` runner. (#50937) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _🍴 'd from https://github.com/flutter/engine/pull/50933, will rebase when merged._ Closes https://github.com/flutter/flutter/issues/143458. A picture is a 1000 words: ![Screenshot 2024-02-23 at 7 01 29 PM](https://github.com/flutter/engine/assets/168174/7254b3be-cc49-4bad-ae43-e61ac4a853ad) This is still noisy, but at least all the output appears to be part of the execution. As you recall, the full logs are always available in the FLUTTER_LOGS_DIR output. --- .../scenario_app/bin/run_android_tests.dart | 195 ++++++++++-------- .../bin/utils/adb_logcat_filtering.dart | 103 ++++++++- testing/scenario_app/bin/utils/logs.dart | 17 +- testing/scenario_app/tool/logcat_reader.dart | 43 ++++ 4 files changed, 261 insertions(+), 97 deletions(-) create mode 100644 testing/scenario_app/tool/logcat_reader.dart diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart index f0ad8276b0465..436cb421424ee 100644 --- a/testing/scenario_app/bin/run_android_tests.dart +++ b/testing/scenario_app/bin/run_android_tests.dart @@ -47,48 +47,52 @@ void main(List args) async { ..addOption( 'adb', help: 'Path to the adb tool', - defaultsTo: engine != null ? join( - engine.srcDir.path, - 'third_party', - 'android_tools', - 'sdk', - 'platform-tools', - 'adb', - ) : null, + defaultsTo: engine != null + ? join( + engine.srcDir.path, + 'third_party', + 'android_tools', + 'sdk', + 'platform-tools', + 'adb', + ) + : null, ) ..addOption( 'ndk-stack', help: 'Path to the ndk-stack tool', - defaultsTo: engine != null ? join( - engine.srcDir.path, - 'third_party', - 'android_tools', - 'ndk', - 'prebuilt', - () { - if (Platform.isLinux) { - return 'linux-x86_64'; - } else if (Platform.isMacOS) { - return 'darwin-x86_64'; - } else if (Platform.isWindows) { - return 'windows-x86_64'; - } else { - throw UnsupportedError('Unsupported platform: ${Platform.operatingSystem}'); - } - }(), - 'bin', - 'ndk-stack', - ) : null, + defaultsTo: engine != null + ? join( + engine.srcDir.path, + 'third_party', + 'android_tools', + 'ndk', + 'prebuilt', + () { + if (Platform.isLinux) { + return 'linux-x86_64'; + } else if (Platform.isMacOS) { + return 'darwin-x86_64'; + } else if (Platform.isWindows) { + return 'windows-x86_64'; + } else { + throw UnsupportedError('Unsupported platform: ${Platform.operatingSystem}'); + } + }(), + 'bin', + 'ndk-stack', + ) + : null, ) ..addOption( 'out-dir', help: 'Out directory', - defaultsTo: - engine?. - outputs(). - where((Output o) => basename(o.path.path).startsWith('android_')). - firstOrNull?. - path.path, + defaultsTo: engine + ?.outputs() + .where((Output o) => basename(o.path.path).startsWith('android_')) + .firstOrNull + ?.path + .path, ) ..addOption( 'smoke-test', @@ -106,14 +110,16 @@ void main(List args) async { ..addOption( 'output-contents-golden', help: 'Path to a file that contains the expected filenames of golden files.', - defaultsTo: engine != null ? join( - engine.srcDir.path, - 'flutter', - 'testing', - 'scenario_app', - 'android', - 'expected_golden_output.txt', - ) : null, + defaultsTo: engine != null + ? join( + engine.srcDir.path, + 'flutter', + 'testing', + 'scenario_app', + 'android', + 'expected_golden_output.txt', + ) + : null, ) ..addOption( 'impeller-backend', @@ -124,8 +130,8 @@ void main(List args) async { ..addOption( 'logs-dir', help: 'The directory to store the logs and screenshots. Defaults to ' - 'the value of the FLUTTER_LOGS_DIR environment variable, if set, ' - 'otherwise it defaults to a path within out-dir.', + 'the value of the FLUTTER_LOGS_DIR environment variable, if set, ' + 'otherwise it defaults to a path within out-dir.', defaultsTo: Platform.environment['FLUTTER_LOGS_DIR'], ); @@ -153,7 +159,10 @@ void main(List args) async { final String? contentsGolden = results['output-contents-golden'] as String?; final _ImpellerBackend? impellerBackend = _ImpellerBackend.tryParse(results['impeller-backend'] as String?); if (enableImpeller && impellerBackend == null) { - panic(['invalid graphics-backend', results['impeller-backend'] as String? ?? '']); + panic([ + 'invalid graphics-backend', + results['impeller-backend'] as String? ?? '' + ]); } final Directory logsDir = Directory(results['logs-dir'] as String? ?? join(outDir.path, 'scenario_app', 'logs')); final String? ndkStack = results['ndk-stack'] as String?; @@ -215,7 +224,10 @@ Future _run({ const ProcessManager pm = LocalProcessManager(); if (!outDir.existsSync()) { - panic(['out-dir does not exist: $outDir', 'make sure to build the selected engine variant']); + panic([ + 'out-dir does not exist: $outDir', + 'make sure to build the selected engine variant' + ]); } if (!adb.existsSync()) { @@ -236,11 +248,17 @@ Future _run({ log('writing logs and screenshots to ${logsDir.path}'); if (!testApk.existsSync()) { - panic(['test apk does not exist: ${testApk.path}', 'make sure to build the selected engine variant']); + panic([ + 'test apk does not exist: ${testApk.path}', + 'make sure to build the selected engine variant' + ]); } if (!appApk.existsSync()) { - panic(['app apk does not exist: ${appApk.path}', 'make sure to build the selected engine variant']); + panic([ + 'app apk does not exist: ${appApk.path}', + 'make sure to build the selected engine variant' + ]); } // Start a TCP socket in the host, and forward it to the device that runs the tests. @@ -248,7 +266,7 @@ Future _run({ // for the screenshots. // On LUCI, the host uploads the screenshots to Skia Gold. SkiaGoldClient? skiaGoldClient; - late ServerSocket server; + late ServerSocket server; final List> pendingComparisons = >[]; await step('Starting server...', () async { server = await ServerSocket.bind(InternetAddress.anyIPv4, _tcpPort); @@ -259,7 +277,8 @@ Future _run({ if (verbose) { stdout.writeln('client connected ${client.remoteAddress.address}:${client.remotePort}'); } - client.transform(const ScreenshotBlobTransformer()).listen((Screenshot screenshot) { + client.transform(const ScreenshotBlobTransformer()).listen( + (Screenshot screenshot) { final String fileName = screenshot.filename; final Uint8List fileContent = screenshot.fileContent; if (verbose) { @@ -277,18 +296,15 @@ Future _run({ } if (isSkiaGoldClientAvailable) { final Future comparison = skiaGoldClient! - .addImg(fileName, goldenFile, - screenshotSize: screenshot.pixelCount) - .catchError((dynamic err) { - panic(['skia gold comparison failed: $err']); - }); + .addImg(fileName, goldenFile, screenshotSize: screenshot.pixelCount) + .catchError((dynamic err) { + panic(['skia gold comparison failed: $err']); + }); pendingComparisons.add(comparison); } - }, - onError: (dynamic err) { + }, onError: (dynamic err) { panic(['error while receiving bytes: $err']); - }, - cancelOnError: true); + }, cancelOnError: true); }); }); @@ -311,27 +327,38 @@ Future _run({ final (Future logcatExitCode, Stream logcatOutput) = getProcessStreams(logcatProcess); logcatProcessExitCode = logcatExitCode; + String? filterProcessId; + logcatOutput.listen((String line) { // Always write to the full log. logcat.writeln(line); // Conditionally parse and write to stderr. final AdbLogLine? adbLogLine = AdbLogLine.tryParse(line); - switch (adbLogLine?.process) { - case null: - break; - case 'ActivityManager': - // These are mostly noise, i.e. "D ActivityManager: freezing 24632 com.blah". - if (adbLogLine!.severity == 'D') { - break; - } - // TODO(matanlurey): Figure out why this isn't 'flutter.scenario' or similar. - // Also, why is there two different names? - case 'utter.scenario': - case 'utter.scenarios': - case 'flutter': - case 'FlutterJNI': - log('[adb] $line'); + if (verbose || adbLogLine == null) { + log(line); + return; + } + + // If we haven't already found a process ID, try to find one. + // The process ID will help us filter out logs from other processes. + filterProcessId ??= adbLogLine.tryParseProcess(); + + // If this is a "verbose" log, possibly skip it. + final bool isVerbose = adbLogLine.isVerbose(filterProcessId: filterProcessId); + if (isVerbose || filterProcessId == null) { + // We've requested verbose output, so print everything. + if (verbose) { + adbLogLine.printFormatted(); + } + return; + } + + // It's a non-verbose log, so print it. + adbLogLine.printFormatted(); + }, onError: (Object? err) { + if (verbose) { + logWarning('logcat stream error: $err'); } }); }); @@ -364,10 +391,7 @@ Future _run({ log('using dimensions: ${json.encode(dimensions)}'); skiaGoldClient = SkiaGoldClient( outDir, - dimensions: { - 'AndroidAPILevel': connectedDeviceAPILevel, - 'GraphicsBackend': enableImpeller ? 'impeller-${impellerBackend!.name}' : 'skia', - }, + dimensions: dimensions, ); }); @@ -412,11 +436,9 @@ Future _run({ 'am', 'instrument', '-w', - if (smokeTestFullPath != null) - '-e class $smokeTestFullPath', + if (smokeTestFullPath != null) '-e class $smokeTestFullPath', 'dev.flutter.scenarios.test/dev.flutter.TestRunner', - if (enableImpeller) - '-e enable-impeller', + if (enableImpeller) '-e enable-impeller', if (impellerBackend != null) '-e impeller-backend ${impellerBackend.name}', ]); @@ -465,7 +487,8 @@ Future _run({ final int exitCode = await pm.runAndForward([ adb.path, 'reverse', - '--remove', 'tcp:3000', + '--remove', + 'tcp:3000', ]); if (exitCode != 0) { panic(['could not unforward port']); @@ -473,14 +496,16 @@ Future _run({ }); await step('Uninstalling app APK...', () async { - final int exitCode = await pm.runAndForward([adb.path, 'uninstall', 'dev.flutter.scenarios']); + final int exitCode = await pm.runAndForward( + [adb.path, 'uninstall', 'dev.flutter.scenarios']); if (exitCode != 0) { panic(['could not uninstall app apk']); } }); await step('Uninstalling test APK...', () async { - final int exitCode = await pm.runAndForward([adb.path, 'uninstall', 'dev.flutter.scenarios.test']); + final int exitCode = await pm.runAndForward( + [adb.path, 'uninstall', 'dev.flutter.scenarios.test']); if (exitCode != 0) { panic(['could not uninstall app apk']); } diff --git a/testing/scenario_app/bin/utils/adb_logcat_filtering.dart b/testing/scenario_app/bin/utils/adb_logcat_filtering.dart index ca67ab17df46d..97852630c0a6a 100644 --- a/testing/scenario_app/bin/utils/adb_logcat_filtering.dart +++ b/testing/scenario_app/bin/utils/adb_logcat_filtering.dart @@ -1,3 +1,7 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + /// Some notes about filtering `adb logcat` output, especially as a result of /// running `adb shell` to instrument the app and test scripts, as it's /// non-trivial and error-prone. @@ -26,6 +30,8 @@ /// See also: . library; +import 'logs.dart'; + /// Represents a line of `adb logcat` output parsed into a structured form. /// /// For example the line: @@ -40,14 +46,15 @@ library; /// with lazy parsing. extension type const AdbLogLine._(Match _match) { // RegEx that parses into the following groups: - // 1. Everything up to the severity (I, W, E, etc.). - // In other words, any whitespace, numbers, hyphens, colons, and periods. - // 2. The severity (a single uppercase letter). - // 3. The name of the process (up to the colon). - // 4. The message (after the colon). + // 1. The time of the log message, such as `02-22 13:54:39.839`. + // 2. The process ID. + // 3. The thread ID. + // 4. The character representing the severity of the log message, such as `I`. + // 5. The tag, such as `ActivityManager`. + // 6. The actual log message. // // This regex is simple versus being more precise. Feel free to improve it. - static final RegExp _pattern = RegExp(r'([^A-Z]*)([A-Z])\s([^:]*)\:\s(.*)'); + static final RegExp _pattern = RegExp(r'(\d+-\d+\s[\d|:]+\.\d+)\s+(\d+)\s+(\d+)\s(\w)\s(\S+)\s*:\s*(.*)'); /// Parses the given [adbLogCatLine] into a structured form. /// @@ -57,15 +64,91 @@ extension type const AdbLogLine._(Match _match) { return match == null ? null : AdbLogLine._(match); } + /// Tries to parse the process that was started, if the log line is about it. + String? tryParseProcess() { + if (name == 'ActivityManager' && message.startsWith('Start proc')) { + // Start proc 6840:d + final RegExpMatch? match = RegExp(r'Start proc (\d+):').firstMatch(message); + return match?.group(1); + } + return null; + } + + /// Returns `true` if the log line is verbose. + bool isVerbose({String? filterProcessId}) => !_isRelevant(filterProcessId: filterProcessId); + bool _isRelevant({String? filterProcessId}) { + // Fatal errors are always useful. + if (severity == 'F') { + return true; + } + + // Debug logs are rarely useful. + if (severity == 'D') { + return false; + } + + // These are "known" noise tags. + if (const { + 'MonitoringInstr', + 'ResourceExtractor', + 'THREAD_STATE', + 'ziparchive', + }.contains(name)) { + return false; + } + + // These are "known" tags useful for debugging. + if (const { + 'utter.scenario', + 'utter.scenarios', + 'TestRunner', + }.contains(name)) { + return true; + } + + // If a process ID is specified, exclude logs _not_ from that process. + if (filterProcessId != null && process != filterProcessId) { + return false; + } + + // And... whatever, include anything with the word "flutter". + return name.toLowerCase().contains('flutter') || message.toLowerCase().contains('flutter'); + } + + /// Logs the line to the console. + void printFormatted() { + final String formatted = '$time [$severity] $name: $message'; + if (severity == 'W' || severity == 'E' || severity == 'F') { + logWarning(formatted); + } else if (name == 'TestRunner') { + logImportant(formatted); + } else { + log(formatted); + } + } + /// The full line of `adb logcat` output. String get line => _match.group(0)!; + /// The time of the log message, such as `02-22 13:54:39.839`. + String get time => _match.group(1)!; + + /// The process ID. + String get process => _match.group(2)!; + + /// The thread ID. + String get thread => _match.group(3)!; + /// The character representing the severity of the log message, such as `I`. - String get severity => _match.group(2)!; + String get severity => _match.group(4)!; - /// The process name, such as `ActivityManager`. - String get process => _match.group(3)!; + /// The tag, such as `ActivityManager`. + String get name => _match.group(5)!; /// The actual log message. - String get message => _match.group(4)!; + String get message => _match.group(6)!; + + String toDebugString() { + return 'AdbLogLine(time: $time, process: $process, thread: $thread, severity: $severity, name: $name, message: $message)'; + } } diff --git a/testing/scenario_app/bin/utils/logs.dart b/testing/scenario_app/bin/utils/logs.dart index b84e8127f29fe..4d883d340e261 100644 --- a/testing/scenario_app/bin/utils/logs.dart +++ b/testing/scenario_app/bin/utils/logs.dart @@ -7,6 +7,7 @@ import 'dart:io'; bool _supportsAnsi = stdout.supportsAnsiEscapes; String _green = _supportsAnsi ? '\u001b[1;32m' : ''; String _red = _supportsAnsi ? '\u001b[31m' : ''; +String _yellow = _supportsAnsi ? '\u001b[33m' : ''; String _gray = _supportsAnsi ? '\u001b[90m' : ''; String _reset = _supportsAnsi? '\u001B[0m' : ''; @@ -22,15 +23,27 @@ Future step(String msg, Future Function() fn) async { } } +void _logWithColor(String color, String msg) { + stdout.writeln('$color$msg$_reset'); +} + void log(String msg) { - stdout.writeln('$_gray$msg$_reset'); + _logWithColor(_gray, msg); +} + +void logImportant(String msg) { + stdout.writeln(msg); +} + +void logWarning(String msg) { + _logWithColor(_yellow, msg); } final class Panic extends Error {} Never panic(List messages) { for (final String message in messages) { - stderr.writeln('$_red$message$_reset'); + _logWithColor(_red, message); } throw Panic(); } diff --git a/testing/scenario_app/tool/logcat_reader.dart b/testing/scenario_app/tool/logcat_reader.dart new file mode 100644 index 0000000000000..909204a1af342 --- /dev/null +++ b/testing/scenario_app/tool/logcat_reader.dart @@ -0,0 +1,43 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:io' as io; + +// It's bad to import a file from `bin` into `tool`. +// However this tool is not very important, so delete it if necessary. +import '../bin/utils/adb_logcat_filtering.dart'; + +/// A tiny tool to read saved `adb logcat` output and perform some analysis. +/// +/// This tool is not meant to be a full-fledged logcat reader. It's just a +/// simple tool that uses the [AdbLogLine] extension type to parse results of +/// `adb logcat` and explain what log tag names are most common. +void main(List args) { + if (args case [final String path]) { + final List parsed = io.File(path) + .readAsLinesSync() + .map(AdbLogLine.tryParse) + .whereType() + // Filter out all debug logs. + .where((AdbLogLine line) => line.severity != 'D') + .toList(); + + final Map tagCounts = {}; + for (final AdbLogLine line in parsed) { + tagCounts[line.name] = (tagCounts[line.name] ?? 0) + 1; + } + + // Print in order of most common to least common. + final List> sorted = tagCounts.entries.toList() + ..sort((MapEntry a, MapEntry b) => b.value.compareTo(a.value)); + for (final MapEntry entry in sorted) { + print("'${entry.key}', // ${entry.value}"); + } + + return; + } + + print('Usage: logcat_reader.dart '); + io.exitCode = 1; +} From adbd1f48e6c76998fbabfef403fcce164a58f753 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 26 Feb 2024 12:06:29 -0800 Subject: [PATCH 05/21] [Impeller] Fix a race that can abort the process if the Vulkan context is destroyed while pipeline creation tasks are pending (#50883) The Vulkan pipeline library queues pipeline creation tasks to a ConcurrentTaskRunner. If the ContextVK is destroyed before these tasks execute, then the ConcurrentMessageLoop destructor will delete the pending tasks. Each task lambda holds a reference to a promise that will be completed with the pipeline. If the task was never run and its promise was never completed, then the promise destructor will complete it with an exception. This will cause a std::abort because Flutter is built without exception support. This PR wraps the promise in an object that completes it with a default value during destruction if the promise was never given a value. --- impeller/base/base_unittests.cc | 17 +++++++++++ impeller/base/promise.h | 28 +++++++++++++++++++ .../backend/vulkan/pipeline_library_vk.cc | 2 +- 3 files changed, 46 insertions(+), 1 deletion(-) diff --git a/impeller/base/base_unittests.cc b/impeller/base/base_unittests.cc index a7c40d868df55..db2c97852be49 100644 --- a/impeller/base/base_unittests.cc +++ b/impeller/base/base_unittests.cc @@ -3,6 +3,7 @@ // found in the LICENSE file. #include "flutter/testing/testing.h" +#include "impeller/base/promise.h" #include "impeller/base/strings.h" #include "impeller/base/thread.h" @@ -233,5 +234,21 @@ TEST(ConditionVariableTest, TestsCriticalSectionAfterWait) { ASSERT_EQ(sum, kThreadCount); } +TEST(BaseTest, NoExceptionPromiseValue) { + NoExceptionPromise wrapper; + std::future future = wrapper.get_future(); + wrapper.set_value(123); + ASSERT_EQ(future.get(), 123); +} + +TEST(BaseTest, NoExceptionPromiseEmpty) { + auto wrapper = std::make_shared>(); + std::future future = wrapper->get_future(); + + // Destroy the empty promise with the future still pending. Verify that the + // process does not abort while destructing the promise. + wrapper.reset(); +} + } // namespace testing } // namespace impeller diff --git a/impeller/base/promise.h b/impeller/base/promise.h index 30d45ae4d3f5f..9f7205eaab60b 100644 --- a/impeller/base/promise.h +++ b/impeller/base/promise.h @@ -17,6 +17,34 @@ std::future RealizedFuture(T t) { return future; } +// Wraps a std::promise and completes the promise with a value during +// destruction if the promise does not already have a value. +// +// By default the std::promise destructor will complete an empty promise with an +// exception. This will fail because Flutter is built without exception support. +template +class NoExceptionPromise { + public: + NoExceptionPromise() = default; + + ~NoExceptionPromise() { + if (!value_set_) { + promise_.set_value({}); + } + } + + std::future get_future() { return promise_.get_future(); } + + void set_value(const T& value) { + promise_.set_value(value); + value_set_ = true; + } + + private: + std::promise promise_; + bool value_set_ = false; +}; + } // namespace impeller #endif // FLUTTER_IMPELLER_BASE_PROMISE_H_ diff --git a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc index 600ef830450d5..130cabd36ae98 100644 --- a/impeller/renderer/backend/vulkan/pipeline_library_vk.cc +++ b/impeller/renderer/backend/vulkan/pipeline_library_vk.cc @@ -170,7 +170,7 @@ PipelineFuture PipelineLibraryVK::GetPipeline( } auto promise = std::make_shared< - std::promise>>>(); + NoExceptionPromise>>>(); auto pipeline_future = PipelineFuture{descriptor, promise->get_future()}; pipelines_[descriptor] = pipeline_future; From d8129b98fdf29dc94efb652481c1f6fdb600fc2a Mon Sep 17 00:00:00 2001 From: John McCutchan Date: Mon, 26 Feb 2024 13:00:49 -0800 Subject: [PATCH 06/21] Update Surface reference after resizing render target in VirtualDisplay based platform views (#50971) Fixes https://github.com/flutter/flutter/issues/142952 --- .../platform/VirtualDisplayController.java | 31 ++++++++++++++++--- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java b/shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java index 1cb44373b1bdf..d041382044895 100644 --- a/shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java +++ b/shell/platform/android/io/flutter/plugin/platform/VirtualDisplayController.java @@ -19,10 +19,19 @@ import androidx.annotation.NonNull; import androidx.annotation.VisibleForTesting; -@TargetApi(20) +@TargetApi(21) class VirtualDisplayController { private static String TAG = "VirtualDisplayController"; + private static VirtualDisplay.Callback callback = + new VirtualDisplay.Callback() { + @Override + public void onPaused() {} + + @Override + public void onResumed() {} + }; + public static VirtualDisplayController create( Context context, AccessibilityEventsDelegate accessibilityEventsDelegate, @@ -40,6 +49,7 @@ public static VirtualDisplayController create( DisplayManager displayManager = (DisplayManager) context.getSystemService(Context.DISPLAY_SERVICE); final DisplayMetrics metrics = context.getResources().getDisplayMetrics(); + // Virtual Display crashes for some PlatformViews if the width or height is bigger // than the physical screen size. We have tried to clamp or scale down the size to prevent // the crash, but both solutions lead to unwanted behavior because the @@ -51,6 +61,7 @@ public static VirtualDisplayController create( // virtual display and AndroidPlatformView widget. // https://github.com/flutter/flutter/issues/93115 renderTarget.resize(width, height); + int flags = 0; VirtualDisplay virtualDisplay = displayManager.createVirtualDisplay( "flutter-vd#" + viewId, @@ -58,7 +69,9 @@ public static VirtualDisplayController create( height, metrics.densityDpi, renderTarget.getSurface(), - 0); + flags, + callback, + null /* handler */); if (virtualDisplay == null) { return null; @@ -148,9 +161,17 @@ public void resize(final int width, final int height, final Runnable onNewSizeFr final DisplayManager displayManager = (DisplayManager) context.getSystemService(Context.DISPLAY_SERVICE); renderTarget.resize(width, height); + int flags = 0; virtualDisplay = displayManager.createVirtualDisplay( - "flutter-vd#" + viewId, width, height, densityDpi, renderTarget.getSurface(), 0); + "flutter-vd#" + viewId, + width, + height, + densityDpi, + renderTarget.getSurface(), + flags, + callback, + null /* handler */); final View embeddedView = getView(); // There's a bug in Android version older than O where view tree observer onDrawListeners don't @@ -210,12 +231,14 @@ public void dispose() { renderTarget.release(); } + // On Android versions 31+ resizing of a Virtual Display's Presentation is natively supported. @TargetApi(31) private void resize31( View embeddedView, int width, int height, final Runnable onNewSizeFrameAvailable) { renderTarget.resize(width, height); - // On Android versions 31+ resizing of a Virtual Display's Presentation is natively supported. virtualDisplay.resize(width, height, densityDpi); + // Must update the surface to match the renderTarget's current surface. + virtualDisplay.setSurface(renderTarget.getSurface()); embeddedView.postDelayed(onNewSizeFrameAvailable, 0); } From a93ca854550431f35d38da8e447ea22ffc4a2cb4 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 26 Feb 2024 13:09:16 -0800 Subject: [PATCH 07/21] [Impeller] disble render pass caches. (#50976) This is related to the crashes in https://github.com/flutter/flutter/issues/144116 > I'm going to disable this caching for now until I understand why its not working for tester. Potentially the cache itself was unsafe and validation is just missing, but I need to do some research. If I don't find anything then i'll consider conditionally disabling for flutter tester builds --- .../vulkan/render_pass_cache_unittests.cc | 36 +------------------ .../renderer/backend/vulkan/render_pass_vk.cc | 18 ++-------- 2 files changed, 3 insertions(+), 51 deletions(-) diff --git a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc index e2fc63ea5796a..bb850c3e7d83b 100644 --- a/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc +++ b/impeller/renderer/backend/vulkan/render_pass_cache_unittests.cc @@ -11,41 +11,7 @@ namespace impeller { namespace testing { -using RendererTest = PlaygroundTest; - -TEST_P(RendererTest, CachesRenderPassAndFramebuffer) { - if (GetBackend() != PlaygroundBackend::kVulkan) { - GTEST_SKIP() << "Test only applies to Vulkan"; - } - - auto allocator = std::make_shared( - GetContext()->GetResourceAllocator()); - - auto render_target = RenderTarget::CreateOffscreenMSAA( - *GetContext(), *allocator, {100, 100}, 1); - auto resolve_texture = - render_target.GetColorAttachments().find(0u)->second.resolve_texture; - auto& texture_vk = TextureVK::Cast(*resolve_texture); - - EXPECT_EQ(texture_vk.GetFramebuffer(), nullptr); - EXPECT_EQ(texture_vk.GetRenderPass(), nullptr); - - auto buffer = GetContext()->CreateCommandBuffer(); - auto render_pass = buffer->CreateRenderPass(render_target); - - EXPECT_NE(texture_vk.GetFramebuffer(), nullptr); - EXPECT_NE(texture_vk.GetRenderPass(), nullptr); - - render_pass->EncodeCommands(); - GetContext()->GetCommandQueue()->Submit({buffer}); - - // Can be reused without error. - auto buffer_2 = GetContext()->CreateCommandBuffer(); - auto render_pass_2 = buffer_2->CreateRenderPass(render_target); - - EXPECT_TRUE(render_pass_2->EncodeCommands()); - EXPECT_TRUE(GetContext()->GetCommandQueue()->Submit({buffer_2}).ok()); -} +// } // namespace testing } // namespace impeller diff --git a/impeller/renderer/backend/vulkan/render_pass_vk.cc b/impeller/renderer/backend/vulkan/render_pass_vk.cc index 28a7013127775..b2f8bde4c114c 100644 --- a/impeller/renderer/backend/vulkan/render_pass_vk.cc +++ b/impeller/renderer/backend/vulkan/render_pass_vk.cc @@ -170,26 +170,16 @@ RenderPassVK::RenderPassVK(const std::shared_ptr& context, return true; }); - SharedHandleVK recycled_render_pass; - SharedHandleVK recycled_framebuffer; - if (resolve_image_vk_) { - recycled_render_pass = TextureVK::Cast(*resolve_image_vk_).GetRenderPass(); - recycled_framebuffer = TextureVK::Cast(*resolve_image_vk_).GetFramebuffer(); - } - const auto& target_size = render_target_.GetRenderTargetSize(); - render_pass_ = - CreateVKRenderPass(vk_context, recycled_render_pass, command_buffer_); + render_pass_ = CreateVKRenderPass(vk_context, nullptr, command_buffer_); if (!render_pass_) { VALIDATION_LOG << "Could not create renderpass."; is_valid_ = false; return; } - auto framebuffer = (recycled_framebuffer == nullptr) - ? CreateVKFramebuffer(vk_context, *render_pass_) - : recycled_framebuffer; + auto framebuffer = CreateVKFramebuffer(vk_context, *render_pass_); if (!framebuffer) { VALIDATION_LOG << "Could not create framebuffer."; is_valid_ = false; @@ -200,10 +190,6 @@ RenderPassVK::RenderPassVK(const std::shared_ptr& context, is_valid_ = false; return; } - if (resolve_image_vk_) { - TextureVK::Cast(*resolve_image_vk_).SetFramebuffer(framebuffer); - TextureVK::Cast(*resolve_image_vk_).SetRenderPass(render_pass_); - } auto clear_values = GetVKClearValues(render_target_); From c24744983bc1b305fbcf54674cacbd40c0ba6572 Mon Sep 17 00:00:00 2001 From: Zachary Anderson Date: Mon, 26 Feb 2024 21:39:05 +0000 Subject: [PATCH 08/21] Run engine unit tests on mac host_debug_unopt_arm64 (#50327) Related https://github.com/flutter/flutter/issues/142868 --- ci/builders/mac_unopt.json | 16 +- impeller/renderer/blit_pass_unittests.cc | 2 + .../embedder/tests/embedder_unittests.cc | 7 + testing/run_tests.py | 5 +- tools/font_subset/test.py | 191 +++++++++--------- 5 files changed, 126 insertions(+), 95 deletions(-) diff --git a/ci/builders/mac_unopt.json b/ci/builders/mac_unopt.json index dc6f18b3a88df..c6674574489ec 100644 --- a/ci/builders/mac_unopt.json +++ b/ci/builders/mac_unopt.json @@ -142,7 +142,21 @@ "$flutter/osx_sdk": { "sdk_version": "15a240d" } - } + }, + "tests": [ + { + "language": "python3", + "name": "Host Tests for host_debug_unopt_arm64", + "script": "flutter/testing/run_tests.py", + "parameters": [ + "--variant", + "host_debug_unopt_arm64", + "--type", + "dart,dart-host,engine", + "--engine-capture-core-dump" + ] + } + ] }, { "properties": { diff --git a/impeller/renderer/blit_pass_unittests.cc b/impeller/renderer/blit_pass_unittests.cc index bffa3babdf974..dbf534d1deda5 100644 --- a/impeller/renderer/blit_pass_unittests.cc +++ b/impeller/renderer/blit_pass_unittests.cc @@ -24,11 +24,13 @@ TEST_P(BlitPassTest, BlitAcrossDifferentPixelFormatsFails) { TextureDescriptor src_desc; src_desc.format = PixelFormat::kA8UNormInt; src_desc.size = {100, 100}; + src_desc.storage_mode = StorageMode::kHostVisible; auto src = context->GetResourceAllocator()->CreateTexture(src_desc); TextureDescriptor dst_format; dst_format.format = PixelFormat::kR8G8B8A8UNormInt; dst_format.size = {100, 100}; + dst_format.storage_mode = StorageMode::kHostVisible; auto dst = context->GetResourceAllocator()->CreateTexture(dst_format); EXPECT_FALSE(blit_pass->AddCopy(src, dst)); diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc index 23f30fdc44705..da76770763d34 100644 --- a/shell/platform/embedder/tests/embedder_unittests.cc +++ b/shell/platform/embedder/tests/embedder_unittests.cc @@ -644,8 +644,15 @@ TEST_F(EmbedderTest, VMAndIsolateSnapshotSizesAreRedundantInAOTMode) { /// Test the layer structure and pixels rendered when using a custom software /// compositor. /// +// TODO(143940): Convert this test to use SkiaGold. +#if FML_OS_MACOSX && FML_ARCH_CPU_ARM64 +TEST_F(EmbedderTest, + DISABLED_CompositorMustBeAbleToRenderKnownSceneWithSoftwareCompositor) { +#else TEST_F(EmbedderTest, CompositorMustBeAbleToRenderKnownSceneWithSoftwareCompositor) { +#endif // FML_OS_MACOSX && FML_ARCH_CPU_ARM64 + auto& context = GetEmbedderContext(EmbedderTestContextType::kSoftwareContext); EmbedderConfigBuilder builder(context); diff --git a/testing/run_tests.py b/testing/run_tests.py index 966a02778e34d..8fd19bafd3dab 100755 --- a/testing/run_tests.py +++ b/testing/run_tests.py @@ -1260,7 +1260,10 @@ def main(): variants_to_skip = ['host_release', 'host_profile'] if ('engine' in types or 'font-subset' in types) and args.variant not in variants_to_skip: - run_cmd(['python3', 'test.py'], cwd=FONT_SUBSET_DIR) + cmd = ['python3', 'test.py', '--variant', args.variant] + if 'arm64' in args.variant: + cmd += ['--target-cpu', 'arm64'] + run_cmd(cmd, cwd=FONT_SUBSET_DIR) if 'impeller-golden' in types: run_impeller_golden_tests(build_dir) diff --git a/tools/font_subset/test.py b/tools/font_subset/test.py index d9f74fb3d6c2c..fceda3af8208e 100755 --- a/tools/font_subset/test.py +++ b/tools/font_subset/test.py @@ -8,46 +8,17 @@ Tests for font-subset ''' +import argparse import filecmp import os import subprocess import sys from zipfile import ZipFile -# Dictionary to map the platform name to the output directory -# of the font artifacts. -PLATFORM_2_PATH = { - 'darwin': 'darwin-x64', - 'linux': 'linux-x64', - 'linux2': 'linux-x64', - 'cygwin': 'windows-x64', - 'win': 'windows-x64', - 'win32': 'windows-x64', -} - SCRIPT_DIR = os.path.dirname(os.path.abspath(__file__)) SRC_DIR = os.path.normpath(os.path.join(SCRIPT_DIR, '..', '..', '..')) MATERIAL_TTF = os.path.join(SCRIPT_DIR, 'fixtures', 'MaterialIcons-Regular.ttf') VARIABLE_MATERIAL_TTF = os.path.join(SCRIPT_DIR, 'fixtures', 'MaterialSymbols-Variable.ttf') -IS_WINDOWS = sys.platform.startswith(('cygwin', 'win')) -EXE = '.exe' if IS_WINDOWS else '' -BAT = '.bat' if IS_WINDOWS else '' -FONT_SUBSET = os.path.join(SRC_DIR, 'out', 'host_debug', 'font-subset' + EXE) -FONT_SUBSET_ZIP = os.path.join( - SRC_DIR, 'out', 'host_debug', 'zip_archives', PLATFORM_2_PATH.get(sys.platform, ''), - 'font-subset.zip' -) -if not os.path.isfile(FONT_SUBSET): - FONT_SUBSET = os.path.join(SRC_DIR, 'out', 'host_debug_unopt', 'font-subset' + EXE) - FONT_SUBSET_ZIP = os.path.join( - SRC_DIR, 'out', 'host_debug_unopt', 'zip_archives', PLATFORM_2_PATH.get(sys.platform, ''), - 'font-subset.zip' - ) -if not os.path.isfile(FONT_SUBSET): - raise Exception( - 'Could not locate font-subset%s in host_debug or host_debug_unopt - build before running this script.' - % EXE - ) COMPARE_TESTS = ( (True, '1.ttf', MATERIAL_TTF, [r'57347']), @@ -91,59 +62,61 @@ ]), ) -FAIL_TESTS = [ - ([FONT_SUBSET, 'output.ttf', 'does-not-exist.ttf'], [ - '1', - ]), # non-existent input font - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], [ - '0xFFFFFFFF', - ]), # Value too big. - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], [ - '-1', - ]), # invalid value - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], [ - 'foo', - ]), # no valid values - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], [ - '0xE003', - '0x12', - '0xE004', - ]), # codepoint not in font - ([FONT_SUBSET, 'non-existent-dir/output.ttf', MATERIAL_TTF], [ - '0xE003', - ]), # dir doesn't exist - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], [ - ' ', - ]), # empty input - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], []), # empty input - ([FONT_SUBSET, 'output.ttf', MATERIAL_TTF], ['']), # empty input - # repeat tests with variable input font - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [ - '0xFFFFFFFF', - ]), # Value too big. - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [ - '-1', - ]), # invalid value - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [ - 'foo', - ]), # no valid values - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [ - '0xE003', - '0x12', - '0xE004', - ]), # codepoint not in font - ([FONT_SUBSET, 'non-existent-dir/output.ttf', VARIABLE_MATERIAL_TTF], [ - '0xE003', - ]), # dir doesn't exist - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], [ - ' ', - ]), # empty input - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], []), # empty input - ([FONT_SUBSET, 'output.ttf', VARIABLE_MATERIAL_TTF], ['']), # empty input -] - - -def RunCmd(cmd, codepoints, fail=False): + +def fail_tests(font_subset): + return [ + ([font_subset, 'output.ttf', 'does-not-exist.ttf'], [ + '1', + ]), # non-existent input font + ([font_subset, 'output.ttf', MATERIAL_TTF], [ + '0xFFFFFFFF', + ]), # Value too big. + ([font_subset, 'output.ttf', MATERIAL_TTF], [ + '-1', + ]), # invalid value + ([font_subset, 'output.ttf', MATERIAL_TTF], [ + 'foo', + ]), # no valid values + ([font_subset, 'output.ttf', MATERIAL_TTF], [ + '0xE003', + '0x12', + '0xE004', + ]), # codepoint not in font + ([font_subset, 'non-existent-dir/output.ttf', MATERIAL_TTF], [ + '0xE003', + ]), # dir doesn't exist + ([font_subset, 'output.ttf', MATERIAL_TTF], [ + ' ', + ]), # empty input + ([font_subset, 'output.ttf', MATERIAL_TTF], []), # empty input + ([font_subset, 'output.ttf', MATERIAL_TTF], ['']), # empty input + # repeat tests with variable input font + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], [ + '0xFFFFFFFF', + ]), # Value too big. + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], [ + '-1', + ]), # invalid value + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], [ + 'foo', + ]), # no valid values + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], [ + '0xE003', + '0x12', + '0xE004', + ]), # codepoint not in font + ([font_subset, 'non-existent-dir/output.ttf', VARIABLE_MATERIAL_TTF], [ + '0xE003', + ]), # dir doesn't exist + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], [ + ' ', + ]), # empty input + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], []), # empty input + ([font_subset, 'output.ttf', VARIABLE_MATERIAL_TTF], ['']), # empty input + ] + + +def run_cmd(cmd, codepoints, fail=False): print('Running command:') print(' %s' % ' '.join(cmd)) print('STDIN: "%s"' % ' '.join(codepoints)) @@ -169,34 +142,66 @@ def RunCmd(cmd, codepoints, fail=False): return p.returncode -def TestZip(): - with ZipFile(FONT_SUBSET_ZIP, 'r') as zip: +def test_zip(font_subset_zip, exe): + with ZipFile(font_subset_zip, 'r') as zip: files = zip.namelist() - if 'font-subset%s' % EXE not in files: - print('expected %s to contain font-subset%s' % (files, EXE)) + if 'font-subset%s' % exe not in files: + print('expected %s to contain font-subset%s' % (files, exe)) return 1 return 0 +# Maps the platform name to the output directory of the font artifacts. +def platform_to_path(os, cpu): + d = { + 'darwin': 'darwin-', + 'linux': 'linux-', + 'linux2': 'linux-', + 'cygwin': 'windows-', + 'win': 'windows-', + 'win32': 'windows-', + } + return d[os] + cpu + + def main(): - print('Using font subset binary at %s (%s)' % (FONT_SUBSET, FONT_SUBSET_ZIP)) + parser = argparse.ArgumentParser(description='Runs font-subset tests.') + parser.add_argument('--variant', type=str, required=True) + parser.add_argument('--target-cpu', type=str, default='x64') + args = parser.parse_args() + variant = args.variant + + is_windows = sys.platform.startswith(('cygwin', 'win')) + exe = '.exe' if is_windows else '' + font_subset = os.path.join(SRC_DIR, 'out', variant, 'font-subset' + exe) + font_subset_zip = os.path.join( + SRC_DIR, 'out', variant, 'zip_archives', platform_to_path(sys.platform, args.target_cpu), + 'font-subset.zip' + ) + if not os.path.isfile(font_subset): + raise Exception( + 'Could not locate font-subset%s in host_debug or host_debug_unopt - build before running this script.' + % exe + ) + + print('Using font subset binary at %s (%s)' % (font_subset, font_subset_zip)) failures = 0 - failures += TestZip() + failures += test_zip(font_subset_zip, exe) for should_pass, golden_font, input_font, codepoints in COMPARE_TESTS: gen_ttf = os.path.join(SCRIPT_DIR, 'gen', golden_font) golden_ttf = os.path.join(SCRIPT_DIR, 'fixtures', golden_font) - cmd = [FONT_SUBSET, gen_ttf, input_font] - RunCmd(cmd, codepoints) + cmd = [font_subset, gen_ttf, input_font] + run_cmd(cmd, codepoints) cmp = filecmp.cmp(gen_ttf, golden_ttf, shallow=False) if (should_pass and not cmp) or (not should_pass and cmp): print('Test case %s failed.' % cmd) failures += 1 with open(os.devnull, 'w') as devnull: - for cmd, codepoints in FAIL_TESTS: - if RunCmd(cmd, codepoints, fail=True) == 0: + for cmd, codepoints in fail_tests(font_subset): + if run_cmd(cmd, codepoints, fail=True) == 0: failures += 1 if failures > 0: From 8814d65c47c961043695a002cbefff099f4f0b33 Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Mon, 26 Feb 2024 17:31:35 -0500 Subject: [PATCH 09/21] Roll Dart SDK from c479735adcf9 to 2876f5684ced (2 revisions) (#50979) https://dart.googlesource.com/sdk.git/+log/c479735adcf9..2876f5684ced 2024-02-26 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.4.0-178.0.dev 2024-02-26 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.4.0-177.0.dev If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/dart-sdk-flutter-engine Please CC dart-vm-team@google.com,jimgraham@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter Engine: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md --- DEPS | 2 +- ci/licenses_golden/licenses_dart | 6 ++---- sky/packages/sky_engine/LICENSE | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/DEPS b/DEPS index 09609aad8c8d1..ab210439de498 100644 --- a/DEPS +++ b/DEPS @@ -62,7 +62,7 @@ vars = { # Dart is: https://github.com/dart-lang/sdk/blob/main/DEPS # You can use //tools/dart/create_updated_flutter_deps.py to produce # updated revision list of existing dependencies. - 'dart_revision': 'c479735adcf91684b66e94747199e3c1b17516b9', + 'dart_revision': '2876f5684ceddf924aa0e2ab7d34a6baf4717496', # WARNING: DO NOT EDIT MANUALLY # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py diff --git a/ci/licenses_golden/licenses_dart b/ci/licenses_golden/licenses_dart index 7788e3f51364f..8054ae04fd7c1 100644 --- a/ci/licenses_golden/licenses_dart +++ b/ci/licenses_golden/licenses_dart @@ -1,4 +1,4 @@ -Signature: aee8d76d23570c7efd9439c5f991502a +Signature: 7fd3eb25284e36712433374678f727d7 ==================================================================================================== LIBRARY: dart @@ -3728,7 +3728,6 @@ ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/class_id.dart + ../ ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/convert_patch.dart + ../../../third_party/dart/LICENSE ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/core_patch.dart + ../../../third_party/dart/LICENSE ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/deferred.dart + ../../../third_party/dart/LICENSE -ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/developer.dart + ../../../third_party/dart/LICENSE ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/double_patch.dart + ../../../third_party/dart/LICENSE ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/errors_patch.dart + ../../../third_party/dart/LICENSE ORIGIN: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/growable_list.dart + ../../../third_party/dart/LICENSE @@ -3804,7 +3803,6 @@ FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/class_id.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/convert_patch.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/core_patch.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/deferred.dart -FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/developer.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/double_patch.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/errors_patch.dart FILE: ../../../third_party/dart/sdk/lib/_internal/wasm/lib/growable_list.dart @@ -4753,7 +4751,7 @@ Exhibit B - "Incompatible With Secondary Licenses" Notice This Source Code Form is "Incompatible With Secondary Licenses", as defined by the Mozilla Public License, v. 2.0. -You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/e5e4aaceaf23200d0cf92ebed1506f915f364b4d +You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/2876f5684ceddf924aa0e2ab7d34a6baf4717496 /third_party/fallback_root_certificates/ ==================================================================================================== diff --git a/sky/packages/sky_engine/LICENSE b/sky/packages/sky_engine/LICENSE index 59552a3f1065a..b143690aed50a 100644 --- a/sky/packages/sky_engine/LICENSE +++ b/sky/packages/sky_engine/LICENSE @@ -31704,7 +31704,7 @@ Exhibit B - "Incompatible With Secondary Licenses" Notice This Source Code Form is "Incompatible With Secondary Licenses", as defined by the Mozilla Public License, v. 2.0. -You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/8a2ca37c39e2fec472edce1e39c5ea0f677bf7b0 +You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/2876f5684ceddf924aa0e2ab7d34a6baf4717496 /third_party/fallback_root_certificates/ -------------------------------------------------------------------------------- From b10cc4c8eeaf95175b86ba2246b55f233f1601d9 Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 26 Feb 2024 14:52:01 -0800 Subject: [PATCH 10/21] [Impeller] Fix a misspelling and name mismatch in a shader test fixture (#50983) --- impeller/fixtures/box_fade.frag | 6 +++--- impeller/fixtures/box_fade.vert | 4 ++-- impeller/fixtures/texture.frag | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/impeller/fixtures/box_fade.frag b/impeller/fixtures/box_fade.frag index 5acb1cba44f84..541096efd5135 100644 --- a/impeller/fixtures/box_fade.frag +++ b/impeller/fixtures/box_fade.frag @@ -9,7 +9,7 @@ uniform FrameInfo { } frame_info; -in vec2 interporlated_texture_coordinates; +in vec2 interpolated_texture_coordinates; out vec4 frag_color; @@ -17,8 +17,8 @@ uniform sampler2D contents1; uniform sampler2D contents2; void main() { - vec4 tex1 = texture(contents1, interporlated_texture_coordinates); - vec4 tex2 = texture(contents2, interporlated_texture_coordinates); + vec4 tex1 = texture(contents1, interpolated_texture_coordinates); + vec4 tex2 = texture(contents2, interpolated_texture_coordinates); frag_color = mix( tex1, tex2, clamp(frame_info.cursor_position.x / frame_info.window_size.x, 0.0, 1.0)); diff --git a/impeller/fixtures/box_fade.vert b/impeller/fixtures/box_fade.vert index 05f52f3c70aff..fd65238d83de7 100644 --- a/impeller/fixtures/box_fade.vert +++ b/impeller/fixtures/box_fade.vert @@ -10,9 +10,9 @@ uniform_buffer; in vec3 vertex_position; in vec2 texture_coordinates; -out vec2 interporlated_texture_coordinates; +out vec2 interpolated_texture_coordinates; void main() { gl_Position = uniform_buffer.mvp * vec4(vertex_position, 1.0); - interporlated_texture_coordinates = texture_coordinates; + interpolated_texture_coordinates = texture_coordinates; } diff --git a/impeller/fixtures/texture.frag b/impeller/fixtures/texture.frag index 0b7091261bc6a..1e95b0f8f8ade 100644 --- a/impeller/fixtures/texture.frag +++ b/impeller/fixtures/texture.frag @@ -2,12 +2,12 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -in vec2 interporlated_texture_coordinates; +in vec2 interpolated_texture_coordinates; out vec4 frag_color; uniform sampler2D texture_contents; void main() { - frag_color = texture(texture_contents, interporlated_texture_coordinates); + frag_color = texture(texture_contents, interpolated_texture_coordinates); } From 86f774237fa4034313aae039f4005ad4ef0aa398 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 26 Feb 2024 15:03:31 -0800 Subject: [PATCH 11/21] [scenario] trigger firstFrameLatch on exception. (#50981) If we hit this exception (which we seem to do a lot), then we weren't triggering the first frame latch. --- .../dev/flutter/scenarios/ExternalTextureFlutterActivity.java | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java index 7a30a865fc856..f06f75fcd801b 100644 --- a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java +++ b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java @@ -418,7 +418,6 @@ private void onImageAvailable(ImageReader reader) { // Simply log and return. Log.i(TAG, "Surface disconnected from ImageWriter", e); image.close(); - return; } Log.v(TAG, "Output image"); From 070290bbae0ad2b9124f65a2f52fb5797c4fa076 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 26 Feb 2024 15:29:32 -0800 Subject: [PATCH 12/21] Refactor args parsing/environment constructor for `scenario_app` (#50980) This moves the ever-growing amount of options and defaults into it's own class(es). The test runner itself has no tests (yet), but this shim will make writing tests easier. I tried to make no other real changes to how the runner functions in this PR. --- testing/scenario_app/bin/README.md | 55 ++-- .../scenario_app/bin/run_android_tests.dart | 229 ++++---------- .../scenario_app/bin/utils/environment.dart | 45 +++ testing/scenario_app/bin/utils/options.dart | 281 ++++++++++++++++++ testing/scenario_app/pubspec.yaml | 1 + 5 files changed, 423 insertions(+), 188 deletions(-) create mode 100644 testing/scenario_app/bin/utils/environment.dart create mode 100644 testing/scenario_app/bin/utils/options.dart diff --git a/testing/scenario_app/bin/README.md b/testing/scenario_app/bin/README.md index 666f83c892d5a..698ea20a63139 100644 --- a/testing/scenario_app/bin/README.md +++ b/testing/scenario_app/bin/README.md @@ -24,23 +24,40 @@ dart bin/android_integration_tests.dart --smoke-test dev.flutter.scenarios.Engin ## Additional arguments -- `--verbose`: Print additional information about the test run. - -- `--adb`: The path to the `adb` tool. Defaults to - `third_party/android_tools/sdk/platform-tools/adb`. - -- `--out-dir`: The directory containing the build artifacts. Defaults to the - last updated build directory in `out/` that starts with `android_`. - -- `--logs-dir`: The directory to store logs and screenshots. Defaults to - `FLUTTER_LOGS_DIR` if set, or `out/.../scenario_app/logs` otherwise. - -- `--use-skia-gold`: Use Skia Gold to compare screenshots. Defaults to true - when running on CI, and false otherwise (i.e. when running locally). If - set to true, `isSkiaGoldClientAvailable` must be true. - -- `--enable-impeller`: Enable Impeller for the Android app. Defaults to - false, which means that the app will use Skia as the graphics backend. +```txt +-v, --verbose Enable verbose logging +-h, --help Print usage information + --[no-]enable-impeller Whether to enable Impeller as the graphics backend. If true, the + test runner will use --impeller-backend if set, otherwise the + default backend will be used. To explicitly run with the Skia + backend, set this to false (--no-enable-impeller). + --impeller-backend The graphics backend to use when --enable-impeller is true. Unlike + the similar option when launching an app, there is no fallback; + that is, either Vulkan or OpenGLES must be specified. + [vulkan (default), opengles] + --logs-dir Path to a directory where logs and screenshots are stored. + --out-dir= Path to a out/{variant} directory where the APKs are built. + Defaults to the latest updated out/ directory that starts with + "android_" if the current working directory is within the engine + repository. + --smoke-test= Fully qualified class name of a single test to run. For example try + "dev.flutter.scenarios.EngineLaunchE2ETest" or + "dev.flutter.scenariosui.ExternalTextureTests". + --output-contents-golden= Path to a file that contains the expected filenames of golden + files. If the current working directory is within the engine + repository, defaults to + ./testing/scenario_app/android/expected_golden_output.txt. +``` -- `--impeller-backend`: The Impeller backend to use for the Android app. - Defaults to 'vulkan'. Only used when `--enable-impeller` is set to true. +## Advanced usage + +```txt + --[no-]use-skia-gold Whether to use Skia Gold to compare screenshots. Defaults to true + on CI and false otherwise. + --adb= Path to the Android Debug Bridge (adb) executable. If the current + working directory is within the engine repository, defaults to + ./third_party/android_tools/sdk/platform-tools/adb. + --ndk-stack= Path to the NDK stack tool. Defaults to the checked-in version in + third_party/android_tools if the current working directory is + within the engine repository on a supported platform. +``` diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart index 436cb421424ee..6a6ed8e9bb659 100644 --- a/testing/scenario_app/bin/run_android_tests.dart +++ b/testing/scenario_app/bin/run_android_tests.dart @@ -7,7 +7,6 @@ import 'dart:convert'; import 'dart:io'; import 'dart:typed_data'; -import 'package:args/args.dart'; import 'package:dir_contents_diff/dir_contents_diff.dart' show dirContentsDiff; import 'package:engine_repo_tools/engine_repo_tools.dart'; import 'package:path/path.dart'; @@ -15,180 +14,73 @@ import 'package:process/process.dart'; import 'package:skia_gold_client/skia_gold_client.dart'; import 'utils/adb_logcat_filtering.dart'; +import 'utils/environment.dart'; import 'utils/logs.dart'; +import 'utils/options.dart'; import 'utils/process_manager_extension.dart'; import 'utils/screenshot_transformer.dart'; -void _withTemporaryCwd(String path, void Function() callback) { - final String originalCwd = Directory.current.path; - Directory.current = Directory(path).parent.path; +// If you update the arguments, update the documentation in the README.md file. +void main(List args) async { + // Get some basic environment information to guide the rest of the program. + final Environment environment = Environment( + isCi: Platform.environment['LUCI_CONTEXT'] != null, + showVerbose: Options.showVerbose(args), + logsDir: Platform.environment['FLUTTER_LOGS_DIR'], + ); - try { - callback(); - } finally { - Directory.current = originalCwd; + // Determine if the CWD is within an engine checkout. + final Engine? localEngineDir = Engine.tryFindWithin(); + + // Show usage if requested. + if (Options.showUsage(args)) { + stdout.writeln(Options.usage( + environment: environment, + localEngineDir: localEngineDir, + )); + return; } -} -// If you update the arguments, update the documentation in the README.md file. -void main(List args) async { - final Engine? engine = Engine.tryFindWithin(); - final ArgParser parser = ArgParser() - ..addFlag( - 'help', - help: 'Prints usage information', - negatable: false, - ) - ..addFlag( - 'verbose', - help: 'Prints verbose output', - negatable: false, - ) - ..addOption( - 'adb', - help: 'Path to the adb tool', - defaultsTo: engine != null - ? join( - engine.srcDir.path, - 'third_party', - 'android_tools', - 'sdk', - 'platform-tools', - 'adb', - ) - : null, - ) - ..addOption( - 'ndk-stack', - help: 'Path to the ndk-stack tool', - defaultsTo: engine != null - ? join( - engine.srcDir.path, - 'third_party', - 'android_tools', - 'ndk', - 'prebuilt', - () { - if (Platform.isLinux) { - return 'linux-x86_64'; - } else if (Platform.isMacOS) { - return 'darwin-x86_64'; - } else if (Platform.isWindows) { - return 'windows-x86_64'; - } else { - throw UnsupportedError('Unsupported platform: ${Platform.operatingSystem}'); - } - }(), - 'bin', - 'ndk-stack', - ) - : null, - ) - ..addOption( - 'out-dir', - help: 'Out directory', - defaultsTo: engine - ?.outputs() - .where((Output o) => basename(o.path.path).startsWith('android_')) - .firstOrNull - ?.path - .path, - ) - ..addOption( - 'smoke-test', - help: 'Runs a single test to verify the setup', - ) - ..addFlag( - 'use-skia-gold', - help: 'Use Skia Gold to compare screenshots.', - defaultsTo: isLuciEnv, - ) - ..addFlag( - 'enable-impeller', - help: 'Enable Impeller for the Android app.', - ) - ..addOption( - 'output-contents-golden', - help: 'Path to a file that contains the expected filenames of golden files.', - defaultsTo: engine != null - ? join( - engine.srcDir.path, - 'flutter', - 'testing', - 'scenario_app', - 'android', - 'expected_golden_output.txt', - ) - : null, - ) - ..addOption( - 'impeller-backend', - help: 'The Impeller backend to use for the Android app.', - allowed: ['vulkan', 'opengles'], - defaultsTo: 'vulkan', - ) - ..addOption( - 'logs-dir', - help: 'The directory to store the logs and screenshots. Defaults to ' - 'the value of the FLUTTER_LOGS_DIR environment variable, if set, ' - 'otherwise it defaults to a path within out-dir.', - defaultsTo: Platform.environment['FLUTTER_LOGS_DIR'], + // Parse the command line arguments. + final Options options; + try { + options = Options.parse( + args, + environment: environment, + localEngine: localEngineDir, ); + } on FormatException catch (error) { + stderr.writeln(error); + stderr.writeln(Options.usage( + environment: environment, + localEngineDir: localEngineDir, + )); + exitCode = 1; + return; + } runZonedGuarded( () async { - final ArgResults results = parser.parse(args); - if (results['help'] as bool) { - stdout.writeln(parser.usage); - return; - } - - if (results['out-dir'] == null) { - panic(['--out-dir is required']); - } - if (results['adb'] == null) { - panic(['--adb is required']); - } - - final bool verbose = results['verbose'] as bool; - final Directory outDir = Directory(results['out-dir'] as String); - final File adb = File(results['adb'] as String); - final bool useSkiaGold = results['use-skia-gold'] as bool; - final String? smokeTest = results['smoke-test'] as String?; - final bool enableImpeller = results['enable-impeller'] as bool; - final String? contentsGolden = results['output-contents-golden'] as String?; - final _ImpellerBackend? impellerBackend = _ImpellerBackend.tryParse(results['impeller-backend'] as String?); - if (enableImpeller && impellerBackend == null) { - panic([ - 'invalid graphics-backend', - results['impeller-backend'] as String? ?? '' - ]); - } - final Directory logsDir = Directory(results['logs-dir'] as String? ?? join(outDir.path, 'scenario_app', 'logs')); - final String? ndkStack = results['ndk-stack'] as String?; - if (ndkStack == null) { - panic(['--ndk-stack is required']); - } await _run( - verbose: verbose, - outDir: outDir, - adb: adb, - smokeTestFullPath: smokeTest, - useSkiaGold: useSkiaGold, - enableImpeller: enableImpeller, - impellerBackend: impellerBackend, - logsDir: logsDir, - contentsGolden: contentsGolden, - ndkStack: ndkStack, + verbose: options.verbose, + outDir: Directory(options.outDir), + adb: File(options.adb), + smokeTestFullPath: options.smokeTest, + useSkiaGold: options.useSkiaGold, + enableImpeller: options.enableImpeller, + impellerBackend: _ImpellerBackend.tryParse(options.impellerBackend), + logsDir: Directory(options.logsDir), + contentsGolden: options.outputContentsGolden, + ndkStack: options.ndkStack, ); exit(0); }, (Object error, StackTrace stackTrace) { if (error is! Panic) { - stderr.writeln(error); + stderr.writeln('Unhandled error: $error'); stderr.writeln(stackTrace); } - exit(1); + exitCode = 1; }, ); } @@ -222,18 +114,6 @@ Future _run({ required String ndkStack, }) async { const ProcessManager pm = LocalProcessManager(); - - if (!outDir.existsSync()) { - panic([ - 'out-dir does not exist: $outDir', - 'make sure to build the selected engine variant' - ]); - } - - if (!adb.existsSync()) { - panic(['cannot find adb: $adb', 'make sure to run gclient sync']); - } - final String scenarioAppPath = join(outDir.path, 'scenario_app'); final String logcatPath = join(logsDir.path, 'logcat.txt'); @@ -521,7 +401,7 @@ Future _run({ // TODO(matanlurey): Resolve this in a better way. On CI this file always exists. File(join(screenshotPath, 'noop.txt')).writeAsStringSync(''); // TODO(gaaclarke): We should move this into dir_contents_diff. - _withTemporaryCwd(contentsGolden, () { + _withTemporaryCwd(dirname(contentsGolden), () { final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath); if (exitCode != 0) { panic(['Output contents incorrect.']); @@ -531,3 +411,14 @@ Future _run({ } } } + +void _withTemporaryCwd(String path, void Function() callback) { + final String originalCwd = Directory.current.path; + Directory.current = Directory(path).path; + + try { + callback(); + } finally { + Directory.current = originalCwd; + } +} diff --git a/testing/scenario_app/bin/utils/environment.dart b/testing/scenario_app/bin/utils/environment.dart new file mode 100644 index 0000000000000..9ad5561f83e24 --- /dev/null +++ b/testing/scenario_app/bin/utils/environment.dart @@ -0,0 +1,45 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:meta/meta.dart'; + +/// An overridable collection of values provided by the environment. +@immutable +final class Environment { + /// Creates a new environment from the given values. + const Environment({ + required this.isCi, + required this.showVerbose, + required this.logsDir, + }); + + /// Whether the current program is running on a CI environment. + /// + /// Useful for determining if certain features should be enabled or disabled + /// based on the environment, or to add safety checks (for example, using + /// confusing or ambiguous flags). + final bool isCi; + + /// Whether the user has requested verbose logging and program output. + final bool showVerbose; + + /// What directory to store logs and screenshots in. + final String? logsDir; + + @override + bool operator ==(Object o) { + return o is Environment && + o.isCi == isCi && + o.showVerbose == showVerbose && + o.logsDir == logsDir; + } + + @override + int get hashCode => Object.hash(isCi, showVerbose, logsDir); + + @override + String toString() { + return 'Environment(isCi: $isCi, showVerbose: $showVerbose, logsDir: $logsDir)'; + } +} diff --git a/testing/scenario_app/bin/utils/options.dart b/testing/scenario_app/bin/utils/options.dart new file mode 100644 index 0000000000000..057256afe1cac --- /dev/null +++ b/testing/scenario_app/bin/utils/options.dart @@ -0,0 +1,281 @@ +import 'dart:io' as io; + +import 'package:args/args.dart'; +import 'package:engine_repo_tools/engine_repo_tools.dart'; +import 'package:path/path.dart' as p; + +import 'environment.dart'; + +/// Command line options and parser for the Android `scenario_app` test runner. +extension type const Options._(ArgResults _args) { + /// Parses the command line [args] into a set of options. + /// + /// Throws a [FormatException] if command line arguments are invalid. + factory Options.parse( + List args, { + required Environment environment, + required Engine? localEngine, + }) { + final ArgResults results = _parser(environment, localEngine).parse(args); + final Options options = Options._(results); + + // The 'adb' tool must exist. + if (results['adb'] == null) { + throw const FormatException('The --adb option must be set.'); + } else if (!io.File(options.adb).existsSync()) { + throw FormatException( + 'The adb tool does not exist at ${options.adb}.', + ); + } + + // The 'ndk-stack' tool must exist. + if (results['ndk-stack'] == null) { + throw const FormatException('The --ndk-stack option must be set.'); + } else if (!io.File(options.ndkStack).existsSync()) { + throw FormatException( + 'The ndk-stack tool does not exist at ${options.ndkStack}.', + ); + } + + // The 'out-dir' must exist. + if (results['out-dir'] == null) { + throw const FormatException('The --out-dir option must be set.'); + } else if (!io.Directory(options.outDir).existsSync()) { + throw FormatException( + 'The out directory does not exist at ${options.outDir}.', + ); + } + + return options; + } + + /// Whether usage information should be shown based on command line [args]. + /// + /// This is a shortcut that can be used to determine if the usage information + /// before parsing the remaining command line arguments. For example: + /// + /// ``` + /// void main(List args) { + /// if (Options.showUsage(args)) { + /// stdout.writeln(Options.usage); + /// return; + /// } + /// final options = Options.parse(args); + /// // ... + /// } + /// ``` + static bool showUsage(List args) { + // If any of the arguments are '--help' or -'h'. + return args.isNotEmpty && args.any((String arg) { + return arg == '--help' || arg == '-h'; + }); + } + + /// Whether verbose logging should be enabled based on command line [args]. + /// + /// This is a shortcut that can be used to determine if verbose logging should + /// be enabled before parsing the remaining command line arguments. For + /// example: + /// + /// ``` + /// void main(List args) { + /// final bool verbose = Options.showVerbose(args); + /// // ... + /// } + /// ``` + static bool showVerbose(List args) { + // If any of the arguments are '--verbose' or -'v'. + return args.isNotEmpty && args.any((String arg) { + return arg == '--verbose' || arg == '-v'; + }); + } + + /// Returns usage information for the `scenario_app` test runner. + /// + /// If [verbose] is `true`, then additional options are shown. + static String usage({ + required Environment environment, + required Engine? localEngineDir, + }) { + return _parser(environment, localEngineDir).usage; + } + + /// Parses the command line [args] into a set of options. + /// + /// Unlike [_miniParser], this parser includes all options. + static ArgParser _parser(Environment environment, Engine? localEngine) { + final bool hideUnusualOptions = !environment.showVerbose; + return ArgParser(usageLineLength: 120) + ..addFlag( + 'verbose', + abbr: 'v', + help: 'Enable verbose logging', + negatable: false, + ) + ..addFlag( + 'help', + abbr: 'h', + help: 'Print usage information', + negatable: false, + ) + ..addFlag( + 'use-skia-gold', + help: + 'Whether to use Skia Gold to compare screenshots. Defaults to true ' + 'on CI and false otherwise.', + defaultsTo: environment.isCi, + hide: hideUnusualOptions, + ) + ..addFlag( + 'enable-impeller', + help: + 'Whether to enable Impeller as the graphics backend. If true, the ' + 'test runner will use --impeller-backend if set, otherwise the ' + 'default backend will be used. To explicitly run with the Skia ' + 'backend, set this to false (--no-enable-impeller).', + ) + ..addOption( + 'impeller-backend', + help: 'The graphics backend to use when --enable-impeller is true. ' + 'Unlike the similar option when launching an app, there is no ' + 'fallback; that is, either Vulkan or OpenGLES must be specified. ', + allowed: ['vulkan', 'opengles'], + defaultsTo: 'vulkan', + ) + ..addOption( + 'logs-dir', + help: 'Path to a directory where logs and screenshots are stored.', + defaultsTo: environment.logsDir, + ) + ..addOption( + 'adb', + help: 'Path to the Android Debug Bridge (adb) executable. ' + 'If the current working directory is within the engine repository, ' + 'defaults to ./third_party/android_tools/sdk/platform-tools/adb.', + defaultsTo: localEngine != null + ? p.join( + localEngine.srcDir.path, + 'third_party', + 'android_tools', + 'sdk', + 'platform-tools', + 'adb', + ) + : null, + valueHelp: 'path/to/adb', + hide: hideUnusualOptions, + ) + ..addOption( + 'ndk-stack', + help: + 'Path to the NDK stack tool. Defaults to the checked-in version in ' + 'third_party/android_tools if the current working directory is ' + 'within the engine repository on a supported platform.', + defaultsTo: localEngine != null && + (io.Platform.isLinux || + io.Platform.isMacOS || + io.Platform.isWindows) + ? p.join( + localEngine.srcDir.path, + 'third_party', + 'android_tools', + 'ndk', + 'prebuilt', + () { + if (io.Platform.isLinux) { + return 'linux-x86_64'; + } else if (io.Platform.isMacOS) { + return 'darwin-x86_64'; + } else if (io.Platform.isWindows) { + return 'windows-x86_64'; + } else { + // Unreachable. + throw UnsupportedError( + 'Unsupported platform: ${io.Platform.operatingSystem}', + ); + } + }(), + 'bin', + 'ndk-stack', + ) + : null, + valueHelp: 'path/to/ndk-stack', + hide: hideUnusualOptions, + ) + ..addOption( + 'out-dir', + help: 'Path to a out/{variant} directory where the APKs are built. ' + 'Defaults to the latest updated out/ directory that starts with ' + '"android_" if the current working directory is within the engine ' + 'repository.', + defaultsTo: environment.isCi ? null : localEngine + ?.outputs() + .where((Output o) => p.basename(o.path.path).startsWith('android_')) + .firstOrNull + ?.path + .path, + mandatory: environment.isCi, + valueHelp: 'path/to/out/android_variant', + ) + ..addOption( + 'smoke-test', + help: 'Fully qualified class name of a single test to run. For example ' + 'try "dev.flutter.scenarios.EngineLaunchE2ETest" or ' + '"dev.flutter.scenariosui.ExternalTextureTests".', + valueHelp: 'package.ClassName', + ) + ..addOption( + 'output-contents-golden', + help: 'Path to a file that contains the expected filenames of golden ' + 'files. If the current working directory is within the engine ' + 'repository, defaults to ./testing/scenario_app/android/' + 'expected_golden_output.txt.', + defaultsTo: localEngine != null + ? p.join( + localEngine.flutterDir.path, + 'testing', + 'scenario_app', + 'android', + 'expected_golden_output.txt', + ) + : null, + valueHelp: 'path/to/golden.txt', + ); + } + + /// Whether verbose logging should be enabled. + bool get verbose => _args['verbose'] as bool; + + /// Whether usage information should be shown. + bool get help => _args['help'] as bool; + + /// Whether to use Skia Gold to compare screenshots. + bool get useSkiaGold => _args['use-skia-gold'] as bool; + + /// Whether to enable Impeller as the graphics backend. + bool get enableImpeller => _args['enable-impeller'] as bool; + + /// The graphics backend to use when --enable-impeller is true. + String get impellerBackend => _args['impeller-backend'] as String; + + /// Path to a directory where logs and screenshots are stored. + String get logsDir { + final String? logsDir = _args['logs-dir'] as String?; + return logsDir ?? p.join(outDir, 'logs'); + } + + /// Path to the Android Debug Bridge (adb) executable. + String get adb => _args['adb'] as String; + + /// Path to the NDK stack tool. + String get ndkStack => _args['ndk-stack'] as String; + + /// Path to a out/{variant} directory where the APKs are built. + String get outDir => _args['out-dir'] as String; + + /// Fully qualified class name of a single test to run. + String? get smokeTest => _args['smoke-test'] as String?; + + /// Path to a file that contains the expected filenames of golden files. + String? get outputContentsGolden => _args['output-contents-golden'] as String; +} diff --git a/testing/scenario_app/pubspec.yaml b/testing/scenario_app/pubspec.yaml index 039ce83e22854..f42e15868a9b2 100644 --- a/testing/scenario_app/pubspec.yaml +++ b/testing/scenario_app/pubspec.yaml @@ -18,6 +18,7 @@ dependencies: args: any dir_contents_diff: any engine_repo_tools: any + meta: any path: any process: any sky_engine: any From 924d5db87d4fdcec102440cdf108e2b386d2b654 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 26 Feb 2024 16:58:22 -0800 Subject: [PATCH 13/21] Revert "Reland 4: Multiview pipeline (#50931)" (#50985) This reverts commit 81035b7d56efd2d2b6c1ebd99d7cbe72cc683f90. Reason: Internal test failure blocking roll OCL:610420483:BASE:610486571:1708978396098:f2c3c31 [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style --- flow/frame_timings.cc | 28 +---- flow/frame_timings.h | 3 - lib/ui/painting/image_dispose_unittests.cc | 10 +- lib/ui/window/platform_configuration.cc | 5 +- lib/ui/window/platform_configuration.h | 5 +- runtime/runtime_controller.cc | 10 +- runtime/runtime_controller.h | 5 +- runtime/runtime_delegate.h | 3 +- shell/common/animator.cc | 74 +++++------ shell/common/animator.h | 10 +- shell/common/animator_unittests.cc | 37 +++--- shell/common/engine.cc | 5 +- shell/common/engine.h | 3 +- shell/common/engine_unittests.cc | 136 +-------------------- shell/common/fixtures/shell_test.dart | 53 -------- shell/common/input_events_unittests.cc | 10 +- shell/common/rasterizer.cc | 1 - shell/common/shell_test.cc | 103 ++++++---------- shell/common/shell_test.h | 50 +++----- shell/common/shell_unittests.cc | 80 ++++-------- testing/dart/platform_view_test.dart | 10 +- 21 files changed, 153 insertions(+), 488 deletions(-) diff --git a/flow/frame_timings.cc b/flow/frame_timings.cc index 07a398a81c42e..339374d77b837 100644 --- a/flow/frame_timings.cc +++ b/flow/frame_timings.cc @@ -13,31 +13,6 @@ namespace flutter { -namespace { - -const char* StateToString(FrameTimingsRecorder::State state) { -#ifndef NDEBUG - switch (state) { - case FrameTimingsRecorder::State::kUninitialized: - return "kUninitialized"; - case FrameTimingsRecorder::State::kVsync: - return "kVsync"; - case FrameTimingsRecorder::State::kBuildStart: - return "kBuildStart"; - case FrameTimingsRecorder::State::kBuildEnd: - return "kBuildEnd"; - case FrameTimingsRecorder::State::kRasterStart: - return "kRasterStart"; - case FrameTimingsRecorder::State::kRasterEnd: - return "kRasterEnd"; - }; - FML_UNREACHABLE(); -#endif - return ""; -} - -} // namespace - std::atomic FrameTimingsRecorder::frame_number_gen_ = {1}; FrameTimingsRecorder::FrameTimingsRecorder() @@ -280,8 +255,7 @@ const char* FrameTimingsRecorder::GetFrameNumberTraceArg() const { } void FrameTimingsRecorder::AssertInState(State state) const { - FML_DCHECK(state_ == state) << "Expected state " << StateToString(state) - << ", actual state " << StateToString(state_); + FML_DCHECK(state_ == state); } } // namespace flutter diff --git a/flow/frame_timings.h b/flow/frame_timings.h index ccccd89dddb88..c81bcc3362298 100644 --- a/flow/frame_timings.h +++ b/flow/frame_timings.h @@ -31,7 +31,6 @@ class FrameTimingsRecorder { public: /// Various states that the recorder can be in. When created the recorder is /// in an unitialized state and transtions in sequential order of the states. - // After adding an item to this enum, modify StateToString accordingly. enum class State : uint32_t { kUninitialized, kVsync, @@ -122,8 +121,6 @@ class FrameTimingsRecorder { /// /// Instead of adding a `GetState` method and asserting on the result, this /// method prevents other logic from relying on the state. - /// - /// In release builds, this call is a no-op. void AssertInState(State state) const; private: diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 93600ca83c93d..0f8bb6d027062 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -5,7 +5,6 @@ #define FML_USED_ON_EMBEDDER #include "flutter/common/task_runners.h" -#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/fml/synchronization/waitable_event.h" #include "flutter/lib/ui/painting/canvas.h" #include "flutter/lib/ui/painting/image.h" @@ -58,10 +57,6 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) { }; Settings settings = CreateSettingsForFixture(); - fml::CountDownLatch frame_latch{2}; - settings.frame_rasterized_callback = [&frame_latch](const FrameTiming& t) { - frame_latch.CountDown(); - }; auto task_runner = CreateNewThread(); TaskRunners task_runners("test", // label GetCurrentTaskRunner(), // platform @@ -88,15 +83,12 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrameAndDisposePictureAndLayer) { shell->RunEngine(std::move(configuration), [&](auto result) { ASSERT_EQ(result, Engine::RunStatus::Success); }); + message_latch_.Wait(); ASSERT_TRUE(current_display_list_); ASSERT_TRUE(current_image_); - // Wait for 2 frames to be rasterized. The 2nd frame releases resources of the - // 1st frame. - frame_latch.Wait(); - // Force a drain the SkiaUnrefQueue. The engine does this normally as frames // pump, but we force it here to make the test more deterministic. message_latch_.Reset(); diff --git a/lib/ui/window/platform_configuration.cc b/lib/ui/window/platform_configuration.cc index 45290d3a28cc7..5e325fbdf4b10 100644 --- a/lib/ui/window/platform_configuration.cc +++ b/lib/ui/window/platform_configuration.cc @@ -453,9 +453,12 @@ void PlatformConfigurationNativeApi::Render(int64_t view_id, Scene* scene, double width, double height) { + // TODO(dkwingsmt): Currently only supports a single window. + // See https://github.com/flutter/flutter/issues/135530, item 2. + FML_DCHECK(view_id == kFlutterImplicitViewId); UIDartState::ThrowIfUIOperationsProhibited(); UIDartState::Current()->platform_configuration()->client()->Render( - view_id, scene, width, height); + scene, width, height); } void PlatformConfigurationNativeApi::SetNeedsReportTimings(bool value) { diff --git a/lib/ui/window/platform_configuration.h b/lib/ui/window/platform_configuration.h index 2517e980f18cc..8914bbb756d40 100644 --- a/lib/ui/window/platform_configuration.h +++ b/lib/ui/window/platform_configuration.h @@ -76,10 +76,7 @@ class PlatformConfigurationClient { /// @brief Updates the client's rendering on the GPU with the newly /// provided Scene. /// - virtual void Render(int64_t view_id, - Scene* scene, - double width, - double height) = 0; + virtual void Render(Scene* scene, double width, double height) = 0; //-------------------------------------------------------------------------- /// @brief Receives an updated semantics tree from the Framework. diff --git a/runtime/runtime_controller.cc b/runtime/runtime_controller.cc index 422b94a1ba8b3..85a3bd9efaac5 100644 --- a/runtime/runtime_controller.cc +++ b/runtime/runtime_controller.cc @@ -340,21 +340,21 @@ void RuntimeController::ScheduleFrame() { client_.ScheduleFrame(); } +// |PlatformConfigurationClient| void RuntimeController::EndWarmUpFrame() { client_.EndWarmUpFrame(); } // |PlatformConfigurationClient| -void RuntimeController::Render(int64_t view_id, - Scene* scene, - double width, - double height) { +void RuntimeController::Render(Scene* scene, double width, double height) { + // TODO(dkwingsmt): Currently only supports a single window. + int64_t view_id = kFlutterImplicitViewId; const ViewportMetrics* view_metrics = UIDartState::Current()->platform_configuration()->GetMetrics(view_id); if (view_metrics == nullptr) { return; } - client_.Render(view_id, scene->takeLayerTree(width, height), + client_.Render(scene->takeLayerTree(width, height), view_metrics->device_pixel_ratio); } diff --git a/runtime/runtime_controller.h b/runtime/runtime_controller.h index 738ae24fd2818..b68750df8751e 100644 --- a/runtime/runtime_controller.h +++ b/runtime/runtime_controller.h @@ -661,10 +661,7 @@ class RuntimeController : public PlatformConfigurationClient { void EndWarmUpFrame() override; // |PlatformConfigurationClient| - void Render(int64_t view_id, - Scene* scene, - double width, - double height) override; + void Render(Scene* scene, double width, double height) override; // |PlatformConfigurationClient| void UpdateSemantics(SemanticsUpdate* update) override; diff --git a/runtime/runtime_delegate.h b/runtime/runtime_delegate.h index 02c636a5e981b..6d3707d27737a 100644 --- a/runtime/runtime_delegate.h +++ b/runtime/runtime_delegate.h @@ -27,8 +27,7 @@ class RuntimeDelegate { virtual void EndWarmUpFrame() = 0; - virtual void Render(int64_t view_id, - std::unique_ptr layer_tree, + virtual void Render(std::unique_ptr layer_tree, float device_pixel_ratio) = 0; virtual void UpdateSemantics(SemanticsNodeUpdates update, diff --git a/shell/common/animator.cc b/shell/common/animator.cc index dbcf6e22eadd4..ad3cea4d1adf2 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -62,10 +62,6 @@ void Animator::BeginFrame( std::unique_ptr frame_timings_recorder) { TRACE_EVENT_ASYNC_END0("flutter", "Frame Request Pending", frame_request_number_); - // Clear layer trees rendered out of a frame. Only Animator::Render called - // within a frame is used. - layer_trees_tasks_.clear(); - frame_request_number_++; frame_timings_recorder_ = std::move(frame_timings_recorder); @@ -116,33 +112,6 @@ void Animator::BeginFrame( dart_frame_deadline_ = frame_target_time.ToEpochDelta(); uint64_t frame_number = frame_timings_recorder_->GetFrameNumber(); delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number); -} - -void Animator::EndFrame() { - FML_DCHECK(frame_timings_recorder_ != nullptr); - if (!layer_trees_tasks_.empty()) { - // The build is completed in OnAnimatorBeginFrame. - frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now()); - - delegate_.OnAnimatorUpdateLatestFrameTargetTime( - frame_timings_recorder_->GetVsyncTargetTime()); - - // Commit the pending continuation. - PipelineProduceResult result = - producer_continuation_.Complete(std::make_unique( - std::move(layer_trees_tasks_), std::move(frame_timings_recorder_))); - - if (!result.success) { - FML_DLOG(INFO) << "Failed to commit to the pipeline"; - } else if (!result.is_first_item) { - // Do nothing. It has been successfully pushed to the pipeline but not as - // the first item. Eventually the 'Rasterizer' will consume it, so we - // don't need to notify the delegate. - } else { - delegate_.OnAnimatorDraw(layer_tree_pipeline_); - } - } - frame_timings_recorder_ = nullptr; if (!frame_scheduled_ && has_rendered_) { // Wait a tad more than 3 60hz frames before reporting a big idle period. @@ -170,18 +139,14 @@ void Animator::EndFrame() { }, kNotifyIdleTaskWaitTime); } - FML_DCHECK(layer_trees_tasks_.empty()); - FML_DCHECK(frame_timings_recorder_ == nullptr); } -void Animator::Render(int64_t view_id, - std::unique_ptr layer_tree, +void Animator::Render(std::unique_ptr layer_tree, float device_pixel_ratio) { has_rendered_ = true; if (!frame_timings_recorder_) { - // Framework can directly call render with a built scene. A major reason is - // to render warm up frames. + // Framework can directly call render with a built scene. frame_timings_recorder_ = std::make_unique(); const fml::TimePoint placeholder_time = fml::TimePoint::Now(); frame_timings_recorder_->RecordVsync(placeholder_time, placeholder_time); @@ -191,9 +156,35 @@ void Animator::Render(int64_t view_id, TRACE_EVENT_WITH_FRAME_NUMBER(frame_timings_recorder_, "flutter", "Animator::Render", /*flow_id_count=*/0, /*flow_ids=*/nullptr); + frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now()); + + delegate_.OnAnimatorUpdateLatestFrameTargetTime( + frame_timings_recorder_->GetVsyncTargetTime()); - layer_trees_tasks_.push_back(std::make_unique( + // TODO(dkwingsmt): Currently only supports a single window. + // See https://github.com/flutter/flutter/issues/135530, item 2. + int64_t view_id = kFlutterImplicitViewId; + std::vector> layer_trees_tasks; + layer_trees_tasks.push_back(std::make_unique( view_id, std::move(layer_tree), device_pixel_ratio)); + // Commit the pending continuation. + PipelineProduceResult result = + producer_continuation_.Complete(std::make_unique( + std::move(layer_trees_tasks), std::move(frame_timings_recorder_))); + + if (!result.success) { + FML_DLOG(INFO) << "No pending continuation to commit"; + return; + } + + if (!result.is_first_item) { + // It has been successfully pushed to the pipeline but not as the first + // item. Eventually the 'Rasterizer' will consume it, so we don't need to + // notify the delegate. + return; + } + + delegate_.OnAnimatorDraw(layer_tree_pipeline_); } const std::weak_ptr Animator::GetVsyncWaiter() const { @@ -265,7 +256,6 @@ void Animator::AwaitVSync() { self->DrawLastLayerTrees(std::move(frame_timings_recorder)); } else { self->BeginFrame(std::move(frame_timings_recorder)); - self->EndFrame(); } } }); @@ -275,9 +265,9 @@ void Animator::AwaitVSync() { } void Animator::EndWarmUpFrame() { - if (!layer_trees_tasks_.empty()) { - EndFrame(); - } + // Do nothing. The warm up frame does not need any additional work to end the + // frame for now. This will change once the pipeline supports multi-view. + // https://github.com/flutter/flutter/issues/142851 } void Animator::ScheduleSecondaryVsyncCallback(uintptr_t id, diff --git a/shell/common/animator.h b/shell/common/animator.h index e76c7b8267484..51bea1d273871 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -76,8 +76,7 @@ class Animator final { /// technically, between Animator::BeginFrame and Animator::EndFrame /// (both private methods). Otherwise, this call will be ignored. /// - void Render(int64_t view_id, - std::unique_ptr layer_tree, + void Render(std::unique_ptr layer_tree, float device_pixel_ratio); const std::weak_ptr GetVsyncWaiter() const; @@ -106,13 +105,7 @@ class Animator final { void EnqueueTraceFlowId(uint64_t trace_flow_id); private: - // Animator's work during a vsync is split into two methods, BeginFrame and - // EndFrame. The two methods should be called synchronously back-to-back to - // avoid being interrupted by a regular vsync. The reason to split them is to - // allow ShellTest::PumpOneFrame to insert a Render in between. - void BeginFrame(std::unique_ptr frame_timings_recorder); - void EndFrame(); bool CanReuseLastLayerTrees(); @@ -129,7 +122,6 @@ class Animator final { std::shared_ptr waiter_; std::unique_ptr frame_timings_recorder_; - std::vector> layer_trees_tasks_; uint64_t frame_request_number_ = 1; fml::TimeDelta dart_frame_deadline_; std::shared_ptr layer_tree_pipeline_; diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index a1f96e7992376..19a851a864c74 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -23,8 +23,6 @@ namespace flutter { namespace testing { -constexpr int64_t kImplicitViewId = 0; - class FakeAnimatorDelegate : public Animator::Delegate { public: MOCK_METHOD(void, @@ -160,30 +158,20 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) { latch.Wait(); ASSERT_FALSE(delegate.notify_idle_called_); - fml::AutoResetWaitableEvent render_latch; // Validate it has not notified idle and try to render. task_runners.GetUITaskRunner()->PostDelayedTask( [&] { ASSERT_FALSE(delegate.notify_idle_called_); - EXPECT_CALL(delegate, OnAnimatorBeginFrame).WillOnce([&] { - auto layer_tree = std::make_unique( - LayerTree::Config(), SkISize::Make(600, 800)); - animator->Render(kImplicitViewId, std::move(layer_tree), 1.0); - render_latch.Signal(); - }); - // Request a frame that builds a layer tree and renders a frame. - // When the frame is rendered, render_latch will be signaled. - animator->RequestFrame(true); + auto layer_tree = std::make_unique(LayerTree::Config(), + SkISize::Make(600, 800)); + animator->Render(std::move(layer_tree), 1.0); task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); }, // See kNotifyIdleTaskWaitTime in animator.cc. fml::TimeDelta::FromMilliseconds(60)); latch.Wait(); - render_latch.Wait(); - // A frame has been rendered, and the next frame request will notify idle. - // But at the moment there isn't another frame request, therefore it still - // hasn't notified idle. + // Still hasn't notified idle because there has been no frame request. task_runners.GetUITaskRunner()->PostTask([&] { ASSERT_FALSE(delegate.notify_idle_called_); // False to avoid getting cals to BeginFrame that will request more frames @@ -236,6 +224,11 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) { }); fml::AutoResetWaitableEvent begin_frame_latch; + EXPECT_CALL(delegate, OnAnimatorBeginFrame) + .WillRepeatedly( + [&](fml::TimePoint frame_target_time, uint64_t frame_number) { + begin_frame_latch.Signal(); + }); // It must always be called when the method 'Animator::Render' is called, // regardless of whether the pipeline is empty or not. EXPECT_CALL(delegate, OnAnimatorUpdateLatestFrameTargetTime).Times(2); @@ -246,16 +239,16 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) { for (int i = 0; i < 2; i++) { task_runners.GetUITaskRunner()->PostTask([&] { - EXPECT_CALL(delegate, OnAnimatorBeginFrame).WillOnce([&] { - auto layer_tree = std::make_unique(LayerTree::Config(), - SkISize::Make(600, 800)); - animator->Render(kImplicitViewId, std::move(layer_tree), 1.0); - begin_frame_latch.Signal(); - }); animator->RequestFrame(); task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); }); begin_frame_latch.Wait(); + + PostTaskSync(task_runners.GetUITaskRunner(), [&] { + auto layer_tree = std::make_unique(LayerTree::Config(), + SkISize::Make(600, 800)); + animator->Render(std::move(layer_tree), 1.0); + }); } PostTaskSync(task_runners.GetUITaskRunner(), [&] { animator.reset(); }); diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 8655e350013bf..1e845fdf3178b 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -468,8 +468,7 @@ void Engine::EndWarmUpFrame() { animator_->EndWarmUpFrame(); } -void Engine::Render(int64_t view_id, - std::unique_ptr layer_tree, +void Engine::Render(std::unique_ptr layer_tree, float device_pixel_ratio) { if (!layer_tree) { return; @@ -480,7 +479,7 @@ void Engine::Render(int64_t view_id, return; } - animator_->Render(view_id, std::move(layer_tree), device_pixel_ratio); + animator_->Render(std::move(layer_tree), device_pixel_ratio); } void Engine::UpdateSemantics(SemanticsNodeUpdates update, diff --git a/shell/common/engine.h b/shell/common/engine.h index 72dfef1a44dfd..050101773766f 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -966,8 +966,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { std::string DefaultRouteName() override; // |RuntimeDelegate| - void Render(int64_t view_id, - std::unique_ptr layer_tree, + void Render(std::unique_ptr layer_tree, float device_pixel_ratio) override; // |RuntimeDelegate| diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index 0998f7081f5a1..e5c7e99fb6d0d 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -29,10 +29,8 @@ namespace { using ::testing::Invoke; using ::testing::ReturnRef; -fml::AutoResetWaitableEvent native_latch; - -void PostSync(const fml::RefPtr& task_runner, - const fml::closure& task) { +static void PostSync(const fml::RefPtr& task_runner, + const fml::closure& task) { fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask(task_runner, [&latch, &task] { task(); @@ -123,7 +121,7 @@ class MockRuntimeDelegate : public RuntimeDelegate { MOCK_METHOD(void, EndWarmUpFrame, (), (override)); MOCK_METHOD(void, Render, - (int64_t, std::unique_ptr, float), + (std::unique_ptr, float), (override)); MOCK_METHOD(void, UpdateSemantics, @@ -619,66 +617,6 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { }); } -TEST_F(EngineTest, AnimatorAcceptsMultipleRenders) { - MockAnimatorDelegate animator_delegate; - std::unique_ptr engine_context; - - std::shared_ptr platform_message_handler = - std::make_shared(); - EXPECT_CALL(delegate_, GetPlatformMessageHandler) - .WillOnce(ReturnRef(platform_message_handler)); - fml::AutoResetWaitableEvent draw_latch; - EXPECT_CALL(animator_delegate, OnAnimatorDraw) - .WillOnce( - Invoke([&draw_latch](const std::shared_ptr& pipeline) { - auto status = - pipeline->Consume([&](std::unique_ptr item) { - EXPECT_EQ(item->layer_tree_tasks.size(), 2u); - EXPECT_EQ(item->layer_tree_tasks[0]->view_id, 1); - EXPECT_EQ(item->layer_tree_tasks[1]->view_id, 2); - }); - EXPECT_EQ(status, PipelineConsumeResult::Done); - draw_latch.Signal(); - })); - EXPECT_CALL(animator_delegate, OnAnimatorBeginFrame) - .WillOnce(Invoke([&engine_context](fml::TimePoint frame_target_time, - uint64_t frame_number) { - engine_context->EngineTaskSync([&](Engine& engine) { - engine.BeginFrame(frame_target_time, frame_number); - }); - })); - - native_latch.Reset(); - AddNativeCallback("NotifyNative", [](auto args) { native_latch.Signal(); }); - - std::unique_ptr animator; - PostSync(task_runners_.GetUITaskRunner(), - [&animator, &animator_delegate, &task_runners = task_runners_] { - animator = std::make_unique( - animator_delegate, task_runners, - static_cast>( - std::make_unique( - task_runners))); - }); - - engine_context = EngineContext::Create(delegate_, settings_, task_runners_, - std::move(animator)); - auto configuration = RunConfiguration::InferFromSettings(settings_); - configuration.SetEntrypoint("onDrawFrameRenderAllViews"); - engine_context->Run(std::move(configuration)); - - engine_context->EngineTaskSync([](Engine& engine) { - engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0}); - engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0}); - }); - - native_latch.Wait(); - - engine_context->EngineTaskSync( - [](Engine& engine) { engine.ScheduleFrame(); }); - draw_latch.Wait(); -} - // The animator should submit to the pipeline the implicit view rendered in a // warm up frame if there's already a continuation (i.e. Animator::BeginFrame // has been called) @@ -827,72 +765,4 @@ TEST_F(EngineTest, UpdateAssetManagerWithEqualManagers) { }); } -// The warm up frame should work if only some of the registered views are -// included. -// -// This test also verifies that the warm up frame can render multiple views. -TEST_F(EngineTest, AnimatorSubmitPartialViewsForWarmUp) { - MockAnimatorDelegate animator_delegate; - std::unique_ptr engine_context; - - std::shared_ptr platform_message_handler = - std::make_shared(); - EXPECT_CALL(delegate_, GetPlatformMessageHandler) - .WillOnce(ReturnRef(platform_message_handler)); - - fml::AutoResetWaitableEvent continuation_ready_latch; - fml::AutoResetWaitableEvent draw_latch; - EXPECT_CALL(animator_delegate, OnAnimatorDraw) - .WillOnce( - Invoke([&draw_latch](const std::shared_ptr& pipeline) { - auto status = - pipeline->Consume([&](std::unique_ptr item) { - EXPECT_EQ(item->layer_tree_tasks.size(), 2u); - EXPECT_EQ(item->layer_tree_tasks[0]->view_id, 1); - EXPECT_EQ(item->layer_tree_tasks[1]->view_id, 2); - }); - EXPECT_EQ(status, PipelineConsumeResult::Done); - draw_latch.Signal(); - })); - EXPECT_CALL(animator_delegate, OnAnimatorBeginFrame) - .WillRepeatedly( - Invoke([&engine_context, &continuation_ready_latch]( - fml::TimePoint frame_target_time, uint64_t frame_number) { - continuation_ready_latch.Signal(); - engine_context->EngineTaskSync([&](Engine& engine) { - engine.BeginFrame(frame_target_time, frame_number); - }); - })); - - std::unique_ptr animator; - PostSync(task_runners_.GetUITaskRunner(), - [&animator, &animator_delegate, &task_runners = task_runners_] { - animator = std::make_unique( - animator_delegate, task_runners, - static_cast>( - std::make_unique( - task_runners))); - }); - - engine_context = EngineContext::Create(delegate_, settings_, task_runners_, - std::move(animator)); - - engine_context->EngineTaskSync([](Engine& engine) { - // Schedule a frame to make the animator create a continuation. - engine.ScheduleFrame(true); - // Add multiple views. - engine.AddView(0, ViewportMetrics{1, 10, 10, 22, 0}); - engine.AddView(1, ViewportMetrics{1, 10, 10, 22, 0}); - engine.AddView(2, ViewportMetrics{1, 10, 10, 22, 0}); - }); - - continuation_ready_latch.Wait(); - - auto configuration = RunConfiguration::InferFromSettings(settings_); - configuration.SetEntrypoint("renderWarmUpView1and2"); - engine_context->Run(std::move(configuration)); - - draw_latch.Wait(); -} - } // namespace flutter diff --git a/shell/common/fixtures/shell_test.dart b/shell/common/fixtures/shell_test.dart index c537cbf2ecfcf..1b52fabe1efe7 100644 --- a/shell/common/fixtures/shell_test.dart +++ b/shell/common/fixtures/shell_test.dart @@ -532,27 +532,6 @@ void testReportViewWidths() { }; } -@pragma('vm:entry-point') -void onDrawFrameRenderAllViews() { - PlatformDispatcher.instance.onDrawFrame = () { - for (final FlutterView view in PlatformDispatcher.instance.views) { - final SceneBuilder builder = SceneBuilder(); - final PictureRecorder recorder = PictureRecorder(); - final Canvas canvas = Canvas(recorder); - canvas.drawPaint(Paint()..color = const Color(0xFFABCDEF)); - final Picture picture = recorder.endRecording(); - builder.addPicture(Offset.zero, picture); - - final Scene scene = builder.build(); - view.render(scene); - - scene.dispose(); - picture.dispose(); - } - }; - notifyNative(); -} - @pragma('vm:entry-point') void renderWarmUpImplicitView() { bool beginFrameCalled = false; @@ -580,35 +559,3 @@ void renderWarmUpImplicitView() { }, ); } - -@pragma('vm:entry-point') -void renderWarmUpView1and2() { - bool beginFrameCalled = false; - - PlatformDispatcher.instance.scheduleWarmUpFrame( - beginFrame: () { - expect(beginFrameCalled, false); - beginFrameCalled = true; - }, - drawFrame: () { - expect(beginFrameCalled, true); - - for (final int viewId in [1, 2]) { - final FlutterView view = PlatformDispatcher.instance.view(id: viewId)!; - - final SceneBuilder builder = SceneBuilder(); - final PictureRecorder recorder = PictureRecorder(); - final Canvas canvas = Canvas(recorder); - canvas.drawPaint(Paint()..color = const Color(0xFFABCDEF)); - final Picture picture = recorder.endRecording(); - builder.addPicture(Offset.zero, picture); - - final Scene scene = builder.build(); - view.render(scene); - - scene.dispose(); - picture.dispose(); - } - } - ); -} diff --git a/shell/common/input_events_unittests.cc b/shell/common/input_events_unittests.cc index 66a2b64a39e02..3824dc4d92bae 100644 --- a/shell/common/input_events_unittests.cc +++ b/shell/common/input_events_unittests.cc @@ -127,11 +127,11 @@ static void TestSimulatedInputEvents( ShellTest::DispatchFakePointerData(shell.get()); i += 1; } - ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame); + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); } // Finally, issue a vsync for the pending event that may be generated duing // the last vsync. - ShellTest::VSyncFlush(shell.get(), &will_draw_new_frame); + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); }); simulation.wait(); @@ -345,7 +345,8 @@ TEST_F(ShellTest, CanCorrectlyPipePointerPacket) { CreateSimulatedPointerData(data, PointerData::Change::kRemove, 3.0, 4.0); packet->SetPointerData(5, data); ShellTest::DispatchPointerData(shell.get(), std::move(packet)); - ShellTest::VSyncFlush(shell.get()); + bool will_draw_new_frame; + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); reportLatch.Wait(); size_t expect_length = 6; @@ -406,7 +407,8 @@ TEST_F(ShellTest, CanCorrectlySynthesizePointerPacket) { CreateSimulatedPointerData(data, PointerData::Change::kRemove, 3.0, 4.0); packet->SetPointerData(3, data); ShellTest::DispatchPointerData(shell.get(), std::move(packet)); - ShellTest::VSyncFlush(shell.get()); + bool will_draw_new_frame; + ShellTest::VSyncFlush(shell.get(), will_draw_new_frame); reportLatch.Wait(); size_t expect_length = 6; diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index ed7bee5a9376c..8d6edce8d958a 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -261,7 +261,6 @@ DrawStatus Rasterizer::Draw(const std::shared_ptr& pipeline) { bool should_resubmit_frame = ShouldResubmitFrame(draw_result); if (should_resubmit_frame) { - FML_CHECK(draw_result.resubmitted_item); auto front_continuation = pipeline->ProduceIfEmpty(); PipelineProduceResult pipeline_result = front_continuation.Complete(std::move(draw_result.resubmitted_item)); diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index efcf43ce82668..e321826ca1615 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -22,39 +22,6 @@ namespace testing { constexpr int64_t kImplicitViewId = 0; -FrameContent ViewContent::NoViews() { - return std::map(); -} - -FrameContent ViewContent::DummyView(double width, double height) { - FrameContent result; - result[kImplicitViewId] = ViewContent{ - .viewport_metrics = {1.0, width, height, 22, 0}, - .builder = {}, - }; - return result; -} - -FrameContent ViewContent::DummyView(flutter::ViewportMetrics viewport_metrics) { - FrameContent result; - result[kImplicitViewId] = ViewContent{ - .viewport_metrics = std::move(viewport_metrics), - .builder = {}, - }; - return result; -} - -FrameContent ViewContent::ImplicitView(double width, - double height, - LayerTreeBuilder builder) { - FrameContent result; - result[kImplicitViewId] = ViewContent{ - .viewport_metrics = {1.0, width, height, 22, 0}, - .builder = std::move(builder), - }; - return result; -} - ShellTest::ShellTest() : thread_host_("io.flutter.test." + GetCurrentTestName() + ".", ThreadHost::Type::kPlatform | ThreadHost::Type::kIo | @@ -125,18 +92,16 @@ void ShellTest::RestartEngine(Shell* shell, RunConfiguration configuration) { ASSERT_TRUE(restarted.get_future().get()); } -void ShellTest::VSyncFlush(Shell* shell, bool* will_draw_new_frame) { +void ShellTest::VSyncFlush(Shell* shell, bool& will_draw_new_frame) { fml::AutoResetWaitableEvent latch; fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetPlatformTaskRunner(), - [shell, will_draw_new_frame, &latch] { + [shell, &will_draw_new_frame, &latch] { // The following UI task ensures that all previous UI tasks are flushed. fml::AutoResetWaitableEvent ui_latch; shell->GetTaskRunners().GetUITaskRunner()->PostTask( - [&ui_latch, will_draw_new_frame]() { - if (will_draw_new_frame != nullptr) { - *will_draw_new_frame = true; - } + [&ui_latch, &will_draw_new_frame]() { + will_draw_new_frame = true; ui_latch.Signal(); }); @@ -189,7 +154,6 @@ void ShellTest::SetViewportMetrics(Shell* shell, double width, double height) { std::make_unique(); recorder->RecordVsync(frame_begin_time, frame_end_time); engine->animator_->BeginFrame(std::move(recorder)); - engine->animator_->EndFrame(); } latch.Signal(); }); @@ -208,22 +172,23 @@ void ShellTest::NotifyIdle(Shell* shell, fml::TimeDelta deadline) { latch.Wait(); } -void ShellTest::PumpOneFrame(Shell* shell) { - PumpOneFrame(shell, ViewContent::DummyView()); +void ShellTest::PumpOneFrame(Shell* shell, + double width, + double height, + LayerTreeBuilder builder) { + PumpOneFrame(shell, {1.0, width, height, 22, 0}, std::move(builder)); } -void ShellTest::PumpOneFrame(Shell* shell, FrameContent frame_content) { +void ShellTest::PumpOneFrame(Shell* shell, + const flutter::ViewportMetrics& viewport_metrics, + LayerTreeBuilder builder) { // Set viewport to nonempty, and call Animator::BeginFrame to make the layer // tree pipeline nonempty. Without either of this, the layer tree below // won't be rasterized. fml::AutoResetWaitableEvent latch; - fml::WeakPtr runtime_delegate = shell->weak_engine_; shell->GetTaskRunners().GetUITaskRunner()->PostTask( - [&latch, engine = shell->weak_engine_, &frame_content, - runtime_delegate]() { - for (auto& [view_id, view_content] : frame_content) { - engine->SetViewportMetrics(view_id, view_content.viewport_metrics); - } + [&latch, engine = shell->weak_engine_, viewport_metrics]() { + engine->SetViewportMetrics(kImplicitViewId, viewport_metrics); const auto frame_begin_time = fml::TimePoint::Now(); const auto frame_end_time = frame_begin_time + fml::TimeDelta::FromSecondsF(1.0 / 60.0); @@ -231,28 +196,28 @@ void ShellTest::PumpOneFrame(Shell* shell, FrameContent frame_content) { std::make_unique(); recorder->RecordVsync(frame_begin_time, frame_end_time); engine->animator_->BeginFrame(std::move(recorder)); + latch.Signal(); + }); + latch.Wait(); - // The BeginFrame phase and the EndFrame phase must be performed in a - // single task, otherwise a normal vsync might be inserted in between, - // causing flaky assertion errors. - - for (auto& [view_id, view_content] : frame_content) { - SkMatrix identity; - identity.setIdentity(); - auto root_layer = std::make_shared(identity); - auto layer_tree = std::make_unique( - LayerTree::Config{.root_layer = root_layer}, - SkISize::Make(view_content.viewport_metrics.physical_width, - view_content.viewport_metrics.physical_height)); - float device_pixel_ratio = static_cast( - view_content.viewport_metrics.device_pixel_ratio); - if (view_content.builder) { - view_content.builder(root_layer); - } - runtime_delegate->Render(view_id, std::move(layer_tree), - device_pixel_ratio); + latch.Reset(); + // Call |Render| to rasterize a layer tree and trigger |OnFrameRasterized| + fml::WeakPtr runtime_delegate = shell->weak_engine_; + shell->GetTaskRunners().GetUITaskRunner()->PostTask( + [&latch, runtime_delegate, &builder, viewport_metrics]() { + SkMatrix identity; + identity.setIdentity(); + auto root_layer = std::make_shared(identity); + auto layer_tree = std::make_unique( + LayerTree::Config{.root_layer = root_layer}, + SkISize::Make(viewport_metrics.physical_width, + viewport_metrics.physical_height)); + float device_pixel_ratio = + static_cast(viewport_metrics.device_pixel_ratio); + if (builder) { + builder(root_layer); } - engine->animator_->EndFrame(); + runtime_delegate->Render(std::move(layer_tree), device_pixel_ratio); latch.Signal(); }); latch.Wait(); diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index c11ad1174dc88..7ded997fbcdc5 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -29,38 +29,6 @@ namespace flutter { namespace testing { -// The signature of ViewContent::builder. -using LayerTreeBuilder = - std::function root)>; -struct ViewContent; -// Defines the content to be rendered to all views of a frame in PumpOneFrame. -using FrameContent = std::map; -// Defines the content to be rendered to a view in PumpOneFrame. -struct ViewContent { - flutter::ViewportMetrics viewport_metrics; - // Given the root layer, this callback builds the layer tree to be rasterized - // in PumpOneFrame. - LayerTreeBuilder builder; - - // Build a frame with no views. This is useful when PumpOneFrame is used just - // to schedule the frame while the frame content is defined by other means. - static FrameContent NoViews(); - - // Build a frame with a single implicit view with the specific size and no - // content. - static FrameContent DummyView(double width = 1, double height = 1); - - // Build a frame with a single implicit view with the specific viewport - // metrics and no content. - static FrameContent DummyView(flutter::ViewportMetrics viewport_metrics); - - // Build a frame with a single implicit view with the specific size and - // content. - static FrameContent ImplicitView(double width, - double height, - LayerTreeBuilder builder); -}; - class ShellTest : public FixtureTest { public: struct Config { @@ -102,14 +70,24 @@ class ShellTest : public FixtureTest { static void RestartEngine(Shell* shell, RunConfiguration configuration); /// Issue as many VSYNC as needed to flush the UI tasks so far, and reset - /// the content of `will_draw_new_frame` to true if it's not nullptr. - static void VSyncFlush(Shell* shell, bool* will_draw_new_frame = nullptr); + /// the `will_draw_new_frame` to true. + static void VSyncFlush(Shell* shell, bool& will_draw_new_frame); + + /// Given the root layer, this callback builds the layer tree to be rasterized + /// in PumpOneFrame. + using LayerTreeBuilder = + std::function root)>; static void SetViewportMetrics(Shell* shell, double width, double height); static void NotifyIdle(Shell* shell, fml::TimeDelta deadline); - static void PumpOneFrame(Shell* shell); - static void PumpOneFrame(Shell* shell, FrameContent frame_content); + static void PumpOneFrame(Shell* shell, + double width = 1, + double height = 1, + LayerTreeBuilder = {}); + static void PumpOneFrame(Shell* shell, + const flutter::ViewportMetrics& viewport_metrics, + LayerTreeBuilder = {}); static void DispatchFakePointerData(Shell* shell); static void DispatchPointerData(Shell* shell, std::unique_ptr packet); diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 162f13d626016..e123769660655 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -42,7 +42,6 @@ #include "flutter/shell/common/switches.h" #include "flutter/shell/common/thread_host.h" #include "flutter/shell/common/vsync_waiter_fallback.h" -#include "flutter/shell/common/vsync_waiters_test.h" #include "flutter/shell/version/version.h" #include "flutter/testing/mock_canvas.h" #include "flutter/testing/testing.h" @@ -885,7 +884,7 @@ TEST_F(ShellTest, ExternalEmbedderNoThreadMerger) { root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); ASSERT_TRUE(end_frame_called); @@ -959,7 +958,7 @@ TEST_F(ShellTest, PushBackdropFilterToVisitedPlatformViews) { backdrop_filter_layer->Add(platform_view_layer2); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); ASSERT_EQ(visited_platform_views, (std::vector{50, 75})); ASSERT_TRUE(stack_75.is_empty()); @@ -1020,7 +1019,7 @@ TEST_F(ShellTest, root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); ASSERT_TRUE(end_frame_called); @@ -1066,12 +1065,9 @@ TEST_F(ShellTest, OnPlatformViewDestroyDisablesThreadMerger) { root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); auto result = shell->WaitForFirstFrame(fml::TimeDelta::Max()); - // Wait for the rasterizer to process the frame. WaitForFirstFrame only waits - // for the Animator, but end_frame_callback is called by the Rasterizer. - PostSync(shell->GetTaskRunners().GetRasterTaskRunner(), [] {}); ASSERT_TRUE(result.ok()) << "Result: " << static_cast(result.code()) << ": " << result.message(); @@ -1136,12 +1132,12 @@ TEST_F(ShellTest, OnPlatformViewDestroyAfterMergingThreads) { root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); // Pump one frame to trigger thread merging. end_frame_latch.Wait(); // Pump another frame to ensure threads are merged and a regular layer tree is // submitted. - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); // Threads are merged here. PlatformViewNotifyDestroy should be executed // successfully. ASSERT_TRUE(fml::TaskRunnerChecker::RunsOnTheSameThread( @@ -1205,7 +1201,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); // Pump one frame and threads aren't merged end_frame_latch.Wait(); ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( @@ -1216,7 +1212,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWhenThreadsAreMerging) { // threads external_view_embedder->UpdatePostPrerollResult( PostPrerollResult::kResubmitFrame); - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); // Now destroy the platform view immediately. // Two things can happen here: @@ -1272,7 +1268,7 @@ TEST_F(ShellTest, SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); // Threads should not be merged. @@ -1311,7 +1307,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWithoutRasterThreadMerger) { SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); // Threads should not be merged. ASSERT_FALSE(fml::TaskRunnerChecker::RunsOnTheSameThread( @@ -1377,7 +1373,7 @@ TEST_F(ShellTest, OnPlatformViewDestroyWithStaticThreadMerging) { SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); ValidateDestroyPlatformView(shell.get()); @@ -1423,7 +1419,7 @@ TEST_F(ShellTest, GetUsedThisFrameShouldBeSetBeforeEndFrame) { SkPoint::Make(10, 10), MakeSizedDisplayList(80, 80), false, false); root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); end_frame_latch.Wait(); ASSERT_FALSE(used_this_frame); @@ -1573,11 +1569,10 @@ TEST_F(ShellTest, WaitForFirstFrameZeroSizeFrame) { configuration.SetEntrypoint("emptyMain"); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get(), ViewContent::DummyView({1.0, 0.0, 0.0, 22, 0})); + PumpOneFrame(shell.get(), {1.0, 0.0, 0.0, 22, 0}); fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::Zero()); - EXPECT_FALSE(result.ok()); - EXPECT_EQ(result.message(), "timeout"); - EXPECT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded); + ASSERT_FALSE(result.ok()); + ASSERT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded); DestroyShell(std::move(shell)); } @@ -2093,7 +2088,6 @@ TEST_F(ShellTest, CanScheduleFrameFromPlatform) { TEST_F(ShellTest, SecondaryVsyncCallbackShouldBeCalledAfterVsyncCallback) { bool is_on_begin_frame_called = false; bool is_secondary_callback_called = false; - bool test_started = false; Settings settings = CreateSettingsForFixture(); TaskRunners task_runners = GetTaskRunnersForFixture(); fml::AutoResetWaitableEvent latch; @@ -2103,18 +2097,12 @@ TEST_F(ShellTest, SecondaryVsyncCallbackShouldBeCalledAfterVsyncCallback) { fml::CountDownLatch count_down_latch(2); AddNativeCallback("NativeOnBeginFrame", CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { - if (!test_started) { - return; - } EXPECT_FALSE(is_on_begin_frame_called); EXPECT_FALSE(is_secondary_callback_called); is_on_begin_frame_called = true; count_down_latch.CountDown(); })); - std::unique_ptr shell = CreateShell({ - .settings = settings, - .task_runners = task_runners, - }); + std::unique_ptr shell = CreateShell(settings, task_runners); ASSERT_TRUE(shell->IsSetup()); auto configuration = RunConfiguration::InferFromSettings(settings); @@ -2127,16 +2115,12 @@ TEST_F(ShellTest, SecondaryVsyncCallbackShouldBeCalledAfterVsyncCallback) { fml::TaskRunner::RunNowOrPostTask( shell->GetTaskRunners().GetUITaskRunner(), [&]() { shell->GetEngine()->ScheduleSecondaryVsyncCallback(0, [&]() { - if (!test_started) { - return; - } EXPECT_TRUE(is_on_begin_frame_called); EXPECT_FALSE(is_secondary_callback_called); is_secondary_callback_called = true; count_down_latch.CountDown(); }); shell->GetEngine()->ScheduleFrame(); - test_started = true; }); count_down_latch.Wait(); EXPECT_TRUE(is_on_begin_frame_called); @@ -2181,7 +2165,7 @@ TEST_F(ShellTest, Screenshot) { root->Add(display_list_layer); }; - PumpOneFrame(shell.get(), ViewContent::ImplicitView(100, 100, builder)); + PumpOneFrame(shell.get(), 100, 100, builder); firstFrameLatch.Wait(); std::promise screenshot_promise; @@ -2566,13 +2550,7 @@ TEST_F(ShellTest, OnServiceProtocolRenderFrameWithRasterStatsWorks) { configuration.SetEntrypoint("scene_with_red_box"); RunEngine(shell.get(), std::move(configuration)); - // Set a non-zero viewport metrics, otherwise the scene would be discarded. - PostSync(shell->GetTaskRunners().GetUITaskRunner(), - [engine = shell->GetEngine()]() { - engine->SetViewportMetrics(kImplicitViewId, - ViewportMetrics{1, 1, 1, 22, 0}); - }); - PumpOneFrame(shell.get(), ViewContent::NoViews()); + PumpOneFrame(shell.get()); ServiceProtocol::Handler::ServiceProtocolMap empty_params; rapidjson::Document document; @@ -2684,7 +2662,7 @@ TEST_F(ShellTest, OnServiceProtocolRenderFrameWithRasterStatsDisableImpeller) { configuration.SetEntrypoint("scene_with_red_box"); RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get(), ViewContent::NoViews()); + PumpOneFrame(shell.get()); ServiceProtocol::Handler::ServiceProtocolMap empty_params; rapidjson::Document document; @@ -2748,16 +2726,14 @@ TEST_F(ShellTest, DISABLED_DiscardLayerTreeOnResize) { RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get(), ViewContent::DummyView( - static_cast(wrong_size.width()), - static_cast(wrong_size.height()))); + PumpOneFrame(shell.get(), static_cast(wrong_size.width()), + static_cast(wrong_size.height())); end_frame_latch.Wait(); // Wrong size, no frames are submitted. ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount()); - PumpOneFrame(shell.get(), ViewContent::DummyView( - static_cast(expected_size.width()), - static_cast(expected_size.height()))); + PumpOneFrame(shell.get(), static_cast(expected_size.width()), + static_cast(expected_size.height())); end_frame_latch.Wait(); // Expected size, 1 frame submitted. ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount()); @@ -2828,9 +2804,8 @@ TEST_F(ShellTest, DISABLED_DiscardResubmittedLayerTreeOnResize) { RunEngine(shell.get(), std::move(configuration)); - PumpOneFrame(shell.get(), ViewContent::DummyView( - static_cast(origin_size.width()), - static_cast(origin_size.height()))); + PumpOneFrame(shell.get(), static_cast(origin_size.width()), + static_cast(origin_size.height())); end_frame_latch.Wait(); ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount()); @@ -2851,9 +2826,8 @@ TEST_F(ShellTest, DISABLED_DiscardResubmittedLayerTreeOnResize) { ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount()); // Threads will be merged at the end of this frame. - PumpOneFrame(shell.get(), - ViewContent::DummyView(static_cast(new_size.width()), - static_cast(new_size.height()))); + PumpOneFrame(shell.get(), static_cast(new_size.width()), + static_cast(new_size.height())); end_frame_latch.Wait(); ASSERT_TRUE(raster_thread_merger_ref->IsMerged()); diff --git a/testing/dart/platform_view_test.dart b/testing/dart/platform_view_test.dart index cd581a22d581d..146c865899952 100644 --- a/testing/dart/platform_view_test.dart +++ b/testing/dart/platform_view_test.dart @@ -10,12 +10,10 @@ void main() { test('PlatformView layers do not emit errors from tester', () async { final SceneBuilder builder = SceneBuilder(); builder.addPlatformView(1); - PlatformDispatcher.instance.onBeginFrame = (Duration duration) { - final Scene scene = builder.build(); - PlatformDispatcher.instance.implicitView!.render(scene); - scene.dispose(); - }; - PlatformDispatcher.instance.scheduleFrame(); + final Scene scene = builder.build(); + + PlatformDispatcher.instance.implicitView!.render(scene); + scene.dispose(); // Test harness asserts that this does not emit an error from the shell logs. }); } From 7e1c44b73c08c6b97e591970cfbd7c03f30b6e5a Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 26 Feb 2024 17:39:23 -0800 Subject: [PATCH 14/21] Migrate Android `scenario_app` to the `SurfaceProducer` API (#50993) Part of testing https://github.com/flutter/flutter/issues/139702. Without this PR, the Impeller + Vulkan Scenario App will draw nothing/potentially crash, because there is no way to draw the (current) `SurfaceTexture`-based textures in Vulkan (and never will be). This change does the following: - Skia -> Nothing - Impeller + OpenGLES -> On newer Android devices, uses `ImageReader` instead - Impeller + Vulkan -> Always uses `ImageReader` See also: https://api.flutter.dev/javadoc/io/flutter/view/TextureRegistry.SurfaceProducer.html. --- .../ExternalTextureFlutterActivity.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java index f06f75fcd801b..8479673a6ca60 100644 --- a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java +++ b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java @@ -11,7 +11,6 @@ import android.graphics.Paint; import android.graphics.Rect; import android.graphics.Shader.TileMode; -import android.graphics.SurfaceTexture; import android.hardware.HardwareBuffer; import android.media.Image; import android.media.ImageReader; @@ -36,7 +35,7 @@ import androidx.annotation.Nullable; import androidx.annotation.RequiresApi; import androidx.core.util.Supplier; -import io.flutter.view.TextureRegistry.SurfaceTextureEntry; +import io.flutter.view.TextureRegistry; import java.io.IOException; import java.nio.ByteBuffer; import java.util.Map; @@ -54,7 +53,7 @@ public class ExternalTextureFlutterActivity extends TestActivity { private final CountDownLatch firstFrameLatch = new CountDownLatch(2); private long textureId = 0; - private SurfaceTextureEntry surfaceTextureEntry; + private TextureRegistry.SurfaceProducer surfaceProducer; @Override protected void onCreate(@Nullable Bundle savedInstanceState) { @@ -143,19 +142,18 @@ private MediaExtractor createMediaExtractor() { public void onPause() { surfaceViewRenderer.destroy(); flutterRenderer.destroy(); - surfaceTextureEntry.release(); + surfaceProducer.release(); super.onPause(); } @Override public void onFlutterUiDisplayed() { - surfaceTextureEntry = - Objects.requireNonNull(getFlutterEngine()).getRenderer().createSurfaceTexture(); - SurfaceTexture surfaceTexture = surfaceTextureEntry.surfaceTexture(); - surfaceTexture.setDefaultBufferSize(SURFACE_WIDTH, SURFACE_HEIGHT); - flutterRenderer.attach(new Surface(surfaceTexture), firstFrameLatch); + surfaceProducer = + Objects.requireNonNull(getFlutterEngine()).getRenderer().createSurfaceProducer(); + surfaceProducer.setSize(SURFACE_WIDTH, SURFACE_HEIGHT); + flutterRenderer.attach(surfaceProducer.getSurface(), firstFrameLatch); flutterRenderer.repaint(); - textureId = surfaceTextureEntry.id(); + textureId = surfaceProducer.id(); super.onFlutterUiDisplayed(); } From 49671561dbbbd3f504adc5892511b6f7934b6580 Mon Sep 17 00:00:00 2001 From: "auto-submit[bot]" <98614782+auto-submit[bot]@users.noreply.github.com> Date: Tue, 27 Feb 2024 01:49:26 +0000 Subject: [PATCH 15/21] Reverts "Migrate Android `scenario_app` to the `SurfaceProducer` API (#50993)" (#50995) Reverts flutter/engine#50993 Initiated by: matanlurey Reason for reverting: The digests will come back negative, as this change shows bugs in the `ImageTexture` implementation. Original PR Author: matanlurey Reviewed By: {jonahwilliams, johnmccutchan} This change reverts the following previous change: Original Description: Part of testing https://github.com/flutter/flutter/issues/139702. Without this PR, the Impeller + Vulkan Scenario App will draw nothing/potentially crash, because there is no way to draw the (current) `SurfaceTexture`-based textures in Vulkan (and never will be). This change does the following: - Skia -> Nothing - Impeller + OpenGLES -> On newer Android devices, uses `ImageReader` instead - Impeller + Vulkan -> Always uses `ImageReader` See also: https://api.flutter.dev/javadoc/io/flutter/view/TextureRegistry.SurfaceProducer.html. --- .../ExternalTextureFlutterActivity.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java index 8479673a6ca60..f06f75fcd801b 100644 --- a/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java +++ b/testing/scenario_app/android/app/src/main/java/dev/flutter/scenarios/ExternalTextureFlutterActivity.java @@ -11,6 +11,7 @@ import android.graphics.Paint; import android.graphics.Rect; import android.graphics.Shader.TileMode; +import android.graphics.SurfaceTexture; import android.hardware.HardwareBuffer; import android.media.Image; import android.media.ImageReader; @@ -35,7 +36,7 @@ import androidx.annotation.Nullable; import androidx.annotation.RequiresApi; import androidx.core.util.Supplier; -import io.flutter.view.TextureRegistry; +import io.flutter.view.TextureRegistry.SurfaceTextureEntry; import java.io.IOException; import java.nio.ByteBuffer; import java.util.Map; @@ -53,7 +54,7 @@ public class ExternalTextureFlutterActivity extends TestActivity { private final CountDownLatch firstFrameLatch = new CountDownLatch(2); private long textureId = 0; - private TextureRegistry.SurfaceProducer surfaceProducer; + private SurfaceTextureEntry surfaceTextureEntry; @Override protected void onCreate(@Nullable Bundle savedInstanceState) { @@ -142,18 +143,19 @@ private MediaExtractor createMediaExtractor() { public void onPause() { surfaceViewRenderer.destroy(); flutterRenderer.destroy(); - surfaceProducer.release(); + surfaceTextureEntry.release(); super.onPause(); } @Override public void onFlutterUiDisplayed() { - surfaceProducer = - Objects.requireNonNull(getFlutterEngine()).getRenderer().createSurfaceProducer(); - surfaceProducer.setSize(SURFACE_WIDTH, SURFACE_HEIGHT); - flutterRenderer.attach(surfaceProducer.getSurface(), firstFrameLatch); + surfaceTextureEntry = + Objects.requireNonNull(getFlutterEngine()).getRenderer().createSurfaceTexture(); + SurfaceTexture surfaceTexture = surfaceTextureEntry.surfaceTexture(); + surfaceTexture.setDefaultBufferSize(SURFACE_WIDTH, SURFACE_HEIGHT); + flutterRenderer.attach(new Surface(surfaceTexture), firstFrameLatch); flutterRenderer.repaint(); - textureId = surfaceProducer.id(); + textureId = surfaceTextureEntry.id(); super.onFlutterUiDisplayed(); } From e4d8c26ced11cc5e39f07af472524744f4f05b8c Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Mon, 26 Feb 2024 20:58:11 -0500 Subject: [PATCH 16/21] Roll Skia from ba3ed5998af3 to aa28c3a30a98 (12 revisions) (#50994) https://skia.googlesource.com/skia.git/+log/ba3ed5998af3..aa28c3a30a98 2024-02-26 brianosman@google.com Handle recursion limit gracefully in the stroker 2024-02-26 johnstiles@google.com Revert "[skif] Add transparent padding to source saveLayers" 2024-02-26 johnstiles@google.com Use nmad primitive in inverse and transcendental functions. 2024-02-26 herb@google.com Reland "Implement ARM only unpremul using SIMD" 2024-02-26 johnstiles@google.com Add Raster Pipeline fused negative multiply-add primitive. 2024-02-26 michaelludwig@google.com [skif] Add transparent padding to source saveLayers 2024-02-26 kjlubick@google.com Try adding a gn clean before building fontations 2024-02-26 herb@google.com Revert "Implement ARM only unpremul using SIMD" 2024-02-26 herb@google.com Implement ARM only unpremul using SIMD 2024-02-26 jvanverth@google.com [graphite] Enable SmallPathAtlas. 2024-02-26 michaelludwig@google.com Update legacy LCD ARGB32 blitter to match RP for transparent pixels 2024-02-26 johnstiles@google.com Add Perlin Noise Raster Pipeline implementation. If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/skia-flutter-autoroll Please CC brianosman@google.com,jimgraham@google.com,rmistry@google.com,robertphillips@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md --- DEPS | 2 +- ci/licenses_golden/licenses_skia | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/DEPS b/DEPS index ab210439de498..2a0e24634bc14 100644 --- a/DEPS +++ b/DEPS @@ -14,7 +14,7 @@ vars = { 'flutter_git': 'https://flutter.googlesource.com', 'skia_git': 'https://skia.googlesource.com', 'llvm_git': 'https://llvm.googlesource.com', - 'skia_revision': 'ba3ed5998af356f0fb7c285b4c3d7c0f1b1c1ad1', + 'skia_revision': 'aa28c3a30a98702854c26f2b06093a08d29a9645', # WARNING: DO NOT EDIT canvaskit_cipd_instance MANUALLY # See `lib/web_ui/README.md` for how to roll CanvasKit to a new version. diff --git a/ci/licenses_golden/licenses_skia b/ci/licenses_golden/licenses_skia index ee5918034642b..a40e3c85a7e66 100644 --- a/ci/licenses_golden/licenses_skia +++ b/ci/licenses_golden/licenses_skia @@ -1,4 +1,4 @@ -Signature: 014fbb8ea53c5ea676ab78839d010b6b +Signature: 5e045c649114e47488257d8c16ed75c2 ==================================================================================================== LIBRARY: etc1 From 984eb257cce8c8a73f09f0e8bf4a11c76e3ff0ee Mon Sep 17 00:00:00 2001 From: Jason Simmons Date: Mon, 26 Feb 2024 17:58:14 -0800 Subject: [PATCH 17/21] Roll buildroot to 21b1b9f2645fada701885108e86aefbcb3b1cca0 (#50991) --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 2a0e24634bc14..8f9cae7783c73 100644 --- a/DEPS +++ b/DEPS @@ -275,7 +275,7 @@ allowed_hosts = [ ] deps = { - 'src': 'https://github.com/flutter/buildroot.git' + '@' + '5413806166ee3b5198b9dd1c1f684d266ad9850c', + 'src': 'https://github.com/flutter/buildroot.git' + '@' + '21b1b9f2645fada701885108e86aefbcb3b1cca0', 'src/flutter/third_party/depot_tools': Var('chromium_git') + '/chromium/tools/depot_tools.git' + '@' + '580b4ff3f5cd0dcaa2eacda28cefe0f45320e8f7', From 56e3f36bbde6e6aeee81014e22b89eaa1683c272 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 26 Feb 2024 18:35:28 -0800 Subject: [PATCH 18/21] Fix usage of `--out-dir` with a relative path. (#50992) @dnfield: > it looks like the new dart script doesn't necessarily work well with relative directories for the out dir --- testing/scenario_app/bin/run_android_tests.dart | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart index 6a6ed8e9bb659..3d43ffd12692f 100644 --- a/testing/scenario_app/bin/run_android_tests.dart +++ b/testing/scenario_app/bin/run_android_tests.dart @@ -401,8 +401,9 @@ Future _run({ // TODO(matanlurey): Resolve this in a better way. On CI this file always exists. File(join(screenshotPath, 'noop.txt')).writeAsStringSync(''); // TODO(gaaclarke): We should move this into dir_contents_diff. - _withTemporaryCwd(dirname(contentsGolden), () { - final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath); + final String diffScreenhotPath = absolute(screenshotPath); + _withTemporaryCwd(absolute(dirname(contentsGolden)), () { + final int exitCode = dirContentsDiff(basename(contentsGolden), diffScreenhotPath); if (exitCode != 0) { panic(['Output contents incorrect.']); } From 6965435111ef39a408cb1a6a7febfff0125ea60b Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Mon, 26 Feb 2024 21:36:15 -0500 Subject: [PATCH 19/21] Roll Dart SDK from 2876f5684ced to 67b2a250747b (1 revision) (#50996) https://dart.googlesource.com/sdk.git/+log/2876f5684ced..67b2a250747b 2024-02-27 dart-internal-merge@dart-ci-internal.iam.gserviceaccount.com Version 3.4.0-179.0.dev If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/dart-sdk-flutter-engine Please CC dart-vm-team@google.com,jimgraham@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter Engine: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md --- DEPS | 2 +- ci/licenses_golden/licenses_dart | 4 ++-- sky/packages/sky_engine/LICENSE | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/DEPS b/DEPS index 8f9cae7783c73..9b24691933867 100644 --- a/DEPS +++ b/DEPS @@ -62,7 +62,7 @@ vars = { # Dart is: https://github.com/dart-lang/sdk/blob/main/DEPS # You can use //tools/dart/create_updated_flutter_deps.py to produce # updated revision list of existing dependencies. - 'dart_revision': '2876f5684ceddf924aa0e2ab7d34a6baf4717496', + 'dart_revision': '67b2a250747b9c25c64b901ffbf17bdf36987331', # WARNING: DO NOT EDIT MANUALLY # The lines between blank lines above and below are generated by a script. See create_updated_flutter_deps.py diff --git a/ci/licenses_golden/licenses_dart b/ci/licenses_golden/licenses_dart index 8054ae04fd7c1..b6b033e128951 100644 --- a/ci/licenses_golden/licenses_dart +++ b/ci/licenses_golden/licenses_dart @@ -1,4 +1,4 @@ -Signature: 7fd3eb25284e36712433374678f727d7 +Signature: 1d4fc3fedb6b5c5054d28cf40a3c91a4 ==================================================================================================== LIBRARY: dart @@ -4751,7 +4751,7 @@ Exhibit B - "Incompatible With Secondary Licenses" Notice This Source Code Form is "Incompatible With Secondary Licenses", as defined by the Mozilla Public License, v. 2.0. -You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/2876f5684ceddf924aa0e2ab7d34a6baf4717496 +You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/67b2a250747b9c25c64b901ffbf17bdf36987331 /third_party/fallback_root_certificates/ ==================================================================================================== diff --git a/sky/packages/sky_engine/LICENSE b/sky/packages/sky_engine/LICENSE index b143690aed50a..a5bb820a410a6 100644 --- a/sky/packages/sky_engine/LICENSE +++ b/sky/packages/sky_engine/LICENSE @@ -31704,7 +31704,7 @@ Exhibit B - "Incompatible With Secondary Licenses" Notice This Source Code Form is "Incompatible With Secondary Licenses", as defined by the Mozilla Public License, v. 2.0. -You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/2876f5684ceddf924aa0e2ab7d34a6baf4717496 +You may obtain a copy of this library's Source Code Form from: https://dart.googlesource.com/sdk/+/67b2a250747b9c25c64b901ffbf17bdf36987331 /third_party/fallback_root_certificates/ -------------------------------------------------------------------------------- From ed3ee6705b9c81a4cfcb7a8b6394ac4fbf14cc6b Mon Sep 17 00:00:00 2001 From: skia-flutter-autoroll Date: Mon, 26 Feb 2024 22:17:22 -0500 Subject: [PATCH 20/21] Roll Skia from aa28c3a30a98 to 2f2a718b27f7 (1 revision) (#50998) https://skia.googlesource.com/skia.git/+log/aa28c3a30a98..2f2a718b27f7 2024-02-27 skia-autoroll@skia-public.iam.gserviceaccount.com Roll vulkan-deps from dd6c2371c85d to cc25d8806a33 (7 revisions) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/skia-flutter-autoroll Please CC brianosman@google.com,jimgraham@google.com,rmistry@google.com,robertphillips@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Skia: https://bugs.chromium.org/p/skia/issues/entry 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md --- DEPS | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEPS b/DEPS index 9b24691933867..91ecaa53fd473 100644 --- a/DEPS +++ b/DEPS @@ -14,7 +14,7 @@ vars = { 'flutter_git': 'https://flutter.googlesource.com', 'skia_git': 'https://skia.googlesource.com', 'llvm_git': 'https://llvm.googlesource.com', - 'skia_revision': 'aa28c3a30a98702854c26f2b06093a08d29a9645', + 'skia_revision': '2f2a718b27f7fe706d739ed097abce9301a14b32', # WARNING: DO NOT EDIT canvaskit_cipd_instance MANUALLY # See `lib/web_ui/README.md` for how to roll CanvasKit to a new version. From 0bc21ea7bc92600702cfd47b69ff9c3825b0fd81 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Mon, 26 Feb 2024 19:17:44 -0800 Subject: [PATCH 21/21] Respect SIGINT (Ctrl-C) for Android scenario_app. (#50989) Closes https://github.com/flutter/flutter/issues/144076. I had to make some other cleanup changes in order to avoid the program hanging. --- .../scenario_app/bin/run_android_tests.dart | 31 +++++++++++++++---- 1 file changed, 25 insertions(+), 6 deletions(-) diff --git a/testing/scenario_app/bin/run_android_tests.dart b/testing/scenario_app/bin/run_android_tests.dart index 3d43ffd12692f..8456ccc0bda45 100644 --- a/testing/scenario_app/bin/run_android_tests.dart +++ b/testing/scenario_app/bin/run_android_tests.dart @@ -59,8 +59,14 @@ void main(List args) async { return; } + // Capture CTRL-C. + late final StreamSubscription onSigint; runZonedGuarded( () async { + onSigint = ProcessSignal.sigint.watch().listen((_) { + onSigint.cancel(); + panic(['Received SIGINT']); + }); await _run( verbose: options.verbose, outDir: Directory(options.outDir), @@ -73,9 +79,11 @@ void main(List args) async { contentsGolden: options.outputContentsGolden, ndkStack: options.ndkStack, ); + onSigint.cancel(); exit(0); }, (Object error, StackTrace stackTrace) { + onSigint.cancel(); if (error is! Panic) { stderr.writeln('Unhandled error: $error'); stderr.writeln(stackTrace); @@ -146,8 +154,9 @@ Future _run({ // for the screenshots. // On LUCI, the host uploads the screenshots to Skia Gold. SkiaGoldClient? skiaGoldClient; - late ServerSocket server; + late final ServerSocket server; final List> pendingComparisons = >[]; + final List pendingConnections = []; await step('Starting server...', () async { server = await ServerSocket.bind(InternetAddress.anyIPv4, _tcpPort); if (verbose) { @@ -157,8 +166,8 @@ Future _run({ if (verbose) { stdout.writeln('client connected ${client.remoteAddress.address}:${client.remotePort}'); } - client.transform(const ScreenshotBlobTransformer()).listen( - (Screenshot screenshot) { + pendingConnections.add(client); + client.transform(const ScreenshotBlobTransformer()).listen((Screenshot screenshot) { final String fileName = screenshot.filename; final Uint8List fileContent = screenshot.fileContent; if (verbose) { @@ -182,9 +191,9 @@ Future _run({ }); pendingComparisons.add(comparison); } - }, onError: (dynamic err) { - panic(['error while receiving bytes: $err']); - }, cancelOnError: true); + }, onDone: () { + pendingConnections.remove(client); + }); }); }); @@ -335,6 +344,16 @@ Future _run({ }); } finally { await server.close(); + for (final Socket client in pendingConnections.toList()) { + client.close(); + } + + await step('Killing test app and test runner...', () async { + final int exitCode = await pm.runAndForward([adb.path, 'shell', 'am', 'force-stop', 'dev.flutter.scenarios']); + if (exitCode != 0) { + panic(['could not kill test app']); + } + }); await step('Killing logcat process...', () async { final bool delivered = logcatProcess.kill(ProcessSignal.sigkill);