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

Port ps_blend to be a brush shader. #2401

Merged
merged 2 commits into from Feb 14, 2018
Merged

Conversation

@glennw
Copy link
Member

glennw commented Feb 9, 2018

Previously, ps_blend applied the blend operation
in 2D screen-space. Now, blends are performed as
a normal brush shader. This provides several benefits:

  • For a transformed primitive, we blend fewer pixels,
    since we only draw the composite step as a transformed
    primitive, rather than a 2D screen aligned rect.

  • We can optionally apply all the usual features
    that are part of the brush shader, specifically
    clip masks, edge AA and pixel snapping.

  • This lays the groundwork for supporting picture
    targets that are selectable to be either rasterized
    in local-space or screen-space, depending on what
    the caller requests as a performance / quality
    trade-off.

Next, we'll port hw_composite and composite shaders
to use the same brush format, and add support for
applying the source texture based on whether it was
rasterized in local or screen space.


This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Feb 9, 2018

This is not quite ready yet:

  • There is a wrench reftest that fails with a pixel difference of 17 in places, when it was pixel-perfect previously. Possibly an accuracy bug somewhere.
  • Still needs a gecko try once that bug is fixed.

But I don't expect those to change the PR massively, so it can probably be reviewed when someone has time.

@kvark
Copy link
Member

kvark commented Feb 11, 2018

:lgtm:


Reviewed 18 of 18 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/picture.rs, line 589 at r1 (raw file):

    pub fn write_gpu_blocks(&self, request: &mut GpuDataRequest) {
        // TODO(gw): It's unfortunate that we pay a fixed cost

we could have an extra GPU block range for the color matrix specifically


Comments from Reviewable

@glennw glennw force-pushed the glennw:180209-blend-brush-2 branch from 0fd3fc9 to 33119a9 Feb 12, 2018
@glennw
Copy link
Member Author

glennw commented Feb 12, 2018

Gecko try looks good for this patch:

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

(apart from some unrelated intermittent failures).

I changed the failing wrench reftest to have a much larger fuzziness value. The issue here is that the text run caching shader doesn't apply any form of snapping, which can result in issues like this. We can fix this separately.

@glennw glennw changed the title [WIP] Port ps_blend to be a brush shader. Port ps_blend to be a brush shader. Feb 12, 2018
@glennw glennw force-pushed the glennw:180209-blend-brush-2 branch from 33119a9 to 14374f9 Feb 12, 2018
@glennw
Copy link
Member Author

glennw commented Feb 12, 2018

@bors-servo r=kvark (via irc)

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2018

📌 Commit 14374f9 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2018

Testing commit 14374f9 with merge c09f892...

bors-servo added a commit that referenced this pull request Feb 12, 2018
Port ps_blend to be a brush shader.

Previously, ps_blend applied the blend operation
in 2D screen-space. Now, blends are performed as
a normal brush shader. This provides several benefits:

* For a transformed primitive, we blend fewer pixels,
  since we only draw the composite step as a transformed
  primitive, rather than a 2D screen aligned rect.

* We can optionally apply all the usual features
  that are part of the brush shader, specifically
  clip masks, edge AA and pixel snapping.

* This lays the groundwork for supporting picture
  targets that are selectable to be either rasterized
  in local-space or screen-space, depending on what
  the caller requests as a performance / quality
  trade-off.

Next, we'll port hw_composite and composite shaders
to use the same brush format, and add support for
applying the source texture based on whether it was
rasterized in local or screen space.

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

bors-servo commented Feb 12, 2018

💔 Test failed - status-travis

@glennw
Copy link
Member Author

glennw commented Feb 12, 2018

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2018

Testing commit 14374f9 with merge 3536060...

bors-servo added a commit that referenced this pull request Feb 12, 2018
Port ps_blend to be a brush shader.

Previously, ps_blend applied the blend operation
in 2D screen-space. Now, blends are performed as
a normal brush shader. This provides several benefits:

* For a transformed primitive, we blend fewer pixels,
  since we only draw the composite step as a transformed
  primitive, rather than a 2D screen aligned rect.

* We can optionally apply all the usual features
  that are part of the brush shader, specifically
  clip masks, edge AA and pixel snapping.

* This lays the groundwork for supporting picture
  targets that are selectable to be either rasterized
  in local-space or screen-space, depending on what
  the caller requests as a performance / quality
  trade-off.

Next, we'll port hw_composite and composite shaders
to use the same brush format, and add support for
applying the source texture based on whether it was
rasterized in local or screen space.

<!-- 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/2401)
<!-- Reviewable:end -->
@glennw
Copy link
Member Author

