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

Add support for specifying an estimated background color for subpixel text #1974

Merged
merged 1 commit into from Nov 3, 2017

Conversation

@mstange
Copy link
Contributor

mstange commented Nov 1, 2017

This is almost ready for review, but not quite.

I've written some extensive documentation for the new text blending at webrender/doc/text-rendering.md: Rendered

Remaining issues:

  • rebase on top of Lee's changes to GlyphFormat
  • decide whether it's reasonable to have individual font rasterization backends do mask.a = mask.max_rgb or whether that's cheap enough to do in the shader - conclusion: do it in the backends
  • if the former, also fix up the linux and windows backends
  • decide whether the middle pass should use a different shader so that regular text rendering doesn't have the overhead of the extra color field and the extra if in the fragment shader - conclusion: use different shader through an #ifdef in the text shader
  • [ ] add some tests filed #1980 to get test support

This change is Reviewable

@glennw
Copy link
Member

glennw commented Nov 1, 2017

@mstange Wow, impressive doc!

@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch 2 times, most recently from f1f9c09 to 3917267 Nov 1, 2017
@mstange mstange changed the title Add support for FontRenderMode::SubpixelWithBgColor. Add support for specifying an estimated background color for subpixel text. Nov 1, 2017
@mstange mstange changed the title Add support for specifying an estimated background color for subpixel text. Add support for specifying an estimated background color for subpixel text Nov 1, 2017
@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch from 3917267 to 834c8b3 Nov 2, 2017
@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch from 834c8b3 to 397beef Nov 2, 2017
@mstange
Copy link
Contributor Author

mstange commented Nov 2, 2017

@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch 5 times, most recently from c981381 to 8737117 Nov 2, 2017
@lsalzman
Copy link
Contributor

lsalzman commented Nov 2, 2017

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2017

@lsalzman: 🔑 Insufficient privileges: Not in reviewers

@mstange
Copy link
Contributor Author

mstange commented Nov 2, 2017

The release tests are running into thread 'RenderBackend' panicked at 'assertion failed: request.current_used_block_num() <= MAX_VERTEX_TEXTURE_WIDTH', /Users/tcworker/webrender/webrender/src/prim_store.rs:623:8. Looking into it.

@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch from 8737117 to 0fffb57 Nov 2, 2017
@mstange mstange force-pushed the mstange:subpixel-with-bg-color branch from 0fffb57 to 59860c7 Nov 2, 2017
@mstange
Copy link
Contributor Author

mstange commented Nov 2, 2017

The test failures should be fixed now.

@glennw
Copy link
Member

glennw commented Nov 2, 2017

@bors-servo r+

We should add some reftests for this too.

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2017

📌 Commit 59860c7 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 2, 2017

Testing commit 59860c7 with merge 479d43a...

bors-servo added a commit that referenced this pull request Nov 2, 2017
Add support for specifying an estimated background color for subpixel text

This is almost ready for review, but not quite.

I've written some extensive documentation for the new text blending at `webrender/doc/text-rendering.md`: [Rendered](https://github.com/mstange/webrender/blob/subpixel-with-bg-color/webrender/doc/text-rendering.md)

Remaining issues:
 - [x] rebase on top of Lee's changes to GlyphFormat
 - [x] decide whether it's reasonable to have individual font rasterization backends do `mask.a = mask.max_rgb` or whether that's cheap enough to do in the shader - conclusion: do it in the backends
 - [x] if the former, also fix up the linux and windows backends
 - [x] decide whether the middle pass should use a different shader so that regular text rendering doesn't have the overhead of the extra color field and the extra `if` in the fragment shader - conclusion: use different shader through an #ifdef in the text shader
 - ~~[ ] add some tests~~ filed #1980 to get test support

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

bors-servo commented Nov 3, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 479d43a to master...

@bors-servo bors-servo merged commit 59860c7 into servo:master Nov 3, 2017
4 checks passed
4 checks passed
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
@glennw
Copy link
Member

glennw commented Nov 3, 2017

@staktrace @mstange Heads up - this PR adds a field to FontInstanceOptions struct that silently compiles in Gecko but results in random data for the background color field. I guess the FFI bindings will need to be regenerated?

@glennw
Copy link
Member

glennw commented Nov 3, 2017

Oh, I see there's a patch ready for this in the wr-future bug, never mind me.

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

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