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

Lazily tile images #2370

Closed
kvark opened this issue Feb 1, 2018 · 14 comments
Closed

Lazily tile images #2370

kvark opened this issue Feb 1, 2018 · 14 comments

Comments

@kvark
Copy link
Member

@kvark kvark commented Feb 1, 2018

Since the blob image doesn't have to be stored in full anywhere, the incoming size is not limited by anything. We are currently tiling all images eagerly in decompose_image, which may produce near-infinite number of primitives for extra large images. We should have a different code path for blobs that would only tile the visible parts of an image.

cc @gankro @nical

@Gankra
Copy link
Contributor

@Gankra Gankra commented Feb 1, 2018

For the sake of getting our CI up and running, I will be mitigating this a bit in gecko (by not emitting a blob image for a content-less video), but this is just a whack-a-mole game, so we should still do this.

@glennw
Copy link
Member

@glennw glennw commented Feb 1, 2018

This is interesting timing.

I've been thinking a bit about (and prototyping) the changes necessary to (a) enable segments / brushes for images and (b) use pre-rendering to allow removing spacing / repeat from the image shader.

As part of this, I was questioning whether the image tiling should be moved inside a single image primitive and handled there. Doing that would allow us to do the tiling lazily, so this would be a benefit / advantage to doing that.

I'll self-assign this for now, since I could possible handle this as part of the work mentioned above that I'll try to get to in the next few weeks.

@glennw glennw self-assigned this Feb 1, 2018
@glennw
Copy link
Member

@glennw glennw commented Feb 1, 2018

Another potential benefit of this - very large non-blob images could then make use of the normal texture cache pages, since they would be tiled to the size of the largest available texture cache slab, without involving a standalone texture cache page.

@nical
Copy link
Collaborator

@nical nical commented Feb 2, 2018

Yeah, doing eager tile decomposition while building the scene was the easy way to get large images to work, but moving this logic to lazily generate segments or primitives during frame building should be strictly better.
This doesn't really need to be about blob images, any kind of image should benefit from being tiled lazily rather than eagerly.

Another potential benefit of this - very large non-blob images could then make use of the normal texture cache pages

I though they were already making use of normal texture cache pages (when they are tiled that is).

@nical nical changed the title Lazily tile blob images Lazily tile images Mar 9, 2018
@nical
Copy link
Collaborator

@nical nical commented Mar 9, 2018

I am having a look at this since the eager tile decomposition was causing grief with very large content-less video elements, and is probably the source of bug 1442293.

It's been a while since I have dug in the frame building code, so I am re-learning a lot about how it works but it seems that we need to remove the decomposition during flattening and:

  • During culling, decompose the image into visible tiles and request the visible texture cache entries, and store information about the visible tiles in the image primitive (prim_store.rs).
  • During batching, emit individual image instances for each tile using the tiling information generated in the earlier step (batch.rs/tiling.rs).
@glennw
Copy link
Member

@glennw glennw commented Mar 9, 2018

Although this would probably fix 1442293, I think it would not be fixing the root cause (see my comments in that bug for details).

I've been thinking quite a bit about how we could implement lazily tiling images, because it turns out to be very similar conceptually to some work I'm doing now to fix partially clipped blurs and drop-shadows. That is, we need to be able to draw a render task / texture cache entry where (a) we draw more than the clipped area requires but (b) less than the total size of the render task (for efficiency reasons). This is effectively the same as tiling - having a large texture entry where only part of that texture is "resident". (I'm not planning to do a full virtual texturing system btw 😁 ).

So, the issue I see with just doing a simple solution like we have now (even if we start to do it lazily), is that we will have filtering quality issues between the image tiles. This means we'll see visible seams during zooming of tiled images, and also with any kind of transform on them.

So, to sum up, I have some ideas for how to solve these, and I'm planning to prototype them in the next week or so (since I need to fix the render task issue for correctness bugs anyway). So if your changes above are relatively simple, it may be worth working on them to get the immediate memory saving (although I don't think it's the root cause for 1442293). On the other hand, it might be worth holding off and seeing if the work I do for partial render tasks is applicable to tiled images (I'm fairly confident it will be).

@glennw
Copy link
Member

@glennw glennw commented Mar 9, 2018

Writing up a bit more detail of the idea here, in case people have comments, or perhaps @nical has the time to prototype this for image tiling, if it makes sense.

We expand the texture cache entry item (that is provided to the GPU cache) to have an extra rect - so now each texture cache entry has both a "total size" and a "resident rect".

For image tiling, during prepare_prim_for_render, we work out which tile segments (e.g. 512x512 blocks) of the image are visible on screen. Then, we use the render task cache (via request_render_task) to get a texture cache image which has a resident rect that is large enough to draw the image on screen without any visibly missing pixels.

The render task cache means that image will be cached, and only need to be rebuilt from the image tiles when scrolling or changing the page in such a way that the cached render task image needs a different set of tiles to be resident.

This has the advantage that the drawn texture is a single contiguous image, so no filtering quality issues.

The downside is rebuilding the visible portion of the image on some frames, but this is really just a few texture blits (since the used tiles that we are blitting would remain valid in the texture cache).

Thoughts? Does that make sense?

@glennw
Copy link
Member

@glennw glennw commented Mar 9, 2018

I think this gets us a fairly simple implementation that (a) does tiling lazily (b) draws a single instance, so minimal CPU work and (c) solves the filtering quality issues across tiles.

@glennw
Copy link
Member

@glennw glennw commented Mar 9, 2018

I guess the stuff above is sort of a superset of lazily drawing individual tiles as proposed above, since we need the same information about which tiles are visible etc. So maybe it does make sense for @nical to do the simple lazy tiling implementation first, and then we could consider building this on top of that later, to resolve the quality issues. Thoughts?

@nical
Copy link
Collaborator

@nical nical commented Mar 11, 2018

I guess the stuff above is sort of a superset of lazily drawing individual tiles as proposed above, since we need the same information about which tiles are visible etc.

Yeah, sounds like the stuff you describe requires the same infrastructure, with potentially an extra pass to merge the tiles back into a single image that is then composited.

The render task cache means that image will be cached, and only need to be rebuilt from the image tiles when scrolling or changing the page in such a way that the cached render task image needs a different set of tiles to be resident.

Scrolling is generally the biggest source of new frames so it's good to keep that in mind (while transforms and zooms are much rarer). We can measure this whenever we get there, but it is probably worth only doing the merging of the tiles in the render task cache if we know we are in a situation where we might get filtering issues, and render the image tiles on screen directly the rest of the time (which shouldn't be hard to do since it could be the same code rendering on screen instead of in the cache).
The way I understand what you wrote, if we are, say, scrolling through a very big image, we'd get a screen-full over overdraw each time we need to rebuild the render task cache (plus the memory overhead of having tiles both in the render task cache and the texture cache).

@glennw
Copy link
Member

@glennw glennw commented Mar 11, 2018

Yup, applying the render task step only when the visual quality will be affected makes sense, and should be easy to do.

@mstange
Copy link
Contributor

@mstange mstange commented Mar 11, 2018

So, the issue I see with just doing a simple solution like we have now (even if we start to do it lazily), is that we will have filtering quality issues between the image tiles.

I think we can get correct filtering even with tiles, if we make the tiles overlap by a few pixels. The necessary overlap size would depend on the scale factor, but maybe we could say "4 pixels overlap is enough for reasonable scales" because at higher scales the blurriness is a bigger problem than seams between tiles.

@nical
Copy link
Collaborator

@nical nical commented May 14, 2018

This landed in #2742.

@nical nical closed this May 14, 2018
@Gankra
Copy link
Contributor

@Gankra Gankra commented 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.