Closed Bug 896250 Opened 9 years ago Closed 9 years ago

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


(Core :: Graphics, defect)

Not set



Tracking Status
firefox24 --- unaffected
firefox25 --- fixed


(Reporter: cpeterson, Assigned: milan)




(Keywords: regression, reproducible)


(3 files)

The Flash animation at 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:
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:
Blocks: 873378
Component: Layout → Graphics
Flags: needinfo?(bgirard)
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?
Flags: needinfo?(bgirard)
(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.
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:

It has to be getting hit unexpectedly.
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.
Flags: needinfo?(bgirard)
Summary: Flash animations are half the expected width and height → Flash content, including YouTube, is clipped to half the expected height and width on Retina display
Matt or Jeff, can you take a look?
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(jmuizelaar)
Flags: needinfo?(bgirard)
I volunteer Joe as he has a Retina display
Flags: needinfo?(jmuizelaar) → needinfo?(joe)
I don't see the problem on my 10.8.3 retina.
And when I say 10.8.3, I of course mean 10.8.4.
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.
Milan: what version of Flash are you testing? I have Flash Player 11.8.800.94, the latest release.
Flags: needinfo?(milan)
I have 11.7.700.225 and was about to update, but ironically, the demonstrated the problem.  So, yes, I can reproduce.
Flags: needinfo?(milan)
BenWa, was there another bug for this?

Chris, this fixes the problem for me.
Attachment #779397 - Flags: review?(bgirard)
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(joe)
Attachment #779397 - Attachment is patch: true
Attachment #779397 - Attachment mime type: text/x-pascal → text/plain
Attachment #779397 - Flags: review?(bgirard) → review?(joe)
Attachment #779397 - Flags: review?(joe) → review+
We need to file a follow-up bug about the fact that plugins are 1/4-clipped as BenWa mentions in comment 10.
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.
Keywords: checkin-needed

Can we get a test for this regression?
Assignee: nobody → milan
Flags: in-testsuite?
Keywords: checkin-needed
FYI this bug appears to fix the problem, on Windows 8 at least, described by this bug report:
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
> 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.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #16)
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.
Duplicate of this bug: 897239
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
(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

Is this going to fix the choppy scrolling on Windows?
(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
> > 
> >
> 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.
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.
If you have the patch on a Try server I'll gladly test it out for you on Windows 8.
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.
Didn't realize that I need to select android reftests manually, so here's my try for that.
Attachment #780679 - Flags: review?(joe)
Thanks all for chasing this, both BenWa and I are offline until Friday/Monday, which is why this dragged on.
Attachment #780679 - Flags: review?(joe) → review+
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.
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.
(In reply to Gary [:streetwolf] from comment #34)
> Will this be making
> 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.
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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 :(
Resolution: FIXED → ---
Whiteboard: [fixed-in-birch]
Target Milestone: mozilla25 → ---
Duplicate of this bug: 897334
I don't get how this can cause the OSX asserts.
Pushing to try again from inbound rather than birch.
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.
This should now be fixed by the backout in bug 873378 comment 37 (with followup in bug 898713).
I can confirm that this is fixed in today's nightly
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.
BenWa, we can certainly morph this bug to cover the comment 10, if you think that is better than opening another bug.
Flags: needinfo?(bgirard)
Cloned to bug 900640.
Flags: needinfo?(bgirard)
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.