Add support for images in display lists. #32268

chinmaygarde · 2022-03-25T22:57:20Z

No description provided.

chinmaygarde · 2022-03-28T22:47:36Z

This is good for a review now. This depends on flutter/impeller#102 in Impeller and the DEPS have been pointed to the fork temporarily for the presubmits.

zanderso · 2022-03-29T02:01:02Z

Looks like a bunch of the DrawImage display list unit tests are failing, like:

[ RUN      ] DisplayList.SingleOpDisplayListsRecapturedViaSkCanvasAreEqual
../../flutter/display_list/display_list_unittests.cc:909: Failure
Value of: dl->Equals(*sk_copy)
  Actual: false
Expected: true
DrawImage[variant 1] == sk_copy

flar

I looked mainly at the /display_list, /flow, /flow/layers, and canvas.cc and image.cc and mainly left some TODO suggestions and nits.

Perhaps someone else more familiar with how we manage the codecs and shell stuff could look at those parts, otherwise I'll try again tomorrow...

display_list/display_list.cc

display_list/display_list_unittests.cc

flar · 2022-03-29T10:21:25Z

flow/surface_frame.cc

@@ -24,7 +25,7 @@ SurfaceFrame::SurfaceFrame(sk_sp<SkSurface> surface,
  FML_DCHECK(submit_callback_);
  if (surface_) {
    canvas_ = surface_->getCanvas();
-  } else {
+  } else if (display_list_fallback) {


Should this then have a final "else" that notifies someone that we gave up on canvas/recorder? If that can't happen, do we need the if part of this else-if?

No, the embedder depends on there being no surface (and hence its canvas) when running with composition enabled. This was added to make sure that use case (and its tests) are supported.

lib/ui/painting/canvas.cc

flar · 2022-03-29T10:32:24Z

display_list/display_list_image.cc

+
+SkIRect DlImage::bounds() const {
+  auto size = dimensions();
+  return SkIRect::MakeXYWH(0, 0, size.fWidth, size.fHeight);


There are SKIRect factories for SkISize and int w,h...

Done. I was hoping to just remove these convenience methods but they did make porting over the old code easier. I'll file a followup.

flar · 2022-03-29T10:41:35Z

Looks like a bunch of the DrawImage display list unit tests are failing, like:

[ RUN      ] DisplayList.SingleOpDisplayListsRecapturedViaSkCanvasAreEqual
../../flutter/display_list/display_list_unittests.cc:909: Failure
Value of: dl->Equals(*sk_copy)
  Actual: false
Expected: true
DrawImage[variant 1] == sk_copy

I think the sk_copy's are DL -> SkCanvas -> DL conversions where we lose the Dl-ness of the DlImage and so we may have to disable them. I think there is a way to set a value in the records to disable them (sk_testing_invalid suggests setting the sk_op_count fields to negative numbers as is done with the DrawPicture and DrawDisplayList cases - do a case sensitive search for those strings and you should see how that is done)

chinmaygarde · 2022-03-29T20:23:56Z

All presubmits (except the engine tree check) are passing. I've landed the Impeller patch and updated the DEPS. This is good for another review.

chinmaygarde · 2022-03-29T20:31:39Z

The guidance for g3 rolls is the same as before. All TU's with the _impeller suffix can be ignored. The TUs that do reference these files do so behind a macro which is absent in internal builds.

zanderso · 2022-03-29T20:37:52Z

lib/ui/painting/image_decoder.cc

@@ -4,13 +4,35 @@

 #include "flutter/lib/ui/painting/image_decoder.h"

-#include <algorithm>


I think we added these explicit includes of <algorithm> for the g3 build. Please verify that they went along with the code that got shuffled around.

I removed all existing headers and tried to include what was being used as there were a few spots where there were spurious includes. But you're right, std::max would need to include algorithm but in the _skia TU. I'll add it there.

zanderso · 2022-03-29T20:38:39Z

lib/ui/painting/image_decoder_skia.cc

+// found in the LICENSE file.
+
+#include "flutter/lib/ui/painting/image_decoder_skia.h"
+


Like do we need to retain #include <algorithm> here?

flar · 2022-03-29T22:09:04Z

lib/ui/painting/image.h

@@ -46,7 +48,7 @@ class CanvasImage final : public RefCountedDartWrappable<CanvasImage> {
 private:
  CanvasImage();

-  flutter::SkiaGPUObject<SkImage> image_;
+  sk_sp<DlImage> image_;


I'm concerned that we moved the SkiaGPUObject wrapper down to an unnecessary level.

That wrapper exists because lib/ui/painting/image.h lives in a Dart wrapper and its lifetime is determined by Dart and its GC in a Dart isolate/thread. That isolate/thread doesn't have the ability to free things like images and so the SkiaGPUObject wrapper forwards the unref to a thread that can.

From this object, that forwarding is needed.

But, when a reference from this field is handed off to the engine, it is managed by a thread that can unref and free images, so it no longer needs the SkiaGPUObject protection. You've just moved that indirection down into the engine object which means that every time we get rid of one of these DlImage wrappers, we'll have to forward that unref to a thread.

Shouldn't this be held in a SkiaGPUObject here and then the DlObject you create here doesn't need to use a SkiaGPUObject?

Using a SkiaGPUObject here would mean that Impeller textures would require an unnecessary thread hop on each collection. Impeller has no restriction on where textures may be collected and it (thread hops) seemed like something that ought to be left to the backend and not performed by default.

lib/ui/painting/display_list_image_gpu.h

flar · 2022-03-29T23:29:39Z

shell/common/platform_view.h

  ///
-  /// @attention  Unlike all other methods on the platform view, this will be
+  /// @attention  Unlike all other methods on the platform view, this will
+  /// be


odd wrapping here...

flar

I spotted a nit about odd wrapping and left a comment.

Everything looks good to me, but I only had passing familiarity with the code in the shell directory so it would be good to get a second pair of eyes on that. Also, much of the impeller decoder appears to happen somewhere else compared to the skia decoder and I'm going to trust that that "somewhere else" is doing the right thing for now.

fluttergithubbot · 2022-03-30T00:45:05Z

This pull request is not suitable for automatic merging in its current state.

The status or check suite build_and_test_linux_unopt_debug has failed. Please fix the issues identified (or deflake) before re-applying this label.

chinmaygarde · 2022-03-30T01:15:32Z

Hmm, the cancellation of build_and_test_linux_unopt_debug is making the presub trip over the tree status. The cancellation was to work around the .ci.yaml validation failure.

chinmaygarde · 2022-03-30T20:31:29Z

Whew, the tree is finally green.

* cb9232b [web] webOnlyWarmupEngine + js-interop APIs (flutter/engine#32017) * 88b7f72 [macOS] Generate gen_snapshot_$arch by default (flutter/engine#32326) * 6dd43e6 Add support for images in display lists. (flutter/engine#32268) * a1106da [web] Make "felt build" wait for both engine and canvaskit builds (flutter/engine#32308)

kevmo314 · 2022-06-03T18:36:02Z

Hi, I think this PR is causing a crash in our app: flutter/flutter#105315

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label Mar 25, 2022

flutter-dashboard bot added embedder Related to the embedder API platform-ios labels Mar 25, 2022

chinmaygarde force-pushed the drawimage branch 3 times, most recently from a52b4c1 to 45e8528 Compare March 28, 2022 21:44

Add support for images in display lists.

3ee9dcb

chinmaygarde force-pushed the drawimage branch from 45e8528 to 3ee9dcb Compare March 28, 2022 22:46

chinmaygarde requested review from zanderso, flar and bdero March 28, 2022 22:46

chinmaygarde removed the Work in progress (WIP) Not ready (yet) for review! label Mar 28, 2022

chinmaygarde mentioned this pull request Mar 28, 2022

Use display list images. flutter/impeller#102

Merged

chinmaygarde added 4 commits March 28, 2022 16:06

PR

7fb2d19

PR

8431258

Licenses

f122a55

PR

8dca8d2

flar reviewed Mar 29, 2022

View reviewed changes

This was referenced Mar 29, 2022

Rendering a display list to an existing builder does not respect opacity and reset attributes. flutter/flutter#100983

Closed

DlImageColorSource should work with DlImages instead of SkImages. flutter/flutter#100984

Closed

chinmaygarde added 3 commits March 29, 2022 12:13

Address CI failures.

94d0e24

PR

e297978

PR

705ed2c

zanderso reviewed Mar 29, 2022

View reviewed changes

PR

af69189

flar reviewed Mar 29, 2022

View reviewed changes

lib/ui/painting/display_list_image_gpu.h Show resolved Hide resolved

flar reviewed Mar 29, 2022

View reviewed changes

flar approved these changes Mar 29, 2022

View reviewed changes

PR

a0d57b5

chinmaygarde closed this Mar 30, 2022

chinmaygarde reopened this Mar 30, 2022

chinmaygarde added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 30, 2022

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 30, 2022

chinmaygarde added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 30, 2022

chinmaygarde merged commit 6dd43e6 into flutter:main Mar 30, 2022

chinmaygarde deleted the drawimage branch March 30, 2022 20:31

engine-flutter-autoroll mentioned this pull request Mar 30, 2022

Roll Engine from ba64178bea8d to a1106daceddf (4 revisions) flutter/flutter#101080

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 30, 2022

6dd43e6 Add support for images in display lists. (flutter/engine#32268)

bd0cce5

zanderso mentioned this pull request Mar 31, 2022

Regression in raster time in opacity_peephole_grid_of_opacity_perf__e2e_summary flutter/flutter#101122

Closed

kevmo314 mentioned this pull request Jun 3, 2022

Custom ImageProvider in Drawer causes engine crash flutter/flutter#105315

Closed

flar mentioned this pull request Apr 18, 2023

Make the display list interface graphics package neutral. flutter/flutter#95434

Closed

		@@ -4,13 +4,35 @@

		#include "flutter/lib/ui/painting/image_decoder.h"

		#include <algorithm>

		// found in the LICENSE file.

		#include "flutter/lib/ui/painting/image_decoder_skia.h"

Add support for images in display lists. #32268

Add support for images in display lists. #32268

Uh oh!

Conversation

chinmaygarde commented Mar 25, 2022

Uh oh!

chinmaygarde commented Mar 28, 2022

Uh oh!

zanderso commented Mar 29, 2022

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar commented Mar 29, 2022

Uh oh!

chinmaygarde commented Mar 29, 2022

Uh oh!

chinmaygarde commented Mar 29, 2022

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flar left a comment

Choose a reason for hiding this comment

Uh oh!

fluttergithubbot commented Mar 30, 2022

Uh oh!

chinmaygarde commented Mar 30, 2022

Uh oh!

chinmaygarde commented Mar 30, 2022

Uh oh!

kevmo314 commented Jun 3, 2022

Uh oh!

Uh oh!