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

Implement the fallback path for the big size image #1405

Closed
JerryShih opened this issue Jun 20, 2017 · 13 comments
Closed

Implement the fallback path for the big size image #1405

JerryShih opened this issue Jun 20, 2017 · 13 comments

Comments

@JerryShih
Copy link
Contributor

@kvark @glennw @nical @sotaroikeda
In webrender, we have the limit size of gl texture. Currently, there is an assertion if the image is to large[1]. When we integrate gecko with WR, there are a lot of crash for this assertion.
I'm trying to create a global shared black texture. If we hit the limitation, show the global shared texture instead.

[1]
cb510c5

@glennw
Copy link
Member

glennw commented Jun 20, 2017

cc @nical

I thought we handled this with tiling support now?

@glennw
Copy link
Member

glennw commented Jun 20, 2017

Ah, probably just not for external images, right?

@nical
Copy link
Contributor

nical commented Jun 20, 2017

The plan is to use tiling for this, I thought we already supported tiled external images, but there might be a bit of missing glue to get the two to work together (I vaguely remember looking into this a while back but now I a not sure what came out of it). It should not be hard to do.

@nical
Copy link
Contributor

nical commented Jul 17, 2017

So I had another look and we already have code to support tiling external buffer images the same way we tile regular images. This is done by specifying the proper stride and offset in the texture cache that is passed to TextureUpdateOp::UpdateForExternalBuffer. ResourceCache::should_tile allows tiling external buffer images under the same conditions as regular images.

@JerryShih Are you referring to other types of external images (like external texture handles) that are bigger than webrender's maximum texture size setting?

@JerryShih
Copy link
Contributor Author

I think the "external buffer" could be used with a large size[1]. But I think this type of buffer could be tiled.

https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/gfx/layers/composite/TextureHost.cpp#590

@nical
Copy link
Contributor

nical commented Aug 2, 2017

@JerryShih I did some testing and as far as I can tell, external image buffers are properly tiled inside WebRender, with two exceptions: masks and yuv images. Gecko currently doesn't support tiling masks and yuv images either (but maybe that doesn't trigger a fatal assertion).

Could you provide some more details about how you run into this issue (when you have some time, no hurry)?

@jrmuizel
Copy link
Collaborator

jrmuizel commented Aug 2, 2017

We're seeing crashes like: https://bugzilla.mozilla.org/show_bug.cgi?id=1383731
where we fail this assert: https://hg.mozilla.org/mozilla-central/annotate/52285ea5e54c/gfx/webrender/src/texture_cache.rs#l693

Shouldn't tiling be preventing this from happening?

@nical
Copy link
Contributor

nical commented Aug 2, 2017

Tiling should indeed prevent this from happening, unless the image is a mask (or a yuv image). I'll look into adding more useful information in the assertion to see where this is coming from.

@JerryShih
Copy link
Contributor Author

I have some tests in:
https://bugzilla.mozilla.org/show_bug.cgi?id=1352092#c20
In conclusion, the WR could have tiling for external image buffer(the BufferTextureHost in gecko), but no tiling for mask and yuv video.

@JerryShih
Copy link
Contributor Author

We could try to implement the tiling for all mask types and yuv buffer, but I think the shader code for masking will become a little bit complicated. So, I will still try to use a dummy texture if we hit the limitation of texture size.

@JerryShih
Copy link
Contributor Author

@kvark @glennw
I'm not sure the usage of this dummy_cache_texture. Why we need this? Do we try to draw a primitive without setting the texture?

dummy_cache_texture_id: TextureId,

Maybe we could reuse this texture if we can't allocate the new texture or we hit the size boundary.

@nical
Copy link
Contributor

nical commented Aug 8, 2017

This dummy cache texture appears to be passed by default for render passes that don't actually need to read from the texture cache. It sounds reasonable to use it as the fallback texture when we fail to allocate an image.

bors-servo pushed a commit that referenced this issue Oct 16, 2017
Handle big size image

@nical @glennw @kvark @sotaroikeda
r?

This patch try to handle the #1405.
If we hit the maximum texture size, turn to use a 1x1 rgba(255,255,255,255) dummy texture.

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

glennw commented Oct 23, 2017

Closing this now since #1564 has landed. Please re-open if there is more work needed.

@glennw glennw closed this as completed Oct 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants