Last Comment Bug 663259 - [Mac] Async Plugin should default to true
: [Mac] Async Plugin should default to true
Status: RESOLVED FIXED
[qa+]
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: All Mac OS X
: -- normal (vote)
: mozilla8
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on: 689072 664728 668269 671639 671916
Blocks: 598425
  Show dependency treegraph
 
Reported: 2011-06-09 15:50 PDT by Benoit Girard (:BenWa)
Modified: 2011-12-21 02:41 PST (History)
23 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
+
wontfix
+
unaffected
unaffected


Attachments
Fix 1.0 (27.85 KB, patch)
2011-06-26 09:19 PDT, Benoit Girard (:BenWa)
jaas: review+
mounir: checkin-
Details | Diff | Review
Part 2: Restore Synchronous plugin rendering, with preference for async rendering (16.82 KB, patch)
2011-06-30 08:39 PDT, Benoit Girard (:BenWa)
smichaud: review+
Details | Diff | Review
Part 3: Enable by default (2.99 KB, patch)
2011-07-05 14:03 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Review
Part 4: Use double buffering to fix flickering with multiple plugins (21.38 KB, patch)
2011-07-05 14:04 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Review
(Optional) Improve resize behavior (4.66 KB, patch)
2011-07-05 14:42 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Review
(Optional) Improve resize behavior v2 (6.38 KB, patch)
2011-07-05 20:30 PDT, Benoit Girard (:BenWa)
cjones.bugs: review+
Details | Diff | Review
Part3+4: Turn on by default, add double buffer + bug fixes (qfolded) (35.61 KB, patch)
2011-07-07 13:40 PDT, Benoit Girard (:BenWa)
matt.woodrow: review+
Details | Diff | Review
text overlay with mozilla.plugins.use_layers=true (64.84 KB, image/jpeg)
2011-10-10 06:38 PDT, Vlad [QA]
no flags Details
Testcase: Multiple plugins (958 bytes, text/html)
2011-10-26 10:22 PDT, Benoit Girard (:BenWa)
no flags Details
Pref off (1.23 KB, patch)
2011-10-26 13:35 PDT, Benoit Girard (:BenWa)
smichaud: review+
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review
Fix Async check in test (1.01 KB, patch)
2011-10-26 15:52 PDT, Benoit Girard (:BenWa)
christian: approval‑mozilla‑aurora+
christian: approval‑mozilla‑beta+
Details | Diff | Review

Description Benoit Girard (:BenWa) 2011-06-09 15:50:38 PDT
When submitting bug 598425 I overlooked that I had a preference set in my local profile. about:support didn't pick it up since we whitelist properties for privacy. We need to set mUsePluginLayersPref to true by default on OSX to reflect that async plugins have been implemented.

Will land this fix after I've investigated the test failures.
Comment 1 Benoit Girard (:BenWa) 2011-06-13 12:36:45 PDT
I've got a patch ready to go but I'm still blocking on reftest failures (mochitests are fine). Some reftest don't appear to have anything to wait for painting.

Romaxa, how did we solve this on other platforms?
Comment 2 Benoit Girard (:BenWa) 2011-06-15 12:47:55 PDT
Progress update: I fixed the timing issues, I'm saving what is being rendered to the layer on disk and it's what we expect however for some reason reftest does not get the layer properly.
Comment 3 d.a. 2011-06-16 15:27:06 PDT
Not sure if you have fixed this issue yet or not, reporting it in cause you have missed it:

I've been getting a lot of these error messages in the Mac OS X Console, they seem to happen if you have 5-6 tabs open containing Flash videos. Depending on what site you are visiting the player may not render anything new (both player and video) until a few of the tabs are closed (also happens in fullscreen).

/Applications/Nightly.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container[1625]	ava error: /SourceCache/AppleVA/AppleVA-4.10.23/Framework/VP3CoreMediaInterface/AVAFVP3CoreMedia.c: IOAccelFindAccelerator failed for display number 4, error = 3758097084 [entry = 9c03, did = 3f0040, service = 0, fbIdx = 0]

This is with mozilla.plugins.use_layers set to true, on a late 2008 MacBook Pro, 9400M 256 MB video card.
Comment 4 Benoit Girard (:BenWa) 2011-06-20 07:37:43 PDT
(In reply to comment #3)
> Not sure if you have fixed this issue yet or not, reporting it in cause you
> have missed it:
> 
> I've been getting a lot of these error messages in the Mac OS X Console,
> they seem to happen if you have 5-6 tabs open containing Flash videos.
> Depending on what site you are visiting the player may not render anything
> new (both player and video) until a few of the tabs are closed (also happens
> in fullscreen).
> 
> /Applications/Nightly.app/Contents/MacOS/plugin-container.app/Contents/MacOS/
> plugin-container[1625]	ava error:
> /SourceCache/AppleVA/AppleVA-4.10.23/Framework/VP3CoreMediaInterface/
> AVAFVP3CoreMedia.c: IOAccelFindAccelerator failed for display number 4,
> error = 3758097084 [entry = 9c03, did = 3f0040, service = 0, fbIdx = 0]
> 
> This is with mozilla.plugins.use_layers set to true, on a late 2008 MacBook
> Pro, 9400M 256 MB video card.

Thanks for the report, unfortunately I have not seen that myself. We should open a bug about this once this issue is resolved.
Comment 5 Benoit Girard (:BenWa) 2011-06-20 07:41:10 PDT
Marking as tracking+ because we're hoping to have this fixed to properly support async plug-ins and make sure the necessary testing and outreach is made with plug-in vendors to resolve any issue they may experience, if any.

If this doesn't make it for the branching then bug 598425 should be backed out.

The hold up right now is still a problem with the reftest not seeing the layer image data that I am hoping to resolve ASAP.
Comment 6 Benoit Girard (:BenWa) 2011-06-26 09:19:36 PDT
Created attachment 542026 [details] [diff] [review]
Fix 1.0
Comment 7 Benoit Girard (:BenWa) 2011-06-26 09:28:20 PDT
Comment on attachment 542026 [details] [diff] [review]
Fix 1.0

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

::: dom/plugins/test/reftest/plugin-background.css
@@ +9,5 @@
>    left:0px; top:0px;
>    width:220px; height:220px;
> +  /* Core Animation alpha blending rounding differs
> +     from the Core Graphics blending, adjust with care */
> +  background-color: rgba(0,255,0, 0.51);

I tested this change across all platforms and it passes. With the previous alpha value of 0.6 we hit rounding problems and are off by 1 red, 1 blue.

::: dom/plugins/test/reftest/reftest.list
@@ +7,5 @@
>  random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) fails-if(!haveTestPlugin) == border-padding-1.html border-padding-1-ref.html # bug 629430
>  random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) fails-if(!haveTestPlugin) == border-padding-2.html border-padding-2-ref.html # bug 629430
>  random-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)) fails-if(!haveTestPlugin) skip-if(Android) == border-padding-3.html border-padding-3-ref.html # bug 629430
>  random-if(cocoaWidget||d2d) fails-if(!haveTestPlugin&&!Android) skip-if(!testPluginIsOOP()) == pluginproblemui-direction-1.html pluginproblemui-direction-1-ref.html
> +random-if(cocoaWidget) fails-if(!haveTestPlugin&&!Android) skip-if(!testPluginIsOOP()) == pluginproblemui-direction-2.html pluginproblemui-direction-2-ref.html

We hit a similar rounding problem as above but the differences are across the board so I wasn't able to fix up this test.

::: gfx/thebes/nsCoreAnimationSupport.h
@@ +75,5 @@
>                                           int aX, int aY,
>                                           int aWidth, int aHeight);
> +
> +#ifdef DEBUG
> +  static void SaveToDisk(nsIOSurface *surf);

This code is useful for debugging so I figured it might be worth checking in. I'll remove it if we don't accept code like this.
Comment 8 Benoit Girard (:BenWa) 2011-06-26 09:39:57 PDT
Comment on attachment 542026 [details] [diff] [review]
Fix 1.0

Wasn't sure who to ask for review, feel free to pass it on if you're overloaded.
Comment 9 Josh Aas 2011-06-26 12:41:05 PDT
Comment on attachment 542026 [details] [diff] [review]
Fix 1.0

