693938 - Remote viewport ThebesBuffer always using CONTENT_ALPHA format

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Oleg Romashin (:romaxa)]

Found that all fennec remote offscreen viewports creating thebesLayer with CONTENT_ALPHA format....

not sure if it is regression, or it is done on purpose, but why do we need ALPHA channel for those thebes layers? with that we are using 2x more memory for each tab remote frame offscreen thebes layer

Comment 1

[Oleg Romashin (:romaxa)]

In ContainerState::PopThebesLayerData
transparentRegion.IsEmpty() - return false, that is why it is not opaque...
not sure if that is related to checkerboard, or bad clipping of remote viewport.
I set it force to true (for big layer size), and it seems to work fine, checkerboard visible, and no other artifacts...

roc do you know what is going on here?

Comment 2

[Oleg Romashin (:romaxa)]

Actually this problem making viewport rotation/updates much slower, because it is forced to be all 32bpp operations. more memory usage on GPU side, and memory bandwidth for composite operations.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't know. FrameLayerBuilder looks at the content of the ThebesLayer to determine whether it's transparent or not. The content will be transparent unless we can find a solid opaque background covering the layer.

Comment 4

[Oleg Romashin (:romaxa)]

hmm in the same place we do call FindOpaqueBackgroundColorFor with 0 index for that layer, and that function just return 0,0,0,0 color, because index is not valid

Comment 5

[Oleg Romashin (:romaxa)]

Attachment: Set Opaque content for ROOT container + make checkerboard opaque

Root Document child list definitely have non-opaque areas, and there is no background which is covering that layer.
Can we just make root scroll frame opaque by default?
Also it make sense to switch checker-board Image layer to Opaque  mode

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The checkerboard image part of your patch is correct.

Also, GetBackgroundImage should be changed to use RGB24 instead of ARGB32 as the format, since the checkerboard image is opaque. That could make compositing it slightly faster.

If there's no background covering the root scrollable layer, why should it be opaque?

Comment 7

[Oleg Romashin (:romaxa)]

Attachment: Resolution problem and rounding of opaque and visible layers area

Adding to not forget conclusions

Comment 8

[Oleg Romashin (:romaxa)]

Attachment: Make background checkerboard opaque

Comment 9

[Oleg Romashin (:romaxa)]

Attachment: Mark PuppetWidget root scroll layer as opaque

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 567020 [details] [diff] [review]
Make background checkerboard opaque

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

The rest is good.

::: layout/ipc/RenderFrameParent.cpp
@@ +442,3 @@
>          }
>          else {
> +          data[y * BOARDSIZE + x] = 0xDDDDDD;

Don't do this, I think you should continue to set the alpha component to 0xFF.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 567021 [details] [diff] [review]
Mark PuppetWidget root scroll layer as opaque

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

::: layout/base/nsDisplayList.cpp
@@ +623,5 @@
> +  if (FrameMetrics::ROOT_SCROLL_ID == id &&
> +      usingDisplayport &&
> +      !(root->GetContentFlags() & Layer::CONTENT_OPAQUE)) {
> +    // See bug 693938, attachment 567017 [details]
> +    NS_WARNING("Root scroll layer hasn't been marked as opaque, let's mark it so");

It's not the root scroll layer ... it's actually the root layer.

I would probably remove the ROOT_SCROLL_ID check and just say that we don't support transparent content with displayports, so if there's a displayport, we force the content to be opaque.

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Make background checkerboard opaque

Set Alpha back

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Mark ScrollPort layer as opaque

Comment 14

[Dão Gottwald [:dao]]

attachment 567394 [details] [diff] [review] fails to apply.

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 567394 [details] [diff] [review]
Make background checkerboard opaque

It is obsolete now after bug 694706 fixed

Comment 16

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/969648d51825

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/969648d51825

Comment 18

[Oleg Romashin (:romaxa)]

This fix reducing memory usage by Fennec scrollable offscreen thebes layer by 2 (10Mb->5Mb for active tab)