Remove the tiling code.

[glennw]

We now have solutions for all the parts of the renderer that previously relied on tiling for
efficiency.

---

This change is 

[glennw]

r? @kvark Sorry for the large patch!

[bors-servo]

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

[kvark]

Review status: 0 of 20 files reviewed at latest revision, 10 unresolved discussions.

---

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

struct AlphaBatchTask {
    vec2 screen_space_origin;

nicely done!

---

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

    // apply the task offset
    vec2 final_pos = device_pos - task.screen_space_origin + task.render_target_origin;

this is way clearer now, 👍

---

webrender/res/ps_composite.vs.glsl, line 12 at r1 (raw file):

    AlphaBatchTask src_task = fetch_alpha_batch_task(pi.user_data.y);

    vec2 dest_origin = dest_task.render_target_origin -

hmm, this seem to diverge from the old code:
Was: dest.task_origin
Now: dest.task_origin - dest.screen_origin + src.screen_origin

---

webrender/res/ps_composite.vs.glsl, line 22 at r1 (raw file):

    vec2 texture_size = vec2(textureSize(sCache, 0));

    vec2 st0 = (backdrop_task.render_target_origin + vec2(0.0, backdrop_task.size.y)) / texture_size;

these st0 and st1 have also diverged from the original. You are essentially Y-flipping the backdrop_task UVs. Is this intentional?

---

webrender/src/renderer.rs, line 1062 at r1 (raw file):

                self.device.bind_draw_target(Some(cache_draw_target), Some(cache_texture_dimensions));

                let src_x = backdrop.data[0] - backdrop.data[4] + source.data[4];

ok, yes, this is a bit (too much) hacky

---

webrender/src/renderer.rs, line 1115 at r1 (raw file):

        let projection = {
            let _gm = self.gpu_profile.add_marker(GPU_TAG_SETUP_TARGET);
            self.device.bind_read_target(render_target);

why is this needed?

---

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

        batch.items.push(PrimitiveBatchItem::StackingContext(layer_index));

        match &mut batch.data {

pretty sure you can get away without &mut here and the line below

---

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

    /// Apply a horizontal blur pass of given radius for this primitive.
    HorizontalBlur(i32, PrimitiveIndex),
    /// Allocate a block of space in target for framebuffer readback.

I think using readback term here is confusing. Perhaps, change it to backup or even copy?

---

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

                        if intersects {
                            break 'outer;

what is the point of early-out here? I don't see any side effects in this inner loop body, so it doesn't seem like anything changes at all if we drop out early. Perhaps, you forgot to assign alpha_batch_index after the loop?

---

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

                        let prim_index = PrimitiveIndex(first_prim_index.0 + i);

                        if let Some(..) = self.prim_store.cpu_bounding_rects[prim_index.0] {

nit: could just check for is_some() instead of a pattern match

---

Comments from Reviewable

[kvark]

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

---

Comments from Reviewable

[kvark]

The change looks fantastic! I've got a few concerns to clarify before proceeding.

---

Review status: all files reviewed at latest revision, 10 unresolved discussions.

---

Comments from Reviewable

[glennw]

Thanks for the review! I'll fix these up in the morning and push an updated / rebased version then.

[glennw]

Review status: 17 of 20 files reviewed at latest revision, 10 unresolved discussions.

---

webrender/res/ps_composite.vs.glsl, line 12 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hmm, this seem to diverge from the old code:
Was: dest.task_origin
Now: dest.task_origin - dest.screen_origin + src.screen_origin

The previous blend code worked on tiles only, so didn't need to take into account offsets of the stacking context within a render target.

---

webrender/res/ps_composite.vs.glsl, line 22 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

these st0 and st1 have also diverged from the original. You are essentially Y-flipping the backdrop_task UVs. Is this intentional?

Yep - reading back from the framebuffer as we do now for composites is upside-down from rendering to a FBO and then sampling it as a texture.

---

webrender/src/renderer.rs, line 1062 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

ok, yes, this is a bit (too much) hacky

Yep - I'm going to try and tidy this up today. It's a bit tricky since these values aren't known until quite late in the frame (once the render target allocation has been done). Have you got any suggestions on a cleaner way to do this? I could perhaps make specific structs and implement From on them, for the RenderTaskData, but I'm open to any other ideas here.

---

webrender/src/renderer.rs, line 1115 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why is this needed?

It's not - fixed :)

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

pretty sure you can get away without &mut here and the line below

Fixed

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

I think using readback term here is confusing. Perhaps, change it to backup or even copy?

Changed to CopyFramebuffer - is that clearer?

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

what is the point of early-out here? I don't see any side effects in this inner loop body, so it doesn't seem like anything changes at all if we drop out early. Perhaps, you forgot to assign alpha_batch_index after the loop?

What it's saying is that as soon as it encounters an intersection with something in the batch it's looking at, stop considering any of the other batches before that. Instead, we just force creation of a new batch in that case.

---

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

Previously, kvark (Dzmitry Malyshau) wrote…

nit: could just check for is_some() instead of a pattern match

Fixed

---

Comments from Reviewable

[kvark]

Thanks for the fixes!

---

Review status: 17 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

webrender/res/ps_composite.vs.glsl, line 22 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Yep - reading back from the framebuffer as we do now for composites is upside-down from rendering to a FBO and then sampling it as a texture.

why do we need to read it upside-down?

---

webrender/src/renderer.rs, line 1062 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Yep - I'm going to try and tidy this up today. It's a bit tricky since these values aren't known until quite late in the frame (once the render target allocation has been done). Have you got any suggestions on a cleaner way to do this? I could perhaps make specific structs and implement From on them, for the RenderTaskData, but I'm open to any other ideas here.

I'm fine with this as a follow-up. And yes, looks like implementing From for some nice structs is the way to go.

---

Comments from Reviewable

[glennw]

Review status: 17 of 20 files reviewed at latest revision, 4 unresolved discussions.

---

webrender/res/ps_composite.vs.glsl, line 22 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do we need to read it upside-down?

This is just the nature of how the GL coordinate system and glBlitFramebuffer works, as far as I know?

---

webrender/src/renderer.rs, line 1062 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I'm fine with this as a follow-up. And yes, looks like implementing From for some nice structs is the way to go.

Sounds good - I'll file a follow up bug to fix this then.

---

Comments from Reviewable

[kvark]

Reviewed 1 of 1 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

webrender/res/ps_composite.vs.glsl, line 22 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This is just the nature of how the GL coordinate system and glBlitFramebuffer works, as far as I know?

Well, glBlitFramebuffer doesn't flip anything upside down unless requested. And GL coordinate system shouldn't be a problem if we are using it consistently. But I don't want to push for any changes here if it works, we can revisit it later on.

---

Comments from Reviewable

[kvark]

@bors-servo r+

[bors-servo]

📌 Commit a9e3aec has been approved by kvark

[bors-servo]

⌛ Testing commit a9e3aec with merge 038ac99...

[bors-servo]

☀️ Test successful - status-travis