Closed Bug 966679 Opened 6 years ago Closed 6 years ago

Webm video lags with basic compositor

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: evilpie, Assigned: mattwoodrow)

References

Details

Attachments

(7 files)

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.
I also see errors from GfxFormatToCairoFormat, because the format is R8G8B8X8, which is not handled.
Okay I think I got it. In LayerManagerComposite::EndTransaction the computed mInvalidRegion is zero.
Depends on: 882447
Tom, does this fix the issue?
Attachment #8370410 - Flags: review?(nical.bugzilla)
Flags: needinfo?(evilpies)
Assignee: nobody → matt.woodrow
Attachment #8370410 - Flags: review?(nical.bugzilla) → review+
Doesn't work for me. I still have to move the mouse.
Flags: needinfo?(evilpies)
I wish we were more consistent with our naming.

B8G8R8{A|X}8 is the most common format, often called {A}RGB though.
Attachment #8371945 - Flags: review?(nical.bugzilla)
(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.
Attachment #8371945 - Flags: review?(nical.bugzilla) → review+
Did you test with remote tabs as well? This patch fixed it with just OMTC enabled, but without e10s.
Thanks Tom, that is indeed what I was missing. And wow, what an amazing source of bugs :)
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.
Attachment #8373075 - Flags: review?(roc)
BeginTransaction is where we make snapshot the layer tree for comparison, we need all the children of the RefLayer to be available.
Attachment #8373076 - Flags: review?(nical.bugzilla)
And now that the children of the RefLayers are available, LayerProperties needs to know to descend into them.
Attachment #8373077 - Flags: review?(roc)
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.
Attachment #8373078 - Flags: review?(nical.bugzilla)
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.
Attachment #8373079 - Flags: review?(roc)
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?
Attachment #8373075 - Flags: review?(roc) → review-
(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.
Attachment #8373076 - Flags: review?(nical.bugzilla) → review+
Attachment #8373078 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8373075 - Flags: review- → review+
Depends on: 983202
You need to log in before you can comment on or make changes to this bug.