Closed Bug 1237083 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
e10s - ---
firefox46 --- fixed

People

(Reporter: dholbert, Assigned: mattwoodrow)

References

(
URL
)

Details

Attachments

(6 files, 5 obsolete files)

testcase 1's HTML file, for easy inspection (doesn't actually show the bug in bugzilla-hosted form; use other tarball attachment)
420 bytes, text/html
Details
testcase 1 as tarball, for viewing locally (view included .html file)
5.47 KB, application/gzip
Details
screencast of bug reproducing, with local copy of testcase 1
228.42 KB, video/ogg
Details
testcase 2 (view locally; requires swf file from test 1)
791 bytes, text/html
Details
screencast of bug on testcase 2
393.68 KB, video/ogg
Details
Don't flatten layers if there is a plugin in the way v3
8.14 KB, patch
roc
: review+
Details | Diff | Splinter Review 
(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.
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
(I'm using 64-bit Linux Nightly 46.0a1 (2016-01-06), with a fresh profile that has e10s disabled.)
URL: http://www.twitch.tv/summonersinnlive...
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).
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
(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)
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
Attachment #8704873 - Attachment description: testcase 1 → testcase 1 HTML, for reference (doesn't show the bug in bugzilla-hosted form; use other tarball attachment)
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.]
Attachment #8704788 - Attachment is obsolete: true
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.)
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)
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
Attachment #8704875 - Attachment description: testcase 1, for viewing locally (view included .html file) → testcase 1 as tarball, for viewing locally (view included .html file)
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).
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)
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).
(confirmed using GDB)
Depends on: 1210899
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)
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+

        Attachment #8706734 -
        Attachment is obsolete: true

        Attachment #8707240 -
        Flags: review?(roc)

    
    

    
  
(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!

    
    

    
  


  
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)

    
    

    
  


  
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+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/39ef70b79aed
Status: NEW → RESOLVED
Closed: 8 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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: