Twitch.tv has repaints on full plugin video area, if e10s is disabled

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: mattwoodrow)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(e10s-, firefox46 fixed)

Details

(URL)

Attachments

(6 attachments, 5 obsolete attachments)

420 bytes, text/html
Details
5.47 KB, application/gzip
Details
228.42 KB, video/ogg
Details
791 bytes, text/html
Details
393.68 KB, video/ogg
Details
8.14 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
(Spinning this off of bug 1231946 comment 3)

STR (which don't work reliably unfortuantely):
 1. Enable paint flashing in about:config ( nglayout.debug.paint_flashing )
 2. Visit http://www.twitch.tv/summonersinnlive/v/18635199
 3. Wait through the ads (~15 seconds I think)

ACTUAL RESULTS:
The video area is continuously invalidated, as fast as possible. (it flashes different colors rapidly over and over)

EXPECTED RESULTS:
Video should be rendered to a layer, which we should be able to get onscreen without triggering invalidation/paint-flashing.


I hit this bug in Firefox Nightly & Release earlier today, as noted in bug 1231946 comment 3.  After a bit more testing, the rapid repainting went away, though.  No idea what caused it. (Maybe some ad that happened to be present & was causing bad interactions? Or maybe some A/B testing?)

So sadly, this isn't reliably reproducible, but I'm filing a bug anyway because it's bad.
(Reporter)

Comment 1

3 years ago
Update: turns out this bug *is* reliably reproducible (with this twitch video), but it only happens with *e10s disabled*. (I was mixed up when I filed it, & didn't realize that the profile I was initially using was an e10s-disabled profile.)

I think the ACTUAL RESULTS are still unexpected; at least, another flash video demo at http://wwwns.akamai.com/hdnetwork/demo/flash/default.html doesn't reproduce this bug.
Summary: Twitch.tv sometimes has continuous repaints on video area → Twitch.tv sometimes continuous repaints on video area, if e10s is disabled
Comment hidden (typo)
(Reporter)

Comment 3

3 years ago
(I'm using 64-bit Linux Nightly 46.0a1 (2016-01-06), with a fresh profile that has e10s disabled.)
Comment hidden (obsolete)
Out of curiosity, does the Flash-based player display the same problem? The embedded player at https://gamesdonequick.com (currently ongoing marathon stream) still has the Flash controls - on twitch.tv itself I always get the HTML5 controls (though the underlying video still uses Flash).
(Reporter)

Comment 6

3 years ago
The gamesdonequick.com version of the video does not display this problem.

Viewing these pages (showing the same livestream) in two different tabs...
  https://gamesdonequick.com/  <--- No flashing over video area
  http://www.twitch.tv/        <--- Continuous flashing over video area
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
(Reporter)

Comment 10

3 years ago
Created attachment 8704869 [details]
"ClearExternalNoVol.swf" flash demo file (for use in testcase)

(Here's a demo .swf file that I found at https://www.adobe.com/support/documentation/en/flash/fl8/samples.html which I'm using in my testcase)
(Reporter)

Comment 11

3 years ago
Created attachment 8704873 [details]
testcase 1's HTML file, for easy inspection (doesn't actually show the bug in bugzilla-hosted form; use other tarball attachment)
(Reporter)

Comment 12

3 years ago
Created attachment 8704875 [details]
testcase 1 as tarball, for viewing locally (view included .html file)

Drat, the testcase doesn't work correctly (the swf doesn't load, or something) in its Bugzilla-hosted form.

Here's a tarball for local viewing, at least. If you extract this and view the included .html file, in a profile with e10s disabled and paint flashing enabled, you should see the bug.
Attachment #8704869 - Attachment is obsolete: true
(Reporter)

Updated

3 years ago
Attachment #8704873 - Attachment description: testcase 1 → testcase 1 HTML, for reference (doesn't show the bug in bugzilla-hosted form; use other tarball attachment)
(Reporter)

Comment 13

3 years ago
So the key ingredients that trigger the bug are:
 (1) text with a transparent background
...overlapping...
 (2) a flash plugin with <param value="transparent" name="wmode">

...and e10s has to be disabled, of course.

[also: I marked some of my old comments as obsolete, so that interested parties can get to this ^^ key point more quickly.]
(Reporter)

Updated

3 years ago
Attachment #8704788 - Attachment is obsolete: true
(Reporter)

Comment 14

3 years ago
AND, importantly:
 * The bug happens even if the overlapping text has opacity:0.
(That's why this happens at twitch, even when the HTML controls are hidden with "opacity:0".)

 * The bug continues to happen if the overlapping text is dynamically removed. It seems like even a brief text/plugin overlap will "taint" the plugin, such that each plugin-rerender from that point on will trigger an invalidation.
(That's why my initial investigation with devtools -- deleting huge chunks of the page -- didn't make the problem go away.)
(Reporter)

Updated

3 years ago
Attachment #8704873 - Attachment description: testcase 1 HTML, for reference (doesn't show the bug in bugzilla-hosted form; use other tarball attachment) → testcase 1's HTML file, for easy inspection (doesn't actually show the bug in bugzilla-hosted form; use other tarball attachment)
(Reporter)

Comment 15

3 years ago
Created attachment 8704881 [details]
screencast of bug reproducing, with local copy of testcase 1
(Reporter)

Comment 16

3 years ago
roc, based on comment 13 and comment 14, do you have any hunches about where the bug might lie?

It makes some sense to me that we'd take an aggressive-invalidation path when we have text overlapping a transparent plugin, but it seems definitely-broken that we get *stuck* on that aggressive-invalidation codepath even after the text is dynamically removed. (and also broken that we'd enter this path with opacity:0 text.)

I don't really know anything about plugin layers, so while I'm open to digging further, I'm not 100% sure where to look. I see that you're in the 'hg blame' for pieces of nsPluginFrame.cpp that smell like they might be related (e.g. "class PluginBackgroundSink : public ReadbackSink") so I'm hoping you might have some clues here... In particular, do you know where we check for text overlapping the transparent plugin and enter the aggressive-invalidation mode noted above?
Flags: needinfo?(roc)
Summary: Twitch.tv sometimes continuous repaints on video area, if e10s is disabled → Twitch.tv has repaints on full plugin video area, if e10s is disabled
(Reporter)

Updated

3 years ago
Attachment #8704875 - Attachment description: testcase 1, for viewing locally (view included .html file) → testcase 1 as tarball, for viewing locally (view included .html file)
(Reporter)

Comment 17

3 years ago
Created attachment 8704992 [details]
testcase 2 (view locally; requires swf file from test 1)

Here's a testcase that demonstrates the "once there's been overlap, plugin drawing is tainted" aspect of this bug. This testcase requires the .swf file included in my earlier tarball (and needs to be viewed locally, since I couldn't get a bugzilla-hosted swf to work).
(Reporter)

Comment 18

3 years ago
Created attachment 8704993 [details]
screencast of bug on testcase 2
This is probably the layer flattening performed by FrameLayerBuilder::BuildContainerLayerFor when it sets flattenToSingleLayer = true. Can you confirm we're hitting that?
Flags: needinfo?(roc)

Updated

3 years ago
tracking-e10s: --- → -
(Reporter)

Comment 20

3 years ago
Yeah, looks like we hit that during the first paint where there's overlap, and then we hit it again on every paint of the testcase thereafter (regardless of whether there's still overlap).
(Reporter)

Comment 21

3 years ago
(confirmed using GDB)
(Reporter)

Updated

3 years ago
Depends on: 1210899
(Assignee)

Comment 22

3 years ago
Created attachment 8706734 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way

Does this fix it for you Daniel?

It does for me on OSX with some prefs changes at least.
Assignee: nobody → matt.woodrow
Attachment #8706734 - Flags: feedback?(dholbert)
(Reporter)

Comment 23

3 years ago
Comment on attachment 8706734 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way

This patch caused a segfault when I loaded my attached "testcase 2" (didn't try any other testcases yet).

>+++ b/layout/base/FrameLayerBuilder.cpp
>@@ -5013,19 +5016,30 @@ ContainerState::Finish(uint32_t* aTextCo

>+
>+        for (uint32_t j = i - 1; j > 0; j--) {
>+          if (mNewChildLayers[j].mVisibleRegion.Intersects(mNewChildLayers[i].mVisibleRegion.GetBounds())) {

We crash because (in my case) i is 0, which means the "uint32_t j = i - 1" expression puts j at MAX_UINT32, and then mNewChildLayers[MAX_UINT32] crashes.

I added a hackaround for this crash locally, by adding an "i != 0" check to the "if" guard that wraps this clause. (Haven't looked at the code enough to be 100% sure this is correct, but it seems reasonable.)

With that change, the patch does indeed seem to fix things for me!
 * No more continuous paint flashing on my attached testcase 1 & testcase 2.
 * No more continuous paint flashing on video area at twitch.tv front page
 * No more continuous paint flashing at http://www.twitch.tv/summonersinnlive/v/18635199 (one caveat - I do see the "current time" box and the video-scrubber "thumb" being repainted every ~second, despite having opacity:0, but that's covered in bug 1237102 I think.)
Attachment #8706734 - Flags: feedback?(dholbert) → feedback+
(Assignee)

Comment 24

3 years ago
Created attachment 8707240 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way
Attachment #8706734 - Attachment is obsolete: true
Attachment #8707240 - Flags: review?(roc)
(Assignee)

Comment 25

3 years ago
(In reply to Daniel Holbert [:dholbert] from comment #23)
> Comment on attachment 8706734 [details] [diff] [review]
> Don't flatten layers if there is a plugin in the way
> 
> This patch caused a segfault when I loaded my attached "testcase 2" (didn't
> try any other testcases yet).
> 
> >+++ b/layout/base/FrameLayerBuilder.cpp
> >@@ -5013,19 +5016,30 @@ ContainerState::Finish(uint32_t* aTextCo
> 
> >+
> >+        for (uint32_t j = i - 1; j > 0; j--) {
> >+          if (mNewChildLayers[j].mVisibleRegion.Intersects(mNewChildLayers[i].mVisibleRegion.GetBounds())) {
> 
> We crash because (in my case) i is 0, which means the "uint32_t j = i - 1"
> expression puts j at MAX_UINT32, and then mNewChildLayers[MAX_UINT32]
> crashes.
> 
> I added a hackaround for this crash locally, by adding an "i != 0" check to
> the "if" guard that wraps this clause. (Haven't looked at the code enough to
> be 100% sure this is correct, but it seems reasonable.)
> 
> With that change, the patch does indeed seem to fix things for me!
>  * No more continuous paint flashing on my attached testcase 1 & testcase 2.
>  * No more continuous paint flashing on video area at twitch.tv front page
>  * No more continuous paint flashing at
> http://www.twitch.tv/summonersinnlive/v/18635199 (one caveat - I do see the
> "current time" box and the video-scrubber "thumb" being repainted every
> ~second, despite having opacity:0, but that's covered in bug 1237102 I
> think.)

Awesome, thanks for checking that!
(Assignee)

Comment 26

3 years ago
Created attachment 8707241 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way v2

Using signed is better, since we actually want to include 0.
Attachment #8707240 - Attachment is obsolete: true
Attachment #8707240 - Flags: review?(roc)
Attachment #8707241 - Flags: review?(roc)
(Assignee)

Comment 27

3 years ago
Created attachment 8707242 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way v3

Forgot to qref, sorry.
Attachment #8707241 - Attachment is obsolete: true
Attachment #8707241 - Flags: review?(roc)
Attachment #8707242 - Flags: review?(roc)
Comment on attachment 8707242 [details] [diff] [review]
Don't flatten layers if there is a plugin in the way v3

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

::: layout/base/FrameLayerBuilder.cpp
@@ +5407,3 @@
>      pixBounds = state.ScaleToOutsidePixels(bounds, false);
>      appUnitsPerDevPixel = state.GetAppUnitsPerDevPixel();
> +    state.Finish(&flags, pixBounds, aChildren, mayFlatten? &hasComponentAlphaChildren : nullptr);

Space before ?
Attachment #8707242 - Flags: review?(roc) → review+

Comment 30

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39ef70b79aed
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
I can confirm that there is no unnecessary paint flashing on twitch with a Nightlyd containing this fix. Unfortunately it doesn't really help performance on my laptop - Flash video performance continues to be abysmal. I think that's a problem with Flash though, perhaps tied to switching to Windows 10. HTML5 video appears to perform much better.

Would this be safe to uplift? I was hoping for more, but it seems like it would still be nice to have for power consumption. I'm particularly thinking of 45 since that branch will be used for the next ESR.
You need to log in before you can comment on or make changes to this bug.