The default bug view has changed. See this FAQ.

Remote viewport ThebesBuffer always using CONTENT_ALPHA format

RESOLVED FIXED in mozilla10

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: romaxa, Assigned: romaxa)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

6 years ago
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
(Assignee)

Comment 1

6 years ago
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?
(Assignee)

Comment 2

6 years ago
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.
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.
(Assignee)

Comment 4

6 years ago
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
(Assignee)

Comment 5

6 years ago
Created attachment 566612 [details] [diff] [review]
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
Attachment #566612 - Flags: feedback?(roc)
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?
(Assignee)

Comment 7

6 years ago
Created attachment 567017 [details]
Resolution problem and rounding of opaque and visible layers area

Adding to not forget conclusions
(Assignee)

Comment 8

6 years ago
Created attachment 567020 [details] [diff] [review]
Make background checkerboard opaque
Assignee: nobody → romaxa
Attachment #566612 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #566612 - Flags: feedback?(roc)
Attachment #567020 - Flags: review?(roc)
(Assignee)

Comment 9

6 years ago
Created attachment 567021 [details] [diff] [review]
Mark PuppetWidget root scroll layer as opaque
Attachment #567021 - Flags: review?(roc)
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.
Attachment #567020 - Flags: review?(roc) → review+
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.
(Assignee)

Comment 12

6 years ago
Created attachment 567394 [details] [diff] [review]
Make background checkerboard opaque

Set Alpha back
Attachment #567020 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Created attachment 567396 [details] [diff] [review]
Mark ScrollPort layer as opaque
Attachment #567021 - Attachment is obsolete: true
Attachment #567021 - Flags: review?(roc)
Attachment #567396 - Flags: review?(roc)
Attachment #567396 - Flags: review?(roc) → review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
attachment 567394 [details] [diff] [review] fails to apply.
Keywords: checkin-needed
(Assignee)

Comment 15

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

It is obsolete now after bug 694706 fixed
Attachment #567394 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/969648d51825
Keywords: checkin-needed
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/969648d51825
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
Whiteboard: [MemShrink]
(Assignee)

Comment 18

6 years ago
This fix reducing memory usage by Fennec scrollable offscreen thebes layer by 2 (10Mb->5Mb for active tab)
Blocks: 764524
You need to log in before you can comment on or make changes to this bug.