Skip to content

Pass device pixel scale via picture render task data. #3150

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

Merged
merged 3 commits into from
Oct 3, 2018

Conversation

gw3583
Copy link
Contributor

@gw3583 gw3583 commented Oct 2, 2018

As a follow up, we will be able to include a per-picture
local scaling factor. This can be used for drawing pictures
in local space at a specific scale factor.


This change is Reviewable

@gw3583 gw3583 requested a review from kvark October 2, 2018 02:05
@gw3583
Copy link
Contributor Author

gw3583 commented Oct 2, 2018

@gw3583
Copy link
Contributor Author

gw3583 commented Oct 2, 2018

Try run looks good.

Copy link
Member

@kvark kvark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

my main concern would be testing this. Does Gecko's CI have any bots with non-unit device/pixel ration? if not, would be good to get that build on a few machines and run

Reviewed 15 of 15 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gw3583)


webrender/res/snap.glsl, line 18 at r1 (raw file):

    //           the test cases we have in Servo that are failing without it
    //           and seem better than not having this at all.
    snap_rect.size = max(snap_rect.size, vec2(1.0 / device_pixel_scale));

given the number of times we need to divide by it, we should probably provide the reciprocal (as an extra) from CPU side


webrender/src/device/gl.rs, line 527 at r1 (raw file):

    id: gl::GLuint,
    u_transform: gl::GLint,
    u_device_pixel_ratio: gl::GLint,

🎉

@bors-servo
Copy link
Contributor

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

gw3583 added 3 commits October 3, 2018 06:29
As a follow up, we will be able to include a per-picture
local scaling factor. This can be used for drawing pictures
in local space at a specific scale factor.
Copy link
Contributor Author

@gw3583 gw3583 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 of 15 files reviewed, 2 unresolved discussions (waiting on @kvark and @gw3583)


webrender/res/snap.glsl, line 18 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

given the number of times we need to divide by it, we should probably provide the reciprocal (as an extra) from CPU side

Filed #3157


webrender/src/device/gl.rs, line 527 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

🎉

Done.

@gw3583
Copy link
Contributor Author

gw3583 commented Oct 2, 2018

Filed #3157 and rebased.

I agree with the testing comment. I did a fair amount of manual testing in wrench with difference ratios, and it seems OK, but I'm not sure how good our CI coverage is. Once I start to leverage this for rasterizing pictures at different scales, we can definitely get some good CI coverage via that.

@kvark
Copy link
Member

kvark commented Oct 3, 2018

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit b0d7136 has been approved by kvark

@bors-servo
Copy link
Contributor

⌛ Testing commit b0d7136 with merge 3eff42b...

bors-servo pushed a commit that referenced this pull request Oct 3, 2018
Pass device pixel scale via picture render task data.

As a follow up, we will be able to include a per-picture
local scaling factor. This can be used for drawing pictures
in local space at a specific scale factor.

<!-- 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/3150)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

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

@bors-servo bors-servo merged commit b0d7136 into servo:master Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants