Add support for caching render tasks across frames and display lists. #2284

glennw · 2018-01-11T07:36:10Z

This patch adds the infrastructure required to allow render tasks
to be rendered into the texture cache, where they can be stored
and referenced across multiple frames.

Since the cache key is based on the contents of the task itself,
the cache items remain valid even when a new display list is provided,
so long as the content of the cached task is the same. When the cache
is determined to be valid, the render task is just drawn as a simple
image, rather than executing the complex render task chain used to
build the output.

Convert box-shadows to make use of this new feature. When a new
box-shadow is required, the render task chain is set up to draw the
minimal box-shadow, and blur it. The blurred result is then stored in
the texture cache, available for use on this and subsequent frames.

Since items are drawn into the texture cache, this will also help
with batching in the future, as we collapse more items to be able to
use a simple shader that draws an image from the texture cache.

Remove blur regions since they are no longer used or required.
Remove old render task sharing within a single frame, it's redundant now.

TODO (as future work):

Support caching of RGBA8 render tasks.
Convert other tasks (e.g. clips, drop-shadows) to use cacheable tasks.
Use this as basis for optimizing rendering of static Pictures (e.g. a picture with a filter chain + image).

This change is

glennw · 2018-01-11T07:37:32Z

r? @kvark

I still need to do a try run - will kick one off shortly.

As you may expect, this has some quite significant performance and power saving wins. Next step for box shadows is to optimize the clip masks and the application of the box shadows (which are still way slower than they need to be).

glennw · 2018-01-11T09:33:43Z

Try run looks good (just one new PASS):

https://hg.mozilla.org/try/rev/2ea7546e4f797bfd4191921ebdccb428e3ffe27a
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ea7546e4f797bfd4191921ebdccb428e3ffe27a

kvark · 2018-01-11T19:52:46Z

Implementation looks good to me, minus a few non-blocking concerns. Amazing work!

Concept-wise, this triggers an alarm. Treating some parts of the texture cache as renderable while mostly using it as a texel storage is not going to make driver's life easier for determining the internal representation of the image (tiling mode, color compression, also cache flush points).

I suppose moving the persistent rendered results into separate texture(s) is not a great option in terms of batching and complexity. Perhaps, we'll investigate later if doing a blit from our temporary render target into texture cache is better than rendering into the texture cache directly. Blit requires the TRANSFER_DST flags in Vulkan, which we already need for copying stuff from CPU into the texture cache (with PBOs), so blitting would not incur any additional requirements for the image layout and such.

Reviewed 21 of 21 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

webrender/res/brush_image.glsl, line 65 at r1 (raw file):

    vUv.z = blur_task.common_data.texture_layer_index;
    vec2 texture_size = vec2(textureSize(sColor0, 0).xy);
    vColor = blur_task.color;

could also unconditionally have vColor output to merge these paths

webrender/src/batch.rs, line 472 at r1 (raw file):

            let batch = self.batch_list.get_suitable_batch(key, pic_metadata.screen_rect.as_ref().expect("bug"));

            let render_task_id = match *pic.surface.as_ref().expect("BUG: no surface for splitting") {

nit: can we match pic.surface and move the panic from expect here to a match arm?

webrender/src/render_task.rs, line 660 at r1 (raw file):

}

pub struct RenderTaskCache {

please add some comments explain what this is

webrender/src/render_task.rs, line 693 at r1 (raw file):

        // from here so that this hash map doesn't
        // grow indefinitely!
        self.entries.retain(|_, value| {

can we do this by asking the texture cache if each particular entry has been evicted (as opposed to introducing FRAMES_BEFORE_EVICTION here)?

webrender/src/render_task.rs, line 712 at r1 (raw file):

                              .entry(key)
                              .or_insert_with(|| {
                                RenderTaskCacheEntry {

nit: or_insert would probably be as good here

webrender/src/render_task.rs, line 723 at r1 (raw file):

            // Invoke user closure to get render task chain
            // to draw this into the texture cache.
            let (render_task_id, user_data) = f(render_tasks);

note to self: refactor to avoid passing a closure there, e.g. by having a temporary object/builder

webrender/src/texture_cache.rs, line 429 at r1 (raw file):

                 DeviceUintRect::new(origin, entry.size))
            }
            None => panic!("BUG: handle not requested earlier in frame"),

could also go with as_ref().expect(...) at the beginning, matching the arm body

webrender/src/tiling.rs, line 519 at r1 (raw file):

}

impl RenderTarget for TextureCacheRenderTarget {

that's one weird implementation... only having add_task doing something meaningful.
Do we have to have this implementation?

Comments from Reviewable

glennw · 2018-01-11T21:04:56Z

Review status: 15 of 21 files reviewed at latest revision, 8 unresolved discussions.

webrender/res/brush_image.glsl, line 65 at r1 (raw file):