Review of attachment 542026 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 10 Mounir Lamouri (:mounir) 2011-06-27 06:02:53 PDT
The patch has been merged to mozilla-central and backed out due to Talos regression.
Comment 11 Marco Bonardo [::mak] 2011-06-27 12:50:38 PDT
Graph server shows that after the backout the regression in Tp4 and Tp5 shutdown measurements disappeared. I'm not sure if tree-management will show the improvement though, it may consider it noise due to the small window.
http://tinyurl.com/5w5ntw3
Comment 12 Benoit Girard (:BenWa) 2011-06-30 08:39:45 PDT
Created attachment 543139 [details] [diff] [review]
Part 2: Restore Synchronous plugin rendering, with preference for async rendering

Because we haven't found a fix for the shutdown regression we are going to restore Synchronous plugin layer rendering (fix the regressions) and allow users to use async plugin layer rendering with the use of misnomer preference 'plugins.use_layers'. The only impact of turning on this pref should be a shutdown regression which I have only seen on Talos. 

This will fix the regressions for the aurora merge, give a change for plugin vendors to test async rendering and give us more time to test the changes on trunk.

Review comment: This patch undoes some of the changes from bug 598425/Part 1 and adds a few changes so that Synchronous and Asynchronous can work together with a preference, since before I had intended for Asynchronous to replace Synchronous.
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-06-30 09:26:09 PDT
Comment on attachment 543139 [details] [diff] [review]
Part 2: Restore Synchronous plugin rendering, with preference for async rendering

This looks fine to me.
Comment 14 Benoit Girard (:BenWa) 2011-06-30 14:05:10 PDT
I was able to reproduce the shutdown regression with the Talos machine. I noticed that Talos had outdated Flash version 10.1. Once I upgraded Flash to 10.3 the shutdown regression disappeared. This means that we don't need a code change to fix the shutdown regression.

With less then a week before the aurora merge I feel like there isn't enough time to test this change thoroughly. I may change my mind if someone is willing to advocate that the change warrants the risk.

Otherwise we should suggest that Flash be updated on the Talos machines. Once the merge happens we should notify tree management about the expected shutdown regression and turn on the preference by default on mozilla-central.
Comment 15 Benjamin Smedberg [:bsmedberg] 2011-06-30 14:14:53 PDT
I would very much like this to be fixed for this cycle for security and performance reasons. I don't think we're going to get a whole lot more testing on nightly users and really need to expose this to the aurora audience to make a release decision. I think we should land the patch with the pref flipped "on" by default, notify tree-management of the expected regression, and watch this carefully through the FF7 aurora cycle.
Comment 17 Benoit Girard (:BenWa) 2011-07-02 10:10:10 PDT
Re-opening since the two patch don't turn on async plugin by default.
Comment 18 Joe Drew (not getting mail) 2011-07-02 21:50:55 PDT
So how will we resolve this between Benjamin and Benoit? I understand where Benjamin is coming from, but I *also* understand what Benoit's saying. 

