Last Comment Bug 693938 - Remote viewport ThebesBuffer always using CONTENT_ALPHA format
: Remote viewport ThebesBuffer always using CONTENT_ALPHA format
Status: RESOLVED FIXED
[MemShrink]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: mozilla10
Assigned To: Oleg Romashin (:romaxa)
:
Mentors:
Depends on:
Blocks: 764524
  Show dependency treegraph
 
Reported: 2011-10-12 01:18 PDT by Oleg Romashin (:romaxa)
Modified: 2012-06-13 13:07 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Set Opaque content for ROOT container + make checkerboard opaque (1.84 KB, patch)
2011-10-12 13:05 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Resolution problem and rounding of opaque and visible layers area (3.48 KB, text/plain)
2011-10-13 23:36 PDT, Oleg Romashin (:romaxa)
no flags Details
Make background checkerboard opaque (2.33 KB, patch)
2011-10-13 23:51 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Review
Mark PuppetWidget root scroll layer as opaque (1.32 KB, patch)
2011-10-13 23:52 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Make background checkerboard opaque (1.79 KB, patch)
2011-10-16 23:02 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Mark ScrollPort layer as opaque (1.29 KB, patch)
2011-10-16 23:25 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Review

Description Oleg Romashin (:romaxa) 2011-10-12 01:18:52 PDT
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) 2011-10-12 01:40:34 PDT
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) 2011-10-12 01:42:04 PDT
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) (Exited; email my personal email if necessary) 2011-10-12 02:16:49 PDT
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) 2011-10-12 09:24:36 PDT
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) 2011-10-12 13:05:54 PDT
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
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-13 16:17:41 PDT
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) 2011-10-13 23:36:41 PDT
Created attachment 567017 [details]
Resolution problem and rounding of opaque and visible layers area

Adding to not forget conclusions
Comment 8 Oleg Romashin (:romaxa) 2011-10-13 23:51:36 PDT
Created attachment 567020 [details] [diff] [review]
Make background checkerboard opaque
Comment 9 Oleg Romashin (:romaxa) 2011-10-13 23:52:18 PDT
Created attachment 567021 [details] [diff] [review]
Mark PuppetWidget root scroll layer as opaque
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-16 22:32:15 PDT
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) (Exited; email my personal email if necessary) 2011-10-16 22:35:41 PDT
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) 2011-10-16 23:02:38 PDT
Created attachment 567394 [details] [diff] [review]
Make background checkerboard opaque

Set Alpha back
Comment 13 Oleg Romashin (:romaxa) 2011-10-16 23:25:54 PDT
Created attachment 567396 [details] [diff] [review]
Mark ScrollPort layer as opaque
Comment 14 Dão Gottwald [:dao] 2011-10-22 12:27:23 PDT
attachment 567394 [details] [diff] [review] fails to apply.
Comment 15 Oleg Romashin (:romaxa) 2011-10-22 12:44:23 PDT
Comment on attachment 567394 [details] [diff] [review]
Make background checkerboard opaque

It is obsolete now after bug 694706 fixed
Comment 17 Ed Morley [:emorley] 2011-10-23 09:28:12 PDT
https://hg.mozilla.org/mozilla-central/rev/969648d51825
Comment 18 Oleg Romashin (:romaxa) 2011-10-23 20:11:49 PDT
This fix reducing memory usage by Fennec scrollable offscreen thebes layer by 2 (10Mb->5Mb for active tab)

Note You need to log in before you can comment on or make changes to this bug.