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 clip masks for text runs. #1855

Merged
merged 1 commit into from Oct 13, 2017
Merged

Support clip masks for text runs. #1855

merged 1 commit into from Oct 13, 2017

Conversation

@glennw
Copy link
Member

glennw commented Oct 12, 2017

This switches our subpixel rendering over to a two pass method
that supports normal alpha blending.

There's some tradeoffs involved here:

  • +++ We get clip masks on subpixel text.
  • +++ CPU batching is much better (no batch breaks on text color change).
  • --- We draw the subpixel text pixels twice.

On balance, this is likely to be a reasonable performance win, because:
(a) We are typically CPU limited rather than GPU limited.
(b) The time in text run FS is typically a small portion of GPU time.

More importantly, this gives us correct clip masking on text, and also
allows us to apply some optimizations in the future:

  • Take advantage of dual source blending where available. This is
    available on most recent hardware, and allows clip masking plus
    subpixel rendering in a single pass.
  • Consider using the old constant color blend method when there
    is no clip mask present, if the two pass method does show up
    as a significant cost.

This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Oct 12, 2017

r? @kvark

(Passes servo/wrench tests OK, but I still need to do a gecko try with this).

@glennw
Copy link
Member Author

glennw commented Oct 12, 2017

Gecko try is green on all configs.

@kvark
Copy link
Member

kvark commented Oct 12, 2017

Code looks good. I haven't evaluated the performance impact yet though.


Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.


webrender/res/ps_text_run.glsl, line 88 at r1 (raw file):

    switch (uMode) {
        case MODE_ALPHA:
            oFragColor = alpha * color * vColor;

I vaguely recall driver issues with output variables assigned in a switch


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

            .uniform_1f(program.u_device_pixel_ratio, self.device_pixel_ratio);
        self.gl
            .uniform_1i(program.u_mode, mode);

I wonder if we can get any perf from caching those uniforms. Device pixel ratio and the mode don't change often.


webrender/src/renderer.rs, line 900 at r1 (raw file):

        mode: M,
        renderer_errors: &mut Vec<RendererError>,
    ) where M: Into<i32> {

would not harm to have a proper name for this type instead of i32, even if just a typedef


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Oct 12, 2017

More importantly, this gives us correct clip masking on text

I guess there is no alternative atm :)
@bors-servo r+


Review status: all files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Oct 12, 2017

📌 Commit dfaa0de has been approved by kvark

@kvark
Copy link
Member

kvark commented Oct 12, 2017

Actually, Glenn is here soon, so I'll wait to let him address the nits.
@bors-servo r-

This switches our subpixel rendering over to a two pass method
that supports normal alpha blending.

There's some tradeoffs involved here:
 * +++ We get clip masks on subpixel text.
 * +++ CPU batching is much better (no batch breaks on text color change).
 * --- We draw the subpixel text pixels twice.

On balance, this is likely to be a reasonable performance win, because:
 (a) We are typically CPU limited rather than GPU limited.
 (b) The time in text run FS is typically a small portion of GPU time.

More importantly, this gives us correct clip masking on text, and also
allows us to apply some optimizations in the future:

* Take advantage of dual source blending where available. This is
  available on most recent hardware, and allows clip masking plus
  subpixel rendering in a single pass.
* Consider using the old constant color blend method when there
  is no clip mask present, if the two pass method does show up
  as a significant cost.
@glennw glennw force-pushed the glennw:subpx-clip-3 branch from dfaa0de to 2e9eb03 Oct 12, 2017
@glennw
Copy link
Member Author

glennw commented Oct 12, 2017

Review status: 15 of 16 files reviewed at latest revision, 3 unresolved discussions.


webrender/res/ps_text_run.glsl, line 88 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I vaguely recall driver issues with output variables assigned in a switch

I think that was only in a very specific case, not in any switch. Maybe merge and see if we encounter any issues?


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

Previously, kvark (Dzmitry Malyshau) wrote…

I wonder if we can get any perf from caching those uniforms. Device pixel ratio and the mode don't change often.

#1864


webrender/src/renderer.rs, line 900 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would not harm to have a proper name for this type instead of i32, even if just a typedef

Done.


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Oct 12, 2017

@kvark Thanks for the quick review!

Comments addressed.

@kvark
Copy link
Member

kvark commented Oct 12, 2017

I think that was only in a very specific case, not in any switch. Maybe merge and see if we encounter any issues?

I think we should make it an informal rule to have a single code path for all output writes. It makes drivers happy, and allows easier code navigation and reasoning.

Anyhow, it's fine to follow-up with. Thanks for addressing my nits!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Oct 12, 2017

📌 Commit 2e9eb03 has been approved by kvark

@glennw
Copy link
Member Author

glennw commented Oct 12, 2017

Yea, that sounds like a good idea - perhaps we need a style guide for shader writing...

@bors-servo
Copy link
Contributor

bors-servo commented Oct 13, 2017

Testing commit 2e9eb03 with merge 4a0a2ce...

bors-servo added a commit that referenced this pull request Oct 13, 2017
Support clip masks for text runs.

This switches our subpixel rendering over to a two pass method
that supports normal alpha blending.

There's some tradeoffs involved here:
 * +++ We get clip masks on subpixel text.
 * +++ CPU batching is much better (no batch breaks on text color change).
 * --- We draw the subpixel text pixels twice.

On balance, this is likely to be a reasonable performance win, because:
 (a) We are typically CPU limited rather than GPU limited.
 (b) The time in text run FS is typically a small portion of GPU time.

More importantly, this gives us correct clip masking on text, and also
allows us to apply some optimizations in the future:

* Take advantage of dual source blending where available. This is
  available on most recent hardware, and allows clip masking plus
  subpixel rendering in a single pass.
* Consider using the old constant color blend method when there
  is no clip mask present, if the two pass method does show up
  as a significant cost.

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

bors-servo commented Oct 13, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing 4a0a2ce to master...

@bors-servo bors-servo merged commit 2e9eb03 into servo:master Oct 13, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 1 file, 3 discussions left (kvark)
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
@staktrace
Copy link
Contributor

staktrace commented Oct 13, 2017

@glennw I believe this change is causing shader compilation failures on Windows:

thread '' panicked at 'called Result::unwrap() on an Err value: Link("debug_font", "Precisions for uniform 'uMode' differ between vertex and fragment shaders\n")', src\libcore\result.rs:860

Full regression range is in https://bugzilla.mozilla.org/show_bug.cgi?id=1407213#c10 but this PR is the one that touches uMode.

@kvark
Copy link
Member

kvark commented Oct 13, 2017

@staktrace could you try with #1872 ?

@staktrace
Copy link
Contributor

staktrace commented Oct 13, 2017

bors-servo added a commit that referenced this pull request Oct 13, 2017
Switched uMode to be accessible to fragment shaders only

Addresses #1855 (comment)

<!-- 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/1872)
<!-- Reviewable:end -->
@staktrace
Copy link
Contributor

staktrace commented Oct 13, 2017

I screwed up the reftest.list files in that try push so it didn't run the reftests. Here's another one (this includes #1873 as well): https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f38daa67ff395256f26bc280b099a930dc26c38

@staktrace
Copy link
Contributor

staktrace commented Oct 13, 2017

Fix verified, thanks!

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

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