Maybe Josh can weigh in here?
Comment 19 Benoit Girard (:BenWa) 2011-07-03 11:19:46 PDT
(In reply to comment #18)
> So how will we resolve this between Benjamin and Benoit? I understand where
> Benjamin is coming from, but I *also* understand what Benoit's saying. 
> 
> Maybe Josh can weigh in here?

I'm not opposed to landing before the merge, especially after Benjamin's argument. I've noticed a problem with multiple plugins on the page, if we decide to land it we will have to land at least one additional fix on aurora. If that's acceptable I'll proceed with landing the changes.
Comment 20 Benoit Girard (:BenWa) 2011-07-05 14:03:02 PDT
Created attachment 544056 [details] [diff] [review]
Part 3: Enable by default
Comment 21 Benoit Girard (:BenWa) 2011-07-05 14:04:06 PDT
Created attachment 544058 [details] [diff] [review]
Part 4: Use double buffering to fix flickering with multiple plugins
Comment 22 Benoit Girard (:BenWa) 2011-07-05 14:42:49 PDT
Created attachment 544068 [details] [diff] [review]
(Optional) Improve resize behavior
Comment 23 d.a. 2011-07-05 15:20:51 PDT
I can no longer reproduce the problem I reported in comment 3, it was fixed when  the fix (attachment 542026 [details] [diff] [review]) landed. I haven't noticed the flickering plugins bug though.
Comment 24 Benoit Girard (:BenWa) 2011-07-05 15:32:07 PDT
(In reply to comment #23)
> I can no longer reproduce the problem I reported in comment 3, it was fixed
> when  the fix (attachment 542026 [details] [diff] [review] [review]) landed. I haven't
> noticed the flickering plugins bug though.

The flickering bug only happened when several plugins where on the page (stress test).

I'm interested to know if you can reproduce the problem when part 3+4 land.
Comment 25 Benoit Girard (:BenWa) 2011-07-05 16:02:36 PDT
Comment on attachment 544068 [details] [diff] [review]
(Optional) Improve resize behavior

This patch fails a single test case, need further investigation to find out why.
Comment 26 Asa Dotzler [:asa] 2011-07-05 19:55:06 PDT
Is there still anything in this bug that the release drivers should be tracking? It's not clear to me if the tracking nom is still relevant.
Comment 27 Benoit Girard (:BenWa) 2011-07-05 20:06:20 PDT
(In reply to comment #26)
> Is there still anything in this bug that the release drivers should be
> tracking? It's not clear to me if the tracking nom is still relevant.

I think once the fixes are reviewed, landed and proven on mozilla-central we may consider getting approval for aurora. See benjamin on Comment #15 for a justification.
Comment 28 Benoit Girard (:BenWa) 2011-07-05 20:30:32 PDT
Created attachment 544133 [details] [diff] [review]
(Optional) Improve resize behavior v2
Comment 29 Benoit Girard (:BenWa) 2011-07-05 20:32:39 PDT
Comment on attachment 544133 [details] [diff] [review]
(Optional) Improve resize behavior v2

Big win with resize esthetics.
Comment 30 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-06 08:45:52 PDT
Comment on attachment 544056 [details] [diff] [review]
Part 3: Enable by default

>+  if (!mInstanceOwner->UseAsyncRendering() &&
>       mInstanceOwner->IsRemoteDrawingCoreAnimation() &&
>       mInstanceOwner->GetEventModel() == NPEventModelCocoa) {
>     return LAYER_ACTIVE;
>   }

As we discussed, please add a comment here that this is the check for
the fallback mode "synchronous painting, but with (gecko) layers."
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-06 08:51:39 PDT
Comment on attachment 544058 [details] [diff] [review]
Part 4: Use double buffering to fix flickering with multiple plugins

>+                caLayer = mozilla::plugins::PluginUtilsOSX::GetCGLayer(CallCGDraw, this);

Drop the |mozilla::plugins::| qualification, it's not unnecessary and
distracting.  And in the other places below.

>diff --git a/dom/plugins/test/mochitest/utils.js b/dom/plugins/test/mochitest/utils.js
>--- a/dom/plugins/test/mochitest/utils.js
>+++ b/dom/plugins/test/mochitest/utils.js
>@@ -1,13 +1,12 @@
> function paintCountIs(plugin, expected, msg) {
>   var count = plugin.getPaintCount();
>   var realExpected = expected;
>-  var isAsync = SimpleTest.testPluginIsOOP() &&
>-    navigator.platform.indexOf("Mac") < 0;
>+  var isAsync = SimpleTest.testPluginIsOOP();

Shouldn't this be in the previous patch?

>diff --git a/gfx/thebes/nsCoreAnimationSupport.h b/gfx/thebes/nsCoreAnimationSupport.h

I don't feel comfortable reviewing this.  Some questions though

 - Is there anything inherently "async" and nsAsyncCARenderer?  That
   is, would it make sense to have a class nsSyncCARenderer?  Or is
   the "Async" part of the name there because of its use for async
   plugin drawing?  If the latter, please drop the "Async".  (Maybe
   nsDoubleBufferedCARenderer?)

 - Does it make sense to move this helper in gfx/thebes?  Would we use
   it elsewhere, or is it pretty specific to dom/plugins/ipc?  If
   there's no use for it elsewhere, let's keep in dom/plugins/ipc for
   now.

r+ for the refactoring in dom/plugins/ipc.
Comment 32 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-06 08:55:23 PDT
Comment on attachment 544133 [details] [diff] [review]
(Optional) Improve resize behavior v2

>         MacIOSurfaceImage::Data ioData;
>         ioData.mIOSurface = ioSurface;
>+        ioData.mSize = gfxIntSize(mShWidth, mShHeight);

Please comment here on why we're returning this size instead of the
ioSurface size.

I much prefer the effects of this patch, happy to take it.
Comment 33 Benoit Girard (:BenWa) 2011-07-06 09:06:16 PDT
(In reply to comment #31)
>  - Is there anything inherently "async" and nsAsyncCARenderer?  That
>    is, would it make sense to have a class nsSyncCARenderer?  Or is
>    the "Async" part of the name there because of its use for async
>    plugin drawing?  If the latter, please drop the "Async".  (Maybe
>    nsDoubleBufferedCARenderer?)
> 
>  - Does it make sense to move this helper in gfx/thebes?  Would we use
>    it elsewhere, or is it pretty specific to dom/plugins/ipc?  If
>    there's no use for it elsewhere, let's keep in dom/plugins/ipc for
>    now.
> 

Good point, I will rename and move the class.
Comment 34 Matt Woodrow (:mattwoodrow) 2011-07-06 11:42:30 PDT
Comment on attachment 544058 [details] [diff] [review]
Part 4: Use double buffering to fix flickering with multiple plugins

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

::: gfx/thebes/nsCoreAnimationSupport.h
@@ +134,5 @@
> +class THEBES_API nsAsyncCARenderer {
> +public:
> +  nsAsyncCARenderer() : mCALayer(nsnull) {}
> +  size_t GetWidth();
> +  size_t GetHeight();

GetFrontSurfaceHeight()?

Or document that this is what it's doing, in contrast to the GetBackSurface* functions.

@@ +140,5 @@
> +  size_t GetBackSurfaceHeight();
> +  IOSurfaceID GetIOSurfaceID();
> +
> +  bool HasBackSurfaceIOSurface();
> +  bool HasIOSurface();

Same here. Is it ever valid to only have one of the pair? This could just be HasIOSurfaces()

@@ +147,5 @@
> +  void SetCALayer(void *aCALayer);
> +  bool InitIOSurface(size_t aWidth, size_t aHeight);
> +  void Render();
> +  void SwapIOSurface();
> +  void ClearIOSurface();

Lucky number 3.

::: gfx/thebes/nsCoreAnimationSupport.mm
@@ +316,5 @@
>    IOSurfacePtr surfaceRef = nsIOSurfaceLib::IOSurfaceLookup(aIOSurfaceID);
>    if (!surfaceRef)
>      return nsnull;
>  
> +  ::CFRetain(surfaceRef);

Why are we retaining this surface now?

Looking at the header file, IOSurfaceLookup already performs a retain so won't this add a second reference that won't ever be free'd?

If we do need to call this, I'd rather do it inside the nsIOSurface constructor.

@@ +322,1 @@
>    nsIOSurface* ioSurface = new nsIOSurface(surfaceRef);

nsIOSurface is refcounted now, this should be nsRefPtr<nsIOSurface> and the return type of the function should be already_AddRefed<nsIOSurface>.

return ioSurface.forget();

@@ +645,5 @@
>  }
>  
> +IOSurfaceID nsCARenderer::GetIOSurfaceID() {
> +  if (!mIOSurface) {
> +    return NULL;

0? This is a uint32_t

@@ +890,5 @@
> +}
> +
> +IOSurfaceID nsAsyncCARenderer::GetIOSurfaceID() {
> +  if (!HasIOSurface()) {
> +    return nsnull;

0 again.

@@ +917,5 @@
> +  if (!mCALayer) {
> +    return false;
> +  }
> +
> +  mFrontSurface = nsIOSurface::CreateIOSurface(aWidth, aHeight);

I think this will leak without the mentioned RefPtr changes. The returned object will have 1 reference, and assigning to a RefPtr will add a reference.
Comment 35 Benoit Girard (:BenWa) 2011-07-07 13:40:51 PDT
Created attachment 544604 [details] [diff] [review]
Part3+4: Turn on by default, add double buffer + bug fixes (qfolded)
Comment 36 christian 2011-07-07 14:46:25 PDT
We do not intend to turn this on in Aurora or Beta so we do not need to track it. The feature needs to land on mozilla-central first and should go through the normal development and release process: tracking:-
Comment 37 :Ehsan Akhgari (busy, don't ask for review please) 2011-07-13 08:56:27 PDT
http://hg.mozilla.org/mozilla-central/rev/5cea57e451dd
Comment 38 beingalink 2011-07-14 09:52:30 PDT
Could it be that with enabling async layers, embedded quicktime has problems being displayed? On http://trailers.apple.com/ I get the sound of the movie but no picture, just a black square.
Comment 39 Benoit Girard (:BenWa) 2011-07-14 10:23:05 PDT
(In reply to comment #38)
> Could it be that with enabling async layers, embedded quicktime has problems
> being displayed? On http://trailers.apple.com/ I get the sound of the movie
> but no picture, just a black square.

You're correct that quicktime does not work properly. The reason is that they special case the rendering on firefox and never seem to detect it properly on the nightlies.
Comment 40 Vlad [QA] 2011-10-10 06:38:13 PDT
Created attachment 565925 [details]
text overlay with mozilla.plugins.use_layers=true

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0

I have manually added the mozilla.plugins.use_layers=true property in about:config and opened about 6-7 tabs containing flash videos (as described in comment #3) , but didn't see any error in error console.
Instead, some writing overlay appears in some tabs.

Are these steps enough to verify the bug? 
What about the overlay?
Comment 41 Benoit Girard (:BenWa) 2011-10-10 07:29:40 PDT
(In reply to Vlad [QA] from comment #40)
> Created attachment 565925 [details]
> text overlay with mozilla.plugins.use_layers=true
> 
> Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101
> Firefox/8.0
> 
> I have manually added the mozilla.plugins.use_layers=true property in
> about:config and opened about 6-7 tabs containing flash videos (as described
> in comment #3) , but didn't see any error in error console.
> Instead, some writing overlay appears in some tabs.
> 
> Are these steps enough to verify the bug? 
> What about the overlay?

The property 'mozilla.plugins.use_layers' no longer exists in Firefox 8.0, I'm surprised your seeing any behavior differences by setting it. You should see 'plugins.use_layers' set to true by default.
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 10:04:06 PDT
qa? due to comment 40.
Comment 43 christian 2011-10-25 16:03:28 PDT
Is there any downside to backing this out? Based on bug 692759 I think this needs to revert to Firefox 7 behavior.
Comment 44 christian 2011-10-26 09:27:03 PDT
(In reply to Christian Legnitto [:LegNeato] from comment #43)
> Is there any downside to backing this out? Based on bug 692759 I think this
> needs to revert to Firefox 7 behavior.

We've decided we want to back this out / revert the pref for Firefox 8 (possibly for Firefox 9 as well). Please land ASAP.
Comment 45 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-26 09:46:39 PDT
What is the implication for QA with this backout?
Comment 46 Benoit Girard (:BenWa) 2011-10-26 09:58:51 PDT
We're going to want to test against the latest version of Flash games and video (youtube, netflix), Quicktime video and Google Talk plugin (Google plus hang out).
Comment 47 Benoit Girard (:BenWa) 2011-10-26 10:00:17 PDT
(In reply to Benoit Girard (:BenWa) from comment #46)

In those we should test zooming the page, scrolling with the plugin in/out of view and switching tabs. If available test the full screen mode.
Comment 48 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-26 10:08:12 PDT
Thanks Benoit. What is the goal of this testing per comment 46 and comment 47 with builds where the patch is backed out?
Comment 49 Benoit Girard (:BenWa) 2011-10-26 10:17:52 PDT
This patch should return the behavior to what we had in FF 7.0. Thus FF 7.0 can provide a reference for the expected behavior. We're interested in looking for visual glitches, corrupted frames, flashing frames and crashes. We should also test the behavior when multiple embedded plugin are on the page (custom page with youtube embed). Attaching a test case for this.
Comment 50 Benoit Girard (:BenWa) 2011-10-26 10:22:47 PDT
Created attachment 569710 [details]
Testcase: Multiple plugins

Wait until the first plugin has buffered. All plugin frames should animate concurrently.
Comment 51 d.a. 2011-10-26 10:48:37 PDT
FWIW I've noticed that the performance when a Flash object is resized is pretty poor. When work was originally being done with async plugins the performance improved greatly but since then the performance has regressed. I filed this in bug 618318. 

Not sure if this is due to async plugins, but anyway if you open the context menu on a YouTube video the video will freeze while the audio keeps playing. I'll try this with the async plugins disabled to see if I can reproduce it.
Comment 52 Benoit Girard (:BenWa) 2011-10-26 11:10:08 PDT
(In reply to d.a. from comment #51)
> FWIW I've noticed that the performance when a Flash object is resized is
> pretty poor. When work was originally being done with async plugins the
> performance improved greatly but since then the performance has regressed. I
> filed this in bug 618318. 
> 

Async plugin should make this better but since not all calls to the plugins are async you can still end up waiting for drawing when making another synchronous call.
 It hard to tell without investigating more but it should be worthwhile to investigate. It could as well be that we slow at reflowing the page.
 
> Not sure if this is due to async plugins, but anyway if you open the context
> menu on a YouTube video the video will freeze while the audio keeps playing.
> I'll try this with the async plugins disabled to see if I can reproduce it.

Yes, the context menu blocks the plugin. I wrote some extra code to re-enter the run loop from inside the context menu even though NPAPI does ask for this. The async code doesn't do this. We decided that we wouldn't fix it. If we want to support that we should open a bug.
Comment 53 Benoit Girard (:BenWa) 2011-10-26 11:11:30 PDT
(In reply to Benoit Girard (:BenWa) from comment #52)
> even though NPAPI does not ask for
> this. 

Does *NOT* ask for this.
Comment 54 Benoit Girard (:BenWa) 2011-10-26 13:35:15 PDT
Created attachment 569774 [details] [diff] [review]
Pref off
Comment 55 Benoit Girard (:BenWa) 2011-10-26 14:35:43 PDT
Comment on attachment 569774 [details] [diff] [review]
Pref off

I've got it ready to land on Beta. I did some smoke test locally and the feature appears to be disabled correctly.
Comment 56 Benoit Girard (:BenWa) 2011-10-26 14:53:13 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/efb42ace58b0
Comment 57 Benoit Girard (:BenWa) 2011-10-26 15:52:04 PDT
Created attachment 569828 [details] [diff] [review]
Fix Async check in test
Comment 58 christian 2011-10-26 15:55:45 PDT
Comment on attachment 569828 [details] [diff] [review]
Fix Async check in test

approving test fix.
Comment 59 Benoit Girard (:BenWa) 2011-10-26 16:07:45 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/fc2bb0c7af44
Comment 60 Virgil Dicu [:virgil] [QA] 2011-10-27 09:58:33 PDT
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0

Checked the behavior on 8Beta 5 using the information from comment 46 and the following. Everything seemed normal while playing youtube, flash games from miniclip.com, using the attachment test case and apple trailer videos. Netflix is not available in my country so I couldn't use that, but all the rest rendered normally (same as F7).

Will the back-up concern Aurora as well? should this be tested there too? Also, are there any more focus areas to test for this bug on Beta?
Comment 61 christian 2011-10-31 15:33:05 PDT
Yes, we are backing out on aurora as well (though it doesn't look like it has landed there yet)
Comment 62 Benoit Girard (:BenWa) 2011-11-01 12:23:15 PDT
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac762e001967
Comment 63 juan becerra [:juanb] 2011-11-03 09:13:57 PDT
Marking as [qa+] to make sure this gets verified where it applies.

Note You need to log in before you can comment on or make changes to this bug.