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

Properly support tiling clip mask images. #3220

Merged
merged 11 commits into from Oct 23, 2018

Conversation

Projects
None yet
4 participants
@emilio
Member

emilio commented Oct 19, 2018

Fixes #2852.


This change is Reviewable

@emilio

This comment has been minimized.

Member

emilio commented Oct 19, 2018

r? @gw3583 / @nical / @kvark

I'd prefer not to land it yet, since the code is basically copy-pasted from image tiling, and it could get some cleanup.

Just want to confirm this approach is the right way to fix this.

I also need to write some reftests for this, but testing of blob images is none as of right now... So I'll add some Gecko reftests to the bug.

If this an acceptable solution I'll clean up and such before get it landed.

@emilio

This comment has been minimized.

Member

emilio commented Oct 19, 2018

I think this should be basically ready for review. I added a bunch of cleanup commits that made me feel less bad about copying the image tiling code, and a bunch of tests (will add reftests that check the blob image stuff in Gecko after this lands).

Once thing that may be problematic is that if I understand correctly this I'm drawing the whole visible area for each tile just to discard the pixels afterwards... I think there must be a way to do better, but not sure how... Also that wouldn't necessarily need to be part of this PR I guess.

In any case... r? :)

@emilio

This comment has been minimized.

@gw3583

Reviewed 14 of 16 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emilio)


wrench/reftests/mask/checkerboard-tiling.yaml, line 1 at r3 (raw file):

---

It'd be good to have a brief comment at the top of each test explaining what functionality / feature it is testing.


wrench/reftests/mask/checkerboard-tiling.yaml, line 10 at r3 (raw file):

        rect: [0, 0, 200, 200]
        repeat: false
        tile-size: 37 # Intentional

An explanation of why would be good here.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 21, 2018

This basically looks good to me (a couple of minor nits). It'd be good to get @nical to take a look over the image tiling changes, if possible, as well.

@emilio

Reviewable status: 18 of 19 files reviewed, 2 unresolved discussions (waiting on @gw3583 and @emilio)


wrench/reftests/mask/checkerboard-tiling.yaml, line 1 at r3 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

It'd be good to have a brief comment at the top of each test explaining what functionality / feature it is testing.

I have left a comment on this one. The only tests that I introduced per se are this one and mask-perspective*, whose purpose is on the test.

The rest of this are copies of existing tests, but with tiling, to ensure that tiled and untiled output is the same.


wrench/reftests/mask/checkerboard-tiling.yaml, line 10 at r3 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

An explanation of why would be good here.

Done.

@nical

This comment has been minimized.

Collaborator

nical commented Oct 22, 2018

Looks good to me!

@emilio

This comment has been minimized.

Member

emilio commented Oct 22, 2018

Thanks!

@bors-servo r=gw3583,nical

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 22, 2018

📌 Commit 0800db0 has been approved by gw3583,nical

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

Trying to wake up bors

@gw3583 gw3583 closed this Oct 23, 2018

@gw3583 gw3583 reopened this Oct 23, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@bors-servo retry

@gw3583

gw3583 approved these changes Oct 23, 2018

@gw3583 gw3583 closed this Oct 23, 2018

@gw3583 gw3583 reopened this Oct 23, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@bors-servo r+ retry force

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 23, 2018

@bors-servo r=gw3583

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

📌 Commit 0800db0 has been approved by gw3583

emilio added some commits Oct 19, 2018

image: simplify for_each_tile_in_range.
And remove the indirect call (&mut FnMut).
@emilio

This comment has been minimized.

Member

emilio commented Oct 23, 2018

@bors-servo r=gw3583,nical

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Oct 23, 2018

📌 Commit 8682c13 has been approved by gw3583,nical

@gw3583 gw3583 merged commit 9b1a2f7 into servo:master Oct 23, 2018

0 of 3 checks passed

Taskcluster (pull_request) TaskGroup: Pending (for pull_request.synchronize)
Details
code-review/reviewable 3 files, 2 discussions left (gw3583)
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment