Closed Bug 485252 Opened 12 years ago Closed 12 years ago

RenderDocument Background color filling need to be optimized.

Categories

(Core :: Layout, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: romaxa, Assigned: romaxa)

Details

(Keywords: perf)

Attachments

(4 files, 4 obsolete files)

Attached file Oprofile data
Currently in RenderDocument function we are filling background area before building display list. e.t.c..

But for some cases this is a bit expensive:
For example we are playing youtube video with flash plugin in windowless opaque mode, and we have video content size almost equals to screen size (N810)...
In this case we are permanently filling background under flash content and then it is completely over-painted by opaque plugin content...

I think we should collect opaque regions in display list, and subtract them from region which we are filling by background color.

In case of slow DisplayList building, _moz_pixman_fill eating CPU resources even more... see attachment
I've made some investigation about playing youtube video with windowless flash on arm device, and I got next results:
With disabled background filling and cached BuildDisplayList we have ~2x performance improvement 8-9fps -> 16-17.

I think we should try to improve BuildDisplayList and store information about region which is need painted by default background.

Also I have some idea about storing list of frames into MozAfterPaint notification event, and use that list for faster DisplayList building.

Roc what do you think about it?
I'm not sure about MozAfterPaint helping for building display list, but background and display list caching definitely helping a lot for animations on page, also I think it will help for video tag.
Ups, probably it was wrong warning... caching and background filling gives only ~2fps...  I need to investigate it more better.
Without caching and with background:
RenderTime [3960,7140,31260x23100]:  0.032898
RenderTime [3960,7140,31260x23100]:  0.039093
RenderTime [3960,7140,31260x23100]:  0.035339
RenderTime [3960,7140,31260x23100]:  0.031739

With DisplayList caching and without background:
RenderTime [3960,7140,31260x23100]:  0.028137
RenderTime [3960,7140,31260x23100]:  0.034485
RenderTime [3960,7140,31260x23100]:  0.027344
RenderTime [3960,7140,31260x23100]:  0.027924
This patch is collecting transparent area (which is not painted by opaque frames), and if that area empty then background filling not happen.
Also there is another problem that CanvasBackground painted anyway even if it will be over painted by opaque flash (or simple opaque image element.)
Also I have made check in nsDisplayItem::OptimizeVisiblity for objectFrame, and it is removing another _moz_pixman_fill... (no check for objectFrame wmode type yet)

In general case this patch removing 2 useless pixman_fill from flash video playing process...

I think we should implement properly nsDisplayItem::IsOpaque for objectFrames, images, <video> tag elements at least.
And use opaque regions for removing initial background filling.

roc can you advice the best way to fix nsDisplayItem::IsOpaque, and can you check current implementation for aTranspRegion calculation?
Attachment #379068 - Flags: review?(roc)
I don't understand why you're adding aTranspRegion to OptimizeVisibility. Just setting IsOpaque correctly on the right nsDisplayItems (and making sure GetBounds returns the actual drawn area so we're not lying about the GetBounds() area being opaque) should be enough here. We already have the logic to avoid painting stuff that's covered by something else that's opaque. But you're absolutely right that we don't set IsOpaque in a lot of situations where we easily could.

So one thing you should do is make nsObjectFrame::BuildDisplayList return an nsDisplayPlugin instead of nsDisplayGeneric, so you can override its IsOpaque and GetBounds methods. GetBounds will have to return the frame's GetContentRect (translated appropriately). IsOpaque should check the plugin's window/windowless mode, and for windowless mode, check NPPVpluginTransparentBool.

Another thing you should do is instead of having PresShell::RenderDocument paint the background color directly, do what nsLayoutUtils::PaintFrame does and actually insert the background color as its own display item. That way the display list optimizer can optimize it out if it's covered by something opaque. In fact it would be great if PresShell::RenderDocument just called nsLayoutUtils::PaintFrame...

Caching nsDisplayLists is probably not a great idea. It would be better to focus on making things opaque and squeeze whatever performance we can get that way first.
Comment on attachment 379068 [details] [diff] [review]
WIP1, Simple fix for flashplayer rendering

Thanks for answer I'll try to create new patch based on comments.
Attachment #379068 - Attachment is obsolete: true
Attachment #379068 - Flags: review?(roc)
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
Attachment #407604 - Flags: review?(roc)
+      nsRegion region(rect);
+      nsRegion transpRegion(rect);
+      list.ComputeVisibility(&builder, &region, &transpRegion);

You don't need transpRegion. When ComputeVisibility returns, 'region' contains the area of the background that's still visible.
Attachment #407619 - Attachment is obsolete: true
Attachment #407967 - Flags: review?(roc)
Attachment #407619 - Flags: review?(roc)
Attached patch Fixed comment, no other changes (obsolete) — Splinter Review
I think that was bad idea to try fill small regions...
We should fill whole area or don't fill anything.
Attachment #407967 - Attachment is obsolete: true
Attachment #407990 - Flags: review?(roc)
Attachment #407967 - Flags: review?(roc)
Comment on attachment 407604 [details] [diff] [review]
Handle flash transparency property

       // XXX we possibly should call nsPluginInstanceVariable_TransparentBool
       // here to optimize for windowless but opaque plugins

Remove comment.

+      if (mInstanceOwner) {
+         nsresult rv;
+         PRBool transparent = PR_FALSE;
+         nsCOMPtr<nsIPluginInstance> pi;
+         if (NS_SUCCEEDED(rv = mInstanceOwner->GetInstance(*getter_AddRefs(pi)))) {
+           pi->GetValue(nsPluginInstanceVariable_TransparentBool,
+                        (void *)&transparent);
+           return !transparent;
+         }
+      }
       return PR_FALSE;

Fix indent to be 2 spaces.
Attachment #407604 - Flags: review?(roc) → review+
+  PRBool backgroundPainted = PR_FALSE;

Call this didPrepareContext.

Add a comment above PrepareContext explaining what it does.
Attachment #407990 - Attachment is obsolete: true
Attachment #408390 - Flags: review?(roc)
Attachment #407990 - Flags: review?(roc)
Attached patch Updated patchSplinter Review
Fixed in:
http://hg.mozilla.org/mozilla-central/rev/7d7637ea3228
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This is might be helpful for fennec rendering, and especially for plugins rendering.
Requesting 1.9.2
Flags: wanted1.9.2?
Comment on attachment 408691 [details] [diff] [review]
Updated patch

Small performance win on pages that paint very slowly
Attachment #408691 - Flags: approval1.9.2?
Flags: wanted1.9.2? → wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.