Matt Woodrow (:mattwoodrow) Reporter
	Description • 15 years ago

Using shark to profile while playing full screen GL video (on OS X 10.6) shows 37% of my CPU time being used within PaintThebesLayers.

We should be able to skip most of this.

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 1 • 15 years ago

I thought we needed those so that we could overlay controls?  If the controls aren't visible there shouldn't be any ThebesLayers being rendered though.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 2 • 15 years ago

This seems very wrong since when we're playing fullscreen video we should not be rendering any ThebesLayers at all.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 3 • 15 years ago

(except when controls are being rendered, of course)

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 15 years ago

We're rendering a ThebesLayer for the black window background. So on every video frame we create a ThebesLayer the size of the screen, fill it with black, upload that to GL, and composite that along with the video.

Retained layers will make this mostly a non-issue because we won't have to update the background.

We could do a little better by identifying ThebesLayers that are simply fills with a single color, and specialize them to some kind of SolidColorLayer. That would save a lot of memory in cases like this.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 15 years ago

Adds a ColorLayer that just paints a solid color everywhere in its visible region.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 6 • 15 years ago

Adds a parameter to nsDisplayItem::IsUniform to return the actual color if the display item is a uniform color.

Then when we build the layer tree, if we find a ThebesLayer where every item in the layer is uniform and they all have the same visible region, the ThebesLayer itself must be uniform, so we replace it with a ColorLayer whose color is the colors of the items composed together.

	Bas Schouten (:bas.schouten)
	Comment 7 • 15 years ago

Comment on attachment 443503 [details] [diff] [review]
Part 1: Add ColorLayer

This looks good to me with a couple of comments.

1. We need to PushRenderTargetOffset around ContainerLayerOGL.cpp line 176, in the RenderLayer function. This is needed for non-opaque layers with their visible rect not starting at 0,0

2. We should probably add a comment about looking for cases where we can optimize using glClear. (Or atleast investigate if there's potential performance gains there.

>+ * Contributor(s):
>+ *   Bas Schouten <bschouten@mozilla.org>

3. I don't think it's fair I take the credit for this! :)

>+  gfxRGBA color = mColor;
>+  color.a *= GetOpacity();

4. Our colors are premultiplied, we need to multiply all components.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 15 years ago

Updated to address those comments.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 15 years ago

Oops

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 10 • 15 years ago

Mats, Vlad, could I get review here? I'd really like to get fullscreen video rocking for some upcoming demos :-)

	Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)
	Comment 11 • 15 years ago

Comment on attachment 443524 [details] [diff] [review]
Part 1 v2

Sorry, looks fine -- in the basic layers impl, there might be a small speed boost by checking if the color is fully opaque, and if so setting OPERATOR_SOURCE -- not sure if pixman will do that optimization internally or not.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 15 years ago

Mats, I need review here ASAP please ... this speeds up some fullscreen demos we're going to be doing.

	Mats Palmgren (inactive)
	Comment 13 • 15 years ago

Sure, I'll have a look...

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 14 • 15 years ago

Comment on attachment 443505 [details] [diff] [review]
Part 2: Use ColorLayers when possible

Mats said he doesn't know this code well.

	David Baron :dbaron:
	Comment 15 • 15 years ago

Comment on attachment 443505 [details] [diff] [review]
Part 2: Use ColorLayers when possible

IsAllUniform could perhaps use a local variable and assign to *aColor
right before returning true, rather than continually using *aColor?

NS_ComposeColors has a comment saying it takes non-premultiplied colors 
(which is what we want here).  However, I don't believe that comment.
It seems to me that aBG and the result are premultiplied, and aFG is
non-premultiplied.  I think there's a missing multiplication by aBG's
alpha component in the color blend operations, and a missing divide by
the final alpha.  But you should definitely check my math!

Other than that this looks fine.  I'll go find a quieter place to sit and think about NS_ComposeColors a drop more.

	David Baron :dbaron:
	Comment 16 • 15 years ago

Never mind, NS_ComposeColors is fine.

	David Baron :dbaron:
	Comment 17 • 15 years ago

(explanation in bug 494664 comment 11)

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 18 • 15 years ago

I checked in the first part:
http://hg.mozilla.org/mozilla-central/rev/86bae2aecffd
I landed the second part and backed it out because it caused many reftest failures. E.g.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1273715233.1273717370.20039.gz

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 19 • 15 years ago

The regressions were because we weren't constraining the area filled by the ColorLayer. It would be slightly difficult to actually restrict the area to region set in SetVisibleRegion --- especially on OpenGL. So I've altered the spec for ColorLayers to say that it can fill any area that includes the visible region. Then during layer construction we set the clip rect on the ColorLayer to restrict it to render to the right rectangle.

I also made it copy the clip rect from the ThebesLayer to the new ColorLayer. Not doing that was also a bug.

	David Baron :dbaron:
	Comment 20 • 15 years ago

Comment on attachment 445050 [details] [diff] [review]
Part 2 v2

r=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 21 • 15 years ago

Try server shows some reftests still fail with this patch.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 22 • 15 years ago

-- Additional fix in BasicLayers.cpp to snap cliprect to pixel boundaries if we can
-- Fix nsDisplayBackground::IsUniform to store RGBA(0,0,0,0) to *aColor instead of aColor

This patch passed on the tryserver (finally!)

	David Baron :dbaron:
	Comment 23 • 15 years ago

Comment on attachment 446092 [details] [diff] [review]
Part 2 v3

r=dbaron

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 24 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/eaea9e3806a3