900592 - Flash video playback not fitting the designated video box

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Karen Rudnitski [:kar]]

Attachment: Screenshot_2013-08-01-13-47-11 video playback.png

Running nightly Fx 25 07-31

On my nexus 4. go to http://www.cbc.ca/player/Shows/Shows/Coronation+Street/ID/2398316050/

Tap to play the video. 

The video playback is very small compared to the size of the video window it should be playing in. Happens in both landscape and portrait.

Comment 1

[Aaron Train [:aaronmt]]

I see this on my Samsung Galaxy SIV (Android 4.3); Flash specific only. Confirmed non-issue on  our native HTML5 player.

Comment 2

[Aaron Train [:aaronmt]]

Standalone video player seems to work out fine; maybe this is layout related.

http://www.cbc.ca/video/swf/UberPlayer.swf?clipId=2398316050&autoPlay=true

Comment 3

[u421692]

The regression window for inbound: 
There was a crash between 1372857907 and 1373949670 due to Bug 890272

GOOD build: 1372857907
CRASHED build:  1372859469
-pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5f652fba751d&tochange=3e1ed95df35a


CRASHED build: 1373949189
BAD build:  1373949670
-pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d054daa3cac1&tochange=c460559cdf97

Comment 4

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

Matt, this smells like a layers bug to me. If so, can you take a look? If not, can you point me in the right direction?

FWIW, you're also in the rather large regression range (http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5f652fba751d&tochange=c460559cdf97)

Comment 5

[Matt Woodrow (:mattwoodrow)]

Can we narrow down the regression range at all?

Bug 867460 seems like the most likely, since it changed around all the shaders that we use.

Snorp, is this the expected failure if we're not applying the texture transform correctly?

Comment 6

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Can we narrow down the regression range at all?
Aaron, can you narrow it down by bisecting with private builds?

> Bug 867460 seems like the most likely, since it changed around all the
> shaders that we use.
> 
> Snorp, is this the expected failure if we're not applying the texture
> transform correctly?
need-info to snorp for this

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

So that *could* possibly be a symptom of not using the texture transform. But usually you get garbage in part of it since you aren't supposed to be sampling from that region. Also it looks like the image is sort of squashed in this? So I'm inclined to think it's something different.

Comment 8

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

Is this related to bug 869368?

Comment 9

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> Is this related to bug 869368?

nexus 4 runs jelly bean, right? My understanding is that bug 869368 is only on gingerbread. Also, it looks like they have different regression ranges, so I'm inclined to guess that they're unrelated. That said, I haven't been able to investigate bug 869368 very far.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Does this affect multiple sites' video playback?  Or is it specific to certain types of layouts?  Since this was tracked for Fennec 25 but didn't get worked on I'm not sure why this would be bumped up to a 26 release blocker bug when it's not getting a lot of dupes or feedback about high user impact in general web browsing.  Please provide more information here about what devices/versions of Android this affects and major sites impacted.

Comment 11

[Aaron Train [:aaronmt]]

Over to Kevin who can help with bi-secting

Comment 12

[Kevin Brosnan [Ex-Mozilla]]

Still working on bisecting this but now have the range to http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=e58c025b75b5&tochange=1ee4f0e1fcd2

Comment 13

[Kevin Brosnan [Ex-Mozilla]]

The first bad revision is:
changeset:   137443:efa23d6bd3fa
user:        Kartikaya Gupta <[e-mail]>
date:        Thu Jul 04 09:02:29 2013 -0400
summary:     Bug 803207 - Kill GetDevicePixelsPerMetaViewportPixel and use widget scaling correctly on Fennec. r=mbrubeck

Kats any thoughts on how to resolve this?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

The size needs to be multiplied by the widget scale for it to get sized correctly on hi-dpi devices. Snorp, do you know where the layout gets the video size from in this case? I don't recall where the JellyBean flash handling code is.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah I noticed this was busted yesterday while trying to figure out 928804. We try to get the resolution here: http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginInstanceOwner.cpp#189

But that always returns 1.0 now. Where is the right place to get the zoom these days?

Comment 16

[Kartikaya Gupta (email:[e-mail])]

As of bug 732971 the resolution is now on the content presshell rather than the root presshell. So the bit that does ->GetRootPresContext() definitely needs to be removed. I'm not sure if mObjectFrame->PresContext() returns a separate pres context for the plugin, or if it returns the content prescontext. If the former you might need replace GetRootPresContext with a GetParent or something; if the latter you just need to take out GetRootPresContext entirely.

However this is a more recent change than bug 803207 which was identified as the culprit for this regression. Are there two different bugs here?

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, removing GetRootPresContext() seems to do the trick. This cbc.ca site claims I don't have Flash, though. Do we have another url that shows this bug?

Comment 18

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> However this is a more recent change than bug 803207 which was identified as
> the culprit for this regression. Are there two different bugs here?

I think I understand what happened. The original bug filed by Karen is likely a dupe of bug 902888, and was fixed on Aug 13. This is outside the regression range in comment 4. A different (but similar-looking) breakage happened recently, with bug 732971, so it appears to be unfixed on tip when in fact it's a slightly different bug. Narrowing the regression range in comment 4 pointed to bug 803207 but I suspect that if were to bisect from tip it would point to bug 732971. The GetRootPresContext change will fix the new bug.

Comment 19

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> I'm not sure if mObjectFrame->PresContext() returns a
> separate pres context for the plugin, or if it returns the content
> prescontext.

There is one prescontext per document. So the object frame will have the same prescontext as all the other frames in the that same document.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

