Currently we are sending plugin surface to content process, and content process does SHMem/X copy of layer surface to Sharable surface.... and send sharable surface to chrome process... I think we should make it so that ShadowImage(plugin)Layer send surface directly to Chrome process without copy.

This seems to be the cause of flash player plugin performance degradation on maemo6

Degradation compared to what?

A direct comparison between fennec versus microb indicates that flash performance on fennec is much slower, almost twice as slow.

How does perf compare to microb with flash running in-process in fennec?(dom.ipc.plugins.enabled=false)

remote.tabs=false yields some like 30% improvement. Chris does this mean dom.ipc.plugins are disabled by that as well? We will check with dom.ipc.plugins separately

No, dom.ipc.plugins is orthogonal to tabs.remote.

ok. So we have slowdown coming from 2 sources: remote tabs and abovementioned problem. We shall file a new bug. Meanwhile we will get numbers with dom.ipc.plugins.enabled set to off

I obtained the following flash timings this morning for the N9 an N900 devices. N9 has the most recent flash image. As far as I know, The N900 is update to date with service packs. N900/Mode FPS FpsMeter.swf 0 (fail) A.html + FpsMeter.swf 48 (runs smooth) N9 (dom.ipc.plugins.enabled = true) FpsMeter.swf 18 (runs smooth) A.html + FpsMeter.swf 28 (runs poorly) N9 (dom.ipc.plugins.enabled = false) FpsMeter.swf 20 (runs poorly) A.html + FpsMeter.swf 28 (runs poorly) I've attached the swf and html files that I used to obtain these results.

To implement that it would be required to change mFrontSurface to be a SurfaceDescriptor instead of a gfxASurface. Than we would need something like a PluginSurfaceImage and would put the SurfaceDescriptor into a PluginSurfaceImage instead of a CairoImage in nsPluginInstanceOwner::SetCurrentImage (nsObjectFrame.cpp).

(In reply to comment #11) > To implement that it would be required to change mFrontSurface to be a > SurfaceDescriptor instead of a gfxASurface. I guess it is better to use a gfxPluginSurface which is a gfxASurface for that.

Add a gfxPluginSurface class. TODO: * Add a PluginImage class * Use the PluginImage instead of CairoImage in nsObjectFrame.cpp (nsPluginInstanceOwner::SetCurrentImage) * Remove the temporary mSurface from gfxPluginSurface

Comment on attachment 487340 [details] [diff] [review] Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP) I'll check it tomorrow

Using an ImageLayer like in the current patch is not the right approach because it is rendered to a surface in the content process, so just what we do not want to do.

Maybe we should add a PluginLayer type instead, where the surface is just swapped into a ShadowPluginLayer instead of painting/rendering anything on the content side. Another question, does it make sense to use the mozilla::layers::SurfaceDescriptor also for the plugins?

Yes, we do want a pluginlayer type eventually.

Comment on attachment 487340 [details] [diff] [review] Add gfxPluginSurface and corresponding PluginImage (Image::PLUGIN_SURFACE) for plugins (WIP) It's not clear to me how these patches get us closer to plugin gfx bypassing the content-process main thread. There's already a plan for this in bug 598867, which should have been set to block this bug. As for the patch itself, a gfxPluginSurface or imagelayer of type plugin doesn't make sense to me. And yes, we should share code with gfx/layers/ipc as much as possible.

We may or may not want a new PluginLayer type eventually, but I don't understand why we need one here. An X pixmap can be passed from plugin to content and from content to compositor without copying, or in bug 598867 directly from plugin to compositor.

> We may or may not want a new PluginLayer type eventually, but I don't I think so too... Practically plugin-child calling SendShow, and in PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface with our XID) and call Invalidate and in nsPluginInstanceOwner::InvalidateRect we call SetCurrentImage/SetData or something like that, and that should in Swap or similar ShadowImageLayer function send XID directly to Chrome process... The only thing we need to do is to connect PluginInstanceParent::RecvShow and ShadowImageLayer::Swap and get back previous layer XID for recycling..

Per Oleg Comment #21, I have attacted a diagram which illustrates how this will work within the layers class hierarchy. Specifically, I'm going to look more closely at how ShadowImageLayerOGL works. What is the difference between: BasicImageLayer, ShadowImageLayer, ImageLayerOGL, BasicShadowableImageLayer and BasicShadowImageLayer?

