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

Fix picture caching on pages with fixed position clips. #3459

Merged
merged 1 commit into from Jan 3, 2019

Conversation

Projects
None yet
5 participants
@gw3583
Copy link
Collaborator

gw3583 commented Jan 3, 2019

Previously, the code used a clip node collector to build an
overall (clipped) bounding rect for the tile cache.

This is needed to ensure that the tiles don't write into the
z-buffer where primitives don't exist on the tiles, so that
primitives earlier in the render order (e.g. scroll bars)
still draw correctly in cases where the content overflows the
scroll region and is clipped.

The clip node collector code handles common cases, but it fails
when there are fixed position clips on small items within the
primitive instance list.

Instead, build the overall clipped world rect and per-primitive
required rects to handle partial tiles. This is quite a
conservative approach, for now. In future we should be able
to optimize these changes quite significantly.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1517117


This change is Reviewable

Fix picture caching on pages with fixed position clips.
Previously, the code used a clip node collector to build an
overall (clipped) bounding rect for the tile cache.

This is needed to ensure that the tiles don't write into the
z-buffer where primitives don't exist on the tiles, so that
primitives earlier in the render order (e.g. scroll bars)
still draw correctly in cases where the content overflows the
scroll region and is clipped.

The clip node collector code handles common cases, but it fails
when there are fixed position clips on small items within the
primitive instance list.

Instead, build the overall clipped world rect and per-primitive
required rects to handle partial tiles. This is quite a
conservative approach, for now. In future we should be able
to optimize these changes quite significantly.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1517117
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 3, 2019

r? @bholley or @kvark

This fixes a number of picture caching glitches on various sites. It's a fairly conservative approach, but I'd rather get the correctness issues fixed and then apply more clever optimizations once enabled.

Pending try (caching disabled):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4f08722b5be28da75a52578fbdefe3ddfbf6aaf

Pending try (caching enabled):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c9665e759bef2f042ef9218d6122dc68e59017

There are a couple of known intermittent failures on try with caching enabled. I know what these are caused by, and they are unrelated to this patch. So it should be fine to merge this after review, since (a) caching is still disabled by default and (b) I'm working on fixes for these now.

@bholley

This comment has been minimized.

Copy link
Contributor

bholley commented Jan 3, 2019

@bors-servo r+

This is a rubber-stamp since kvark is out, he should give it a real review when he gets back.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2019

📌 Commit e480f0a has been approved by bholley

@Darkspirit

This comment has been minimized.

Copy link

Darkspirit commented Jan 3, 2019

As bugspam indicated, this also fixes bug 1516839, bug 1516753, bug 1515540 and bug 1512784.

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2019

⌛️ Testing commit e480f0a with merge 4ff07fe...

bors-servo added a commit that referenced this pull request Jan 3, 2019

Auto merge of #3459 - gw3583:batch-scissor-8, r=bholley
Fix picture caching on pages with fixed position clips.

Previously, the code used a clip node collector to build an
overall (clipped) bounding rect for the tile cache.

This is needed to ensure that the tiles don't write into the
z-buffer where primitives don't exist on the tiles, so that
primitives earlier in the render order (e.g. scroll bars)
still draw correctly in cases where the content overflows the
scroll region and is clipped.

The clip node collector code handles common cases, but it fails
when there are fixed position clips on small items within the
primitive instance list.

Instead, build the overall clipped world rect and per-primitive
required rects to handle partial tiles. This is quite a
conservative approach, for now. In future we should be able
to optimize these changes quite significantly.

This fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1517117

<!-- 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/3459)
<!-- Reviewable:end -->
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 3, 2019

@Darkspirit Thank you for re-testing all those bugs against this try! That was going to be my first job this morning 😀

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 3, 2019

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: bholley
Pushing 4ff07fe to master...

@bors-servo bors-servo merged commit e480f0a into servo:master Jan 3, 2019

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

@gw3583 gw3583 deleted the gw3583:batch-scissor-8 branch Jan 3, 2019

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 4, 2019

Bug 1517579 - Update webrender to commit 4ff07fe75b0c28ee1987092b3eef…
…ed3017f5a25e (WR PR #3459). r=kats

servo/webrender#3459

Differential Revision: https://phabricator.services.mozilla.com/D15692

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 4, 2019

@kvark
Copy link
Member

kvark left a comment

better late then never review

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @gw3583)


webrender/src/clip.rs, line 1343 at r1 (raw file):

// root at the end of primitive preparation.
#[derive(Debug)]
pub struct ClipNodeCollector {

is there still value in having this type, given the triviality of the logic it carries?


webrender/src/picture.rs, line 209 at r1 (raw file):

        self.opacity_bindings.reset();
        self.image_keys.reset();
        self.needed_rects.clear();

are we missing current_rects.clear() here?


webrender/src/picture.rs, line 225 at r1 (raw file):

            for (needed, current) in self.needed_rects.iter().zip(self.current_rects.iter()) {
                if !current.contains_rect(needed) {
                    return false;

it's not desirable to have this return false versus the other results that are expression driven.
Would be good to rewrite it as:

self.needed_rects
  .iter()
  .zip(&self.current-rects)
  .all(|(needed, current)| current.contains_rect(needed))

webrender/src/picture.rs, line 288 at r1 (raw file):

    pub pending_blits: Vec<TileBlit>,
    /// The current world bounding rect of this tile cache. This is used
    /// to derive a local clip rect, such that we don't obscure in the

Deriving local from the world is rather conservative if transformations are involved. Is this the best we can do?


webrender/src/picture.rs, line 731 at r1 (raw file):

            self.map_local_to_world.set_target_spatial_node(
                clip_chain_node.spatial_node_index,

I'm confused. The mapper is called "local_to_world" but the target node is clip_chain_node.spatial_node_index. What makes it guarantee to be the world? Why aren't we using the ROOT_SPATIAL_NODE_INDEX then?


webrender/src/picture.rs, line 744 at r1 (raw file):

                    // Clips that are not in the root coordinate system are not axis-aligned,
                    // so we need to treat them as normal style clips with vertices.
                    if clip_spatial_node.coordinate_system_id == CoordinateSystemId(0) {

to follow-up, why aren't we comparing it with the coordinate system of the clip chain node?


webrender/src/picture.rs, line 753 at r1 (raw file):

                            world_clip_rect = match world_clip_rect.intersection(&clip_world_rect) {
                                Some(rect) => rect,
                                None => return,

this return looks a bit dangerous... don't we need to set up any state before exiting the function?
for example, what happens if there is another clip node up the chain that has a different coordinate system, which we'd need to affect clip_chain_uids and clip_vertices (if it wasn't early returning)?


webrender/src/picture.rs, line 787 at r1 (raw file):

        resource_cache: &mut ResourceCache,
        gpu_cache: &mut GpuCache,
        clip_store: &ClipStore,

niiice


webrender/src/picture.rs, line 808 at r1 (raw file):

                //           a partially clipped tile, which would be a significant
                //           optimization for the common case (non-clipped tiles).
                let needed_rect = match world_clip_rect.intersection(&tile.world_rect).and_then(|r| r.intersection(screen_world_rect)) {

looks like we could move the intersection of world_clip_rect and screen_world_rect out of the tile loop


webrender/src/picture.rs, line 956 at r1 (raw file):

                self.pending_blits.push(TileBlit {
                    target: cache_item,
                    src_offset: src_origin,

this is probably a stupid question, but... what happens if one frame we blit the visible portion of a tile that doesn't cover the whole tile. Would we consider tile to be completely available for the next frames (and forget that we only blitted part of it)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment