New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove most use of the local prim rect by primitive templates. #3381

Merged
merged 2 commits into from Dec 4, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 3, 2018

The current picture caching code invalidates cached regions more
than expected, due to how gecko handles scroll offsets.

If a display list is received, then a scroll event occurs, and then
a new display list is sent, the local rects will contain different
coordinates, since the scroll offsets are baked into the local
rects rather than in the scroll / reference frames. This results
in primitives being interned with a different value, and cached
surfaces being incorrectly invalidated.

Since the way this works is non-trivial to change in Gecko, we need
to handle this in WR.

If we change primitive and clip keys and templates to store their
rectangles in a true local primitive space (where the origin is
always zero) then the interning will work as expected. To do this,
we'll store the origin of a primitive in the instance, and only
the size in the template.

A side benefit of this approach is that test cases like dl_mutate
will become much faster, since the only difference between the
thousands of rectangles is the origin. This will effectively
allow true instancing of primitives with common data.

This is the first step towards that - by removing reliance of the
primitive local rect from the primitive template code. The rest
will be done as a series of incremental patches.


This change is Reviewable

Remove most use of the local prim rect by primitive templates.
The current picture caching code invalidates cached regions more
than expected, due to how gecko handles scroll offsets.

If a display list is received, then a scroll event occurs, and then
a new display list is sent, the local rects will contain different
coordinates, since the scroll offsets are baked into the local
rects rather than in the scroll / reference frames. This results
in primitives being interned with a different value, and cached
surfaces being incorrectly invalidated.

Since the way this works is non-trivial to change in Gecko, we need
to handle this in WR.

If we change primitive and clip keys and templates to store their
rectangles in a true local primitive space (where the origin is
always zero) then the interning will work as expected. To do this,
we'll store the origin of a primitive in the instance, and only
the size in the template.

A side benefit of this approach is that test cases like dl_mutate
will become much faster, since the only difference between the
thousands of rectangles is the origin. This will effectively
allow true instancing of primitives with common data.

This is the first step towards that - by removing reliance of the
primitive local rect from the primitive template code. The rest
will be done as a series of incremental patches.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 3, 2018

r? @kvark or @nical

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 4, 2018

If a display list is received, then a scroll event occurs, and then
a new display list is sent, the local rects will contain different
coordinates, since the scroll offsets are baked into the local
rects rather than in the scroll / reference frames.

Why isn't a scroll just changing a transform property binding without re-sending the DL or affecting the local rects?

@kvark

kvark approved these changes Dec 4, 2018

Copy link
Member

kvark left a comment

A few concerns, otherwise LGTM

prim_rect,
[0.0; 4],
);
}
PrimitiveTemplateKind::LinearGradient { ref brush_segments, .. } |
PrimitiveTemplateKind::RadialGradient { ref brush_segments, .. } => {
for segment in brush_segments {

This comment has been minimized.

@kvark

kvark Dec 4, 2018

Member

is it fine to still be writing the full rects for the segments?

@@ -586,7 +586,7 @@ impl AlphaBatchBuilder {
};

let instance = PrimitiveInstanceData::from(BrushInstance {
segment_index: 0,
segment_index: INVALID_SEGMENT_INDEX,

This comment has been minimized.

@kvark

kvark Dec 4, 2018

Member

Is this unrelated? Are we changing this segment index at some later point?

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @kvark)


webrender/src/batch.rs, line 589 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Is this unrelated? Are we changing this segment index at some later point?

INVALID_SEGMENT_INDEX is a sentinel value that tells the brush shader code to use the prim rect in the header, and that there is no segments in the trailing GPU cache data.


webrender/src/prim_store.rs, line 1190 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is it fine to still be writing the full rects for the segments?

No, that needs to change too. My plan is to make them zero-based (relative to the primitive instance origin) in a follow up patch to this.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 4, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 4, 2018

📌 Commit 3f30831 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 4, 2018

⌛️ Testing commit 3f30831 with merge add1538...

bors-servo added a commit that referenced this pull request Dec 4, 2018

Auto merge of #3381 - gw3583:seg-fix-1, r=kvark
Remove most use of the local prim rect by primitive templates.

The current picture caching code invalidates cached regions more
than expected, due to how gecko handles scroll offsets.

If a display list is received, then a scroll event occurs, and then
a new display list is sent, the local rects will contain different
coordinates, since the scroll offsets are baked into the local
rects rather than in the scroll / reference frames. This results
in primitives being interned with a different value, and cached
surfaces being incorrectly invalidated.

Since the way this works is non-trivial to change in Gecko, we need
to handle this in WR.

If we change primitive and clip keys and templates to store their
rectangles in a true local primitive space (where the origin is
always zero) then the interning will work as expected. To do this,
we'll store the origin of a primitive in the instance, and only
the size in the template.

A side benefit of this approach is that test cases like dl_mutate
will become much faster, since the only difference between the
thousands of rectangles is the origin. This will effectively
allow true instancing of primitives with common data.

This is the first step towards that - by removing reliance of the
primitive local rect from the primitive template code. The rest
will be done as a series of incremental patches.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3381)
<!-- Reviewable:end -->
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 4, 2018

If a display list is received, then a scroll event occurs, and then
a new display list is sent, the local rects will contain different
coordinates, since the scroll offsets are baked into the local
rects rather than in the scroll / reference frames.

Why isn't a scroll just changing a transform property binding without re-sending the DL or affecting the local rects?

It is. However, if you scroll in a way that adjusts the scroll transform (as above), then do something that triggers Gecko to send a new DL (e.g. hover over an element that highlights), then Gecko will send a new DL where the current scroll offsets are baked into the local primitive rects, rather than a scroll frame. 😞

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 4, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing add1538 to master...

@bors-servo bors-servo merged commit 3f30831 into servo:master Dec 4, 2018

3 of 4 checks passed

code-review/reviewable 2 files, 2 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gw3583 gw3583 deleted the gw3583:seg-fix-1 branch Dec 4, 2018

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 4, 2018

Bug 1511814 - Update webrender to commit add1538234a214a371e471bff677…
…34afd7c3ca95 (WR PR #3381). r=kats

servo/webrender#3381

Differential Revision: https://phabricator.services.mozilla.com/D13724

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment