1343450 - Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ethan Lin[:ethlin]]

nsDisplayCanvasBackgroundColor should be a easy one and it's used widely.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer

There is an alpha blending problem when turning on the pref. It looks like the painted layer still need the display item to blend correctly. The testcase[1] is an example. But for opaque content, the result should be correct.

[1] layout/reftests/backgrounds/viewport-translucent-color-2.html

Comment 2

[Markus Stange [:mstange]]

This could be caused by https://github.com/servo/webrender/issues/889 , which affects any transparent PaintedLayer. We didn't hit it so much in the past because most of our PaintedLayers contained the canvas background color which made it opaque.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8842720 [details] [diff] [review]
Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer

Review of attachment 8842720 [details] [diff] [review]:
-----------------------------------------------------------------

Is there any real advantage to doing this instead of just using the ColorLayer?

We already have the code to create a ColorLayer (and we probably want to retain it for now) and the WebRenderColorLayer code, so maintaining both (equally valid) paths seems wasteful.

The code looks good though, so happy to r+ if you can convince me of the need.

Comment 4

[Ethan Lin[:ethlin]]

I also think about this. It looks like there are two approaches to do the conversion. One is convert to existing layer(s). The other one is convert to DisplayItemLayer and rewrite the WR commands. In my opinion, for QR we should convert everything to DisplayItemLayer and just leave PaintedLayer, CanvasLayer, and ImageLayer. So probably we can do some enhancements for DisplayItemLayer in the future, like accumulating displayitems in DisplayItemLayer. In this way we should remove WebRenderColorLayer, WebRenderBorderLayer, etc.
It's just my opinion. Mason, should we convert nsDisplayCanvasBackgroundColor to DisplayItemLayer?

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Ethan Lin[:ethlin] from comment #4)
> I also think about this. It looks like there are two approaches to do the
> conversion. One is convert to existing layer(s). The other one is convert to
> DisplayItemLayer and rewrite the WR commands. In my opinion, for QR we
> should convert everything to DisplayItemLayer and just leave PaintedLayer,
> CanvasLayer, and ImageLayer. So probably we can do some enhancements for
> DisplayItemLayer in the future, like accumulating displayitems in
> DisplayItemLayer. In this way we should remove WebRenderColorLayer,
> WebRenderBorderLayer, etc.
> It's just my opinion. Mason, should we convert
> nsDisplayCanvasBackgroundColor to DisplayItemLayer?

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8842720 [details] [diff] [review]
> Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer
> 
> Review of attachment 8842720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Is there any real advantage to doing this instead of just using the
> ColorLayer?
> 
> We already have the code to create a ColorLayer (and we probably want to
> retain it for now) and the WebRenderColorLayer code, so maintaining both
> (equally valid) paths seems wasteful.
> 
> The code looks good though, so happy to r+ if you can convince me of the
> need.

I agree with Ethan here. I think the overriding philosophy should be that we should be able to remove WebRenderColorLayer / WebRenderBorderLayer and can probably narrow it down to PaintedLayer, CanvasLayer, ImageLayer (anything that will fallback on the old path) and anything that generates webrender commands should use DisplayItemLayer. We created DisplayItemLayer specifically because we wanted to retain the frame / display item data / know what type we had and only translate things to WR at the last possible moment. It's better to have as much information as possible until the end rather than throw it away early then try to recover it. I know in this case it doesn't apply, but I'd rather not make exceptions as we'll have to go hunting for exceptions later versus a clean "it always generates WR commands". In this specific case for example, if we directly translated it to a ColorLayer, we'd lose the information that this is a canvas background color (not sure it matters down the line, but I assume so otherwise we wouldn't make a nsDisplayCanvasBackgroundColor item in the first place).

The other thing I'm against is breaking down a display item into two layers (It's not clear to me FLB can even do this right now, which I think is good). If 1 display item broke down into two layers, all of layout would have to deal with two more layers instead of 1, slowing things down.

How about this. Can we rewrite the patch such that we delete WebRenderColorLayer, convert WebRenderColorLayer into a DisplayItemLayer and generate the commands there? Then we can reuse the code to generate WR commands for WebRenderColorLayer for nsDisplayCanvasBackgroundColor.

Comment 6

[Matt Woodrow (:mattwoodrow)]

