1351733 - Youtube settings widget is janky when playing fullscreen video

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(Away)]

https://perfht.ml/2obdq0L

Was playing a youtube video at full screen on a 4K monitor and using the mouse to change settings on the video widget.

It looks like the content process was so busy painting that it couldn't respond to my mouse events smoothly.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Bas, is working on some patches that should improve the situation here.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

I'd like the priority re-triaged, in view of the target hardware, given that this is a 4k setup.

Comment 3

[Bas Schouten (:bas.schouten)]

So, the idea here is more or less that 4K monitors are relatively important amongst early adopters and trendsetters, and if there's an easy win we can have here that would be nice. Mason, the rasterize here seems to be uploading a large blur surface, you've done work on blurs/shadows recently I believe, is there anything we can easily done here to avoid the large blur surface?

Comment 4

[Mason Chang [Inactive] [:mchang]]

(In reply to Bas Schouten (:bas.schouten) from comment #3)
> So, the idea here is more or less that 4K monitors are relatively important
> amongst early adopters and trendsetters, and if there's an easy win we can
> have here that would be nice. Mason, the rasterize here seems to be
> uploading a large blur surface, you've done work on blurs/shadows recently I
> believe, is there anything we can easily done here to avoid the large blur
> surface?

This is using giant box shadows and is going through an alternate path than what we use for box shadows, and therefore doesn't use the fast box shadow path. I'll take a look.

Comment 5

[(Away)]

Attachment: aboutsupport.txt

about:support per request

With layers.acceleration.disabled, the responsiveness of the menu is much better (but still far from buttery smooth).

The lag seems to be worse as I increase the play speed, which I guess makes sense -- cpu has more work to do?

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Reuse D2D Source Surfaces with Draw Target Dual

In this specific case, we were re-reading a D2D surface [1] because the source surface was a dual surface type. This would do a readback from SourceSurfaceD2D, then reupload it again. The best.

This patch checks this case and if the SourceSurfaceDual is a D2D surface, we reuse it instead.

Local timings for the readback have wildly different perf numbers, ranging from 0.001ms to 20 ms in the worst case. With this patch, it's a consistent ~0.001ms to 0.002ms for GetImageForSurface. Total time to run the text box shadow goes from ~1-4ms locally to ~0.5 ms.

But dmajor's machine seems to be much worse than mine. I'm on a 5K iMac.

[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp#1875

Comment 7

[Mason Chang [Inactive] [:mchang]]

I think in a follow up, we have to stop creating a draw target and resulting bitmap everytime we want to blur a box shadow. The blur itself isn't too slow here.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 8858427 [details] [diff] [review]
Reuse D2D Source Surfaces with Draw Target Dual

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

::: gfx/2d/DrawTargetD2D1.cpp
@@ +12,4 @@
>  #include "RadialGradientEffectD2D1.h"
> +#include "nsDebug.h"
> +#include "mozilla/TimeStamp.h"
> +#include "nsAppRunner.h"

I don't see why we need these external dependencies, I'd prefer not to have these inside Moz2D code.

Comment 9

[Bas Schouten (:bas.schouten)]

Matt, I'd also like to know why we use DT-dual here and if we need to. YouTube perf is important and DT-dual does not generally make for great perf.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Try looks good - https://treeherder.mozilla.org/#/jobs?repo=try&revision=ed66c6d149be874247e8607e7bc807606d0b7562

Also talking with dmajor, this doesn't completely solve his perf problems. I can't totally reproduce it on my 5K iMac. Once this lands, can you please profile again + file a follow up?

Thanks.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b759f18c941
Reuse D2D Source Surfaces with Draw Target Dual. r=bas

Comment 12

[Mason Chang [Inactive] [:mchang]]

(In reply to Mason Chang [:mchang] from comment #10)
> Try looks good -
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ed66c6d149be874247e8607e7bc807606d0b7562
> 
> Also talking with dmajor, this doesn't completely solve his perf problems. I
> can't totally reproduce it on my 5K iMac. Once this lands, can you please
> profile again + file a follow up?
> 
> Thanks.

Also, is Chrome / Edge slow for you?

Comment 13

[Matt Woodrow (:mattwoodrow)]

(In reply to Bas Schouten (:bas.schouten) from comment #9)
> Matt, I'd also like to know why we use DT-dual here and if we need to.
> YouTube perf is important and DT-dual does not generally make for great perf.

The video itself is an active layer, and then we have no opaque background between the overlay text and the video layer.

We're using DT-dual to keep subpixel-AA for the text and basically prioritizing text quality over performance.

It's possible we could not do this, but we've had really bad push-back in the past for regressing subpixel-AA text rendering, and I'm not overly enthused about complex heuristics to make youtube fast.

The best solution might be to not do this for screens with high enough pixel density, that's probably a gfx team decision though.


I'm also concerned about this patch, it seems like we're returning an image for just one of the dual surfaces? Won't that result in incorrect rendering somewhere, since it'll have a forced black background?

The original idea (pre-moz2d) was that trying to read from a dual surface (apart from during compositing) was an error, since there's not really any sane way to get what you want from them.

Comment 14

[Matt Woodrow (:mattwoodrow)]

There's also quite bad layerization happening, if you enable layer borders you can see the DT-dual/component alpha layers (light greenish-blue) jumping around frequently.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b759f18c941

Comment 16

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #13)
> (In reply to Bas Schouten (:bas.schouten) from comment #9)
> > Matt, I'd also like to know why we use DT-dual here and if we need to.
> > YouTube perf is important and DT-dual does not generally make for great perf.
> 
> The video itself is an active layer, and then we have no opaque background
> between the overlay text and the video layer.
> 
> We're using DT-dual to keep subpixel-AA for the text and basically
> prioritizing text quality over performance.
> 
> It's possible we could not do this, but we've had really bad push-back in
> the past for regressing subpixel-AA text rendering, and I'm not overly
> enthused about complex heuristics to make youtube fast.
> 
> The best solution might be to not do this for screens with high enough pixel
> density, that's probably a gfx team decision though.
> 
> 
> I'm also concerned about this patch, it seems like we're returning an image
> for just one of the dual surfaces? Won't that result in incorrect rendering
> somewhere, since it'll have a forced black background?
> 
> The original idea (pre-moz2d) was that trying to read from a dual surface
> (apart from during compositing) was an error, since there's not really any
> sane way to get what you want from them.

Yeah that's what I was confused about too, with reading only one surface. However, the previous logic would read the first surface anyway[1], so it was somehow working. I'm not really sure what the proper thing to do in a Dual Target situation then.

[1] http://searchfox.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp#1875