(In reply to comment #10) > I obtained the following flash timings this morning for the N9 an N900 > devices. N9 has the most recent flash image. As far as I know, The > N900 is update to date with service packs. > > N900/Mode FPS > FpsMeter.swf 0 (fail) > A.html + FpsMeter.swf 48 (runs smooth) > Are these numbers measuring fennec or microb? What's the "0 (fail)"? > N9 (dom.ipc.plugins.enabled = true) > FpsMeter.swf 18 (runs smooth) > A.html + FpsMeter.swf 28 (runs poorly) > > N9 (dom.ipc.plugins.enabled = false) > FpsMeter.swf 20 (runs poorly) > A.html + FpsMeter.swf 28 (runs poorly) Is for the tabs.remote=true or false? It would be nice to get data for both configurations, to know how much the overhead of tabs.remote=true is reducing fps. Also, kudos to romaxa for async plugin painting, which is most likely why dom.ipc.plugins.enabled=true has no overhead compared to false.

(In reply to comment #22) > What is the difference between: > > BasicImageLayer, ShadowImageLayer, ImageLayerOGL, BasicShadowableImageLayer and > BasicShadowImageLayer? Basic*ImageLayer are software-rendered images, currently used for plugin surfaces and <video> frames. *ImageLayerOGL are the same but in a format that allows faster compositing with GL. The Shadow* variants are the cross-process partners of "real" layers; for example, the content process might have a BasicImageLayer, and the chrome process would have a ShadowImageLayer corresponding to that. The design is at [1] but that's a bit outdated and doesn't go into specifics. [1] https://wiki.mozilla.org/Gecko:CrossProcessLayers

(In reply to comment #24) > Are these numbers measuring fennec or microb? What's the "0 (fail)"? microb > > N9 (dom.ipc.plugins.enabled = true) > > FpsMeter.swf 18 (runs smooth) > > A.html + FpsMeter.swf 28 (runs poorly) > > > > N9 (dom.ipc.plugins.enabled = false) > > FpsMeter.swf 20 (runs poorly) > > A.html + FpsMeter.swf 28 (runs poorly) > > Is for the tabs.remote=true or false? It would be nice to get data for both > configurations, to know how much the overhead of tabs.remote=true is reducing > fps. Ok. I'll add this to the next set of timings. > Also, kudos to romaxa for async plugin painting, which is most likely why > dom.ipc.plugins.enabled=true has no overhead compared to false. Nice!

> What's the "0 (fail)"? Did not work at all -- no output.

Quick update --- I'm currently working on bug 564086 which will allow the plugin subprocess to communicate directly with the chrome process. In parallel, there are some other problems that need to be solved - The content process's layer tree needs to have a Layer that represents plugin frames, even though that layer won't be backed with a real surface or be renderable. One approach to this is bug 598872, but I'm not sure it's the best way. Another possibility might be to make a special ImageLayer type that serves the same purpose as a Ref/PlaceholderLayer. - Need some way to look up a "phantom" plugin layer, like what bug 598872 would refer to, in the chrome process when the layer tree is painted. - Need a way to invalidate and repaint in the chrome process when a plugin has an updated surface. Should be fairly easy. - The painting model in the plugin subprocess needs to be planned out. It should be simpler on the whole than the current async-painting model, I think (no need to deal with nsObjectFrame), but the details of when surfaces are created, how they're shared, etc. need to worked out. - It would be nice to use PLayers for plugin subprocesses, for several reasons. Does the current async plugin-painting model require features not present in PLayers.ipdl? If so, we should add them I think; in the long run it would be nice to share all this code. If adding those features looks hard, we'll need an intermediate PPluginCompositor protocol or something like that.

(In reply to comment #24) > Is for the tabs.remote=true or false? It would be nice to get data for both > configurations, to know how much the overhead of tabs.remote=true is reducing > fps. According to my tests it is tabs.remote=true 8fps, tabs.remote=false 16fps.

(In reply to comment #28) > Quick update --- I'm currently working on bug 564086 which will allow the > plugin subprocess to communicate directly with the chrome process. In > parallel, there are some other problems that need to be solved > > ... That should definitely be the right approach to the problem. (In reply to comment #21) > > We may or may not want a new PluginLayer type eventually, but I don't > I think so too... > > Practically plugin-child calling SendShow, and in > PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface > with our XID) and call Invalidate > > and in nsPluginInstanceOwner::InvalidateRect we call SetCurrentImage/SetData or > something like that, and that should in Swap or similar ShadowImageLayer > function send XID directly to Chrome process... > > The only thing we need to do is to connect PluginInstanceParent::RecvShow and > ShadowImageLayer::Swap and get back previous layer XID for recycling.. I checked if it possible to do that as a temporary hack for now, but the problem is that ShadowImageLayer::Swap just swaps internal used gfxSharedImageSurfaces. So it would probably easier to add a PluginLayer which also understands gfxXlibSurface but than there is still the problem that the ShadowLayer swap happens somewhere in BasicShadowLayerManager::EndTransaction called from nsDisplayList::PaintForFrame so it is not so easy to get the previous XID back into the PluginInstanceParent. But maybe it would work with three XIDs which are recycled. But it is a really hackish approach.

Updated Image of Oleg Comment #21

> Practically plugin-child calling SendShow, and in > PluginInstanceParent::RecvShow we initialize mFrontSurface (as gfxXlibSurface > with our XID) and call Invalidate Sounds like the consensus is that plugins are the way to go. Sadly, I don't see any examples of their usage in the "gfx" folder. Where is there documentation on how to integrate with the plugin system? Is there any wiki links or visio diagrams that would be good to illustrate their usage.

WIP. The idea is to have a PluginLayer which just forwards the surface from the plugin to the chrome process.

Requires bug 610155 Directly forward surface in ShadowableImageLayer when possible.

I don't understand the dependency on bug 610155 or the new patches. Was the source of the tabs.remote=true overhead discovered?

I've now had a chance to look at both approaches, the atlternative approach using ImageLayer looks superior. It uses static variables to bypass all the layer cludge. Adding to an already overburdened deeply nested layer class hierarchy, as the PluginSurfaceLayer proposes does not look tenable.

deeply nested layer class hierarchy? The max depth is 3.

Are you including all the multi-inheritance in your count?

No, that doesn't increase the depth. We could add some kind of PluginLayer. I think we will, in the future. But we don't need it here and now, as far as I can tell.

>> We could add some kind of PluginLayer. I think we will, in the future. Ok. sounds fine. I'm looking at how to both proposals at the moment. I didn't mean to criticize the depth of the class hiearchy, there is nothing inherently wrong with it, I was just expressing a concern that the proposal would duplicate an entire branch, which is a common criticism of object orientated designs.

>> I don't understand the dependency on bug 610155 or the new patches. Was the >> source of the tabs.remote=true overhead discovered? I dont think we have yet. Oleg's patch for 610155 seems relevent here. Why couldn't we do something similar for flash? Why cant we just build upon his work. I'm still studying it, but it looks to me exactly like what we need. (In reply to comment #35)

The multiple classes you'd need to implement for PluginLayer are just the different chunks of functionality that any design would need: a way to render the plugin with and without GL, and a way to forward the PluginLayer from a content process to a compositing process.

This bug as spec'd depends on bug 598867, which we're not even planning to take for <video> in 4.0. We need to either change tack of this bug to "share plugin surfaces more efficiently with the chrome process" which is more feasible for 4.0, then re-evaluate blocking status; or, blocking- this.

You want to create a protocol that bridges the plugin subprocess and the the UI process. Let's call that "PBridgeCompositorPlugin". The syntax to set that up is PContent.ipdl: child spawns PPluginModule; //... PBridgeCompositorPlugin: bridges PContent, PPluginModule //... You have to create the the bridge from within the process that has references to the two actors being bridged. Here it's the content process: it can send messages to the compositor process (through ContentChild) and to the plugin subprocess (through PluginModuleParent). The IPDL compiler infers this and should generate an interface like namespace PBridgeCompositorPlugin { bool Bridge(PContentChild*, PPluginModuleParent*); } Then within the content process, your code will call that function. Behind the scenes, messages will be sent to the compositor and plugin subprocesses, and IPDL-generated code will require Alloc*() methods to create the bridge endpoints. Does this make sense? If your implementation looks like this, what errors are you seeing?

Tested on Linux X11 and Maemo works great

Here is Plugin SetLayerID API which is passing unique shadowable Image LayerID provided in bug 598872 to PluginInstanceChild.

Comment on attachment 561122 [details] [diff] [review] Plugin Parent LayerID AP + ObjectFrame setup call >+ nsRefPtr<nsNPAPIPluginInstance> inst; >+ GetPluginInstance(getter_AddRefs(inst)); >+ if (inst && imglayer->GetUniqueID()) { >+ inst->SetLayerID(imglayer->GetUniqueID()); >+ } This is going to set the ID (sending an IPC message) on every paint that involves a plugin instance, one message per plugin instance. That's not good. This should be done one when the container is initialized. What happens if the PluginInstanceChild tries to send over a buffer before it has an ID set?

> This is going to set the ID (sending an IPC message) on every paint that > involves a plugin instance, one message per plugin instance. That's not > good. This should be done one when the container is initialized. I think that was before and not true now, before we had BuildLayer call on each paint, but now it called only when we need to create new layer.. at least my printf's show that.. > > What happens if the PluginInstanceChild tries to send over a buffer before > it has an ID set? This is ShadowableImageLayer and ID created in ctor.

(In reply to Oleg Romashin (:romaxa) from comment #49) > > This is going to set the ID (sending an IPC message) on every paint that > > involves a plugin instance, one message per plugin instance. That's not > > good. This should be done one when the container is initialized. > > I think that was before and not true now, before we had BuildLayer call on > each paint, but now it called only when we need to create new layer.. at > least my printf's show that.. > Would be good to know for sure. > > > > What happens if the PluginInstanceChild tries to send over a buffer before > > it has an ID set? > This is ShadowableImageLayer and ID created in ctor. Are you guaranteeing that UpdatePluginGeometry() or whatever it is that makes the plugin instance think it's visible is called after the layer is created?

... if that's the case, it needs to be documented. Otherwise bad things will happen.

> Are you guaranteeing that UpdatePluginGeometry() or whatever it is that > makes the plugin instance think it's visible is called after the layer is > created? It is controlled by PluginInstanceOwner implementation and it call special function in order to suspend plugin painting... so plugin in that case will just stop calling invalidates and PluginInstanceChild will stop calling SendShow(or bridge::Show)... In my implementation I'm just bridging SendShow calls... If by some reason layout decide to destroy plugin layer, then it will be unregistered from BridgeParent, and we never call swap/paint for that layer, until layout create and send new layer ID.

Fixed multiple plugin's instance handling, destruction, updated to latest change in bug 598872... Practically we need patches from bug 598872 and patches from this bug in order to make PluginBridge working with Shmem and X11 based plugins

Kind of very simple exposure of PluginLayer API based derived from ImageLayer... Does not provide any special type or name, just ImageLayer with additional method. Derived all other ImageLayer implementations from PluginLayer in order to reduce dummy wrappers count.

Comment on attachment 561122 [details] [diff] [review] Plugin Parent LayerID AP + ObjectFrame setup call >diff --git a/layout/generic/nsObjectFrame.cpp b/layout/generic/nsObjectFrame.cpp >--- a/layout/generic/nsObjectFrame.cpp >+++ b/layout/generic/nsObjectFrame.cpp >@@ -1626,16 +1626,21 @@ nsObjectFrame::BuildLayer(nsDisplayListB > } > > NS_ASSERTION(layer->GetType() == Layer::TYPE_IMAGE, "Bad layer type"); > > ImageLayer* imglayer = static_cast<ImageLayer*>(layer.get()); > UpdateImageLayer(container, r); > > imglayer->SetContainer(container); >+ nsRefPtr<nsNPAPIPluginInstance> inst; >+ GetPluginInstance(getter_AddRefs(inst)); >+ if (inst && imglayer->GetUniqueID()) { >+ inst->SetLayerID(imglayer->GetUniqueID()); >+ } I'm about 90% sure this is going to be called on every paint. Explain to me why it's not going to be.

Comment on attachment 561156 [details] [diff] [review] Plugin bridge impl wip2 >diff --git a/dom/plugins/ipc/PBridgeCompositorPlugin.ipdl b/dom/plugins/ipc/PBridgeCompositorPlugin.ipdl >+ >+namespace mozilla { >+namespace plugins { >+ >+struct SurfaceLayersDescriptorD3D10 { >+ WindowsHandle handle; >+}; >+ Don't duplicate all this stuff from PLayers. Use PLayers directly instead. See below... >+// (Bridge protocols can have different semantics than the endpoints >+// they bridge) >+rpc protocol PBridgeCompositorPlugin { This shouldn't be rpc. Use sync. >+ bridges PContent, PPluginModule; >+ ... manages PLayers; The CreateBridge()/OpenPluginCompositorBridge() interaction doesn't make sense to me. It looks unnecessary. I skimmed the rest and it looks mostly OK.

Is it really the right idea to have PluginLayers wrap ImageLayer? It seems to me that PluginLayers should have their own interface. For one thing, we really just want to be able to set a native surface for a PluginLayer, right? We don't need Images, or ImageContainers.

I mean "inherit from", not "wrap".

Comment on attachment 562561 [details] [diff] [review] API to forward unique ID to plugin instance Review of attachment 562561 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. But don't push this patch until we have a working plugin solution, so I can see how it all fits together. ::: dom/plugins/ipc/PluginLibrary.h @@ +120,5 @@ > #if defined(MOZ_WIDGET_QT) && (MOZ_PLATFORM_MAEMO == 6) > virtual nsresult HandleGUIEvent(NPP instance, const nsGUIEvent&, bool*) = 0; > #endif > + /** > + * Set to PluginInstance Unique ID of layer which is used for instance rendering "Send to PluginInstance"

Resolving old bugs which are likely not relevant any more, since NPAPI plugins are deprecated.