glennw commented Feb 12, 2018

Hmmm, a couple of mac-specific failures. I'll investigate those further - maybe there is an accuracy issue somewhere that's not being picked up by the gecko tests.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 12, 2018

💔 Test failed - status-taskcluster

@bors-servo
Copy link
Contributor

bors-servo commented Feb 13, 2018

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

Previously, ps_blend applied the blend operation
in 2D screen-space. Now, blends are performed as
a normal brush shader. This provides several benefits:

* For a transformed primitive, we blend fewer pixels,
  since we only draw the composite step as a transformed
  primitive, rather than a 2D screen aligned rect.

* We can optionally apply all the usual features
  that are part of the brush shader, specifically
  clip masks, edge AA and pixel snapping.

* This lays the groundwork for supporting picture
  targets that are selectable to be either rasterized
  in local-space or screen-space, depending on what
  the caller requests as a performance / quality
  trade-off.

Next, we'll port hw_composite and composite shaders
to use the same brush format, and add support for
applying the source texture based on whether it was
rasterized in local or screen space.
@glennw glennw force-pushed the glennw:180209-blend-brush-2 branch from 14374f9 to c58d671 Feb 14, 2018
@glennw
Copy link
Member Author

glennw commented Feb 14, 2018

Rebased and added a commit that (I think) fixes the accuracy issues. It drops the max difference in the wrench tests from 17 down to 1, which is what I'd expect. Still need to do a gecko try to see if it breaks anything there.

Also pass complete VertexInfo struct to brushes.
@glennw glennw force-pushed the glennw:180209-blend-brush-2 branch from c58d671 to fdc1f4e Feb 14, 2018
@glennw
Copy link
Member Author

glennw commented Feb 14, 2018

@kvark OK, I think this is finally ready to go (after review of the most recent commit).

The gecko try looks good now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c9acdce54c6958eabea3777b288c4d55f9788e7

There is one failure (expected due to #2408) and one unrelated intermittent failure.

@kvark
kvark approved these changes Feb 14, 2018
@kvark
Copy link
Member

kvark commented Feb 14, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2018

📌 Commit fdc1f4e has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 14, 2018

Testing commit fdc1f4e with merge 330b237...

bors-servo added a commit that referenced this pull request Feb 14, 2018
Port ps_blend to be a brush shader.

Previously, ps_blend applied the blend operation
in 2D screen-space. Now, blends are performed as
a normal brush shader. This provides several benefits:

* For a transformed primitive, we blend fewer pixels,
  since we only draw the composite step as a transformed
  primitive, rather than a 2D screen aligned rect.

* We can optionally apply all the usual features
  that are part of the brush shader, specifically
  clip masks, edge AA and pixel snapping.

* This lays the groundwork for supporting picture
  targets that are selectable to be either rasterized
  in local-space or screen-space, depending on what
  the caller requests as a performance / quality
  trade-off.

Next, we'll port hw_composite and composite shaders
to use the same brush format, and add support for
applying the source texture based on whether it was
rasterized in local or screen space.

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

bors-servo commented Feb 14, 2018

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing 330b237 to master...

@bors-servo bors-servo merged commit fdc1f4e into servo:master Feb 14, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 16 files, 1 discussion 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.