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

Port image primitives to use interning. #3350

Merged
merged 2 commits into from Nov 26, 2018

Conversation

Projects
None yet
4 participants
@gw3583
Collaborator

gw3583 commented Nov 26, 2018

This change is Reviewable

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 26, 2018

This is a large patch - it's mostly shuffling code around to make it work with interning. It also includes #3349, and may be simpler to review after that lands.

This is quite untidy in places - we're taking on a bit of technical debt here to get stuff ported to interned primitives and enable picture caching. Once that is in place, it will definitely be worth a tidy up pass over these new structures, to tidy up the interfaces and pack the structures better.

r? @kvark or @nical

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 26, 2018

Try run is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cf0b7225af4e51b2107d3ada2644b12ffae2fd1&selectedJob=213758505

There is 1/255 pixel difference in a couple of tests on Windows. These same tests are marked fuzzy on other renderers, so we can do the same here. There is a perma-crash in 'C' - that is unrelated to this patch (see https://bugzilla.mozilla.org/show_bug.cgi?id=1509635#c11 for details). The other couple of failures look to be unrelated intermittents.

@nical

nical approved these changes Nov 26, 2018

@kvark

kvark approved these changes Nov 26, 2018

:lgtm:

Reviewed 5 of 5 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gw3583)


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

                };

                let specified_blend_mode = BlendMode::PremultipliedAlpha;

why are we always using premultiplied here?


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

                let image_instance = &ctx.prim_store.images[*image_instance_index];
                let opacity_binding = ctx.prim_store.get_opacity_binding(image_instance.opacity_binding_index);
                let specified_blend_mode = match alpha_type {

it would be nice to not re-define the variable. I find it confusing (and one of the most arguable Rust language features). Let's either have mut or with a different name. In some cases, re-defining is fine if the body of the function isn't big, or at least the re-defining happens within the vicinity of the first definition (e.g. acceptin an Option<> in a function and then getting its contents), but not in this case.


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

                };
                let request = ImageRequest {
                    key: *key,

is key pattern matched by ref implicitly? why are we explicitly putting ref source then?
those implicit refs, uh

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

☔️ The latest upstream changes (presumably #3349) made this pull request unmergeable. Please resolve the merge conflicts.

@gw3583

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark)


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

Previously, kvark (Dzmitry Malyshau) wrote…

why are we always using premultiplied here?

Because the only remaining primitives in this legacy path are the gradients, which have a specified blend mode of premultiplied alpha (previously was coming from the get_blend_mode method). The specified blend mode is kind of like CSS specified properties - it can still get overridden below to be opaque based on various other properties.


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

Previously, kvark (Dzmitry Malyshau) wrote…

it would be nice to not re-define the variable. I find it confusing (and one of the most arguable Rust language features). Let's either have mut or with a different name. In some cases, re-defining is fine if the body of the function isn't big, or at least the re-defining happens within the vicinity of the first definition (e.g. acceptin an Option<> in a function and then getting its contents), but not in this case.

I don't think this does re-define the variable? Each of the definitions of specified_blend_mode is in a separate branch of the match {}


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

Previously, kvark (Dzmitry Malyshau) wrote…

is key pattern matched by ref implicitly? why are we explicitly putting ref source then?
those implicit refs, uh

Yea, it's implicit. I removed the ref on source for consistency.

@gw3583 gw3583 force-pushed the gw3583:intern-image-3 branch from 22aa10d to 37e9435 Nov 26, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 26, 2018

Thanks @nical and @kvark. Rebased this on master and addressed the review comments above.

@kvark

This comment has been minimized.

Member

kvark commented Nov 26, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

📌 Commit 37e9435 has been approved by kvark

bors-servo added a commit that referenced this pull request Nov 26, 2018

Auto merge of #3350 - gw3583:intern-image-3, r=kvark
Port image primitives to use interning.

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

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

⌛️ Testing commit 37e9435 with merge 05d4ecc...

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 26, 2018

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

@bors-servo bors-servo merged commit 37e9435 into servo:master Nov 26, 2018

2 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
code-review/reviewable 1 file, 3 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details

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

WR Updater Bot
Bug 1510090 - Update webrender to commit 05d4eccfa6dd7f667a1f74b12134…
…257a85bea047 (WR PR #3350). r=kats

servo/webrender#3350

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

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

jankeromnes pushed a commit to jankeromnes/gecko that referenced this pull request Nov 27, 2018

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