Youtube settings widget is janky when playing fullscreen video

RESOLVED FIXED in Firefox 55

Status

()

Core
Graphics
RESOLVED FIXED
26 days ago
6 days ago

People

(Reporter: dmajor, Assigned: mchang)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [gfx-noted][qf:p1])

Attachments

(2 attachments)

(Reporter)

Description

26 days ago
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.
Bas, is working on some patches that should improve the situation here.
Whiteboard: [qf] → [qf:p1]
I'd like the priority re-triaged, in view of the target hardware, given that this is a 4k setup.
Whiteboard: [qf:p1] → [gfx-noted][qf]
Whiteboard: [gfx-noted][qf] → [gfx-noted][qf:p1]
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?
Flags: needinfo?(mchang)
(Assignee)

Comment 4

10 days ago
(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.
Assignee: nobody → mchang
Flags: needinfo?(mchang)
(Reporter)

Comment 5

10 days ago
Created attachment 8858361 [details]
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?
(Assignee)

Comment 6

10 days ago
Created attachment 8858427 [details] [diff] [review]
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
Attachment #8858427 - Flags: review?(bas)
(Assignee)

Comment 7

10 days ago
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 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.
Attachment #8858427 - Flags: review?(bas) → review+
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.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 10

7 days ago
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.
Flags: needinfo?(dmajor)

Comment 11

7 days ago
Pushed by mchang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b759f18c941
Reuse D2D Source Surfaces with Draw Target Dual. r=bas
(Assignee)

Comment 12

7 days ago
(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?
(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.
Flags: needinfo?(matt.woodrow)
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

6 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b759f18c941
Status: NEW → RESOLVED
Last Resolved: 6 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 16

6 days ago
(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
(Reporter)

Updated

6 days ago
Flags: needinfo?(dmajor)
See Also: → bug 1357571
You need to log in before you can comment on or make changes to this bug.