Skip to content
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

support font subpixel AA with content transforms #2058

Merged
merged 1 commit into from Nov 21, 2017

Conversation

@lsalzman
Copy link
Contributor

lsalzman commented Nov 18, 2017

This solves issue #1808. It ensures that we render subpixel AA in device space rather than layer local space, so as to side-step the issue with subpixels getting deformed by a layer transform.

The engine of this is FontTransform, which is the upper 2x2 submatrix of the layer transform and which is stored in the FontInstance - automatically and necessarily becoming part of the font key. That transform has to be passed down to the font backend so that rasterization actually uses it.

In the text shader, we can just optionally derive this transform from the layer transform, without having to pass in any other state. We have to make sure we only conditionally use it, as not all glyphs will be pre-transformed. To that end, I made TextShader a replacement for PrimitiveShader (as discussed with Glenn).


This change is Reviewable

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 3ccfb15 to bcdcb10 Nov 18, 2017
@glennw
Copy link
Member

glennw commented Nov 19, 2017

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Nov 19, 2017

This looks great! We'll need a gecko try run before merging though. I'll handle running it through the Servo tests.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

Oh, one other thing - we should add some wrench reftest cases - rotation, skew etc on both subpx and alpha test runs. Also with some text shadow cases. These can be done as a follow up.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

There's a few gecko try failures that @lsalzman is working on - we know the cause.

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from bcdcb10 to 7eefbba Nov 20, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 20, 2017

Fixed issue with computing snap offsets in the text shader.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

I think we should have at least a couple of reftests (just on one platform is enough) for this stuff - it's so fiddly to get right and easy to regress.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2017

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

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 7eefbba to 5dd133e Nov 20, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 20, 2017

Fixed merge conflicts.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 20, 2017

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

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 5dd133e to 6fb96a3 Nov 20, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 20, 2017

Added subpixel rotation, scale, and skew transform reftests.

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 6fb96a3 to c3ac74c Nov 20, 2017
@glennw
Copy link
Member

glennw commented Nov 20, 2017

Based on discussions with @lsalzman I think this is blocked until we get some fixes to the mac dependency crates.

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from c3ac74c to 9d02cfa Nov 20, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 20, 2017

Fixed core-graphics-rs to have the functionality needed and updated this to use it.

@glennw
Copy link
Member

glennw commented Nov 20, 2017

Reviewed 20 of 20 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Nov 21, 2017

@lsalzman has done a try run and got green results, so this looks good to go. Thanks!

@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

📌 Commit 9d02cfa has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

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

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

🔒 Merge conflict

@glennw
Copy link
Member

glennw commented Nov 21, 2017

Needs a rebase.

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 9d02cfa to 74c54e1 Nov 21, 2017
@glennw
Copy link
Member

glennw commented Nov 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

📌 Commit 74c54e1 has been approved by glennw

@lsalzman lsalzman force-pushed the lsalzman:font-transform branch from 74c54e1 to f323a31 Nov 21, 2017
@glennw
Copy link
Member

glennw commented Nov 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

📌 Commit f323a31 has been approved by glennw

@glennw
Copy link
Member

glennw commented Nov 21, 2017

@bors-servo delegate

@glennw
Copy link
Member

glennw commented Nov 21, 2017

@bors-servo delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

✌️ @lsalzman can now approve this pull request

@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 21, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #2064
@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

📌 Commit f323a31 has been approved by lsalzman

@bors-servo
Copy link
Contributor

bors-servo commented Nov 21, 2017

Testing commit f323a31 with merge 7753167...

bors-servo added a commit that referenced this pull request Nov 21, 2017
support font subpixel AA with content transforms

This solves issue #1808. It ensures that we render subpixel AA in device space rather than layer local space, so as to side-step the issue with subpixels getting deformed by a layer transform.

The engine of this is FontTransform, which is the upper 2x2 submatrix of the layer transform and which is stored in the FontInstance - automatically and necessarily becoming part of the font key. That transform has to be passed down to the font backend so that rasterization actually uses it.

In the text shader, we can just optionally derive this transform from the layer transform, without having to pass in any other state. We have to make sure we only conditionally use it, as not all glyphs will be pre-transformed. To that end, I made TextShader a replacement for PrimitiveShader (as discussed with Glenn).

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

bors-servo commented Nov 21, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: lsalzman
Pushing 7753167 to master...

@bors-servo bors-servo merged commit f323a31 into servo:master Nov 21, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 2 files left (glennw)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 23, 2017

Fixes #1808

@lsalzman lsalzman mentioned this pull request Nov 24, 2017
@kvark kvark mentioned this pull request Sep 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.