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 dual-source blending path for subpixel text when supported. #2244

Merged
merged 1 commit into from Dec 21, 2017

Conversation

@glennw
Copy link
Member

glennw commented Dec 20, 2017

This makes it possible to share batches when the text color is
different between runs. This can result in significantly better
draw call batching.

Add option to wrench to disable dual source blending path.

Add reftest option to control if dual-source blending is used.


This change is Reviewable

This makes it possible to share batches when the text color is
different between runs. This can result in significantly better
draw call batching.

Add option to wrench to disable dual source blending path.

Add reftest option to control if dual-source blending is used.
@glennw
Copy link
Member Author

glennw commented Dec 20, 2017

r? @kvark

The dual-source integration itself is reasonably simple. The code to weave all the options through wrench etc is a bit ugly - if you have suggestions for another way to achieve that, let me know :)

(Will start a gecko try shortly)

@glennw
Copy link
Member Author

glennw commented Dec 20, 2017

Anecdotally this does seem to give a significant performance win on several sites I tested (not all sites - there are a couple of other known causes of bad batch break behaviour we need to fix too).

@glennw
Copy link
Member Author

glennw commented Dec 20, 2017

Try looks good, I think.

One fail that's expected from current master, a couple of unrelated intermittents.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f410cd46b756acd94e0ab1ed5d20f7eb5b306485

@kvark
Copy link
Member

kvark commented Dec 20, 2017

the code to weave all the options through wrench etc is a bit ugly - if you have suggestions for another way to achieve that, let me know :)

Yes, this is exactly what I meant in #2111 by saying:

The format chosen ("xxx.list") doesn't seem to scale nicely


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


webrender/src/frame_builder.rs, line 77 at r1 (raw file):

    pub default_font_render_mode: FontRenderMode,
    pub debug: bool,
    pub dual_source_blending_is_supported: bool,

this seems a bit out of place, shouldn't it be inside Device instead?


webrender/src/render_backend.rs, line 595 at r1 (raw file):

                            // that are created.
                            self.frame_config
                                .dual_source_blending_is_enabled = enable;

should this check for _supported before setting?


webrender_api/src/api.rs, line 779 at r1 (raw file):

    pub fn load_capture(&self, path: PathBuf) {
        let msg = ApiMsg::DebugCommand(DebugCommand::LoadCapture(path));
        self.send_message(msg);

jeez, it's almost 2018 and the diff programs are not yet smart enough to detect the new code along the less indented boundary :(


wrench/reftests/text/reftest.list, line 33 at r1 (raw file):

fuzzy(1,4) platform(linux) options(disable-subpixel) == colors.yaml colors-alpha.png
# Run without dual-source blending path, batches are broken when text colors change.
fuzzy(1,6) platform(linux) options(disable-dual-source-blending) draw_calls(5) == colors.yaml colors-subpx.png

argh


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Dec 20, 2017

I only got minor concerns about this, feel free to r=me please.
And thanks!

@glennw
Copy link
Member Author

glennw commented Dec 20, 2017

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


webrender/src/frame_builder.rs, line 77 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

this seems a bit out of place, shouldn't it be inside Device instead?

This is where it's a bit weird how it threads the information through. The main issue is that the render backend needs to know this, since it affects batching (whether the color of the text is relevant to the batch key). Since the backend doesn't have access to the Device, we need to provide it here.


webrender/src/render_backend.rs, line 595 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

should this check for _supported before setting?

The enabled flag is really more like "enabled if available" - it's ANDed with the is available flag when passing the details to the batching code on whether to use dual-source blending.


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Dec 20, 2017

@kvark Thanks for the review. Added a couple of explanations for your questions. Let me know if there's changes needed based on that, otherwise I think this is ready to go :)

@kvark
Copy link
Member

kvark commented Dec 20, 2017

Thanks!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

📌 Commit 5f6b386 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

Testing commit 5f6b386 with merge 45dc6a3...

bors-servo added a commit that referenced this pull request Dec 20, 2017
Add dual-source blending path for subpixel text when supported.

This makes it possible to share batches when the text color is
different between runs. This can result in significantly better
draw call batching.

Add option to wrench to disable dual source blending path.

Add reftest option to control if dual-source blending is used.

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

bors-servo commented Dec 21, 2017

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

@bors-servo bors-servo merged commit 5f6b386 into servo:master Dec 21, 2017
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 4 discussions left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
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

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