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

Fix caching display lists with opacity bindings that are values. #3481

Merged
merged 1 commit into from Jan 7, 2019

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Jan 7, 2019

Most of the time when property bindings are used, they are bound
to a specific key that can be animation. However, they can also
be set to a fixed value.

It was previously possible for a property binding with a fixed
value to not be included in a cached picture dependency. This
meant a stale tile could be used in cases where a new display
list arrives with the same property binding IDs but different
values, where everything else is the same.

This patch includes fixed value property bindings in the tile
descriptor's opacity binding dependencies.

This is a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1518050


This change is Reviewable

Fix caching display lists with opacity bindings that are values.
Most of the time when property bindings are used, they are bound
to a specific key that can be animation. However, they can also
be set to a fixed value.

It was previously possible for a property binding with a fixed
value to not be included in a cached picture dependency. This
meant a stale tile could be used in cases where a new display
list arrives with the same property binding IDs but different
values, where everything else is the same.

This patch includes fixed value property bindings in the tile
descriptor's opacity binding dependencies.

This is a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1518050
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 7, 2019

tile.is_valid = false;
break;
for binding in tile.descriptor.opacity_bindings.items() {
if let OpacityBinding::Binding(id) = binding {

This comment has been minimized.

@staktrace

staktrace Jan 7, 2019

Contributor

I don't know this code, but from reading your comment in the PR it seems like you'd want an else clause here (for the OpacityBinding::Value case) that sets tile.is_valid = false?

This comment has been minimized.

@staktrace

staktrace Jan 7, 2019

Contributor

Oh, never mind. I see ComparableVec::is_valid will do that check

This comment has been minimized.

@gw3583

gw3583 Jan 7, 2019

Collaborator

This case is handled by the opacity bindings being stored in a ComparableVec which checks the values are the same. So we only need this extra indirection for the binding case where the value is stored elsewhere.

@staktrace

This comment has been minimized.

Copy link
Contributor

staktrace commented Jan 7, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 7, 2019

📌 Commit 02be813 has been approved by staktrace

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 7, 2019

⌛️ Testing commit 02be813 with merge ca28ae6...

bors-servo added a commit that referenced this pull request Jan 7, 2019

Auto merge of #3481 - gw3583:binding-deps, r=staktrace
Fix caching display lists with opacity bindings that are values.

Most of the time when property bindings are used, they are bound
to a specific key that can be animation. However, they can also
be set to a fixed value.

It was previously possible for a property binding with a fixed
value to not be included in a cached picture dependency. This
meant a stale tile could be used in cases where a new display
list arrives with the same property binding IDs but different
values, where everything else is the same.

This patch includes fixed value property bindings in the tile
descriptor's opacity binding dependencies.

This is a fix for https://bugzilla.mozilla.org/show_bug.cgi?id=1518050

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

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 7, 2019

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

@bors-servo bors-servo merged commit 02be813 into servo:master Jan 7, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 8, 2019

Bug 1518390 - Update webrender to commit ca28ae618d249976a755db5419e5…
…ec7c36bbfd1c (WR PR #3481). r=kats

servo/webrender#3481

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

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

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 8, 2019

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