WIP: API to track damage rectangles #21009

knopp · 2020-09-06T19:56:57Z

Description

This PR adds api to track damage rectangles during rendering. Usage is relatively simple, it introduces the following structure in compositor_context.h

struct FrameDamage {
  // in: description for frame previously rasterized in target framebuffer
  const DamageContext::FrameDescription* previous_frame_description;

  // out: description for frame being rasterized
  std::unique_ptr<DamageContext::FrameDescription> frame_description;

  // out: area in framebuffer that has changed between frames;
  // if optional is empty, whole frame was repainted
  std::optional<DamageArea> damage_area;
};

This can be provided as argument to CompositorContext::ScopedFrame::Raster. The caller is responsible for providing previous_frame_description, which was obtained during previous rasterization to current framebuffer, as well as storing incoming frame_description.

Related Issues

flutter/flutter#33939

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the [contributor guide] and followed the process outlined there for submitting PRs.
I signed the [CLA].
I read and followed the [C++, Objective-C, Java style guides] for the engine.
I read the [tree hygiene] wiki page, which explains my responsibilities.
I updated/added relevant documentation.
All existing and new tests are passing.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

This shouldn't be breaking change, if frame_damage is not provided (default).
However it does prevent rendering of picture, physical shape and texture layers that are outside of cull rect.

Did any tests fail when you ran them? Please read [handling breaking changes].

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

matrix.preTranslate(x,y) is identical to matrix.setConcat(matrix, SkMatrix::Translate(x,y));

Damage rectangle can be determined by providing optional FrameDamage argument to CompositorContext::ScopedFrame::Render

flar · 2020-09-07T01:14:19Z

flow/damage_context.cc

