966679 - Webm video lags with basic compositor

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Tom S [:evilpie]]

Just playing http://video.webmfiles.org/big-buck-bunny_trailer.webm in a single tab lags. I also tested this without e10s and just OMTC. With OpenGL enabled it's smooth.

I tried profiling this, but perf is kind of limited, but there seem to be various locking functions and "copy_user_enhanced_fast_string" at the top, which would indicate some overhead here.

Comment 1

[Tom S [:evilpie]]

I also see errors from GfxFormatToCairoFormat, because the format is R8G8B8X8, which is not handled.

Comment 2

[Tom S [:evilpie]]

Okay I think I got it. In LayerManagerComposite::EndTransaction the computed mInvalidRegion is zero.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Manually invalidate the layer tree for async video

Tom, does this fix the issue?

Comment 4

[Tom S [:evilpie]]

Doesn't work for me. I still have to move the mouse.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix texture format

I wish we were more consistent with our naming.

B8G8R8{A|X}8 is the most common format, often called {A}RGB though.

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Tom Schuster [:evilpie] from comment #4)
> Doesn't work for me. I still have to move the mouse.

Interesting. I tested this locally, and the patch works for me.

Comment 7

[Tom S [:evilpie]]

Did you test with remote tabs as well? This patch fixed it with just OMTC enabled, but without e10s.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Thanks Tom, that is indeed what I was missing. And wow, what an amazing source of bugs :)

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Make nglayout.debug.widget_update_flashing work with BasicCompositor

This should help diagnose performance problems with BasicCompositor. If it's flashing more than the areas of the screen that are changing, then we're probably wasting CPU time compositing unchanged pixels.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Resolve ref layers before BeginTransaction

BeginTransaction is where we make snapshot the layer tree for comparison, we need all the children of the RefLayer to be available.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Make LayerProperties aware of RefLayers

And now that the children of the RefLayers are available, LayerProperties needs to know to descend into them.

Comment 12

[Matt Woodrow (:mattwoodrow)]

Attachment: Resolve ref layers during IPDL transactions

Again, we need the full layer tree available since this is making modifications to the layer tree which we need to detect.

We can't leave it resolved for the entire time though, since we can end up removing the root layer from the layer manager, and the un-resolve process fails.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Call Mutated if a layer has an invalid region

This is a bit gross, better ideas appreciated.

mInvalidRegion is a property that we share across IPDL, but we never call Mutated. Unfortunately we change this value from outside of a layer transaction (from ImageLib calling into nsIFrame::InvalidateLayer), so we can't call Mutated() directly.

Comment 14

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

Comment on attachment 8373075 [details] [diff] [review]
Make nglayout.debug.widget_update_flashing work with BasicCompositor

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +608,5 @@
> +    float r = float(rand()) / RAND_MAX;
> +    float g = float(rand()) / RAND_MAX;
> +    float b = float(rand()) / RAND_MAX;
> +    mRenderTarget->mDrawTarget->FillRect(ToRect(mInvalidRegion.GetBounds()),
> +                                         ColorPattern(Color(r, g, b, 0.2)));

Seems to me it would be easy, and somewhat better, to just iterate through mInvalidRegion's rects so we get accurate results?

Comment 15

[Matt Woodrow (:mattwoodrow)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Comment on attachment 8373075 [details] [diff] [review]
> Make nglayout.debug.widget_update_flashing work with BasicCompositor
> 
> Review of attachment 8373075 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicCompositor.cpp
> @@ +608,5 @@
> > +    float r = float(rand()) / RAND_MAX;
> > +    float g = float(rand()) / RAND_MAX;
> > +    float b = float(rand()) / RAND_MAX;
> > +    mRenderTarget->mDrawTarget->FillRect(ToRect(mInvalidRegion.GetBounds()),
> > +                                         ColorPattern(Color(r, g, b, 0.2)));
> 
> Seems to me it would be easy, and somewhat better, to just iterate through
> mInvalidRegion's rects so we get accurate results?

We're currently clipped to mInvalidRegion, so it should be accurate already. I could put this after the PopClip call, and iterate over the rects, but I don't think it's worth it.

Comment 16

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a0f3fde9843
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9b18d294464

Comment 17

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

Comment on attachment 8373075 [details] [diff] [review]
Make nglayout.debug.widget_update_flashing work with BasicCompositor

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +608,5 @@
> +    float r = float(rand()) / RAND_MAX;
> +    float g = float(rand()) / RAND_MAX;
> +    float b = float(rand()) / RAND_MAX;
> +    mRenderTarget->mDrawTarget->FillRect(ToRect(mInvalidRegion.GetBounds()),
> +                                         ColorPattern(Color(r, g, b, 0.2)));

OK, just comment here that we're already clipping to mInvalidRegion so drawing the bounds is OK.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a0f3fde9843
https://hg.mozilla.org/mozilla-central/rev/d9b18d294464

Comment 19

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/89753b0b94ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb75b644eab0
https://hg.mozilla.org/integration/mozilla-inbound/rev/37ad7918adb5
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a581ddf842b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c7d4dddd104

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/89753b0b94ca
https://hg.mozilla.org/mozilla-central/rev/cb75b644eab0
https://hg.mozilla.org/mozilla-central/rev/37ad7918adb5
https://hg.mozilla.org/mozilla-central/rev/8a581ddf842b
https://hg.mozilla.org/mozilla-central/rev/8c7d4dddd104
https://hg.mozilla.org/mozilla-central/rev/9ce7c3a70c13