Fix Shell::Screenshot for Impeller #50072

dnfield · 2024-01-26T03:27:15Z

Shell::Screenshot was impelemnted in a Skia-only way and would crash when invoked. Adds test coverage for the breaking path on iOS that ends up calling FlutterView drawLayer.

jonahwilliams

LGTM this seems pretty solid

jonahwilliams · 2024-01-26T03:52:58Z

shell/common/rasterizer.cc

+      latch.Signal();
+      return;
+    }
+    auto buffer_view = impeller::DeviceBuffer::AsBufferView(buffer);


nit: unused?

removed it.

…ll_screenshot

gaaclarke · 2024-01-26T19:27:44Z

shell/common/rasterizer.cc

+  root_surface_transformation.reset();
+
+  auto frame = compositor_context.AcquireFrame(
+      surface_context,              // skia context


The style for these should be /*arg_name=*/ so that they are checked by the linter.

Done in follow up.

gaaclarke · 2024-01-26T19:34:49Z

shell/common/rasterizer.cc

+
+  impeller::DlDispatcher dispatcher;
+  builder.Build()->Dispatch(dispatcher);
+  const auto& picture = dispatcher.EndRecordingAsPicture();


https://google.github.io/styleguide/cppguide.html#Type_deduction

I think this one is pretty clear.

It's not if it is clear to you, "Use type deduction only if it makes the code clearer to readers who aren't familiar with the project, or if it makes the code safer. Do not use it merely to avoid the inconvenience of writing an explicit type."

This isn't my personal style, I'm just reporting based on the style guide we've agreed to follow =)

gaaclarke · 2024-01-26T19:39:00Z

shell/common/rasterizer.cc

+  auto buffer =
+      impeller_context->GetResourceAllocator()->CreateBuffer(buffer_desc);
+  auto command_buffer = impeller_context->CreateCommandBuffer();
+  command_buffer->SetLabel("BlitTextureToBuffer Command Buffer");


This code is very similar to the code we have in vulkan_screenshotter.cc. Despite the name, it's actually doing this. It would be nice if we could share this. PTAL.

There's a few places we have code a little like this, and trying to clean all of them up was getting to be pretty invasive. I think it's worth doing but didn't want to block the rest of this fix on it.

gaaclarke · 2024-01-26T19:39:52Z

shell/common/rasterizer.cc

+                     &latch](impeller::CommandBuffer::Status status) {
+    if (status != impeller::CommandBuffer::Status::kCompleted) {
+      FML_LOG(ERROR) << "Failed to complete blit pass.";
+      latch.Signal();


nit: put this in a fml::ScopedCleanup.

gaaclarke · 2024-01-26T19:43:15Z

testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m

+  UIViewController* rootVC = appDelegate.window.rootViewController;
+  [rootVC presentViewController:self.flutterViewController animated:NO completion:nil];
+
+  CGColorSpaceRef color_space = CGColorSpaceCreateDeviceRGB();


I believe this is leaking, there should be a CGColorSpaceRelease.

Adding this in a follow up, and for the CGContextRelease

gaaclarke · 2024-01-26T19:44:12Z

testing/scenario_app/ios/Scenarios/ScenariosTests/FlutterViewControllerTest.m

+      return;
+    }
+
+    dispatch_after(dispatch_time(DISPATCH_TIME_NOW, NSEC_PER_SEC), dispatch_get_main_queue(),


you can just use dispatch_async here and below

I want a delay so we get a chance to render some frames.

gaaclarke · 2024-01-26T19:45:29Z

testing/scenario_app/lib/src/solid_blue.dart

+    final PictureRecorder recorder = PictureRecorder();
+    final Canvas canvas = Canvas(recorder);
+
+    canvas.drawPaint(Paint()..color = const Color(0xFF0000FF));


This is in BGRA? I would have expected RGBA at this level, probably worth commenting.

It's ARGB - this is the normal dart:ui color class...

gaaclarke · 2024-01-26T19:46:45Z

sorry, late to the party, there is a few things in there that should be fixed (like the leak)

…142347) flutter/engine@525bd7d...a65a1b5 2024-01-26 [email protected] Roll Skia from c32aa37effcc to 6279c88b9e29 (1 revision) (flutter/engine#50098) 2024-01-26 [email protected] [Impeller] add compute pass API for memory barriers, re-enable for Vulkan. (flutter/engine#49946) 2024-01-26 [email protected] Introduce a prototype of a "header guard enforcement" tool (flutter/engine#48903) 2024-01-26 [email protected] Roll Skia from e24124912cc3 to c32aa37effcc (1 revision) (flutter/engine#50094) 2024-01-26 [email protected] [web] Do not wipe the PlatformViewManager when disposing of a view. (flutter/engine#49991) 2024-01-26 [email protected] Finish landing missing/incorrect header guards across `flutter/engine` (flutter/engine#50069) 2024-01-26 [email protected] Roll Skia from 32f6bff0f193 to e24124912cc3 (2 revisions) (flutter/engine#50093) 2024-01-26 [email protected] Roll Skia from cbdf09d69efc to 32f6bff0f193 (3 revisions) (flutter/engine#50092) 2024-01-26 [email protected] Roll Dart SDK from 5636e338e0b9 to 7337995bc851 (1 revision) (flutter/engine#50087) 2024-01-26 [email protected] Fix Shell::Screenshot for Impeller (flutter/engine#50072) 2024-01-26 [email protected] Roll Skia from ae73baacb793 to cbdf09d69efc (1 revision) (flutter/engine#50085) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://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

`FlutterViewControllerTest testDrawLayer` created a callback which strongly referenced itself in its own body as part of an asynchronous recursive loop. The recursion was unnecessary and the test consistently passes, even if run on repeat > 100 times without it. Now that there's only one call, eliminates the unnecessary local and inlines it into the `dispatch_after` call. This was originally introduced in #50072. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

…6249) `FlutterViewControllerTest testDrawLayer` created a callback which strongly referenced itself in its own body as part of an asynchronous recursive loop. The recursion was unnecessary and the test consistently passes, even if run on repeat > 100 times without it. Now that there's only one call, eliminates the unnecessary local and inlines it into the `dispatch_after` call. This was originally introduced in flutter/engine#50072. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style

Fix Shell::Screenshot for Impeller

b25cb3e

dnfield requested review from gaaclarke and chinmaygarde January 26, 2024 03:27

github-actions bot added the platform-ios label Jan 26, 2024

dnfield requested a review from jonahwilliams January 26, 2024 03:48

build fixes

5e537fc

github-actions bot added the platform-android label Jan 26, 2024

Update solid_blue.dart

86c6b83

jonahwilliams approved these changes Jan 26, 2024

View reviewed changes

dnfield added 5 commits January 25, 2024 19:55

unused code

8eb4a20

Merge branch 'shell_screenshot' of github.com:dnfield/engine into she…

68c0972

…ll_screenshot

fixes

ef93ac3

wrong line...

8705e8b

unnecessary forward decl

bc58bcb

dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 26, 2024

auto-submit bot merged commit b307a6c into flutter:main Jan 26, 2024

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 65d1291c3add to ddb3b354a30b (2 revisions) flutter/flutter#142324

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

8f69166

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 65d1291c3add to 45e11649ab83 (3 revisions) flutter/flutter#142328

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

c90f72b

gaaclarke reviewed Jan 26, 2024

View reviewed changes

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 65d1291c3add to 182b5951eb7b (5 revisions) flutter/flutter#142331

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

57c5ac6

dnfield deleted the shell_screenshot branch January 26, 2024 20:34

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to 70039d3f23fd (7 revisions) flutter/flutter#142337

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

510ae95

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to daf139b27125 (8 revisions) flutter/flutter#142340

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

6fd25f6

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to d27b18d912d7 (10 revisions) flutter/flutter#142346

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

003fcbb

engine-flutter-autoroll mentioned this pull request Jan 26, 2024

Roll Flutter Engine from 525bd7dcf7f3 to a65a1b55e06a (11 revisions) flutter/flutter#142347

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 26, 2024

b307a6cfe Fix Shell::Screenshot for Impeller (flutter/engine#50072)

0cb85e7

cbracken mentioned this pull request Oct 30, 2024

iOS: Eliminate strong retain loop in Scenario tests #56249

Merged

8 tasks

Fix Shell::Screenshot for Impeller #50072

Fix Shell::Screenshot for Impeller #50072

Uh oh!

Conversation

dnfield commented Jan 26, 2024

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jan 26, 2024

Uh oh!

Uh oh!