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

[RFC] Handle image spacing by creating a pre-spaced image in the texture cache? #2312

Closed
glennw opened this issue Jan 17, 2018 · 10 comments
Closed
Assignees

Comments

@glennw
Copy link
Member

@glennw glennw commented Jan 17, 2018

Some complexity in the image shader comes from the support for spacing.

This is not a massive issue, but it does complicate a few things:

  • The image shader is more complex than otherwise, making it more difficult to unify with other similar shaders (e.g. brush_image and ps_text_run).
  • As soon as an image uses spacing, we move it to be in the alpha pass, and don't get the benefit of drawing an otherwise opaque image.

A naive solution is to just produce a CPU instance per repetition. However, web authors often take a 1px wide image and repeat it thousands of times across a page, which causes a huge number of instances to be required.

Instead, we could take advantage of the recently landed support for persistent caching of render tasks in the texture cache. We could, for instance:

  1. In the case of spacing, create a texture cache entry that has the source image, and the spacing pre-rendered. This would mean (a) remove all spacing support from the image shader, and (b) those images get marked as alpha, leaving the original source image marked as opaque, simplifying some of the logic.
  2. Detect when there is a small(ish) number of instances are required, and draw those as separate instances instead. This would allow us to use the opaque pass for images with spacing in this case.
  3. For the case of a small (e.g. 1x4) image repeated a lot, we could pre-render that into a persistent RT cached entry, where we manually apply the repeat 64 times. Then, we would greatly reduce the number of CPU-side instances we'd need to create for this case.

Those optimizations are somewhat orthogonal, but all based on the idea of pre-rendering some spacing or repeating into the texture cache when needed. I think this could simplify our image shader quite a bit, and also allow us to draw more cases in the opaque pass.

Thoughts?

@glennw
Copy link
Member Author

@glennw glennw commented Jan 17, 2018

@glennw
Copy link
Member Author

@glennw glennw commented Jan 17, 2018

also @mstange

@glennw
Copy link
Member Author

@glennw glennw commented Jan 17, 2018

Spacing, in particular, is something that is unfortunate to pay the shader cost for on all images, since it's rarely used.

Paying the cost on spaced images by pre-rendering that image once, and removing the cost from the image shader seems like a win on the vast majority of content.

@nical
Copy link
Collaborator

@nical nical commented Jan 17, 2018

I like this a lot. For rare cases where the spacing is very big, we can always detect it and fall back to the code that creates an instance per primitive.

@kvark
Copy link
Member

@kvark kvark commented Jan 17, 2018

In terms of performance and cleanness, having an instance per image repeat makes most sense to me. If the web author requests a google of repeats, and we populate those in the instance data, we are screwed for sure. However, what if we don't have instance data?

So here is another one:
4) Issue an instanced call to draw an image repeat per instance, but without any per-instance data. Given gl_InstanceID and the info about initial position and spacing, the vertex shader can produce required positions and texture coordinates.

This way we aren't going to be stuck preparing the work for GPU. In the worst case, the GPU will be slow to process those vertices, but a similar effect is surely achievable by other means (generating a thousand large-radius box shadows, for example), so we don't need to try to hard to avoid this specifically for spaced images.

@mstange
Copy link
Contributor

@mstange mstange commented Jan 17, 2018

I don't have strong opinions on these suggestions, but they sound reasonable to me, and they're listed in decreasing order of usefulness.

Spaced repeats are used extremely rarely. The CSS property/value combination background-repeat: space is not very well-known and hasn't been universally supported until fairly recently. It's a good approach to only make users of this feature pay for it, rather than having it negatively affect all repeated image drawing.

@pcwalton
Copy link
Collaborator

@pcwalton pcwalton commented Jan 20, 2018

Didn't Servo hit this exact problem years ago? :)

I think prerendering the spacing into the texture cache seems like the way to go.

  1. Issue an instanced call to draw an image repeat per instance, but without any per-instance data. Given gl_InstanceID and the info about initial position and spacing, the vertex shader can produce required positions and texture coordinates.

I'm worried about the vertex shading and rasterization engine load for large repeats of 1px images. We hit this fairly early on in Servo, as above.

@kvark
Copy link
Member

@kvark kvark commented Jan 20, 2018

I'm worried about the vertex shading and rasterization engine load for large repeats of 1px images.

Is this a hypothetical valid use-case we are talking about or just a malicious/benchmarking web page? If the former, can I see a link? If the latter, we should really be fine with doing lots of GPU work, as long as we don't hang/crash.

@pcwalton
Copy link
Collaborator

@pcwalton pcwalton commented Jan 20, 2018

I remember it being a problem years ago in Servo. I don't remember the pages offhand. But people do have tiny repeated backgrounds. Google brings up http://supernovathemes.com/55-small-lite-background-images-for-website-you-can-repeat/

@nical
Copy link
Collaborator

@nical nical commented May 14, 2018

This was implemented in #2664.

@nical nical closed this May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.