+
+  for (auto i = preroll_context.mutators_stack.Begin();
+       i != preroll_context.mutators_stack.End(); ++i) {
+    auto type = (*i)->GetType();


A performance idea. Rather than keep the mutators in a stack, keep them in a manually linked list starting from most recent. This loop then becomes e.mutators = preroll_context.mutators. No copy is needed because each chain will be unique even if future nodes mutate preroll_context.mutators and once a linked-list-mutator is added to the chain, its next pointer never changes. It can be popped, and another mutator can be added in its place, but it will never point at a different "next mutator".

(with shared pointers in the linked list of course)

flar · 2020-09-07T01:22:00Z

flow/damage_context.h

+
+  struct DamageResult {
+    std::unique_ptr<FrameDescription> frame_description;
+    SkRect damage_rect;


Rather than hard code this to have a single exposed damage rect, why not have the rect be private and have an add method that can choose to do a single rect or multiple rects. For this first pass it might just join to a single global rect. In the future we can maintain a list of rects.

Readback should support iterating a list of damage rects (which would be just one for now).
Optionally, a global damage rect could be requested for cases where the caller wants to do a quick rejection of something on the entire set of damage regardless of whether the implementation kept multiple rects or not.

I wanted to talk about this. I agree that api should expose a list.

In theory we can use SkRegion to get a perfect list of non overlapping rectangles, but the issue here is that we also need to restrict rendering to those rectangles. That'd need either a preroll (for culling) and a paint for each rectangle, or cull only once for union and have skia clip to a complex shape.

So we might want to find a middle ground, i.e. if we have two small non overlapping rectangle in each corner, render them separately, but for most overlapped rectangles just join them.

I don't think we need to implement the multi-rect accumulation in the first pass, I was just saying that we should create an implementation that will work when we do. SkRegion supports iterating rectangles and if we had an IRect vector that would as well.

We should be able to pre-roll only once and then do multiple paint passes.

This commit introduces DamageArea class instead of exposing single rect. The CompositorContext behavior is changed so that it culls on DamageArea::bounds() and then clips and paints individual rects (DamageArea::GetRects()). At this point DamageArea only provides one rect identical to bounds.

flar · 2020-09-07T01:27:59Z

flow/damage_context.cc

+DamageContext::DamageResult DamageContext::FinishFrame() {
+  DamageResult res;
+  LayerEntrySet entries;
+  entries.insert(layer_entries_.begin(), layer_entries_.end());


Order matters. If we draw a rectangle and then a circle in one frame and in the next we've reversed their order and drawn the circle and then the rectangle (ignoring the fact that simply drawing a rectangle and a circle would get combined into a Picture, but generalize those concepts to 2 different engine layers that change order) - we need to repaint them. Using a set says that as long as they are still in the scene in the same device location we don't care when they get repainted. That is not true.

Indeed. Thanks for pointing this out. I'll address this.

knopp@b50df38 should take care of layers that are identical but painted in different order.

fredlee12345678 · 2020-09-07T02:48:34Z

@knopp do you save the previous painting to a texture ? I didn't found it. Maybe I miss your key point. In this solution, raster thread will need to call every layer's preroll and paint, even I just update a small rect, is that right?

knopp · 2020-09-07T23:04:26Z

@fredlee12345678, if damage rect is requested, a quick preroll (no raster cache) will first happen for every layer to determine damage rect, and then another "real" preroll, with raster cache enabled will be performed that will also cull layers that are outside of dirty rect.

after that, paint will happen for layers that have not been culled by preroll (layers that intersect damage rect) and the painting will also be clipped to damage rect by skia.

fredlee12345678 · 2020-09-08T03:50:49Z

@fredlee12345678, if damage rect is requested, a quick preroll (no raster cache) will first happen for every layer to determine damage rect, and then another "real" preroll, with raster cache enabled will be performed that will also cull layers that are outside of dirty rect.

after that, paint will happen for layers that have not been culled by preroll (layers that intersect damage rect) and the painting will also be clipped to damage rect by skia.

So...For example, I write a scroll view using grid widget have 10 cells, and I just change 1 cell. So my question is that, how many preroll function and paint function will you call? as your description, so will call 10 quick preroll, and 10 "real" preroll, and how many paint will happen? just painting the dirty layer, so how about the other 9 layers? do you restore the result of other 9 layers in damage context or other place?

These need to contribute to damage rect

IIf we go with deep compare this certainly helps performance However ideally we should just hash the content and compare hashes

This needs to be done as quickly as possible in order to determine dirty rectangle which will be used to cull layers in “real” preroll.

knopp · 2020-09-08T09:14:21Z

@fredlee12345678 how many layers will be painted depends on whether each cell has repaint boundary and also whether the backbuffer is intact. so in case, where cell has repaint boundary and back buffer was blit from and not flipped, we can in theory repaint just one cell.

fredlee12345678 · 2020-09-08T09:50:15Z

@fredlee12345678 how many layers will be painted depends on whether each cell has repaint boundary and also whether the backbuffer is intact. so in case, where cell has repaint boundary and back buffer was blit from and not flipped, we can in theory repaint just one cell.
@knopp Great!! It seems very good. As you mentioned " where cell has repaint boundary and back buffer was blit from and not flipped, we can in theory repaint just one cell." In this case, how about other cells?

In compositor_context.cc:
`// Clearing canvas after preroll reduces one render target switch when preroll
// paints some raster cache.

if (canvas()) {
if (needs_save_layer) {
FML_LOG(INFO) << "Using SaveLayer to protect non-readback surface";
SkRect bounds = SkRect::Make(layer_tree.frame_size());
SkPaint paint;
paint.setBlendMode(SkBlendMode::kSrc);
canvas()->saveLayer(&bounds, &paint);
}
canvas()->clear(SK_ColorTRANSPARENT);
}`

The canvas has been cleared. If you don't paint the other cells, it will blank. So how do you resolve this case?

knopp · 2020-09-08T09:56:00Z

@fredlee12345678, if you look right before the code you pasted:

  if (canvas() && damage_rect != frame_rect) {
    canvas()->save();
    canvas()->clipRect(damage_rect);
  }

Clearing the canvas and subsequent drawing is clipped on damage rect.

liyuqian · 2020-09-10T00:15:43Z

Left some suggestions in flutter/flutter#33939 (comment)

knopp · 2020-09-11T23:26:27Z

Moved to #21126.

auto-assign bot requested a review from gw280 September 6, 2020 19:57

googlebot added the cla: yes label Sep 6, 2020

knopp force-pushed the damage_rect_wip_pr branch from 7a54b54 to ded66c2 Compare September 6, 2020 20:09

knopp added 3 commits September 6, 2020 22:18

Use preTranslate when applying offset to matrix

7e9d793

matrix.preTranslate(x,y) is identical to matrix.setConcat(matrix, SkMatrix::Translate(x,y));

Do not paint layers when outside of cull rect

af5a9ed

Allow to specify initial cull_rect for preroll

c247ac1

knopp force-pushed the damage_rect_wip_pr branch 2 times, most recently from 0519e1f to fc8fffc Compare September 6, 2020 20:37

Add damage_context to determine damage recangle

f1b22b3

Damage rectangle can be determined by providing optional FrameDamage argument to CompositorContext::ScopedFrame::Render

knopp force-pushed the damage_rect_wip_pr branch from fc8fffc to f1b22b3 Compare September 6, 2020 20:41

knopp requested review from flar and liyuqian September 6, 2020 22:01

flar reviewed Sep 7, 2020

View reviewed changes

knopp force-pushed the damage_rect_wip_pr branch 3 times, most recently from b74cc95 to 116b346 Compare September 7, 2020 22:51

knopp force-pushed the damage_rect_wip_pr branch from 116b346 to 4805738 Compare September 7, 2020 23:35

knopp added 4 commits September 8, 2020 11:08

Detect reordered layers that are otherwise identical

b50df38

These need to contribute to damage rect

Cache serialized picture

62ff5e2

IIf we go with deep compare this certainly helps performance However ideally we should just hash the content and compare hashes

Ignore raster cache while doing damage rect preroll

4890bc9

This needs to be done as quickly as possible in order to determine dirty rectangle which will be used to cull layers in “real” preroll.

Add missing &

7184a54

knopp force-pushed the damage_rect_wip_pr branch from 659936f to 7184a54 Compare September 8, 2020 09:09

Introduce DamageArea to not expose damage rect directly

970713d

liyuqian mentioned this pull request Sep 10, 2020

Track damage rect and only draw inside it flutter/flutter#33939

Closed

knopp mentioned this pull request Sep 11, 2020

WIP: API to track damage rectangles #21126

Closed

12 tasks

knopp closed this Sep 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: API to track damage rectangles #21009

WIP: API to track damage rectangles #21009

knopp commented Sep 6, 2020 •

edited

flar Sep 7, 2020

flar Sep 7, 2020

flar Sep 7, 2020

knopp Sep 7, 2020 •

edited

flar Sep 7, 2020

knopp Sep 8, 2020

flar Sep 7, 2020

knopp Sep 7, 2020

knopp Sep 7, 2020 •

edited

fredlee12345678 commented Sep 7, 2020

knopp commented Sep 7, 2020

fredlee12345678 commented Sep 8, 2020

knopp commented Sep 8, 2020

fredlee12345678 commented Sep 8, 2020

knopp commented Sep 8, 2020

liyuqian commented Sep 10, 2020

knopp commented Sep 11, 2020

WIP: API to track damage rectangles #21009

WIP: API to track damage rectangles #21009

Conversation

knopp commented Sep 6, 2020 • edited

Description

Related Issues

Checklist

Breaking Change

flar Sep 7, 2020

Choose a reason for hiding this comment

flar Sep 7, 2020

Choose a reason for hiding this comment

flar Sep 7, 2020

Choose a reason for hiding this comment

knopp Sep 7, 2020 • edited

Choose a reason for hiding this comment

flar Sep 7, 2020

Choose a reason for hiding this comment

knopp Sep 8, 2020

Choose a reason for hiding this comment

flar Sep 7, 2020

Choose a reason for hiding this comment

knopp Sep 7, 2020

Choose a reason for hiding this comment

knopp Sep 7, 2020 • edited

Choose a reason for hiding this comment

fredlee12345678 commented Sep 7, 2020

knopp commented Sep 7, 2020

fredlee12345678 commented Sep 8, 2020

knopp commented Sep 8, 2020

fredlee12345678 commented Sep 8, 2020

knopp commented Sep 8, 2020

liyuqian commented Sep 10, 2020

knopp commented Sep 11, 2020

knopp commented Sep 6, 2020 •

edited

knopp Sep 7, 2020 •

edited

knopp Sep 7, 2020 •

edited