Correctly handle blend primitives, where the contents are clipped.

[glennw]

In some situations, the items within a filter stacking context will
have clips applied. In this case, we minimize the allocated size
of the intermediate surface, to avoid drawing redundant information
that won't be seen.

However, if the stacking context itself does not have the same clip
applied, then we need to ensure that when we draw the blend primitive,
we don't sample outside the smaller region of the intermediate
surface that was rendered.

Ideally, the stacking context should have a clip applied to it so
that we don't waste time drawing transparent pixels. Adding a clip
to the stacking context here would provide an optimization win in
many cases. However, even without one present, we should still
ensure correct rendering, thus this fix is required.

Also fix a case where brush primitives with segments were not setting
the correct bounding rect for a render task. This wasn't causing a
problem here, but may have caused other issues.

---

This change is 

[glennw]

This fixes at least one of the bugs listed in #2438. I think it will probably fix most or all of them, but I haven't confirmed that.

Pending try run to see if this breaks any existing reftests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6987fdf2f5cad510d74179bb7196829fec1e00e

r? @kvark

[glennw]

@staktrace Does the above bit seem reasonable? (specifically, does it seem reasonable from the Gecko side to assign the clip-chain ancestor to the stacking context as well as the contents of the stacking context?). This won't be necessary for correctness, but would be a performance optimization in some cases.

[glennw]

Try looks good, I think.

There are a couple of failures - one is a WR backend panic (Vector image error Unknown error) - which I assume is unrelated to this change.

[glennw]

The other failure looks like an unrelated intermittent.

[kvark]

Great stuff! But one (non-minor) concern

---

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

webrender/res/brush_blend.glsl, line 152 at r1 (raw file):

    // Fail-safe to ensure that we don't sample outside the rendered
    // portion of a blend source.
    color.a *= point_inside_rect(vUv.xy, vUvBounds.xy, vUvBounds.zw);

I don't think this is correct. The vUv.xy is allowed to be outside vUvBounds, hence why we need those bounds :)

---

Comments from Reviewable

[kvark]

@bors-servo r+

[bors-servo]

📌 Commit 69814e7 has been approved by kvark

[glennw]

@kvark Thanks. Updated the naming, as discussed in IRC :)

[bors-servo]

⌛ Testing commit 69814e7 with merge 2beaccc...

[staktrace]

@staktrace Does the above bit seem reasonable? (specifically, does it seem reasonable from the Gecko side to assign the clip-chain ancestor to the stacking context as well as the contents of the stacking context?). This won't be necessary for correctness, but would be a performance optimization in some cases.

Just to be clear - you're asking for a change on the gecko side here, for better performance in WR, correct? Assuming that's correct - I think what you're proposing might be a little tricky to do, because we process the nsDisplayFilter item that generates the stacking context before we ever recurse into the contents, which is where the clips are (on the child items of the nsDisplayFilter). So if I understand correctly, we'd have to walk through the child items in nsDisplayFilter::CreateWebRenderCommands (just before we create the stacking context), and check to see if all the items have the same clip, and if so, apply it to the stacking context as well.

Rather than doing it this way, it might be easier to do it at a higher level, when we first create the nsDisplayFilter item. I'm not sure of the details of how the clips are assigned to that item but it might be easier at that point to ensure the item has the same clip chain as the items to which the filter applies. Either way, it might be worth filing a bug about it in bugzilla and after the Final Clipping Rewrite(TM) lands we can revisit this and see how hard it would be.

[glennw]

@staktrace That's correct, but having thought about it a bit more, I think we can do this optimization entirely within WR itself. I'll add an issue for it.

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: kvark
Pushing 2beaccc to master...

[mstange]

specifically, does it seem reasonable from the Gecko side to assign the clip-chain ancestor to the stacking context as well as the contents of the stacking context?

Note that doing this would only be correct for filters which commute with clips, i.e. where it doesn't matter visually whether the clip is inside or outside the filter. This is only the case for filters where each pixel is processed independently, e.g. for color filters. It is not the case for filters which affect geometry or which sample from multiple pixels, e.g. blur and drop-shadow.

However, Gecko could still apply a clip for the maximum post-filter bounds of the filter, to the filter stacking context. However, the post-filter bounds can change if items within the filter are scrolled or transformed asynchronously, so the clip would have to account for those asynchronous changes as well. It seems better to compute the bounds per frame instead of per SetDisplayList because if you do it perf frame, you're at a point where you have all the necessary information.

[glennw]

@mstange Yup, agreed.