Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upRemove aligned gradient shader, port to brush. #2441
Conversation
|
Added a follow up commit that switches the tiling / repeat of linear gradients to be done on CPU rather than in the shader (exactly the same implementation details as how this was done for radial gradients previously). |
|
Added another commit that ports the linear gradient primitive to be a brush shader. |
glennw
changed the title
|
The overall summary of this PR:
Pending try: |
|
Looks sane to me! Reviewed 2 of 7 files at r1, 1 of 5 files at r2, 8 of 8 files at r3. webrender/tests/angle_shader_validation.rs, line 104 at r3 (raw file):
let's have at least one of them with dithering feature Comments from Reviewable |
|
@kvark Thanks for the review! I updated the validation to include the dither feature. The reftest failures are from previous PRs (the input/radio are from the local-clip-rect + box-shadow PR, and the border-radii are from the ellipse distance optimization PR). Both are fuzzed in the latest WR update. |
|
@bors-servo r+ |
|
|
Remove aligned gradient shader, port to brush. Instead, push aligned gradients through the linear angle gradient shader (for now). This will simplify converting linear gradient code over to be a brush shader. The optimization here was always a bit dubious (it made more sense when we first started WR, when the angle gradient shader was more expensive than it is now). Instead, what we can do in the future if gradients ever show up in the GPU profile is support vertex colors for the rectangle shader and decompose simple aligned gradients into a series of rectangle segments with different vertex colors. The benefit of this is reduced shader changes and simpler batching. <!-- 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/2441) <!-- Reviewable:end -->
|
|
|
@bors-servo retry |
|
|
|
|
Instead, push aligned gradients through the linear angle gradient shader (for now). This will simplify converting linear gradient code over to be a brush shader. The optimization here was always a bit dubious (it made more sense when we first started WR, when the angle gradient shader was more expensive than it is now). Instead, what we can do in the future if gradients ever show up in the GPU profile is support vertex colors for the rectangle shader and decompose simple aligned gradients into a series of rectangle segments with different vertex colors. The benefit of this is reduced shader changes and simpler batching.
|
Rebased. @bors-servo r=kvark |
|
|
Remove aligned gradient shader, port to brush. Instead, push aligned gradients through the linear angle gradient shader (for now). This will simplify converting linear gradient code over to be a brush shader. The optimization here was always a bit dubious (it made more sense when we first started WR, when the angle gradient shader was more expensive than it is now). Instead, what we can do in the future if gradients ever show up in the GPU profile is support vertex colors for the rectangle shader and decompose simple aligned gradients into a series of rectangle segments with different vertex colors. The benefit of this is reduced shader changes and simpler batching. <!-- 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/2441) <!-- Reviewable:end -->
|
|
|
@staktrace Ugh, there are some gradient reftest failures I missed seeing in the try push above - these are going to cause failures in the wr-future bug. I'll look into these as soon as possible (they weren't there initially - possibly a rebasing mistake I made or something like that). |
|
Interestingly, they only appear in a debug build - which might be how I managed to miss them. I also can't reproduce locally with a release build. Compiling a debug build now. |
|
Can't reproduce locally with rustc debug enabled. Trying another build with full debug enabled. |
|
I'll do a local debug build as well to see if I can repro. |
|
I've been able to reproduce a failure in layout/reftests/css-gradients/height-dependence-2.html but very sporadically. I tried getting WR captures of it but they came out blank, so I don't know how useful that will be. The failure as reported by the reftest harness shows the "pad pad pad" text not running all the way to the bottom of the rect in the test image (there's only 16 lines of it vs. 32 in the reference image). I did, however, get a rr recording of the failure (since I reproduced it with rr's chaos mode). If you have any suggestions on how to debug it I'm all ears. |
|
height-dependence-2-captures.zip ^ the captures along with the reftest harness output (for use in reftest analyzer) if they're of any use to anybody. |
|
Are you sure the breakage is caused by this PR though? Gradients don't appear to be rendering any differently between the captures. And the PR doesn't seem to touch anything on either the clip chain side or glyph positioning. |
|
We're not sure it's this PR - we are bisecting a couple of patches to find the cause. |
|
Pretty sure it's from this PR, I retriggered the R4 jobs a few times to make sure: at 3797d9c - R4 is green at b2acbc9 - R4 is green (but retriggers are still going) at 2d2d7b7 - R4 is orange |
|
Yup, that seems fairly conclusive. There's 3 commits in that patch, so I'll try to narrow down the cause there. |
|
Haven't managed to reproduce locally yet. Here are pending try pushes with each of the 3 commits in this PR reverted though, which should help narrow it down: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f9443cde2d85641c2510ecf02b2be704cffda36 https://treeherder.mozilla.org/#/jobs?repo=try&revision=c62948592adb70ed9534648cdeeeac0611b117ef https://treeherder.mozilla.org/#/jobs?repo=try&revision=1cb15ddf82a8ce99c59f0d21688d73b7be82bd48 https://treeherder.mozilla.org/#/jobs?repo=try&revision=928c958792ddab98bc73ba934cdec41df307a91c Although, TC seems to be having network issues right now |
|
Those try pushes suggest that it's likely to be the middle commit (1628f2e) that is causing the problem. It's possible that may be exposing a race condition elsewhere. |
|
The |
|
Oh, I see what the problem is. Working on a fix now. |
|
Speaking of GPU cache issues, we are lacking the appropriate toolset - #2453 |
This is a potential fix for the intermittent failures here: servo#2441 (comment) I still can't reproduce them locally. However, I could see that the GPU cache size in that test was getting dangerously close to the maximum size of the texture supported by the GL implementation. That would fit with the issues we're seeing on CI, where there seems to be items not being drawn, and they are affected even though the test in question doesn't draw any gradients. This is not a very elegant fix - however it will hopefully fix the issues we are seeing on CI for now. We can revisit this later to consider a better fix.
|
Potential fix here - #2454 - try run in progress to see if it helps. |
Share gradient GPU cache entries for repeated gradient primitives. This is a potential fix for the intermittent failures here: #2441 (comment) I still can't reproduce them locally. However, I could see that the GPU cache size in that test was getting dangerously close to the maximum size of the texture supported by the GL implementation. That would fit with the issues we're seeing on CI, where there seems to be items not being drawn, and they are affected even though the test in question doesn't draw any gradients. This is not a very elegant fix - however it will hopefully fix the issues we are seeing on CI for now. We can revisit this later to consider a better fix. <!-- 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/2454) <!-- Reviewable:end -->
|
I confirm this is a problem with exceeding maximum texture size of 8K. |
glennw commentedFeb 19, 2018
•
edited by larsbergstrom
Instead, push aligned gradients through the linear angle gradient
shader (for now).
This will simplify converting linear gradient code over to be
a brush shader. The optimization here was always a bit dubious
(it made more sense when we first started WR, when the angle
gradient shader was more expensive than it is now).
Instead, what we can do in the future if gradients ever show
up in the GPU profile is support vertex colors for the rectangle
shader and decompose simple aligned gradients into a series
of rectangle segments with different vertex colors.
The benefit of this is reduced shader changes and simpler batching.
This change is