[WIP] Display list picture prototype #25234

flar · 2021-03-25T11:29:46Z

This PR is now ready for review.

This PR represents an alternative storage format for our rendering that removes a large dependency on the SkPicture format and improves our ability to inspect rendering operations for future optimization work.

An SkPicture will still be involved as the current way we handle text with SkParagraph objects requires an SkPicture to store the output of the paragraph rendering, but that limitation can be overcome with some more work.

~~This is just a first pass prototype for what it might look like to use our own recording format in lieu of SkPicture. The design is basic and skips a lot of advanced rendering for now:~~

~~images, paragraphs, paths~~ (all are implemented, although paragraphs are turned into SkPictures for now)
~~filters and shaders~~ (now implemented)
transform(4x4) (we pass the appropriate parts of the transform to Skia as they support, but our bounds calculations in the Dart code may be inaccurate for anything beyond 2x3)

~~Most of that can be implemented as soon as I add code to digest the "native Skia objects" used to represent those attributes and operations.~~

~~This PR is being run through pre-submit testing with the new mechanism enabled to check for incompatibilities and fix them, but it will eventually be pushed with the mechanism turned off.~~ (testing done, default set back to Skia versions of the objects - all unit tests pass using the DisplayList versions save for a test that makes assumptions that a generated Picture object will be using an SkPicture object natively - the test will have to be disabled if we switch over to the new display list code permanently).

flutter-dashboard · 2021-03-25T11:29:48Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde · 2021-03-25T16:02:00Z

It's awesome to have demonstrated that the current API can be decomposed down to canvas ops. What isn't clear to me right now the framework to be used to determine how to rearrange these ops based on optimizations we want to perform. Perhaps you are going to attempt that in a later patch. My guess is that once this framework is in place, we will need to rethink these ops. Right now, they are just based on the current Skia API.

BTW, I am not super concerned that the remaining ops cannot be handled with just as much ease. Would Dart FFI help at all here to represent these native objects?

ds84182 · 2021-04-03T16:17:52Z

This prototype is looking great so far!

I'm wondering if display list size & growth could have a net negative effect on highly dynamic pictures, like Rive animations. Of course it would be great to profile before acting on that, as the benefits from building display lists entirely in Dart, instead of jumping back and forth between Dart and C++ through the native API, probably outweighs any benefits from chaining new op + data buffers instead of growing them.

flar · 2021-04-05T17:51:35Z

This prototype is looking great so far!

I'm wondering if display list size & growth could have a net negative effect on highly dynamic pictures, like Rive animations. Of course it would be great to profile before acting on that, as the benefits from building display lists entirely in Dart, instead of jumping back and forth between Dart and C++ through the native API, probably outweighs any benefits from chaining new op + data buffers instead of growing them.

The most recent commit switched from growable lists to caching the longest so far. It needs more work to detect when the cached list has a legacy length that isn't needed any more (a statistics set associated with the buffer perhaps), but before the change I was seeing multi-millisecond spikes in the UI thread while running an app that produced long display lists and afterwards it was very smooth with a very fast average time.

The current prototype is quite runnable - feel free to download it for testing and report back (which commit hash was used and) the results you might see on some complicated examples. The "contributing" guideline in the main Flutter repo has information on building a local engine and testing with it.

zanderso · 2021-05-06T17:40:28Z

What is the next step for this?

flar · 2021-05-06T19:33:06Z

I've been working on getting our engine's "picture cache" to add support for these display lists for at least some parity on performance.

…during shader warmup, pass down object list

…d code

flow/display_list_interpreter.cc

dnfield · 2021-05-11T19:55:50Z

flow/display_list_interpreter.cc

+    FML_LOG(ERROR) << "Starting ops: " << ops_vector_->size()
+                   << ", data: " << data_vector_->size()
+                   << ", refs: " << ref_vector_->size();
+    FML_LOG(ERROR) << "Remaining ops: " << (it.ops_end - it.ops)
+                   << ", data: " << (it.data_end - it.data)
+                   << ", refs: " << (it.refs_end - it.refs)
+                   << ", save count delta: "
+                   << (canvas->getSaveCount() - entrySaveCount);


These will probably get changed to FML_DCHECKs right?

dnfield · 2021-05-11T19:55:58Z

flow/display_list_interpreter.cc

