Remove manual interpolation of local space in transform shaders. #2069

glennw · 2017-11-21T04:42:05Z

Instead, for vertex transform shaders we now write out:
(world.xy, z * world.w, world.w)

This gives us constant Z across the polygon for draw ordering.

It also means we can use the hardware interpolators instead of
manually dividing by Z in the fragment shader for local_pos.

This is mostly preparation work for switching shaders over to
be brush primitives, but has the happy side effect of:

Fixing a small number of Gecko + Servo reftests.
Simpler and faster code.

This change is

glennw · 2017-11-21T04:42:19Z

r? @kvark

New passes in Servo:
▶ PASS [expected FAIL] /css/css-transforms/css-transforms-3d-on-anonymous-block-001.html
▶ PASS [expected FAIL] /_mozilla/css/transform_skew_a.html

New passes in Gecko:
transform-3d/split-non-ortho1.html

Gecko try is here (looks to be all green except for the PASS above and intermittents):
https://hg.mozilla.org/try/rev/9d2110ecd88f77aec63dd5cfd8c0d58ce08b1128
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d2110ecd88f77aec63dd5cfd8c0d58ce08b1128

glennw · 2017-11-21T04:44:21Z

There's clearly some more tidy up that can be done as a follow up with this change (e.g. we could unify write_vertex and write_transform_vertex quite easily), but I think it's reasonable to do those as follow ups as we switch primitives over to be brush shaders.

glennw · 2017-11-21T04:48:11Z

To save any more rebase hassles for @lsalzman, we should make sure #2058 lands before this (I'll rebase on that if needed, but review of this should still be fine in the meantime).

bors-servo · 2017-11-21T06:37:14Z

☔ The latest upstream changes (presumably #2058) made this pull request unmergeable. Please resolve the merge conflicts.

glennw · 2017-11-21T07:43:37Z

Rebased over the rotated text PR.

kvark · 2017-11-21T21:37:21Z

I really like where this is going! 👍
A few concerns need to be cleared out though.

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

webrender/res/prim_shared.glsl, line 652 at r1 (raw file):

    // Calculate a clip rect from local_rect + local clip + layer clip.
    RectWithEndpoint local_rect = to_rect_with_endpoint(instance_rect);
    local_rect.p0 = clamp_rect(local_rect.p0, local_clip_rect);

can we just clamp to clip_rect instead?

webrender/res/prim_shared.glsl, line 665 at r1 (raw file):

    // can do some math on the projection matrix to work out a variable
    // amount to extrude.
    float extrude_distance = 4.0;

I'm still a bit confused about 4 here. I'd prefer us either to be precise for the simple case only (of 1:1 scale with the screen, so that the extrude = 0.5), or for the general case, but not for an arbitrary point in-between.

webrender/res/prim_shared.glsl, line 665 at r1 (raw file):

    // can do some math on the projection matrix to work out a variable
    // amount to extrude.
    float extrude_distance = 4.0;

TODO (possibly, for myself): use clip_edge_mask here

webrender/res/prim_shared.glsl, line 688 at r1 (raw file):

    // Apply offsets for the render task to get correct screen location.
    gl_Position.xy += task.common_data.task_rect.p0 - task.content_origin;

interesting, so this is changing the semantics: before we had translation to the target space followed by uTransform, now we have the opposite order

webrender/res/prim_shared.glsl, line 704 at r1 (raw file):

        prim.local_rect,
        prim.local_clip_rect,
        vec4(1.0),

why is this changed?

webrender/res/prim_shared.glsl, line 823 at r1 (raw file):

}

float init_transform_fs(vec2 local_pos) {

sweeet!

webrender/res/ps_image.glsl, line 87 at r1 (raw file):

    // We clamp the texture coordinate calculation here to the local rectangle boundaries,
    // which makes the edge of the texture stretch instead of repeat.
    vec2 upper_bound_mask = step(vLocalRect.zw, vLocalPos);

hmm, this would give us a different position, wouldn't it?
previous code used perspective division here in FS, the new code uses non-perspective interpolated XY

wrench/reftests/transforms/perspective.png, line 0 at r1 (raw file):
so I assume you needed to change this because now it's no longer perspective...

Comments from Reviewable

glennw · 2017-11-21T22:40:14Z

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

webrender/res/prim_shared.glsl, line 652 at r1 (raw file):