Skip to content

Conversation

@nical
Copy link
Contributor

@nical nical commented Apr 26, 2018

This PR implements decomposing tiled images during frame building using visibility information to avoid issues with very large blob images.
This version does not use segments, avoiding issues with the maximum contiguous gpu cache alloc size as well as a few hacks that I had to write to get repetitions to work with segments.

As a bonus the non-brush image primitive and ps_image shader are now obsolete so I removed them.


This change is Reviewable

@nical
Copy link
Contributor Author

nical commented Apr 26, 2018

Fixes #2689.

@glennw
Copy link
Member

glennw commented Apr 26, 2018

Reviewed 14 of 14 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


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

        // Tiled images have a cache handle per visible tile and an invalid handle in the
        // primitive metadata.
        let prim_cache_address = gpu_cache.try_get_address(&prim_metadata.gpu_location);

I think we'd be better to match on the prim kind here. Having a fallible call here may mask bugs for other primitive types, if the cache address is not valid when it's expected to be.


webrender/src/device.rs, line 1253 at r1 (raw file):

            dest_rect.origin.x,
            dest_rect.origin.y,
            dest_rect.origin.x + src_rect.size.width,

I don't understand this change. This would mean that we can never do a blit which is a stretch?


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

    /// Similar to get_address except that it accepts invalid cache handles in which case it
    /// returns an invalid cache address.
    pub fn try_get_address(&self, id: &GpuCacheHandle) -> GpuCacheAddress {

We can remove this, given the comment above about this method call.


webrender/src/prim_store.rs, line 1360 at r1 (raw file):

                                                    request.push(*tile_rect);
                                                    request.push(tight_clip_rect);
                                                    request.write_segment(*tile_rect, [1.0, 1.0, 0.0, 0.0]);

The GPU cache format for an image brush has changed recently - in between the prim rect and the segment info are two colors - they can just be set to PremultipliedColorF::WHITE in this case. We should abstract the code to write a brush image GPU request into a function (doesn't need to be done this PR though).


webrender/src/prim_store.rs, line 2162 at r1 (raw file):

}

fn compute_conservatrive_visible_rect(

nit: conservative spelling


webrender/src/prim_store.rs, line 2183 at r1 (raw file):

fn edge_flags_for_tile_spacing(tile_spacing: &LayerSize) -> EdgeAaSegmentMask {
    // If the number of gpu blocks for the request changes we can't reuse

This comment doesn't seem relevant here?


wrench/reftests/transforms/local-clip.png, line 0 at r1 (raw file):
Do we know why this reference image changed?


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Apr 26, 2018

@nical Amazing PR, thanks!

This looks great to me - it will be fantastic to finally remove the legacy image primitive.

There are a few comments from reviewable above, but they are all minor details. The sooner we can get this merged the better, since it will bitrot easily. Things left to complete below:

  • Address the reviewable comments above.
  • Rebase.
  • Gecko try run.
  • Merge!

@nical
Copy link
Contributor Author

nical commented Apr 27, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


webrender/src/prim_store.rs, line 2183 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This comment doesn't seem relevant here?

Oops, copy-pasta accident.


wrench/reftests/transforms/local-clip.png, line at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Do we know why this reference image changed?

This shouldn't be part of the final PR. I updated this reference image after changing the way the antialiasing is clipped in the image brush shader but this was in the PR about support repetitions in brush shaders and ended up in this commit queue by accident (this branch has been rebased many times). This PR will be quite a bit smaller once I have merged the two PRs that it sits on top of (#2644 and #2664).


Comments from Reviewable

@nical
Copy link
Contributor Author

nical commented Apr 27, 2018

webrender/src/prim_store.rs, line 1360 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

The GPU cache format for an image brush has changed recently - in between the prim rect and the segment info are two colors - they can just be set to PremultipliedColorF::WHITE in this case. We should abstract the code to write a brush image GPU request into a function (doesn't need to be done this PR though).

I don't see where these colors are being added currently. Has this changed landed?


Comments from Reviewable

@nical
Copy link
Contributor Author

nical commented Apr 27, 2018

webrender/src/prim_store.rs, line 1360 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

I don't see where these colors are being added currently. Has this changed landed?

Err nevermind, I hadn't properly rebased.


Comments from Reviewable

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

Successfully merging this pull request may close these issues.

2 participants