Closed Bug 900592 Opened 6 years ago Closed 6 years ago

Flash video playback not fitting the designated video box

Categories

(Firefox for Android :: General, defect)

25 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox22 --- unaffected
firefox23 --- unaffected
firefox24 --- unaffected
firefox25 --- wontfix
firefox26 - wontfix
firefox27 --- wontfix
firefox28 --- affected
firefox29 --- affected
fennec + ---

People

(Reporter: krudnitski, Assigned: snorp)

References

()

Details

(Keywords: flashplayer, regression, reproducible)

Attachments

(4 files, 2 obsolete files)

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.
I see this on my Samsung Galaxy SIV (Android 4.3); Flash specific only. Confirmed non-issue on  our native HTML5 player.
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
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
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)
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
tracking-fennec: ? → 25+
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?
Flags: needinfo?(matt.woodrow)
(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
Flags: needinfo?(snorp)
Flags: needinfo?(aaron.train)
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.
Flags: needinfo?(snorp)
Flags: needinfo?(dkeeler)
(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.
Flags: needinfo?(dkeeler)
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.
Keywords: qawanted
Over to Kevin who can help with bi-secting
Flags: needinfo?(aaron.train)
Flags: needinfo?(kbrosnan)
tracking-fennec: 25+ → +
Flags: needinfo?(kbrosnan)
The first bad revision is:
changeset:   137443:efa23d6bd3fa
user:        Kartikaya Gupta <kgupta@mozilla.com>
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?
Blocks: 803207
Flags: needinfo?(bugmail.mozilla)
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.
Flags: needinfo?(bugmail.mozilla)
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?
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?
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
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.
Setting flags and dependencies based on my understanding of what happened.
I can reproduce this on all channels.
This seems to fix it
Attachment #8333828 - Flags: review?(bugmail.mozilla)
(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 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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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).
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 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.
Attachment #8333828 - Flags: review?(bugmail.mozilla) → review+
Attached image Screenshot of WIP
And this is what it ended up looking like.
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 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 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
Attachment #8333828 - Flags: review+ → review-
Summary: video playback not fitting the designated video box → Flash video playback not fitting the designated video box
Status: NEW → ASSIGNED
Assignee: matt.woodrow → bugmail.mozilla
Attachment #8333828 - Attachment is obsolete: true
Attachment #8339331 - Flags: review?(tnikkel) → review+
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.
Also see this on the Galaxy Note III (Android 4.3)
Assignee: bugmail.mozilla → snorp
Depends on the GetCumulativeResolution() patch from bug 947523
Attachment #8339331 - Attachment is obsolete: true
Attachment #8358815 - Flags: review?(tnikkel)
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 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.
Attachment #8358815 - Flags: review?(tnikkel)
Yeah, we should separate those. This bug is really about the video problem anyway. I filed 959223 for the resolution issue.
https://hg.mozilla.org/mozilla-central/rev/ef7bd965a6c6
https://hg.mozilla.org/mozilla-central/rev/afb247393486
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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
Sorry, I had the wrong bug number in my commit message. It's bug 959223.
Flags: needinfo?(snorp)
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)
Please open a new bug Paul.
Blocks: 985188
You need to log in before you can comment on or make changes to this bug.