WIP: API to track damage rectangles #21126

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

Description

This PR replaces #21009

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-14T21:45:17Z

flow/layers/backdrop_filter_layer.cc

+bool BackdropFilterLayer::compare(const Layer* l1, const Layer* l2) {
+  const auto* bf1 = reinterpret_cast<const BackdropFilterLayer*>(l1);
+  const auto* bf2 = reinterpret_cast<const BackdropFilterLayer*>(l2);
+  auto d1 = bf1->filter_->serialize();


ImageFilter objects should be stable from frame to frame. They should only be replaced in a tree when their contents differ. Are you seeing a lot of cases where the object is different even though the serialized form is the same? If so, then we should fix that.

I'll look into it.

flar · 2020-09-14T21:47:32Z

flow/layers/image_filter_layer.cc

+bool ImageFilterLayer::compare(const Layer* l1, const Layer* l2) {
+  const auto* bf1 = reinterpret_cast<const ImageFilterLayer*>(l1);
+  const auto* bf2 = reinterpret_cast<const ImageFilterLayer*>(l2);
+  auto d1 = bf1->filter_->serialize();


Same comment about ImageFilter stability allowing a comparison of the object rather than the contents.

flar · 2020-09-14T21:50:33Z

flow/damage_context.cc

+  if (index == size_t(-1)) {
+    layer_entries_.push_back(std::move(e));
+  } else {
+    layer_entries_.insert(layer_entries_.begin() + index, std::move(e));


While it isn't likely to be a problem in practice due to the tree-traversal nature of when this operation is called, an insertion here can invalidate tracking places by remembering a "count". It would be more stable to have layers that need to "go back and insert a contribution" to insert the contribution before recursing to children, or if they need to do it after the fact then to insert a dummy contribution to claim the slot that gets replaced after recursing to the children?

The reason for this is that pushing layer contribution requires paint bounds. For some layers (i.e. image filter) those are known only after prerolling the children. So we first preroll the children to determine paint bounds and then contribute the layer at order below children.

flar · 2020-09-14T22:00:24Z

flow/layers/backdrop_filter_layer.cc

+  if (context->damage_context) {
+    // Adjust display bounds to layers below this layer
+    context->damage_context->ApplyImageFilter(0, count, filter_.get(), matrix,
+                                              paint_bounds());


Why is this not done up above before recursing to children?

Because before recursing to children we don't know the bounds.

However this is not going to work anyway I'm afraid. I don't think we can partially redraw clipped portion of backdrop filter. I didn't really think this through. The problem is, that at the edges of clip, it samples pixels outside of clip, which means it would sample pixels from its own children painted in previous frame.

So right now it seems that the easiest solution to get correct output would be to check if the final damage area intersect any backdrop filter, and if it does, extend it to cover entire backdrop filter (+filter bounds?). That way we always redraw entire backdrop filter, but only if any layer that needs to be redrawn intersects it.

I think I understand what you are getting at, but if the BDF expands the dirty regions below it to include its source region then I think it is OK. It has to do that in a way that is not clipped, though.

For most layers there is a simple "I require no input from the screen and I affect these pixels".
For BDF it has both "I require input from this region (regardless of clipping) and I then affect these pixels (which is subject to the clip)". The trick is to express that "where you get your data from" concept with a different handling of clipping.

I see that BDF suffers from a cancerous growth of bounds. If it needs to produce output for x,y,w,h then it needs input from x-d,y-d,w+2d,h+2d. But, if we then redraw that new rectangle in order to give it the input it needs and we do that in the original framebuffer then that rendering would require input from x-2d,y-2d,w+4d,h+4d, rinse and repeat up to the maximum clipped bounds of the BDF output (+d).

If we render into an intermediate buffer instead and then just copy the clipped output into the framebuffer, then we can get away with just one expansion of the bounds. The original need to render x,y,w,h would be performed into a surface that was w+2d,h+2d in size. When it was done we'd copy out the middle w,h part of it into the framebuffer. That requires an extra surface to do, but if the repaint region was tiny then it wouldn't matter.

Alternately, for a clipped BDF, we just repaint its clipped size +d and if that is small enough then doing it in place might be the best choice rather than using an intermediate buffer.

Indeed. Rendering everything in w+2d,h+2d surface and then just blitting w,h should fix this. But I was also thinking about another optimization. With damage context we should be able determine, that between past and current frame no layer below BDF (that intersects it) needs to be repainted. If that's the case for n subsequent frames, perhaps we could take snapshot of BDF area (after blur and before children) and use it until invalidated. To go even further, if only small area underneath changes, blur it (+d) and for the rest use the snapshot.

I have to think this through. I think for the next iteration I'll focus on making things render correctly (at the expense of repainting entire BDF if needed).

flar · 2020-09-14T22:00:57Z

flow/layers/backdrop_filter_layer.cc

+                                              paint_bounds());
+    // Push entry before children
+    context->damage_context->PushLayerContribution(this, compare, matrix, *context,
+                                            count);


Why does this need to be pushed after the children are prerolled?

It needs to be done after children are prerolled, because otherwise we don't know the paint bounds.

Can it be pushed and then have its bounds updated later? Something like:

Contribution c = push(me); ...recurse...; c.adjustBounds();

Sure. I wanted to make LayerContribution private but I don't have any strong feelings about it. Pushing it on arbitrary index does feel bit weird, so this is probably better.

If it only needs to adjust the bounds, then it could be private and hidden behind a more public interface that exposes just that functionality.

flar · 2020-09-14T22:12:09Z

flow/damage_context.h

+    std::shared_ptr<const Layer> layer;
+    LayerComparator comparator;
+
+    std::vector<std::shared_ptr<Mutator>> mutators;


If the DamageContext (and/or the PrerollContext) kept a linked list of Mutator objects then that would save a lot of vector creation of vectors that mostly contain the same objects.

A LayerContribution would simply copy over the pointer from the Context at the time of its creation.

Adding a mutator would simply be "mutator_links = MutatorLink(Mutator, mutator_links)"
Removing a mutator would simply be "mutator_links = mutator_links.next"

Comparisons could even be short circuited. Once we determine that linkX from the previous frame matches linkY from the new frame, then we can tag them as being equal (which implies that their "next" chains are also equal) then we can tag them as equal and avoid recursive comparisons when they appear opposite each other on future chain tests.

I remember this from first review. I like the idea, but I think the list would need to be double linked (mutator stack exposes both iterator and reverse_iterator). And the iterators would also need to be implemented.

Double linking the list would remove the ability to snapshot it with a pointer copy. The idea was that the earlier entries on the chain, coming from the top-level layers, would stay there and occasionally have a new mutator point to them. But if they also need a reverse pointer then we run into a problem.

The only place I saw iteration was in the comparison of the list. Is that any simpler than double-side-by-side iterations of the two linked lists instead?

The stack seems to be iterated from bottom to top in EmbedderLayers::PushPlatformViewLayer and in FuchsiaExternalViewEmbedder::BeginFrame. I mean technically even with double linked the 'prev' pointer (top to bottom) never changes, so we can "snapshot" the top mutator, as long as we ignore the other pointer and only iterate to the bottom, which is enough for damage_context.

Ah, yes, if we replace the stack in the PrerollContext, then that would be an issue. If we shadow the stack in the DamageContext then we can get away with it. Since we are currently shadowing the stack in every Contribution object then shadowing the stack once in DamageContext would still reduce storage requirements by a large margin.

I think you are correct that snapshotting would still work if the snapshot list is only iterated forward, but I need to mull it a little more.

Also, only doing the linked list in a shadow copy means we don't need to modify the entire engine. Perhaps we could do that for starters and if this gets in there and gets vetted then we can have a follow-on task to promote that to the PrerollContext.

Possible solution: 76bca33#diff-7b84dd247cd3f78e219d26495e874ef4

It doesn't disrupt existing code in any way and reference to MutatorNode is essentially snapshot of MutatorStack.

flar · 2020-09-14T22:13:30Z

flow/layers/image_filter_layer.cc

+    // Adjust display bounds for newly added layers
+    context->damage_context->ApplyImageFilter(
+        count, context->damage_context->layer_entries_count(), filter_.get(),
+        matrix, paint_bounds());


Perhaps add a synthetic mutator to the DamageContext that performs these bounds adjustments on the fly?

flar · 2020-09-14T22:13:48Z

flow/layers/image_filter_layer.cc

+
+    // Push entry before children
+    context->damage_context->PushLayerContribution(this, compare, matrix, *context,
+                                            count);


Can this be inserted before recursion to avoid having to remember the count?

knopp · 2020-09-18T16:13:51Z

@flar, thanks for the feedback! I incorporated the changes in #21267.

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

googlebot added the cla: yes label Sep 11, 2020

knopp mentioned this pull request Sep 11, 2020

WIP: API to track damage rectangles #21009

Closed

12 tasks

auto-assign bot requested a review from iskakaushik September 11, 2020 23:26

Add damage_context to determine damage rectangle

495210e

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

knopp force-pushed the damage_rect_wip_pr2 branch from 6e39c76 to 495210e Compare September 11, 2020 23:28

liyuqian requested review from flar and removed request for iskakaushik September 11, 2020 23:52

flar requested changes Sep 14, 2020

View reviewed changes

knopp mentioned this pull request Sep 18, 2020

WIP: API to track damage rectangles #21267

Closed

12 tasks

knopp closed this Sep 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: API to track damage rectangles #21126

WIP: API to track damage rectangles #21126

knopp commented Sep 11, 2020

flar Sep 14, 2020

knopp Sep 14, 2020

flar Sep 14, 2020

flar Sep 14, 2020

knopp Sep 14, 2020

flar Sep 14, 2020

knopp Sep 14, 2020 •

edited

flar Sep 14, 2020

flar Sep 14, 2020

knopp Sep 15, 2020 •

edited

flar Sep 14, 2020

knopp Sep 14, 2020

flar Sep 14, 2020

knopp Sep 14, 2020

flar Sep 14, 2020

flar Sep 14, 2020

knopp Sep 14, 2020 •

edited

flar Sep 14, 2020

knopp Sep 14, 2020

flar Sep 14, 2020

knopp Sep 15, 2020 •

edited

flar Sep 14, 2020

flar Sep 14, 2020

knopp commented Sep 18, 2020

WIP: API to track damage rectangles #21126

WIP: API to track damage rectangles #21126

Conversation

knopp commented Sep 11, 2020

Description

Related Issues

Checklist

Breaking Change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knopp commented Sep 18, 2020

knopp Sep 14, 2020 •

edited

knopp Sep 15, 2020 •

edited

knopp Sep 14, 2020 •

edited

knopp Sep 15, 2020 •

edited