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 glyph transforms with alpha and mono font render modes #2095

Merged
merged 2 commits into from Nov 24, 2017

Conversation

@lsalzman
Copy link
Contributor

lsalzman commented Nov 24, 2017

This fixes the downstream Gecko bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=1413793

Essentially, the bug on display above is that we have FontRenderMode::Alpha with a scaling content transform. But glyph transforms only work in subpixel, so the alpha glyph gets rendered at pre-transformed scale, and then later transformed, resulting in the blurry mess.

After discussion with Glenn, we decided it made sense to extend support for glyph transforms to the other render modes than just subpixel, which is fairly trivial and does the job. The only render mode that does not get glyph transforms is FontRenderMode::Bitmap, since we're assuming we're pulling a pre-rasterized glyph out of the font and pre-transforming it would be a waste.

The only little bit of complexity here was that bitmap handling in the Windows backend needed some cleaning. I made FontRenderMode::Bitmap now tie in with requests of FontInstancePlatformOptions::use_embedded_bitmap. Further, I cleaned up handling of the dwrite modes to more closely match Skia, so that essentially we punt to GDI more often and more consistently when we need to.


This change is Reviewable

@glennw
Copy link
Member

glennw commented Nov 24, 2017

Could we add a reftest for transform + alpha (png reference on Linux would be fine), and do a gecko try run?

@lsalzman lsalzman force-pushed the lsalzman:transformed-alpha branch from 0d8b060 to 8625701 Nov 24, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 24, 2017

Try run in progress, working on adding a reftest now.

@lsalzman lsalzman force-pushed the lsalzman:transformed-alpha branch 2 times, most recently from 9a41a19 to 2988fc0 Nov 24, 2017
@glennw
Copy link
Member

glennw commented Nov 24, 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 24, 2017

Looks great! Delegating since this is ready to go but will need rebasing once the other pending font PR merges.

@bors-servo delegate+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

✌️ @lsalzman can now approve this pull request

@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 24, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

📌 Commit 2988fc0 has been approved by lsalzman

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

🔒 Merge conflict

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

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

@lsalzman lsalzman force-pushed the lsalzman:transformed-alpha branch from 2988fc0 to 7ed043d Nov 24, 2017
@lsalzman
Copy link
Contributor Author

lsalzman commented Nov 24, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

📌 Commit 45ac4ad has been approved by lsalzman

@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

Testing commit 45ac4ad with merge a2883d0...

bors-servo added a commit that referenced this pull request Nov 24, 2017
support glyph transforms with alpha and mono font render modes

This fixes the downstream Gecko bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=1413793

Essentially, the bug on display above is that we have FontRenderMode::Alpha with a scaling content transform. But glyph transforms only work in subpixel, so the alpha glyph gets rendered at pre-transformed scale, and then later transformed, resulting in the blurry mess.

After discussion with Glenn, we decided it made sense to extend support for glyph transforms to the other render modes than just subpixel, which is fairly trivial and does the job. The only render mode that does not get glyph transforms is FontRenderMode::Bitmap, since we're assuming we're pulling a pre-rasterized glyph out of the font and pre-transforming it would be a waste.

The only little bit of complexity here was that bitmap handling in the Windows backend needed some cleaning. I made FontRenderMode::Bitmap now tie in with requests of FontInstancePlatformOptions::use_embedded_bitmap. Further, I cleaned up handling of the dwrite modes to more closely match Skia, so that essentially we punt to GDI more often and more consistently when we need to.

<!-- 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/2095)
<!-- Reviewable:end -->
@lsalzman lsalzman mentioned this pull request Nov 24, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Nov 24, 2017

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

@bors-servo bors-servo merged commit 45ac4ad into servo:master Nov 24, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 3 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
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.