896250 - Flash content, including YouTube, is clipped to half the expected height and width on Retina display

Status: RESOLVED FIXED

Description

[Chris Peterson [:cpeterson]]

The Flash animation at http://www.informationisbeautiful.net/play/snake-oil-supplements/ is half the expected width and height. This is a regression in Nightly 25 build 2013-07-20.

Here is the pushlog from build 2013-07-19 and 2013-07-20:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b2c82ae98db&tochange=0983f1a0961c

Comment 1

[Chris Peterson [:cpeterson]]

BenWa: could this Flash problem be a regression from bug 873378?

I bisected the mozilla-inbound TBPL builds and I believe the regression is from this range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9fe5a05edd9&tochange=bdf71cb870ac

Comment 2

[Benoit Girard (:BenWa)]

Ohh it's possible and quite interesting. If it is that would mean that we don't support drawing plugin layers without an active layer.

Is this with a retina macbook?

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Benoit Girard (:BenWa) from comment #2)
> Is this with a retina macbook?

Yes, I forgot to mention I'm using a Retina MacBook. If I change the gfx.hidpi.enabled pref from 2 (default) to 0 (disabled), then Flash animations are rendered correctly.

Comment 4

[Benoit Girard (:BenWa)]

I wonder if there's a bug. I'm at a conference right now and don't have a retina. Can you try breaking on this line:
http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp#2090

It has to be getting hit unexpectedly.

Comment 5

[Chris Peterson [:cpeterson]]

Sorry, I don't have a Mac build environment to debug this.

Can you recommend someone else to investigate? Clipped Flash content, including YouTube videos, on Retina displays seems like a pretty critical regression.

Comment 6

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

Matt or Jeff, can you take a look?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

I volunteer Joe as he has a Retina display

Comment 8

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

I don't see the problem on my 10.8.3 retina.

Comment 9

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

And when I say 10.8.3, I of course mean 10.8.4.

Comment 10

[Benoit Girard (:BenWa)]

Try forcing LAYER_NONE for nsPluginFrame. I suspect the bug is inacte plugins drawing in the Thebes layer don't honor the npapi content scale.

Comment 11

[Chris Peterson [:cpeterson]]

Milan: what version of Flash are you testing? I have Flash Player 11.8.800.94, the latest release.

Comment 12

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

Created attachment 779389 [details]
http://www.adobe.com/software/flash/about/ in the nightly

I have 11.7.700.225 and was about to update, but ironically, the http://www.adobe.com/software/flash/about/ demonstrated the problem.  So, yes, I can reproduce.

Comment 13

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

Created attachment 779397 [details] [diff] [review]
Return the max number of layers, as it was meant to be.

BenWa, was there another bug for this?

Chris, this fixes the problem for me.

Comment 14

[Joe Drew (not getting mail)]

We need to file a follow-up bug about the fact that plugins are 1/4-clipped as BenWa mentions in comment 10.

Comment 15

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

BenWa, I'll let you file the bug Joe mentions in Comment 14, I want to make sure it's correctly done :)  We'll keep this bug for the recent regression that exposed that bug.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7

Can we get a test for this regression?

Comment 17

[Gary [:streetwolf]]

FYI this bug appears to fix the problem, on Windows 8 at least, described by this bug report:

https://bugzilla.mozilla.org/show_bug.cgi?id=896198

Comment 18

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7
> 
> Can we get a test for this regression?

Good question.  We don't currently have any Flash tests, but I'll take a look at what could be done, as these would be useful.

Comment 19

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

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/1e1b4d5c51b7
> 
If the backout does fix Android orange, it's probably the bug 873378 that wasn't passing it, but because of the typo fixed here, it was never using 20 as the layer max, but 1 instead.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android reftest-2 failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/7aa3b74214e4

https://tbpl.mozilla.org/php/getParsedLog.php?id=25623153&tree=Mozilla-Inbound

Comment 22

[Dave Hylands [:dhylands]]

master is currently broken for B2G until this patch lands (it was broken when bug 873378 landed).

If this isn't fixed really soon, then I'd like to land a #ifdef MOZ_WIDGET_GONK version of this patch that correct the return value for gonk

Comment 23

[Dave Hylands [:dhylands]]

Created attachment 780679 [details] [diff] [review]
Set layers.max-active to 1 for mobile, since 20 is failing reftest

https://tbpl.mozilla.org/?tree=Try&rev=c7ed91cc82f2

Comment 24

[Gary [:streetwolf]]