if this is fixed by bug 732971 and those patches are low risk to uplift, please nominate them over there. Not tracking this bug since it's not apparent it's widely reproducible on other sites.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Setting flags and dependencies based on my understanding of what happened.

Comment 22

[Kevin Brosnan [Ex-Mozilla]]

I can reproduce this on all channels.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Get the correct widget resolution for Flash plugin dimensions

This seems to fix it

Comment 24

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kevin Brosnan [:kbrosnan] from comment #22)
> I can reproduce this on all channels.

Indeed, I can too. I also tested the inbound build right before bug 732971 landed and that also had the same problem (along with the initially-upside-down-video problem). So then I was wrong and bug 732971 is unrelated to this. I'll test the patch that takes out the RootPresContext() to see if I can reproduce snorp's results.

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8333828 [details] [diff] [review]
Get the correct widget resolution for Flash plugin dimensions

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

This doesn't fix it for me (neither the size problem nor the initially-upside-down video). As before, zooming in any amount fixes the upside-down problem but not the size.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Comment on attachment 8333828 [details] [diff] [review]
> Get the correct widget resolution for Flash plugin dimensions
> 
> Review of attachment 8333828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This doesn't fix it for me (neither the size problem nor the
> initially-upside-down video). As before, zooming in any amount fixes the
> upside-down problem but not the size.

Yeah it does not fix the upside-down thing for me either, but it seemed to fix the video size issue. Which url are you using to test? The cbc.ca one does not work for me (does not properly detect flash).

Comment 27

[Kartikaya Gupta (email:[e-mail])]

I'm using cbc.ca. Some of the videos don't detect it properly but others do. I just click around on the episode thumbnails until one of them loads.

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8333828 [details] [diff] [review]
Get the correct widget resolution for Flash plugin dimensions

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

This change is correct and so should land, but it doesn't fix the problem this bug is about. I believe the current behaviour is intentional on the part of Flash. I don't know why they're doing it, but the values they are passing in to anp_video_setWindowDimensions place the video where it is, and they also put the toolbar at half-height rather than at the bottom of the plugin area.

I'll attach the patch that I used to try to make the video bigger assuming some mismatched units, and while that made the video bigger it still didn't position it correctly or clip correctly.

Comment 29

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

This is what I tried.

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Attachment: Screenshot of WIP

And this is what it ended up looking like.

Comment 31

[Kartikaya Gupta (email:[e-mail])]

Just to clarify, the smoking gun in my mind is that the video toolbar is the correct width but positioned assuming the video is half-size. The correct width implies Flash knows how big the window area is. If it knows how big the window area is it should know how big to make the video. If it were making the video big, it would position the toolbar lower down (and there would be a bug in our code where the video is not scaled correctly). But it's not positioning the toolbar lower down, so it's not actually making the video bigger. If we try to make the video bigger on our end then it's basically fighting what Flash is trying to do and so ends up looking wonky like in comment 30.

Comment 32

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8333828 [details] [diff] [review]
Get the correct widget resolution for Flash plugin dimensions

Technically, should this be the cumulative resolution instead of the local resolution? So that a plugin in an iframe will pick up the resolution of the parent document.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Ah yes, you're right.

Comment 34

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8333828 [details] [diff] [review]
Get the correct widget resolution for Flash plugin dimensions

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

What :tn said. This needs to calculate the cumulative resolution, similar to the code at [1]. Just multiply the resolutions all the way up the presshell chain.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#698

Comment 35

[Kevin Brosnan [Ex-Mozilla]]

Looks like http://radar.weather.gov/radar.php?rid=LSX&product=N0R&overlay=01101111&loop=yes is also a testcase for this bug.

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Get the correct widget resolution for Flash plugin dimensions

Comment 37

[Timothy Nikkel (:tnikkel)]

Oh, just found that we already have code that does the same walk in nsDisplayList. Would be just dandy if we could just have one copy of this code.

Comment 38

[Aaron Train [:aaronmt]]

Also see this on the Galaxy Note III (Android 4.3)

Comment 39

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix Flash invalidation and sizing issue wrt resolution

Depends on the GetCumulativeResolution() patch from bug 947523

Comment 40

[Kartikaya Gupta (email:[e-mail])]

You should probably copy that patch over to this bug and change the bug number on it. That patch landed on b2g-1.2 only and it might get pretty confusing if we land just that one patch on master as well.

Comment 41

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8358815 [details] [diff] [review]
Fix Flash invalidation and sizing issue wrt resolution

Can we separate the moving of NotifySize fix for bug 957694 from the resolution fix into two patches? I think that will make both these changes easier to understand and reason about later.

Comment 42

[James Willcox (:snorp) ([e-mail]) (he/him)]

Yeah, we should separate those. This bug is really about the video problem anyway. I filed 959223 for the resolution issue.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ef7bd965a6c6
https://hg.mozilla.org/mozilla-central/rev/afb247393486

Comment 44

[Ryan VanderMeulen [:RyanVM]]

Please mark the bug when landing on release branches. Also, some indication of approval in the bug is a good thing.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8f6bdf8db3d
https://hg.mozilla.org/releases/mozilla-beta/rev/85dea29cbde9

Comment 45

[James Willcox (:snorp) ([e-mail]) (he/him)]

Sorry, I had the wrong bug number in my commit message. It's bug 959223.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

Good times.

Comment 47

[Paul Feher]

The initial issue is still reproducible using Nightly 29.0a1 (2014-01-23) on Nexus 4 (Android 4.4.2) and LG Optimus 4X (Android 4.1.2)

Comment 48

[Aaron Train [:aaronmt]]

Please open a new bug Paul.