We have other callers that create ColorLayer, including an internal optimization within FrameLayerBuilder. Converting all those callers to support both ColorLayer & DisplayItemLayer is a lot of duplicated code, more so than just maintaining WebRenderColorLayer imo.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8842720 [details] [diff] [review]
Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer

Review of attachment 8842720 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsCanvasFrame.cpp
@@ +303,5 @@
> +  nsCanvasFrame* frame = static_cast<nsCanvasFrame*>(mFrame);
> +  nsPoint offset = ToReferenceFrame();
> +  nsRect bgClipRect = frame->CanvasArea() + offset;
> +  int32_t appUnitsPerDevPixel = mFrame->PresContext()->AppUnitsPerDevPixel();
> +  if (NS_GET_A(mColor) == 0) {

nit: Don't need this since we won't build a layer if the alpha is 0 already in BuildLayer.

Comment 8

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> We have other callers that create ColorLayer, including an internal
> optimization within FrameLayerBuilder. Converting all those callers to
> support both ColorLayer & DisplayItemLayer is a lot of duplicated code, more
> so than just maintaining WebRenderColorLayer imo.

Doh that's annoying, which means we're going to have WebRenderColorLayer around. That changes the calculus a bit. The only last argument I would have is it might be easier to debug a WebRenderDisplayItem since it directly links which display item it is whereas you're going to have to backtrack a bit to find out that nsDisplayCanvasBackgroundColor generates a color layer.

I think it's conceptually cleaner to always have a WebRenderDisplayItemLayer, but this might be a small enough case where the extra code isn't worth it. But this isn't that much code. I think I could go either way on this case.

Comment 9

[Markus Stange [:mstange]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> We have other callers that create ColorLayer, including an internal
> optimization within FrameLayerBuilder.

On the other hand, once all uniform solid color display items are LAYER_ACTIVE and able to create a WebRenderDisplayItem layer, then we'll never reach that case in FrameLayerBuilder, so WebRenderLayerManager could just return nullptr from CreateColorLayer and nothing would notice. Except... there's one caller of CreateColorLayer in PresShell::Paint when it tries to paint a view that doesn't have a frame. Not sure what to do about that one.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8842720 [details] [diff] [review]
Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer

Review of attachment 8842720 [details] [diff] [review]:
-----------------------------------------------------------------

I'm still not entirely convinced that this is helpful, but it's not much code, so it's fine with me.

Comment 11

[Ethan Lin[:ethlin]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> Comment on attachment 8842720 [details] [diff] [review]
> Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer
> 
> Review of attachment 8842720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm still not entirely convinced that this is helpful, but it's not much
> code, so it's fine with me.

Thanks for the r+. I think this case is the minimum case for DisplayItemLayer. I'll think about how to reduce the duplicated code when we try to convert all WebRenderColorLayer to DisplayItemLayer.

Comment 12

[Jamie Nicol [:jnicol]]

So the conclusion is that we're using DisplayItemLayer? I'm asking for bug 1339575 (nsDisplaySolidColor), because I was going to make it create a ColorLayer, but would like to be consistent with this.

Comment 13

[Ethan Lin[:ethlin]]

(In reply to Jamie Nicol [:jnicol] from comment #12)
> So the conclusion is that we're using DisplayItemLayer? I'm asking for bug
> 1339575 (nsDisplaySolidColor), because I was going to make it create a
> ColorLayer, but would like to be consistent with this.

I think currently we should keep using ColorLayer for other items until everyone is fine with replacing WebRenderColorLayer by DisplayItemLayer.

Comment 14

[Ethan Lin[:ethlin]]

try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa674d4205f14296b12626ae30eb0dc25a499626&selectedJob=82090633

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/93b5f8f13084
Convert nsDisplayCanvasBackgroundColor to WebRenderDisplayItemLayer. r=mattwoodrow

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/0d2742faf779
Keep gfxPrefs sorted alphabetically. r=me

Comment 17

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/93b5f8f13084
https://hg.mozilla.org/mozilla-central/rev/0d2742faf779

Comment 18

[Mason Chang [Inactive] [:mchang]]

When can we enable this by default?

Comment 19

[Ethan Lin[:ethlin]]

(In reply to Mason Chang [:mchang] from comment #18)
> When can we enable this by default?

Currently we almost finish first round display item conversion. I'll start to enable border / color display items by default. I will create a bug for turning on nsDisplayCanvasBackgroundColor.