Basic preserve-3d support #1169

kvark · 2017-04-26T17:55:59Z

Goes towards #739

This is a basic implementation that features a green test case. It can be merged independently, but more work is expected to solidify it.
There is a lot of interaction of "preserve3d" stacking contexts with filters, composite ops, and such, that is not thought-through or tested at the moment. The new code should be totally safe as long as the client continues to use TransformStyle::Flat (as both Gecko/Servo still do).

r? @glennw

This change is

bors-servo · 2017-04-26T20:12:35Z

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

glennw · 2017-04-26T22:32:51Z

Reviewed 17 of 22 files at r1.
Review status: 17 of 22 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

wrench/reftests/split/simple.yaml, line 7 at r1 (raw file):

    - type: stacking-context
      bounds: [0, 0, 1024, 1024]
      transform-style: preserve3d

CSS uses preserve-3d - let's use the CSS keyword in YAML files.

Comments from Reviewable

glennw · 2017-04-26T22:33:22Z

Partial review only so far - I'll be working through the remaining files today.

glennw · 2017-04-26T23:08:54Z

Reviewed 5 of 22 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

webrender/res/ps_split_composite.vs.glsl, line 26 at r1 (raw file):

    vec2 normalized_pos = mix(
        mix(geometry.points[0], geometry.points[1], aPosition.x),

Extracting this into a bilerp() function might make the intent clearer.

webrender/src/frame.rs, line 440 at r1 (raw file):

                                              level == 0,
                                              composition_operations,
                                              bounds.clone(),

I think this type might be Copy?

webrender/src/frame_builder.rs, line 85 at r1 (raw file):

}

fn make_polygon(sc: &StackingContext, node: &ClipScrollNode, anchor: usize)

Could you explain how the anchor parameter works?

webrender/src/frame_builder.rs, line 254 at r1 (raw file):

                // is the root of the page
                let isolation = &mut self.stacking_context_store[parent_index.0].isolation;
                if *isolation != ContextIsolation::None {

What could cause this - I guess if we have mix-blend-mode on a preserve-3d?

webrender/src/frame_builder.rs, line 1145 at r1 (raw file):

                    if stacking_context.isolation == ContextIsolation::Items {
                        let task_size = DeviceIntSize::from_untyped(&stacking_context.local_bounds.size.ceil().to_i32().to_untyped());

This doesn't seem right - the local bounds won't take the zoom / transform etc into effect. Shouldn't this be based on the screen bounding rect?

webrender/src/frame_builder.rs, line 1155 at r1 (raw file):

                    }

                    if composite_count == 0 && stacking_context.isolation == ContextIsolation::Full {

It might be clearer to combine this if and the one above into a match statement?

webrender/src/frame_builder.rs, line 1284 at r1 (raw file):

                    }

                    let group_index_opt = match stacking_context.isolation {

I'm not sure I fully understand this part - does this imply that any primitives in a preserve-3d context get rendered without any transform to the intermediate surface?

Comments from Reviewable

glennw · 2017-04-26T23:10:11Z

@kvark In general this looks great! It's far fewer lines of code than I expected (although I guess it helps having the plane splitting crate separate). I have a few questions about the PR above that I don't fully understand - it might be easier to discuss on IRC / Vidyo?

glennw · 2017-04-26T23:16:24Z

@kvark Ah, I think I understand a bit better now. So the current implementation renders those intermediate sources untransformed into a target. Then it splits them and composites them, applying the transform when compositing the split planes - is that right?

I think that if we do it this way, we may have quality issues when the transformed planes have an interesting transform on them - since, I think, we'll end up drawing things like text and borders at the un-zoomed quality level into the intermediate target.

If that's right - I wonder if we could do it the inverse way - draw the intermediate surfaces transformed at full zoom / scale, and then composite them in screen space?

I'm not sure how easy / feasible that is, or if I'm completely mis-understanding something - does that make any sense? :)

glennw · 2017-04-26T23:17:52Z

@kvark Also, even if the above is correct - it might still make sense to merge this now, as a step towards that?

kvark · 2017-04-27T01:08:00Z

Reviewed 1 of 5 files at r2.
Review status: 19 of 23 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

webrender/res/ps_split_composite.vs.glsl, line 26 at r1 (raw file):