+  int entrySaveCount = canvas->getSaveCount();
+  Iterator it(this);
+  while (it.HasOp()) {
+    FML_LOG(INFO) << DescribeNextOp(it);


lib/ui/painting.dart

lib/ui/painting/display_list.cc

dnfield · 2021-05-11T20:53:39Z

lib/ui/painting/display_list.cc

+      std::make_shared<std::vector<DisplayListRefHolder>>();
+  int obj_index = 0;
+  SkSamplingOptions sampling = DisplayListInterpreter::NearestSampling;
+  for (uint8_t op : *ops_vector) {


This seems like logic we should be testing. Could we move it to a dedicated class or method(s) that get tested? It would also make this method easier to read.

Which logic?

It appears to me that I may not have adequate assertions/protections that the vectors are all of the appropriate length for iterating.

The parsing logic in here - the whole switch statement etc. We should have tests that say something like given this input, we get this data structure back.

lib/ui/painting/display_list.h

dnfield · 2021-05-11T20:56:42Z

Very cool, I'm excited for this.

Would you consider breaking this up into some smaller PRs to ease review?

yjbanov

This PR needs a substantial amount of tests for the amount of functionality it's adding.

My other thought is that there's a big overlap with the existing web code. We also have a plain Dart implementation of a "display list", including bounds estimation, including a plain Dart path bounds estimator. Can we somehow share this code?

lib/ui/painting.dart

yjbanov · 2021-05-12T00:15:39Z

lib/ui/painting.dart

+    _updatePaintData(paint, _drawMask);
+    _addOp(_CanvasOp.drawPath, 0);
+    _addPathData(path);
+    _addBounds(path.getBounds());


Path.getBounds overestimates its size, which is fine for the purposes of computing the bounds of the picture (I'm assuming this is to be used for optimization, such as pruning of paint ops). This is also the case on the web. Example:

https://github.com/flutter/goldens/blob/master/engine/web/paint_bounds_for_line_rotated.png

yjbanov · 2021-05-12T00:20:07Z

BTW, the web version of this also has a pretty decent test suite, including golden tests: https://github.com/flutter/goldens/blob/master/engine/web/paint_bounds_for_line_rotated.png. So I think sharing code would be beneficial both for web and mobile.

yjbanov · 2021-05-12T00:26:05Z

An SkPicture will still be involved as the current way we handle text with SkParagraph objects requires an SkPicture to store the output of the paragraph rendering, but that limitation can be overcome with some more work.

On the web we'll be soon migrating from drawParagraph to drawGlyphs (for memory reasons), so we won't need SkParagraph for pictures. Perhaps it will be useful here too.

flar · 2021-05-13T00:47:11Z

@yjbanov Is there a good way to share this with web? I remember having a conversation with you that suggested that there was a reason a bit of code was duplicated between web and dart:ui based on the capabilities of how to create a dart package.

flar · 2021-05-13T20:59:13Z

An SkPicture will still be involved as the current way we handle text with SkParagraph objects requires an SkPicture to store the output of the paragraph rendering, but that limitation can be overcome with some more work.

On the web we'll be soon migrating from drawParagraph to drawGlyphs (for memory reasons), so we won't need SkParagraph for pictures. Perhaps it will be useful here too.

That would be something for @jason-simmons to consider and comment on.

dnfield · 2021-05-19T02:46:11Z

flow/display_list_interpreter.h

+    const SkImage* GetImage() { return (refs++)->image.get(); }
+    const SkVertices* GetVertices() { return (refs++)->vertices.get(); }
+    const SkPicture* GetSkPicture() { return (refs++)->picture.get(); }


I would find it helpful if we documented why we're returning a raw pointer here and what the ownership expectations are. It seems like we're saying that at this pointer is only good until the iterator is advanced again?

I'm a little worried that this will be brittle to refactoring and testing. It would be easier to reason about this API and review future refactoring of it if it just had a clear ownership model - but at least if it's documented we won't have to re-read it to figure out what's going on.

I'm waffling on whether to return Refs or pointers here. For most cases, we don't need to take ownership of these values because they are used in a single op that goes out of scope.

Note that I still return Refs for attributes because SkPaint takes refs (for obvious reasons).

dnfield · 2021-05-19T02:46:46Z

flow/layers/display_list_layer.cc

-  //   << draw_rect_.top() << ", "
-  //   << draw_rect_.right() << ", "
-  //   << draw_rect_.bottom() << "]";
+  // TODO(flar): implement DisplayList raster caching


nit: please link an issue here explaining what needs to be done, perhaps with a link to the commit(s) containing the start of the implementation.

I will want to file issues eventually, but I feel odd filing an issue for a piece of code that isn't merged yet and this code may have a few more weeks before it can be merged.

dnfield · 2021-05-19T02:46:56Z

flow/layers/display_list_layer.cc

-  //   << paint_bounds().right() << ", "
-  //   << paint_bounds().bottom() << "] "
-  //   << ops_vector_.size() << " ops";
+  // TODO(flar): implement DisplayList raster caching


dnfield · 2021-05-19T02:49:58Z

lib/ui/painting.dart

  static Uint32List _emptyInts = _emptyData.buffer.asUint32List();
  static Float32List _emptyFloats = _emptyData.buffer.asFloat32List();
-  static List<Object> _deadObjects = List<Object>.empty(growable: false);
+  static List<NativeFieldWrapperClass2> _deadObjects = List<NativeFieldWrapperClass2>.empty(growable: false);


I think this could just be static const List<NativeFieldWrapperClass2> _deadObjects = <NativeFieldWrapperClass2>[];.

For that matter, you could just inline that into the dispose method below.

Originally I was going to use these as a flag for the StateError below to detect when a developer was trying to reuse a Canvas after endRecording, but I instead test for the Recorder link so I don't really need most of these values to be static any more.

I'm not sure of the cost of creating the empty byte data and then creating an int/float buffer from it, though.

If it's const it basically is baked into the snapshot and is o(1) to construct at runtime

dnfield

This is looking better! Tests?

…back

flar · 2021-06-24T02:46:36Z

This prototype will likely be replaced by #26928

zanderso · 2021-06-24T15:27:18Z

I'll close this PR under the assumption that it will be superseded by #26928.

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

google-cla bot added the cla: yes label Mar 25, 2021

googlebot added the cla: yes label Mar 25, 2021

zanderso mentioned this pull request Apr 21, 2021

[Performance] Gif will make GC frequently, and it'll make the phone heat up flutter/flutter#80702

Closed

flar force-pushed the DisplayListPicture-implementation branch from b914126 to bb77c80 Compare May 7, 2021 02:42

flar added 19 commits May 9, 2021 21:47

prototype of dart-level display list replacement for (Sk)Picture

74ae193

partially working for most canvas calls, but corrupts after a while

3d8f8a9

move storage of data to native DisplayList and implement toImage()

8e1bd5e

Fix a number of inconsistent data vector accesses, memory corruption …

a33ee18

…during shader warmup, pass down object list

fix angle extraction and missing break statements

bb9f648

remove/silence overly verbose debugging prints

23c5f0f

foreach macro dispatching of canvas ops

e75156d

de-inline description function, fix an ops arg count

e79fc24

embrace SkSamplingOptions, preparing for SkFilterQuality removal

3c36735

Formalize parameter descriptions of display list ops, remove some dea…

d3f2ea2

…d code

move Int/Float lists to data array and implement drawPoints

f299605

implement drawImage calls and setColor/ImageFilter

d2d6284

add support for shaders and vertices

7f18ebf

implement drawParagraph via SkPicture

9ba2de0

adjust some paint attribute defaults and masks

90a37ce

add ability to render new _DisplayListPicture objects

e4c52bd

implement drawPath/Shadow and a few bounds improvements

e6c51c5

implement bounds updates for last few canvas ops

1702fa3

fix paint attribute bit constants

fe78657

import cmath for M_PI on Windows

ea82498

zanderso mentioned this pull request May 10, 2021

Long queue times on Engine presubmits flutter/flutter#82204

Closed

flar added 4 commits May 10, 2021 11:59

remove dead code

8a747f0

remove use of problematic M_PI constant

c13700e

more accurate radian conversion

87aeac5

reverted default back to Skia pictures for compatibility

dc51f7a

flar removed the Work in progress (WIP) Not ready (yet) for review! label May 10, 2021

missing filterQuality args on SkiaCanvas drawAtlas native calls

b37b350

flar mentioned this pull request May 11, 2021

SceneBuilder.addPicture returns the layer #26074

Merged

dnfield suggested changes May 11, 2021

View reviewed changes

yjbanov reviewed May 12, 2021

View reviewed changes

review feedback

2d8711b

dnfield reviewed May 19, 2021

View reviewed changes

flar added 2 commits May 21, 2021 15:01

remove unused includes and dependency directories

9d69ead

better error handling in DisplayList constructor and more review feed…

68b620d

…back

chinmaygarde added the Work in progress (WIP) Not ready (yet) for review! label May 27, 2021

flar mentioned this pull request Jun 24, 2021

Implement a DisplayList mechanism similar to the Skia SkLiteDL mechanism #26928

Merged

flar changed the title ~~Display list picture prototype~~ [WIP] Display list picture prototype Jun 24, 2021

zanderso closed this Jun 24, 2021

[WIP] Display list picture prototype #25234

[WIP] Display list picture prototype #25234

Uh oh!

Conversation

flar commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Mar 25, 2021

Uh oh!

chinmaygarde commented Mar 25, 2021

Uh oh!

ds84182 commented Apr 3, 2021

Uh oh!

flar commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zanderso commented May 6, 2021

Uh oh!

flar commented May 6, 2021

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dnfield commented May 11, 2021

Uh oh!

yjbanov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yjbanov commented May 12, 2021

Uh oh!

yjbanov commented May 12, 2021

Uh oh!

flar commented May 13, 2021

Uh oh!

flar commented May 13, 2021

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!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

flar commented Jun 24, 2021

Uh oh!

zanderso commented Jun 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

flar commented Mar 25, 2021 •

edited

Loading

flar commented Apr 5, 2021 •

edited

Loading