(In reply to Dave Hylands [:dhylands] from comment #23)
> Created attachment 780679 [details] [diff] [review]
> Set layers.max-active to 1 for mobile, since 20 is failing reftest
> 
> https://tbpl.mozilla.org/?tree=Try&rev=c7ed91cc82f2

Is this going to fix the choppy scrolling on Windows?

Comment 25

[Dave Hylands [:dhylands]]

(In reply to Gary [:streetwolf] from comment #24)
> (In reply to Dave Hylands [:dhylands] from comment #23)
> > Created attachment 780679 [details] [diff] [review]
> > Set layers.max-active to 1 for mobile, since 20 is failing reftest
> > 
> > https://tbpl.mozilla.org/?tree=Try&rev=c7ed91cc82f2
> 
> Is this going to fix the choppy scrolling on Windows?

I don't know (I don't have a windows machine). With the tree the way it is right now, max-active layers is being set to 1 for everybody, which breaks b2g, and may cause other effects on other platforms - but I'm not sure.

With the "corrected" patch max-active layers would be -1 for all platforms except android, where it would be 20, except that this makes the android reftest fail.

The patch I added makes max-active layers be 1 for android (so reftest passes), and -1 for everybody else, so things go back to the way they were before bug 873378 landed.

Comment 26

[Gary [:streetwolf]]

When this patch first landed on inbound it did fix my scrolling problem on Windows 8 but unfortunately broke Android.  On the Windows side is your new patch doing the same thing as the original patch?  If so then it should fix scrolling on Windows.

Comment 27

[Gary [:streetwolf]]

If you have the patch on a Try server I'll gladly test it out for you on Windows 8.

Comment 28

[Gary [:streetwolf]]

Found your Try server and your patch does indeed fix the scrolling on Windows 8.  Here's hoping it doesn't break Android again because scrolling on Windows without this patch is terrible.

Comment 29

[Dave Hylands [:dhylands]]

Didn't realize that I need to select android reftests manually, so here's my try for that.
https://tbpl.mozilla.org/?tree=Try&rev=88bed1e45718

Comment 30

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

Thanks all for chasing this, both BenWa and I are offline until Friday/Monday, which is why this dragged on.

Comment 31

[Dave Hylands [:dhylands]]

So I'd like to go ahead and land these 2 patches.

Presumably, this bug should be left open until the layers stuff for android gets sorted out.

Comment 32

[Dave Hylands [:dhylands]]

https://hg.mozilla.org/projects/birch/rev/4f35e943f939
https://hg.mozilla.org/projects/birch/rev/da5fa63e90cb

Comment 33

[Dave Hylands [:dhylands]]

I talked to joe on IRC and apparently there is a new bug for flash not working, so this bug can follow the normal wokflow.

Comment 34

[Gary [:streetwolf]]

Will this be making https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/ soon?

Comment 35

[Dave Hylands [:dhylands]]

(In reply to Gary [:streetwolf] from comment #34)
> Will this be making
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> inbound-win32/ soon?

I landed on birch (which is essentially b2g-inbound), which RyanVM merges to m-c, assuming everything is green. Presumably m-i-win32 then syncs up with m-c periodically, so it should pick it up the next merge after this lands on m-c.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/da5fa63e90cb
https://hg.mozilla.org/mozilla-central/rev/4f35e943f939

Comment 37

[Ryan VanderMeulen [:RyanVM]]

Unfortunately, I had to back this out again due to OSX debug asserts that appeared once birch was merged to m-c. Apparently this patch didn't get along well with something that landed on m-c after your last Try push :(
https://hg.mozilla.org/mozilla-central/rev/46d73e889cb4

https://tbpl.mozilla.org/php/getParsedLog.php?id=25752577&tree=Mozilla-Central
https://tbpl.mozilla.org/php/getParsedLog.php?id=25750893&tree=Mozilla-Central

Comment 39

[Dave Hylands [:dhylands]]

I don't get how this can cause the OSX asserts.

Comment 40

[Dave Hylands [:dhylands]]

Pushing to try again from inbound rather than birch.

Comment 41

[Dave Hylands [:dhylands]]

Oops - here's the link
https://tbpl.mozilla.org/?tree=Try&rev=6d37160d9abf

Comment 42

[Gary [:streetwolf]]

New build fixes the scrolling on Windows. Seeing all the problems there are under mobile why can't this fix make it's way to Windows?  My understanding of the update process is woefully lacking.

Comment 43

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7]

This should now be fixed by the backout in bug 873378 comment 37 (with followup in bug 898713).

Comment 44

[Brad Lassey [:blassey] (use needinfo?)]

I can confirm that this is fixed in today's nightly

Comment 45

[Benoit Girard (:BenWa)]

See Comment 10. I think the underlying issue is still there. i.e. That we don't draw inactive plugin layers correctly with Retina scaling. This is 'fixed' because in nearly all the cases plugin layers are active but may not always be.

Comment 46

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

BenWa, we can certainly morph this bug to cover the comment 10, if you think that is better than opening another bug.

Comment 47

[Benoit Girard (:BenWa)]

Cloned to bug 900640.