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 upPrerender tile spacing and use the brush shader. #2664
Conversation
|
This might only accidentally work, I didn't write any code to clear the target where the transparent pixels are (I'm guessing we don't automatically clear before blits), and we might want to implement this some other way. But it seems to work (at least in simple cases) with a relatively simple change which is satisfying. |
|
I am still a bit unsure about where the line is between the two but we might want to use a picture instead of a render task for this in order to support baking gradients. |
|
Color render targets are always cleared to transparent before any render tasks are drawn into them (there is a possible optimization opportunity to skip this in cases where it doesn't matter). The gradient + spacing question is an interesting one - baking them might be a bit tricky in cases where there is a transform active, I think. We can ponder that once we get the image + spacing baking working though :) |
|
|
|
|
|
Relevant review comment from #2695
Oops, I didn't know we support/need to do blits that stretch. I'll update the PR to support that properly. |
nical
changed the title
|
@glennw I added a commit that adds explicit support for padding in blit render tasks instead of using the target rect size and disallowing stretching blits (although we currently have an assertion checking that the source and destination sizes are equal). |
|
Actually, now that I am poking at this I wonder why we even do two blits and not just one when isolating sub-rects (and prerendering spacing since it uses the same code). I can see things don't show up when I do a single blit but it's not intuitive to me why we need it. |
|
When we pre-render spacing, we're doing the following: If we don't do the intermediate blit, the source texture is the same as the destination FBO texture, which is undefined behaviour in GL. |
|
(I haven't looked at this in detail, below is just a guess). I suspect that when drawing items to the texture cache as a final destination, we don't issue any clears on that target type? So the implicit assumption is that a task that targets the texture cache ends up writing every pixel. If that's the case - we should perhaps add support for clear mode (per render task) to texture cache targets, and a clear mode of "DontCare" if the task knows it will write to every pixel, to avoid clearing the task area in that case. |
|
The remaining reftest failure (blend-clipped.yaml) is caused by a difference introduced in the first patch that simplifies the repeated primitive parameters. Gecko shouldn't send non-integer stretch sizes to webrender (although it's something that we haven't fixed yet, so it currently does). I am tempted to change the reftest so that it uses the same stretch size and rect to avoid this configuration, although we could wait for gecko to fix the way it snaps stretch sizes to do that. |
|
I punted on clearing the padding in the second blit and just used the first blit to insert the spacing. We can look into having more involved mechanisms later, but I suspect in the more general case we'll want to move to actually use a picture with more commands instead of just a couple of blit render tasks to better support some of the cases where we currently get sampling artifacts. @glennw Edit: Hold on, the gecko try run of #2704 has more failures than I expected, I'll do another push with this one specifically before we merge it. |
|
The reftest change seems fine. I don't quite follow how the padding / clearing works though - if we don't clear, won't the previous contents of that texture cache allocation be left in the empty space? |
|
The first blit properly clears beforehand and we add the padding there. The second one blits the image including the pixels of the padding we prerendered so it shouldn't need a clear (unless blitting does some kind of blending). |
|
Oh, of course. That makes sense :) Is this ready to go then, or do we want to do another try push as you mentioned above? |
|
Yeah, waiting for https://treeherder.mozilla.org/#/jobs?repo=try&revision=d20a0909031b4cd589f4b064e59afe4c81796af9 On the other PR's try push there is a number of tests with showing shape with a bit of blurriness at the bottom that I suspect come from here, unfortunately. |
|
I think I figured the main problem out, hopefully this try push will come back green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95142a60f5aa1e2b32b383b578aaea41eaeee372 Edit: it looks great so far with an unexpected pass as a bonus \o/ |
|
Reviewed 2 of 5 files at r1, 3 of 6 files at r2, 1 of 1 files at r3. Comments from Reviewable |
|
@bors-servo r+ |
|
|
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 -->
|
|
nical commentedApr 16, 2018
•
edited by larsbergstrom
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.
This change is