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 repetition in brush shaders #2621

Merged
merged 3 commits into from Apr 17, 2018
Merged

Conversation

@nical
Copy link
Collaborator

nical commented Apr 5, 2018

Add support for repeating in the shader rather than decomposing on the CPU.


This change is Reviewable

@nical nical force-pushed the nical:brush-img-repeat branch from 29db2af to 0568f60 Apr 5, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 5, 2018

If we do this we can implement repeating images and gradients much more easily than with CPU-side decomposition. I think that the extra per-fragment cost is small enough that it shouldn't matter for the non-repeated case, while it saves a lot of CPU work in the repeated case. If the extra instructions are really a concern we can probably optimize it some more.
It would also let repeated images and gradients benefit from clipping optimizations that use segments.

Spacing would be implemented on top of this and #2619 using a render task to add the transparent space.

@glennw what do you think?

@nical nical changed the title [WIP] Support repeatition in brush shaders [WIP] Support repetition in brush shaders Apr 5, 2018
@glennw
Copy link
Member

glennw commented Apr 6, 2018

Yup, I agree, it makes sense to do the repetition here and handle spacing separately.

There's a number of CI test failures - I haven't looked into what causes them yet. We'll also want a try run once those are fixed up.

@nical nical force-pushed the nical:brush-img-repeat branch 2 times, most recently from 4a3952e to 04b11ea Apr 9, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 9, 2018

The remaining failure is a sampling artifact when a non-axis-aligned transform is applied due to the inflation of the primitive causing the extra pixels to be on the wrong repetition.

I am looking into addressing this by passing the (equivalent of the) non-inflated local rect to the fragment shader and clamping the uv based on that before the modulo.
It's a bit sad to add an extra varying vec4 for this, but I can't think of a cheaper way right now.

@nical nical force-pushed the nical:brush-img-repeat branch from 04b11ea to 46994cc Apr 10, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 10, 2018

The sampling issue due to the aa inflation is fixed. The solution is a little ugly but it doesn't cost anything in the opaque pass and removes the "fail-safe" part in the brush_image shader which was even uglier in the sense that it was removing half of the anti-aliasing shades (making inflation useless). So as a bonus the antialiasing of transformed images looks noticeably better now.

@nical nical changed the title [WIP] Support repetition in brush shaders Support repetition in brush shaders Apr 10, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 10, 2018

Looks like I managed to break another test in the process. Investigating.

@nical nical force-pushed the nical:brush-img-repeat branch 3 times, most recently from e4e9eca to 7ee166a Apr 11, 2018
@nical nical requested a review from glennw Apr 11, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 11, 2018

The remaining issues are fixed.

I suggest reviewing the image part separately from the gradients. The idea is the same but while the gradient shaders both operate in local layout space (-ish), the image shader works in texture space which introduces a few subtle differences (for example an offset to take into account).
The gradient shaders use a strecthed repeated size in layout space (local_rect / repetition), while the image shader scales vUv instead of stretching the rect, achieves the same result but in a way that fits the rest of the shader.

@nical
Copy link
Collaborator Author

nical commented Apr 13, 2018

r? @glennw

@kvark
Copy link
Member

kvark commented Apr 13, 2018

Note: reviewed in #2644

@glennw
glennw approved these changes Apr 16, 2018
Copy link
Member

glennw left a comment

(reviewed in #2644)

@nical
Copy link
Collaborator Author

nical commented Apr 16, 2018

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2018

📌 Commit 7ee166a has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Apr 16, 2018

Testing commit 7ee166a with merge 2410227...

bors-servo added a commit that referenced this pull request Apr 16, 2018
Support repetition in brush shaders

Add support for repeating in the shader rather than decomposing on the CPU.

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

bors-servo commented Apr 16, 2018

💔 Test failed - status-taskcluster

@nical
Copy link
Collaborator Author

nical commented Apr 16, 2018

New reftest failures from outer space :(

@nical
Copy link
Collaborator Author

nical commented Apr 16, 2018

@glennw looks like this was broken by this code:

request.push(shadow_rect);
from #2656 which writes some data for the drop shadow in the spot where this PR writes the repeat parameter.
I am not sure how this works (the drop shadow hack). I'll have a look tomorrow but if off the top of your head there is a simple way to shift that data by another vec4 and leave the parameter available for repetitions it'd be great.

@nical nical force-pushed the nical:brush-img-repeat branch 2 times, most recently from 2a820a7 to d6e50f3 Apr 17, 2018
@nical nical force-pushed the nical:brush-img-repeat branch from d6e50f3 to a90c04d Apr 17, 2018
@nical
Copy link
Collaborator Author

nical commented Apr 17, 2018

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Apr 17, 2018

📌 Commit a90c04d has been approved by glennw

bors-servo added a commit that referenced this pull request Apr 17, 2018
Support repetition in brush shaders

Add support for repeating in the shader rather than decomposing on the CPU.

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

bors-servo commented Apr 17, 2018

Testing commit a90c04d with merge b360f8a...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 17, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing b360f8a to master...

@bors-servo bors-servo merged commit a90c04d into servo:master Apr 17, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 12 files left
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
bors-servo added a commit that referenced this pull request Apr 26, 2018
Use the brush shaders for repeated images/gradient in some cases.

Built on top of #2621, this PR makes use of the repetition support added in the brush shaders in the simple case (no tile spacing).

<!-- 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/2644)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Apr 26, 2018
Use the brush shaders for repeated images/gradient in some cases.

Built on top of #2621, this PR makes use of the repetition support added in the brush shaders in the simple case (no tile spacing).

<!-- 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/2644)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Apr 26, 2018
Use the brush shaders for repeated images/gradient in some cases.

Built on top of #2621, this PR makes use of the repetition support added in the brush shaders in the simple case (no tile spacing).

<!-- 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/2644)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Apr 27, 2018
Use the brush shaders for repeated images/gradient in some cases.

Built on top of #2621, this PR makes use of the repetition support added in the brush shaders in the simple case (no tile spacing).

<!-- 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/2644)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request May 4, 2018
Prerender tile spacing and use the brush shader.

On top of #2621 and #2644, bake the tile spacing into an image using a blit render task.
This lets us use the brush shader with tile spacing. In order for spacing to fully work the way we intend it to we need a bit more work (see #2659), but since the image shader we use for tile spacing doesn't do the right thing either, I don't think it would be a problem to move forward with this independently of #2659's resolution.

<!-- 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/2664)
<!-- Reviewable:end -->
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.