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

Include picture composite mode in primitive key for pictures. #3426

Merged
merged 1 commit into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 17, 2018

We need to ensure that a display list that is otherwise the same
except for a different compositing mode is detected by picture
caching code as invalid.

For example, if a picture has brightness(0.5) and then the next
display list is exactly the same but with brightness(0.6).

This patch adds a not particularly elegant key for picture
composite modes that is stored in the interned primitive data
for a picture, ensuring these cases are correctly detected.


This change is Reviewable

Include picture composite mode in primitive key for pictures.
We need to ensure that a display list that is otherwise the same
except for a different compositing mode is detected by picture
caching code as invalid.

For example, if a picture has brightness(0.5) and then the next
display list is exactly the same but with brightness(0.6).

This patch adds a not particularly elegant key for picture
composite modes that is stored in the interned primitive data
for a picture, ensuring these cases are correctly detected.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

Try run looks good.

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 17, 2018

Did you consider to only cache pre-composition results (which would not have this problem)? With this path, if there is an opacity animation, wouldn't we then invalidate the picture every frame?

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

I do think it's something worth considering in the future as we make the caching support more granular.

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Dec 17, 2018

What is the motivation for caching the post-composition contents?

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

The point of this change is so that a nested child picture inside a tile cache which had a composite mode is correctly detected as invalidating that tile when its parameters change. For example, imagine you have a picture / tile cache, and that has [image, opacity [image], rectangle] - we need to know that the opacity filter has changed to redraw that tile, just as we need to know if any primitive / clip has changed for that cached surface.

@kvark

kvark approved these changes Dec 17, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 17, 2018

@bors-servo r=kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

📌 Commit 8807c03 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

⌛️ Testing commit 8807c03 with merge b0e4a13...

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

Auto merge of #3426 - gw3583:pic-key, r=kvark
Include picture composite mode in primitive key for pictures.

We need to ensure that a display list that is otherwise the same
except for a different compositing mode is detected by picture
caching code as invalid.

For example, if a picture has brightness(0.5) and then the next
display list is exactly the same but with brightness(0.6).

This patch adds a not particularly elegant key for picture
composite modes that is stored in the interned primitive data
for a picture, ensuring these cases are correctly detected.

<!-- 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/3426)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 17, 2018

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

@bors-servo bors-servo merged commit 8807c03 into servo:master Dec 17, 2018

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
match mode {
Some(PictureCompositeMode::MixBlend(mode)) => {
match mode {
MixBlendMode::Normal => PictureCompositeKey::Identity,

This comment has been minimized.

@kvark

kvark Dec 18, 2018

Member

why don't we include a variant like MixBlend(MixBlendMode) into PictureCompositeKey instead of duplicating the variants?

This comment has been minimized.

@gw3583

gw3583 Dec 18, 2018

Collaborator

Duh, I should have done that! Will do next time I'm in this area.

}
Some(PictureCompositeMode::Filter(op)) => {
match op {
FilterOp::Blur(value) => PictureCompositeKey::Blur(Au::from_f32_px(value)),

This comment has been minimized.

@kvark

kvark Dec 18, 2018

Member

similarly, it looks like we should turn FilterOp<T> to be generic and just have a variant like Filter(FilterOp<Au>) in the composite key

This comment has been minimized.

@gw3583

gw3583 Dec 18, 2018

Collaborator

I wasn't sure if that would break the public API / bindgen bindings, so I tried to leave the public API as is.

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

Bug 1514944 - Update webrender to commit b0e4a13491f9f780222d3aa374fa…
…8f0c07508e73 (WR PR #3426). r=kats

servo/webrender#3426

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

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

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

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