Restructure parts of batch.rs, port line shader to brush.

[glennw]

This patch has no functional changes - it is necessary preparation
work for some upcoming changes (handling text/box shadow pictures
as part of the normal batching process). It's also a small improvement
on the existing code structure itself.

---

This change is 

[glennw]

r? @kvark

I've split this re-structuring patch (which doesn't need super careful review, it's just shuffling code around) from the work I am doing on top of it, to hopefully make the review process a bit simpler.

If I get the follow up work finished today, I'll add them as separate commits on top of this :)

[glennw]

There's enough changes in the follow up commits that I should do a gecko try run now, before we merge this. Will kick one off shortly.

[glennw]

The follow up commits now include the work to port the line decoration shader to be a brush type.

Try run looks good so far:
https://hg.mozilla.org/try/rev/5773449cd3bf741e68ca31a01d97a8a7416db76e
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5773449cd3bf741e68ca31a01d97a8a7416db76e

The first commit has the majority of the line diffs, and has no functional changes. Only the last 3 commits need proper review. If it's easier to review as separate PRs, let me know and I can split them up easily.

[kvark]

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

---

webrender/src/batch.rs, line 516 at r3 (raw file):

            // picture types.
            // TODO(gw): Support culling on shadow image types.
            if pic_type != PictureType::Image || metadata.screen_rect.is_some() {

the exclusion of Image type appears to be a functional change. Simply ignoring it doesn't seem correct. Should we instead turn it into an assert! or spew an error! at the beginning of the method?

---

webrender/src/batch.rs, line 773 at r3 (raw file):

                let is_shadow = pic_type == PictureType::TextShadow;

                let font_transform = if is_shadow {

is this another functional change? I'd expect this transform be controlled by the ContentOrigin enum (Screen/Local).

---

Comments from Reviewable

[glennw]

Review status: 2 of 16 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/src/batch.rs, line 516 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

the exclusion of Image type appears to be a functional change. Simply ignoring it doesn't seem correct. Should we instead turn it into an assert! or spew an error! at the beginning of the method?

Since there was previously a separate code path for non-Image pictures, they didn't execute any culling step at all. So adding this condition here makes that the same as previous (even though it's a bit inelegant for now).

---

webrender/src/batch.rs, line 773 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is this another functional change? I'd expect this transform be controlled by the ContentOrigin enum (Screen/Local).

It probably makes sense for it to be controlled by that in the future (once the content origin is actually configurable), but it should match the existing behaviour. In the previous code, the alternative code path selection was based on the type of the picture, so I've matched that here.

---

Comments from Reviewable

[kvark]

Review status: 2 of 16 files reviewed at latest revision, 1 unresolved discussion.

---

webrender/src/batch.rs, line 773 at r3 (raw file):

Previously, glennw (Glenn Watson) wrote…

It probably makes sense for it to be controlled by that in the future (once the content origin is actually configurable), but it should match the existing behaviour. In the previous code, the alternative code path selection was based on the type of the picture, so I've matched that here.

Understood. Perhaps, adding a comment here would help our future efforts ;)

---

Comments from Reviewable

[kvark]

Reviewed 14 of 14 files at r4.
Review status: 15 of 16 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/res/prim_shared.glsl, line 246 at r4 (raw file):

    RenderTaskCommonData common_data;
    vec2 content_origin;
    float pic_kind_and_raster_mode;

I think it would be more scalable to have uint flags here, extracted from the input. The current sign + positive integer scheme is a bit too specific.

---

Comments from Reviewable

[kvark]

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

---

webrender/src/renderer.rs, line 1615 at r3 (raw file):

    ps_angle_gradient: PrimitiveShader,
    ps_radial_gradient: PrimitiveShader,
    ps_line: PrimitiveShader,

nice :) one up, two down

---

Comments from Reviewable

[glennw]

Review status: 15 of 16 files reviewed at latest revision, 3 unresolved discussions.

---

webrender/res/prim_shared.glsl, line 246 at r4 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think it would be more scalable to have uint flags here, extracted from the input. The current sign + positive integer scheme is a bit too specific.

The render task data is a float texture - so do you mean transmute a u32 in that data to an f32, and then use a shader instruction to reinterpret cast from f32 -> u32, or is there an easier way you can think of to do this?

---

webrender/src/batch.rs, line 773 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Understood. Perhaps, adding a comment here would help our future efforts ;)

Done.

---

Comments from Reviewable

[glennw]

@kvark Thanks for the review! Added a comment to the code, and a question on what you meant by the uint flags.

[kvark]

Review status: 15 of 16 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/res/prim_shared.glsl, line 246 at r4 (raw file):

Previously, glennw (Glenn Watson) wrote…

The render task data is a float texture - so do you mean transmute a u32 in that data to an f32, and then use a shader instruction to reinterpret cast from f32 -> u32, or is there an easier way you can think of to do this?

Even if you just static-cast int to float and back (without re-interpretation), there are 23 mantissa bits available - should be enough for now :)

i.e. Rust side does: flags as f32, shader side does int(flags)

---

Comments from Reviewable

[kvark]

@glennw please feel free to merge with r=me if you don't feel like playing with flags - it's a minor concern.

[glennw]

@kvark Thanks - I have left the flags as is for now to avoid a new try run, but I added a TODO comment in the code saying we should switch this to uint flags in the future.

@bors-servo r=kvark

[bors-servo]

📌 Commit 6035650 has been approved by kvark

[bors-servo]

⌛ Testing commit 6035650 with merge 6f5fb8e...

[bors-servo]

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing 6f5fb8e to master...