We should create simple layers implementation for plugins, and make it works at least for NPP_DrawImage NPAPI, and probably for HandleEvent.

Comment on attachment 436652 [details] [diff] [review] Plugin layers imageSurface WIP1 roc could you check this patch, and probably answer on some questions in there?

I have some questions about layers: 1) In case of CAIRO_SURFACE layers (BasicImageLayer::Paint), are we doing clipping for expose area rectangle? for example we have flash plugin input field and caret blinking inside... 2) Is it possible to detect target surface type when I'm preparing layer for plugin object? for example if we are going to paint our layer to X-Surface, then it is better to prepare layer as gfxXlib CAIRO_SURFACE, if it is gfxImageSurface, then it is better to create layer as imageSurface, also it would be nice to know imageSurface format (32, 24, 16...)

(In reply to comment #3) > I have some questions about layers: > 1) In case of CAIRO_SURFACE layers (BasicImageLayer::Paint), are we doing > clipping for expose area rectangle? for example we have flash plugin input > field and caret blinking inside... The paint operations are clipped to the expose area rectangle, yes. > 2) Is it possible to detect target surface type when I'm preparing layer for > plugin object? for example if we are going to paint our layer to X-Surface, > then it is better to prepare layer as gfxXlib CAIRO_SURFACE, if it is > gfxImageSurface, then it is better to create layer as imageSurface, also it > would be nice to know imageSurface format (32, 24, 16...) In the common case, we'll be drawing into the layer manager you set up in your nsWindow, and you should optimize for that. If your code sometimes uses a BasicLayerManager and sometimes a GL layer manager, you can check the layer manager type to see which one you created: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.h#178 But my guess is that if you're ultimately going to draw using the X API you should create an Xlib surface, and if you're going to draw using NPP_DrawRect you should create an image surface. If you're going to draw using the X API and you create an image surface, then some nasty fallback is going to occur right away. About the patch: GetLayerManagerForDoc should not be duplicated. Move it to nsLayoutUtils or nsContentUtils and share it. + // Should I create here imageContainer or some GL container GL has its own ImageContainer implementation, so this comment is not needed.

Using layers for plugins can't really help you (but can hurt you) right now. Once you're using a GL layer manager --- or retained BasicLayers --- you might start to benefit.

GetLayerManagerForDoc - moved to nsLayoutUtils BasicLayer now contain xlibSurface Here we have one benefit comparing to default plugins implementation - we are not createing temp XSurface for each plugin paint, also we are not requesting plugin to paint stuff when it is just external expose event (no request to plugin) Would be nice to understand how to make this layer painted on top of fennec canvas (avoid rendering plugin to tiles)

If it is possible to render this xsurface layer directly to widget surface, then we can kill current fennec pluginObserver and absolutePositioned frame hack without loosing performance

+ mPaintWithLayers = getenv("MOZ_PLUGIN_LAYER") ? PR_TRUE : PR_FALSE; != null While this might be good for testing, I don't really want to check it in. We should use layers when that's the right thing to do. + // Should I create here imageContainer or some GL container Remove this comment. If you're using a GL layers backend, this will create a GL ImageContainer. + transform.Scale(r.Width()/pluginWindowSize.width, r.Height()/pluginWindowSize.height); When would this scale factor not be 1.0? I still don't think you're getting much benefit here. You might remove a little bit of overhead, but you could be caching the X surface even if you weren't rendering using layers. And if the app is painting to an X surface anyway, you're *adding* considerable overhead by forcing drawing to go through a temporary surface. I appreciate your work to get this working, but I think we have more important things to be doing, like getting the GL layers backend to work on Maemo.

> I appreciate your work to get this working, but I think we have more important > things to be doing, like getting the GL layers backend to work on Maemo. This work I'm doing to get some understanding and practice with new layers environment... also understand how this are going to work in general... GL stuff is in process now, and Fred is doing that part... I'm currently trying to make sure that we don't have any perf regression by enabling IPC plugins on mobile environment.. and trying to find or fix all related problems. Btw: I have 0 practice about GL programming... ;) I think Fred is better person to do that stuff

New version. Created based on default X-Windowless API. Tested on Qt Fennec (with X backend). works fine with Desktop flash in Opaque mode. Still problem with transparent mode (need to implement alpha extraction). Anyone would like to check it? feedback?

Comment on attachment 446512 [details] [diff] [review] WIP2, Layers + windowles-X IPC async Alright well I'm not familiar with Layers/X11 so I can't be very helpful for those parts. As far as the design for async draw goes I say it's a good design but it needs to be modified to work cross-platform. Perhaps we might want to create a shared surface object that the PluginInstanceParent/Child will pass around when flushing. This shared surface can hold some of the platform specific code and be used to pass the Shmem back and forth.

This patch works fine with GTK rendering (opaque/transparent with desktop flash plugin), and with QT (opaque/transparent with test plugin, and opaque with flash plugin). I think I need initial review for this patch

With this patch we can recognize that plugins are supports rendering with alpha channel, and if not, then we use Renderer... Also for IPC we are going to support transparentAlpha always, because PluginInstanceChild will do double painting and alpha extraction on plugin process side.

I think this is more clear implementation, less X dependencies, easier porting to another platforms.

BackSurface creation moved to pluginChild process Added ForcePaint to messageUtils, and removed flush. mIsOwnBackBuffer replacsed by dummy Shmem in NativeID case (XID, IOSurface ID) Added Shmem and nativeID arguments to Flush, and FlushFinished methods.

I've added small hack for recognizing OOP plugins and using Renderer class in case of non-OOP plugins (to be able to render transparent plugins with alpha extraction)

Fixed rendering to gfxImageSurface (default offscreen) Need feedback about current hacks (see comments in patch)

Comment on attachment 447383 [details] [diff] [review] Updated patch, need feedback Added common methods for shmem and nativeID (XID/IOSurface) types

It would be nice to get new NPAPI approved.

(Filed bug 568713 on the problem with diff view and diff -r.)

Comment on attachment 447824 [details] [diff] [review] More cleanup, and fixed rendering with shmem on Qt::Raster backend I'm not sure that I've thought through this enough to know what the best approaches would be here. It would help me to have more comments explaining why you use particular approaches. However, here are some thoughts: Something like visibility notification will be needed to stop the plugin painting away busily when it is not visible, and to destroy the cached surfaces when they are not needed. I first thought it could be a timeout after the last time the plugin-wrapper (PluginInstanceParent) gets a paint request. However retained layers will need to do something similar, so maybe it can be a notification from removal of the ObjectFrame layer wrapper. >+ // Parent notify child that Flush has been handled completely >+ // and returns back buffer ownership to child >+ async FlushFinished(Shmem backMem, uint32_t nativeID); >+ // Async rendering >+ // Child notify parent that backbuffer has updated are, >+ // and it need to be flushed to front buffer. >+ // this moving backBuffer ownership to parent. >+ // backbuffer must not be modified until FlushFinished received from parent >+ async Flush(NPRect rect, Shmem backMem, uint32_t nativeID); >+ // Alloc small shmem for simple ownership handling >+ AllocShmem(1, SharedMemory::TYPE_BASIC, &mBackShmem); Is this shmem necessary for high speed locking, or could IPDL be used to negotiate access? It may be unnecessary to create a shared memory segment. Can IPDL provide a gentleman's agreement over mutual exclusion of access to backBuffer? (I assume this is what "ownership" is referring to rather than responsibility for destroying the backBuffer itself.) (In reply to comment #15) > Also for IPC we are going to support transparentAlpha always, because > PluginInstanceChild will do double painting and alpha extraction on plugin > process side. >+ mDoAlphaExtraction = (mIsLayer >+ && mWsInfo.colormap == gfxASurface::ImageFormatARGB32 >+ && mIsTransparent >+ && !mIsSupportsAlpha); Requiring the plugin wrapper (PluginInstanceParent) to support transparentAlpha always is making transparent plugins that don't support transparentAlpha also use alpha extraction. For image-based browsers, the alpha extraction would have happened anyway, but for xlib-based browsers this is adding additional work. Alpha extraction is expensive and should be avoided where possible. I suspect the best way to do async transparent plugins is to pass the plugin the background and get it to draw on top of the background, like what Chromium does: http://www.chromium.org/developers/design-documents/plugin-architecture This is additional complexity though, so maybe a first stage is to only do opaque or supportsTransparentAlpha plugins async. It looks like the paint-with-xlib/alpha-extraction in PluginInstanceChild needs to either use a NativeRender or at least share much of the code. >+ if (!PaintRectToSurface(mIsTransparent ? plPaintRect : aRect, fallBackSurface)) >+ return false; Didn't understand the different rectangle sizes here. >+ if (mBackShmem.IsWritable()) >+ AsyncCall(&PluginInstanceChild::InvalidateRectDelayed, this); This could detect if an InvalidateRectDelayed has already been scheduled and skip scheduling another. The 60 Hz timer I imagined would delay the first InvalidateRectDelayed by max(1/60 - (now - last_paint_time), 0). A better timer might use (some fraction of) the time between invalidate from the PluginInstanceParent and the next paint to set the time between plugin paints. However, multiple invalidates off one event are already accumulated into one paint, so I'm not sure how complicated to get here. It might be reasonable to try this without the delay, and add a delay if it turns out to be necessary. >+ // Hack for recognizing OOP plugins >+ // Any better option should be used here? >+ case 6000: { nsNPAPIPlugin could be made to remember whether whether the plugin is OOP, and some API added to get that info. I'm not yet clear why this is needed, though. >+ // Store surface pointer to window structure >+ // to be able to use it in IPC plugins as front layer >+ // FIXME need more legal way to send front layer surface to PluginInstanceParent Maybe we can have a hash table to look up layer or surface from native id (maybe from pointers in the case of shmem)? >+ if (mIsOOP) { I haven't grasped why the OOP case is different here. >+ } else if (mLayerSurface->GetType() == gfxASurface::SurfaceTypeImage) { >+ ws_info->visual = nsnull; >+ format = static_cast<gfxImageSurface*>(mLayerSurface.get())->Format(); >+ >+ } >+ // Store preferable image format for backBuffer into colormap field >+ // this should go directly to PluginInstanceChild >+ ws_info->colormap = format; Can this use the DrawImage API instead of passing codes through the XLib API? >+ // For non-OOP plugins we should use Renderer class. >+ if (!mIsOOP) { Is this conditional more a decision based on whether the plugin (or plugin wrapper) supports drawing to an ARGB surface? i.e. Does the Renderer need to be used if the plugin can draw to an ARGB surface? If it is reasonable to treat the plugin wrapper as an NPAPI plugin, then that would be the tidiest thing to do (until we decide that all plugins will be OOP).

If async drawing would benefit from a more versatile interface than NPAPI, then perhaps nsNPAPIPlugin could provide the PluginInstanceChild to the nsNPAPIPluginInstance and nsObjectFrame could access methods on the PluginInstanceChild directly.

I mean, perhaps nsNPAPIPlugin could provide the PluginInstanceParent to the nsNPAPIPluginInstance and nsObjectFrame could access methods on the PluginInstanceParent directly.

Is this bug about asynchronously drawn OOP plugins, or is this still entirely synchronous?

(In reply to comment #30) > Is this bug about asynchronously drawn OOP plugins, or is this still entirely > synchronous? yes, it is about async rendering. With this patch Expose->Layout->Paint does not wait plugin process anymore.

Try server show some oranges/crashes failures... need to fix the one by one. http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280264548.1280266565.867.gz MozillaTry/1280262230.1280262697.16983.gz MozillaTry/1280264548.1280266979.2540.gz MozillaTry/1280264548.1280267815.6318.gz MozillaTry/1280264548.1280266810.1898.gz MozillaTry/1280262230.1280263140.18747.gz MozillaTry/1280262230.1280263140.18756.gz

We have a problem with async layer rendering and these reftests: REFTEST TEST-UNEXPECTED-FAIL | modules/plugin/test/reftest/plugin-alpha-zindex.html | REFTEST TEST-UNEXPECTED-FAIL | modules/plugin/test/reftest/plugin-alpha-opacity.html | Problem is that current layer implementation is not preparing layer content on first ::BuildLayer call... it is just sending paint request to child process. and reftest system taking screenshot before we getting right plugin layer content (InvalidateRect event), and as result test failed Any ideas what should be done here? should I do sync call for first layer paint?

No, we should wait for it somehow.

roc, given that asynchronous painting has security wins in addition to performance wins, I think this should block, and probably block early (beta4). Do you agree, and do you know who the final reviewer will be? Romaxa, it looks like this patch only implements async painting for OOPP plugins. Is there a reason you chose to do it this way, instead of always doing async painting? At least on Mac we're still going to be running some plugins in-process for the forseeable future.

> plugins. Is there a reason you chose to do it this way, instead of always doing > async painting? No reason, I just haven't thought about it > At least on Mac we're still going to be running some plugins > in-process for the forseeable future. I think it is not hard to do, at least I can make implementation for X11.

Blocking per comment 37.

(In reply to comment #37) > roc, given that asynchronous painting has security wins in addition to > performance wins, I think this should block, and probably block early (beta4). > Do you agree, and do you know who the final reviewer will be? Yes, I agree that asynchronous plugin painting is highly desirable. Chris Jones, Benoit Girard, Josh and Karl can probably do the review. A major issue is how to handle transparent plugin instances. On Mac it's not a problem, we can draw the plugin to an RGBA surface using Quartz or Core Animation. On Windows and X, we cannot using the standard windowless painting APIs; we have to use alpha extraction or background propagation, both of which are slow and complicated. Background propagation will be tricky to make work with layers. On Windows and X, I think we might have to stick with synchronous painting for transparent plugin instances for Firefox 4. At least, let's start with asynchronous painting for opaque instances.

What about reftests? what is triggering snapshots taking from test page? should I change reftests scripts and trigger snapshots taking by some other event, or do some other trick in layout to cheat reftest scripts?

blocking2.0: beta4+ ⇒ --- (That's ^^^ an accident, right?)

(In reply to comment #42) > blocking2.0: beta4+ ⇒ --- > (That's ^^^ an accident, right?) Hmmm, yep, I did not want to do that, probably I start typing in wrong tab, and submit outdated bugzilla page.... and bugzilla did not warn me about it...

(In reply to comment #41) > What about reftests? what is triggering snapshots taking from test page? The load event, normally. > should I change reftests scripts and trigger snapshots taking by some other > event, or do some other trick in layout to cheat reftest scripts? Rather than fix all reftests containing plugins, I think it would make more sense for reftest.js to automatically wait for pending plugin paints to complete, if possible. The hard case is tests where we change an attribute or invoke a plugin method, then return, then the reftest snapshot happens and we expect the snapshot to show the new color or other state of the plugin. We need to figure out how to make those kinds of tests work. We could have the reftest harness pass a flag to drawWindow to make plugin painting synchronous for that case? But that would interfere with our desire to use the USE_WIDGET_LAYERS flag to just grab the window contents directly in reftests.

restoring blocking request

(In reply to comment #40) > A major issue is how to handle transparent plugin instances. > ... > On Windows and X, we cannot using the standard windowless painting > APIs; we have to use alpha extraction or background propagation, both of which > are slow and complicated. Alpha extraction may not be as bad as I first thought. With GDI plugins, we do this using a DIB, so there's no readback, unless the plugin is doing readback to paint to the DIB. With X11 plugins, Flash Player uses XShmGetImage or similar to fetch the background that we provide, so, if readback is going to happen, it will have happened already and hopefully the Pixmap will migrate out of video ram. In fact, providing (for alpha-recovery) a fresh Pixmap for Flash Player to GetImage from may be better than providing a ThebesLayerBuffer that is more likely to be in video memory after we've used it for compositing operations. Running transparent Flash across a network is going to be bad whatever we do.

In my current patch I have alpha-extraction code, that working fine on linux. yep, that code could be optimized a bit with XShmGetImage. Also my patch has support for AlphaSupport flag, and if plugin say (with new NPAPI variable) yes I do support rendering with alpha, then we can get just ARGB32 content from plugin... and flash for example support that But we need NP_Variable to be able to do that. https://mail.mozilla.org/pipermail/plugin-futures/2010-April/000064.html NPPVpluginTransparentAlphaBool

Also have some problem with 546071-1.html reftest... it seems to load fine, but 546071-1.html show 1 pixel difference on left but pixel has gray color, and picture is smooth, comparing to 546071-1-ref.html... Is it something related to pixel-snap?

(In reply to comment #49) > Is it something related to pixel-snap? Yes, looks like it is.

sounds like test is not valid anymore, because non-layer-plugins rendering was integer only, and now we can paint layers to non-integer position and that makes test and ref test not matching anymore

The plugin rectangle edges should always be snapped to the nearest pixel edges.

Ok, pixels snapping is fixed. But I got another problem, more layers related... http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1280629443.1280630427.9604.gz&fulltext=1#err0 REFTEST TEST-START | reftest/tests/modules/plugin/test/reftest/plugin-alpha-opacity.html ++DOMWINDOW == 83 (0xb0be2c0) [serial = 8823] [outer = 0x908a780] RMX:ERR: GetLayerState without mInstanceOwner! RMX:ERR: GetLayerState without mInstanceOwner! RMX:ERR: BuildLayer without mInstanceOwner! ###!!! ASSERTION: We shouldn't be drawing into a layer with no items!: 'entry', file layout/base/FrameLayerBuilder.cpp, line 1273 mozilla::FrameLayerBuilder::DrawThebesLayer [layout/base/FrameLayerBuilder.cpp:1274] Layer trying to paint, but no mInstanceOwner yet created... and after that it fail with assertion.. I was not able to reproduce this with short plugin reftest run reftest/tests/modules/plugin/test/reftest/reftest.list But I reproducible after full reftests run. :roc do you have ideas what it could be, and why we endup with "drawing into a layer with no items!"?

Locally it was reproducible only with build from try server, if I'm compiling this by myself, then it is not reproducible ....

oh, when mInstanceOwner == null, I'm returning LAYER_INACTIVE, probably it should be LAYER_NONE.

Yes

Regarding the reftest issue, I think we'll need a flag in the plugin instance to track whether we are expecting to see a pending paint. This flag would be set when we instantiate the plugin and when we get an NPN_Invalidate, and cleared when we get a paint. When we're painting with PAINT_SYNC_DECODE_IMAGES, and there is a pending plugin paint, we should wait for that paint to come through.

Comment on attachment 461538 [details] [diff] [review] add reftest-async style for taking screenshot on second invalidate yep, this workaround does not work always... and very unstable...

(In reply to comment #57) > Regarding the reftest issue, I think we'll need a flag in the plugin instance > to track whether we are expecting to see a pending paint. This flag would be > set when we instantiate the plugin and when we get an NPN_Invalidate, and > cleared when we get a paint. When we're painting with PAINT_SYNC_DECODE_IMAGES, > and there is a pending plugin paint, we should wait for that paint to come > through. Oleg and I discussed this on IRC. Instead of forcing asynchronously-painted plugins to paint synchronously, what if we instead (i) tracked pending plugin paints in the reftest harness; (ii) had testplugin notify when it painted; and (iii) only ran drawWindow() after the number of outstanding plugin paints hit 0? I'm hesitant to add a third painting path (synchronous-paint-of-asynchronously-painted-plugins) just for reftests.

Comment on attachment 460979 [details] [diff] [review] Fixed oranges caused by SetWindow while pending plugin->event() call (re-enter problem) Drive-by comment: >diff -r 880b6cdd7dd8 dom/plugins/PPluginInstance.ipdl >--- a/dom/plugins/PPluginInstance.ipdl >+++ b/dom/plugins/PPluginInstance.ipdl >@@ -87,16 +87,26 @@ child: > rpc NPP_HandleEvent_IOSurface(NPRemoteEvent event, uint32_t surfaceid) > returns (int16_t handled); > // special cases of HandleEvent to make mediating races simpler > rpc Paint(NPRemoteEvent event) > returns (int16_t handled); > // this is only used on windows to forward WM_WINDOWPOSCHANGE > async WindowPosChanged(NPRemoteEvent event); > >+ // Async Rendering >+ // Parent notify child that front layer is dirty and need urgent repaint >+ async ForcePaint(NPRect rect); >+ // Parent notify child that Flush has been handled completely >+ // and returns back buffer ownership to child >+ async FlushFinished(Shmem backMem, uint32_t nativeID); >+ // Suspend paint called before SetWindow call >+ // to make sure that child do not send any Flush notifications, and don't break ownership handling >+ rpc SuspendPaint(); >+ > rpc NPP_Destroy() > returns (NPError rv); > > parent: > rpc NPN_GetValue_NPNVjavascriptEnabledBool() > returns (bool value, NPError result); > rpc NPN_GetValue_NPNVisOfflineBool() > returns (bool value, NPError result); >@@ -129,16 +139,23 @@ parent: > * but IPDL doesn't allow that for constructors. > */ > rpc PStreamNotify(nsCString url, nsCString target, bool post, > nsCString buffer, bool file) > returns (NPError result); > > async NPN_InvalidateRect(NPRect rect); > >+ // Async rendering >+ // Child notify parent that backbuffer has updated are, >+ // and it need to be flushed to front buffer. >+ // this moving backBuffer ownership to parent. >+ // backbuffer must not be modified until FlushFinished received from parent >+ async Flush(NPRect rect, Shmem backMem, uint32_t nativeID); >+ Why is this 'async' instead of 'sync'? With 'sync', the logic is much simpler, and plugin repaints are still asynchronous from the browser's perspective. The generic cross-process layers code (which I hope to use here "soon") does 'sync' buffer updates, but it has the luxury of relying on the refresh driver. It looks like this patch throttles paints already (?), so that should be fine. It would be nice to have this code evolve towards a common model.

(In reply to comment #60) > Why is this 'async' instead of 'sync'? With 'sync', the logic is much simpler, > and plugin repaints are still asynchronous from the browser's perspective. The > generic cross-process layers code (which I hope to use here "soon") does 'sync' > buffer updates, but it has the luxury of relying on the refresh driver. It > looks like this patch throttles paints already (?), so that should be fine. It > would be nice to have this code evolve towards a common model. Thinking about this more, the generic cross-process layers code also relies on batch updates, which should only happen at most [refresh driver] Hz (50Hz)? We currently don't have layer-tree info in plugin subprocesses (that is, which instances are on the same "page"), so I'd be OK with this approach for now as long as we plan to unify the two models in a followup.

(In reply to comment #59) > Oleg and I discussed this on IRC. Instead of forcing asynchronously-painted > plugins to paint synchronously, what if we instead (i) tracked pending plugin > paints in the reftest harness; (ii) had testplugin notify when it painted; and > (iii) only ran drawWindow() after the number of outstanding plugin paints hit > 0? I'm hesitant to add a third painting path > (synchronous-paint-of-asynchronously-painted-plugins) just for reftests. It's not necessarily just for reftests. It's for any caller of drawWindow (other than those that pass the special flag to allow partial image decode results), or anyone who calls nsIPresShell::RenderDocument. Page snapshotting extensions, stuff like that. I don't know how we would track pending plugin paints in the reftest harness, anyway. I think we can make this Just Work for API consumers. I think we should.

As long as this "sync" paint happens at script-safe times (so that we're not perpetuating bug 552512.

Sorry, missed this yesterday. (In reply to comment #62) > (In reply to comment #59) > > Oleg and I discussed this on IRC. Instead of forcing asynchronously-painted > > plugins to paint synchronously, what if we instead (i) tracked pending plugin > > paints in the reftest harness; (ii) had testplugin notify when it painted; and > > (iii) only ran drawWindow() after the number of outstanding plugin paints hit > > 0? I'm hesitant to add a third painting path > > (synchronous-paint-of-asynchronously-painted-plugins) just for reftests. > > It's not necessarily just for reftests. It's for any caller of drawWindow > (other than those that pass the special flag to allow partial image decode > results), or anyone who calls nsIPresShell::RenderDocument. Page snapshotting > extensions, stuff like that. > For foreground documents, we'll always have a consistent view of the plugin layer that's at most 1/[refresh driver]Hz out of date or so. That seems good enough to me, except for special cases like reftests. Are you concerned about snapshotting background documents? That does seem like it would require a forced-sync repaint. > I don't know how we would track pending plugin paints in the reftest harness, > anyway. > We do similar things in plugin mochitests, it's not hard. (In reply to comment #63) > As long as this "sync" paint happens at script-safe times (so that we're not > perpetuating bug 552512. This should go through the same painting path as in the current impl and so inherit the existing guards. Hopefully we can re-use the same NPAPI interface, but if not, we need to add it to the race mediator.

(In reply to comment #64) > (In reply to comment #62) > > It's not necessarily just for reftests. It's for any caller of drawWindow > > (other than those that pass the special flag to allow partial image decode > > results), or anyone who calls nsIPresShell::RenderDocument. Page snapshotting > > extensions, stuff like that. > > For foreground documents, we'll always have a consistent view of the plugin > layer that's at most 1/[refresh driver]Hz out of date or so. That seems good > enough to me, except for special cases like reftests. It normally would be, but it leaves special cases with odd behavior, which it would be better to avoid. > Are you concerned about > snapshotting background documents? That does seem like it would require a > forced-sync repaint. Well, I haven't checked the latest patches but I think if we're using async painting we have to have a copy of the plugin buffer around even if there is no retained layer, so drawing a background document or offscreen plugin via drawWindow should not always need a synchronous paint. > We do similar things in plugin mochitests, it's not hard. Which tests? > (In reply to comment #63) > > As long as this "sync" paint happens at script-safe times (so that we're not > > perpetuating bug 552512. > > This should go through the same painting path as in the current impl and so > inherit the existing guards. Hopefully we can re-use the same NPAPI > interface, but if not, we need to add it to the race mediator. Actually bsmedberg's right, this is a problem. As I understand it, the race mediator does not prevent a plugin from performing a synchronous NPN_Evaluate during painting. So the sync-paint approach is bad, and drawWindow in general is just doomed to sometimes show an out-of-date plugin :-(. So, some reftest harness changes are required. One approach: when a plugin paints, but we are expecting an asynchronous update from the plugin based on the flag in comment #57, do something which makes nsPresContext::IsDOMPaintEventPending return true. The reftest harness already checks that to wait for a state where there are no pending invalidates after flushing rendering. The only change required to the reftest harness would be to modify WhenMozAfterPaintFlushed so that when it receives a MozAfterPaint event, the "handler()" function should call WhenMozAfterPaintFlushed again instead of calling the continuation directly.

(In reply to comment #65) > (In reply to comment #64) > > (In reply to comment #62) > > > It's not necessarily just for reftests. It's for any caller of drawWindow > > > (other than those that pass the special flag to allow partial image decode > > > results), or anyone who calls nsIPresShell::RenderDocument. Page snapshotting > > > extensions, stuff like that. > > > > For foreground documents, we'll always have a consistent view of the plugin > > layer that's at most 1/[refresh driver]Hz out of date or so. That seems good > > enough to me, except for special cases like reftests. > > It normally would be, but it leaves special cases with odd behavior, which it > would be better to avoid. > Yeah, that's a good point. > > Are you concerned about > > snapshotting background documents? That does seem like it would require a > > forced-sync repaint. > > Well, I haven't checked the latest patches but I think if we're using async > painting we have to have a copy of the plugin buffer around even if there is no > retained layer, so drawing a background document or offscreen plugin via > drawWindow should not always need a synchronous paint. > I don't think we always necessarily need a backing buffer, though I haven't read the patches either. We could just keep accumulating Invalidate()s and never ask for pixels. > > We do similar things in plugin mochitests, it's not hard. > > Which tests? > http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/test_painting.html?force=1 is in the neighborhood of what we'd want. > > (In reply to comment #63) > > > As long as this "sync" paint happens at script-safe times (so that we're not > > > perpetuating bug 552512. > > > > This should go through the same painting path as in the current impl and so > > inherit the existing guards. Hopefully we can re-use the same NPAPI > > interface, but if not, we need to add it to the race mediator. > > Actually bsmedberg's right, this is a problem. As I understand it, the race > mediator does not prevent a plugin from performing a synchronous NPN_Evaluate > during painting. So the sync-paint approach is bad, and drawWindow in general > is just doomed to sometimes show an out-of-date plugin :-(. > That wouldn't be a race, so right. But AFAIK we haven't seen plugins in the wild responding to paint requests with NPRuntime calls. This is the old problem that we have even with in-process plugins.

I think we have seen such things in the wild. Unfortunately I can't prove it because the crash reporter database needs to be improved before we can collect the data with my work in bug 552512. I would very much like to be insulated from the possibility.

Agreed. I only brought that up because I don't think a new synchronous painting interface is going to cause any problems we don't already have (unless the race mediator isn't taught about it).

Here is the patch which is not using anymore 6000, mIsOOP hacks... and just exposing new API to PluginLibrary.. SetupLayer RenderLayer Currently implemented only for remote plugins, and still has some recursive/race problems... but if someone wants to check API params and common design, feedback are welcome

Oleg says this isn't going to make it for beta4, pushing to beta5, but I don't think we can push this any further than that.

This version mostly working everywhere and using new API. Still need to accumulate feedback about reftests and fix it according to comments.

This version locally pass all reftests without reftest change. I have made 2 new events which plugin layer sending if builder-ShouldSyncDecodeImages == true. 1-st event - MozPaintWait - fired when new plugin layer created and waiting for paint 2-nd event - MozPaintWaitFinished - fired when new plugin layer got first paint (initialized). I've modified reftests.js to handle these events. reftests starting normally, and always listening for new events, and if MozPaintWait event received, then reftests stopping first attempt and waiting until amount of MozPaintWait == MozPaintWaitFinished it means that all async layers painted and has initialized data content.

What about the tests that call a plugin method (e.g. setColor) and then do a drawWindow and expect to see the new color? It seems to me such tests would fail with this patch because nothing would clear mIsPainted. So how are those tests working? I hope we're not making them into random-orange. Indeed, you're actually setting mIsPainted to true in nsPluginInstanceOwner::InvalidateRect, which I don't understand. Shouldn't we be setting it in PluginInstanceChild::RecvFlushFinished? I think this approach could work if we were able to fire the MozPaintWait event from nsPluginInstanceOwner::InvalidateRect when mIsPainted is cleared there and MozPaintWaitFinished from RecvFlushFinished when mIsPainted is set there.

> tests working? I hope we're not making them into random-orange. Ok, I see your point, I will make such test, and try to check how to make it work. > nsPluginInstanceOwner::InvalidateRect, which I don't understand. Shouldn't we > be setting it in PluginInstanceChild::RecvFlushFinished? Actually RecvFlushFinished calling InvalidateRect that why it there... but it is hack, and I think we should have another API method like FlushFinished, and call layout invalidate there. > from nsPluginInstanceOwner::InvalidateRect when mIsPainted is cleared there Yep, I think it will work, and I'll fix it this way... thanks.

Ok, seems latest patch: http://hg.mozilla.org/try/rev/aa824ad61b7e pass all plugin related reftests/mochitests. The only plugin oranges I see with --setpref=dom.ipc.plugins.enabled=false, but I think it is easy to fix. Also need to remove debug printfs and do other cleanup before review.

This version has fix for PluginLayers when ipc.enabled=false. (implementing async layers rendering for non-ipc plugins). Does anybody want to look at this patch? feedback review?

Comment on attachment 467268 [details] [diff] [review] Green tests version I need some feedback about this patch in general and in details.. It works much better, and have green boxes, I think it is very close to final version.

Instead of _compute_alpha_values, can you use gfxAlphaRecovery? We have SSE2 code in there now for x86. It also has better handling for the case where the plugin image changes between painting onto black and painting onto white.

use gfxAlphaRecovery, fix right pixel snapping, fixed test_painting.html work more stable (no repeating force paint events)

Ah and here is try build for this version: http://hg.mozilla.org/try/rev/240b331c0898 I see only RGL and M1 (test_play_events) - oranges

I haven't done a detailed review yet, just looked over it. + // Async version of SetWindow call + async SetupLayer(int type, int format, NPRemoteWindow window); Document these parameters + // Child notify parent that backbuffer has updated are, "are"? + // and it need to be flushed to front buffer. + // this moving backBuffer ownership to parent. "backbuffer" + // backbuffer must not be modified until FlushFinished received from parent + rpc Flush(NPRect rect, Shmem backMem, uint32_t nativeID) + returns (Shmem rtnbuffer); We need a lot of documentation here or elsewhere explaining what the overall strategy is. For example you have to read the whole patch to find out that we try to share native surfaces via nativeID in preference to using shared memory. The way flushing and buffer ownership works should also be documented. + nsresult SetupLayerInst(void* aSurface, NPWindow* window); + nsresult RenderLayerInst(NPRect* aRect); + nsresult IsLayerPaintedInst(PRBool *aIsPainted); Drop the "Inst" suffix? IsLayerPainted should just return PRBool directly and should probably be called IsLayerUpToDate. + if (mIsLayer) { + nsCOMPtr<nsIRunnable> event = new nsDelayedInvalidateEvent(owner, invalidRect); + if (event) + NS_DispatchToCurrentThread(event); Why are we running this off an event? Shouldn't PaintWaitFinishedListenerW (Why the "W"?) be doing something to trigger a snapshot and end the test if we were only waiting for the plugin paint? Maybe call AfterPaintListener or AttrModifiedListener? mIsPendingPaintReply should probably be mWaitingForPaint or something like that. + nsRefPtr<gfxContext> ctx = new gfxContext(mLayerSurface); + if (ctx->UserToDevicePixelSnapped(aRect)) + aRect = ctx->DeviceToUser(aRect); What is this supposed to do? I think it doesn't do anything, because no transform is set on ctx. nsPluginInstanceOwner::GetPluginLayer should be called CreatePluginLayer. + bool createNew = mBackSurface ? false : true; !mBackSurface + if (!createNew) + createNew = frontSize != gfxUtils::GetASurfaceSize(mBackSurface); + if (!createNew) + createNew = stype != mBackSurface->GetType(); + if (!createNew) + createNew = sctype != mBackSurface->GetContentType(); + if (!createNew) + return true; Just use one big boolean expression with || + mIsSupportsAlpha = getenv("IS_APLHA_SUPP") ? true : false; Is that documented? What does it do? Also, use != nsnull + // then we create cached temporary surface + // used for double painting with alpha extraction or rendering to image surface + nsRefPtr<gfxASurface> fallBackSurface; mFallbackSurface In PluginInstanceChild::PaintRectWithAlphaExtraction we should probably consider using a single pixmap of twice the height, and reorganize the code to a) fill the top half with black and the bottom half with white, b) draw the plugin onto the top half and again into the bottom half, and c) read back the whole pixmap at once into a single image surface where we perform alpha extraction, then d) draw the result back to the mBackSurface. But let's not do that in this patch. PluginInstanceChild::PaintRectWithAlphaExtraction should be refactored so the separate code for white and black painting is shared in a helper function. I wonder if the patch could be split up for easier reviewing and landing. Maybe at least one patch to add the plugin-side support and interfaces, and another patch to use those interfaces on the browser side, and maybe a separate patch to add the X11 optimizations?

> + nsresult SetupLayerInst(void* aSurface, NPWindow* window); > + nsresult RenderLayerInst(NPRect* aRect); > + nsresult IsLayerPaintedInst(PRBool *aIsPainted); I make it to be different from nsIPluginInstance.idl interfaces... but using the same params. But ok, I'll change that, othwrwise name is a bit ugly > + if (mIsLayer) { > + nsCOMPtr<nsIRunnable> event = new nsDelayedInvalidateEvent(owner, > invalidRect); > + if (event) > + NS_DispatchToCurrentThread(event); > > Why are we running this off an event? Plugins don't really like to get expose event in InvalidateRect, and expecting that InvalidateRect call is async. Flash for example playing slowly if we call expose from Invalidate (at least it was 2 month ago), also some tests where failing due some wierd recursion or unexpected paint during invalidate. > > Shouldn't PaintWaitFinishedListenerW (Why the "W"?) be doing something to Ah, I was using it for fast debugging and making difference between functions in "reftest-wait" and normal reftests > trigger a snapshot and end the test if we were only waiting for the plugin > paint? Maybe call AfterPaintListener or AttrModifiedListener? Need to check that, and runn tests > > + nsRefPtr<gfxContext> ctx = new gfxContext(mLayerSurface); > + if (ctx->UserToDevicePixelSnapped(aRect)) > + aRect = ctx->DeviceToUser(aRect); > > What is this supposed to do? I think it doesn't do anything, because no > transform is set on ctx. That is fixing snap to integer pixel problem, 546071-1.html test, and other similar tests... I was asking someone you or karl, and I understood that we should always snap plugin surface to integer pixels, otherwise cairo-layer trying to emulate half pixel painting and that breaking tests... This is simple version of this functionality: http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#5183 > > nsPluginInstanceOwner::GetPluginLayer should be called CreatePluginLayer. yep, before GetPluginLayer was trying to get exiting image container, when GetLeafLayerFor was not available... > > + mIsSupportsAlpha = getenv("IS_APLHA_SUPP") ? true : false; > Is that documented? What does it do? Also, use != nsnull That was used for plugins wich supports TransparentAlphaValue... and some tests But not we can remove this. > I wonder if the patch could be split up for easier reviewing and landing. Maybe > at least one patch to add the plugin-side support and interfaces, and another > patch to use those interfaces on the browser side, and maybe a separate patch > to add the X11 optimizations? Need to check what can be done here...

(In reply to comment #85) > > + nsRefPtr<gfxContext> ctx = new gfxContext(mLayerSurface); > > + if (ctx->UserToDevicePixelSnapped(aRect)) > > + aRect = ctx->DeviceToUser(aRect); > > > > What is this supposed to do? I think it doesn't do anything, because no > > transform is set on ctx. > That is fixing snap to integer pixel problem, 546071-1.html test, and other > similar tests... > I was asking someone you or karl, and I understood that we should always snap > plugin surface to integer pixels, otherwise cairo-layer trying to emulate half > pixel painting and that breaking tests... > This is simple version of this functionality: > http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#5183 Yes, but there we have a context which might have a transform in it. Here you don't. The same code can be written without using a context, just do aRect.Round(). And the code to compute the snapped plugin rect can be shared between nsObjectFrame::BuildLayer and GetLayerState. Although actually I think GetLayerState should just be returning LAYER_ACTIVE all the time. Was there any reason you didn't do this?

Fixed latest comments, fixed more stable work with test_flush_on_paint.html test. Prevent plugin rendering(Flush) when child and parent buffers are different.

Feature freeze is now beta6, so pushing this out.

Comment on attachment 469805 [details] [diff] [review] v4. Ok, I'll try to separate API's, implementation, and usage parts for easier review

Comment on attachment 470248 [details] [diff] [review] gfxUtils for plugin layers These should be methods of gfxASurface. GetSize should just be a virtual method that return -1,-1 when we don't have the size (default implementation in gfxASurface).

Also, you need gfx review from someone not me, say Joe.

+ nsCOMPtr<nsIDOMEventTarget> dispatchTarget = do_QueryInterface(ourWindow); + nsCOMPtr<nsIDOMEventTarget> eventTarget = dispatchTarget; Surely you don't need both of these lines FireDOMPaintWait(Finished)Event can share code, only the event name is different. + gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true); + gBrowser.addEventListener("MozPaintWaitFinished", PaintWaitFinishedListener, true); Why are you doing this is in the !shouldWait case? It might be too late. In general, OnDocumentLoaded seems too late to catch MozPaintWait. How can you be sure it didn't fire before the onload event?

+ // Async Rendering + // Parent notify child that front layer is dirty and need urgent repaint + async ForcePaint(NPRect rect); It's not clear from the comment what this means or when this would be needed. + // @param backMem - dummy shared memory, just for native surface ownership handling Can you explain in more detail why backMem is needed? + gfxRect mAccRect; This is not a good name. How about mAccumulatedInvalidRect? + PRBool mDoAlphaExtraction; + PRBool mSupportNonDefaultVisual; PRPackedBool + bool mPengineForcePaintRequest; Why "mPengine"? + // Back layer surface + nsRefPtr<gfxASurface> mBackSurface; + Shmem mBackShmem; + // Accumulated invalidate rect, while back buffer is not accessible + gfxRect mAccRect; + // Cached plugin transparent mode value + PRBool mIsTransparent; + // True while plugin waiting to be painted to the screen + PRBool mSuspendPainting; + // True while plugin-child waiting for HandleEvent finish + PRBool mPendingPluginCall; + // Timeout in milliseconds of pending async Invalidate flush call + PRInt32 mIsPendingInvalidateTimeout; + // We can't setup layer and drop back surface while we are in pending plugin call + // if that happen then remeber pending SetupLayer call and do it later + PRBool mPendingSetWindow; PRPackedBools Still missing documentation of what exactly the protocol does, e.g., what the back-surface and front-surface are used for at different times.

I was trying to implement it this way, but someone advice me to push it into gfxUtils...

roc is this description better http://hg.mozilla.org/users/romaxa_gmail.com/layer-patches/file/bf98c4d1c893/03_plugin_api_part.diff#l42 ?

Comment on attachment 470457 [details] [diff] [review] Updated, create virtual ASurface method While surfaces in general don't have size, I'm ok with adding a getter that returns (-1, -1) in those cases. What we shouldn't do, though, is store mSize on every ASurface. Instead, just keep the storage in the derived classes. Also, in talking to Jeff about this issue, we decided that we shouldn't store mSize at all. Instead, we should call the appropriate cairo functions to get the size at the appropriate times. Can you file a followup bug on Core::Graphics to do that?

New bug created: https://bugzilla.mozilla.org/show_bug.cgi?id=592037

Comment on attachment 470542 [details] [diff] [review] Fixed GetSize method >diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp > #if defined(XP_WIN) || defined(WINCE) > #include "gfxWindowsPlatform.h" > #endif >+#include "gfxImageSurface.h" >+#ifdef MOZ_X11 >+#include "gfxXlibSurface.h" >+#endif These includes aren't needed anymore. Great otherwise.

Comment on attachment 470458 [details] [diff] [review] reftests api, updated. v2 Rename gWaitCounter to gExplicitPendingPaintCounter. The rest looks good to me, but dbaron should review changes to reftest.js. You probably should add a comment to the patch explaining what the MozPaintWaitFinished and MozPaintWait events are and when they fire.

Oleg, can you submit a new version of "plugin module API part"? + PRBool EnsureLayerSurface(const nsIntSize& aSize, + PRBool aForce, + PRBool* aIsTransparent = nsnull); Document what aForce means and what the return value means + if (!getenv("DISABLE_PLUGIN_LAYER") && + (!mPluginWindow || mPluginWindow->type == NPWindowTypeDrawable)) If we need to control this, use a pref instead of an environment variable? + PRBool IsPainted(); This needs documentation + // Update plugin window dimensions + mPluginWindow->width = aSize.width; + mPluginWindow->height = aSize.height; If we're changing mPluginWindow, don't we have to CallSetWindow? + if (aIsTransparent) + *aIsTransparent = transparent; {} + if (transparent) { + nsRefPtr<gfxContext> ctx = new gfxContext(mLayerSurface); + ctx->SetOperator(gfxContext::OPERATOR_CLEAR); + ctx->Paint(); + } Why do we have to do this? nsObjectFrame::BuildLayer and nsPluginInstanceOwner::CreatePluginLayer size the plugin layer based on the plugin frame rect. But I think we should be sizing the layer always to the window size we send to CallSetWindow. + nsRefPtr<Layer> layer = + (aBuilder->LayerBuilder()->GetLeafLayerFor(aBuilder, aManager, aItem)); Lose unnecessary parents. Also, something here needs to forget about the old layer if it has the wrong size. + // Set a transform on the layer to draw the video in the right place To draw the *plugin* + if (mWaitingForPaint && IsPainted()) { + // AnswerFlush always calling InvalidateRect and we should + // notify reftests about paint finished + mObjectFrame->PresContext()->FireDOMEvent(NS_LITERAL_STRING("MozPaintWaitFinished")); + mWaitingForPaint = false; + } Why are we firing MozPaintWaitFinished during an *Invalidate*? Also FireDOMEvent should be called FireDOMEventAtWindow or something to tell us what the event target is. + nsIntRect clipRect(aRect->left, aRect->top, aRect->right - aRect->left, aRect->bottom - aRect->top); + if (transparent) { + ctx->Save(); + ctx->SetOperator(gfxContext::OPERATOR_CLEAR); + ctx->Rectangle(gfxRect(clipRect.x, clipRect.y, clipRect.width, clipRect.height), PR_TRUE); + ctx->Clip(); + ctx->Paint(); + ctx->Restore(); + } Instead of doing Save/Clip/Paint/Restore you can just do Fill. Do a NewPath first. I'm not sure why we need to do this clearing, though. Can't we just use OPERATOR_SOURCE before we call renderer.Draw?

+ // Plugin NPN_InvalidateRect initiate event for HandleEvent paint call (render to backsurface), + // When plugin finished painting, PluginInstanceChild makes rpc call to PluginInstanceParent (Flush/FlushNative). + // Also PluginChild stop rendering to backsurface and only accumulate Invalidate rectangles. + // Rendering to backsurface will be resumed when pluginChild receive FlushFinished from PluginParent, + // which mean that previous plugin content has been rendered to screen + // + // PluginParent in AnswerFlush[Native] method sync back<->front surfaces content and call ObjectFrame InvalidateRect + // InvalidateRect initiate new layout paint event and ObjectFrame::BuildLayer call RenderLayer. + // RenderLayer calling FlushFinished to PluginChild and that can continue rendering again. Instead of copying the back surface to the front surface in AnswerFlush, why not rotate ownership of the buffers? We could rename Flush[Native] to Show[Native]. That method would pass ownership of the plugin's back-buffer to the PluginParent, making it the PluginParent's new front buffer, and return the PluginParent's current front buffer to the plugin to become the plugin's new back-buffer.

That's not quite right. I wrote down some requirements and came up with what I think is the optimal approach: https://wiki.mozilla.org/Gecko:AsyncPluginPainting The key idea is for the plugin code to copy from the front buffer to the back buffer just before it draws into the back buffer --- if it actually needs that copy operation. With that approach, the plugin can control all modifications to the buffers.

Comment on attachment 470769 [details] [diff] [review] reftest.js updated to comment 106, and added smal doc about new functionality ups, something wrong with this reftest.js version.. reftests hang need to check what is going on

Comment on attachment 470458 [details] [diff] [review] reftests api, updated. v2 this seems broken

(In reply to comment #109) > The key idea is for the plugin code to copy from the front buffer to the back > buffer just before it draws into the back buffer --- if it actually needs that > copy operation. A small change could provide an optimization: - CopyArea(front, back, wholePluginRect - area); + CopyArea(front, back, previousArea - area); where previousArea is initially wholePluginRect. (There may also be resizes to consider; I haven't caught up with the initial state of the buffers.) previousArea may often be equal to area, in which case the copy would be a no-op. On X, CopyArea will often be accelerated, so on some systems will happen on the VRAM buffer. Avoiding dirtying the VRAM buffer could save readback into the system memory copy for GetImage.

Ok, I hope I fix all comments from comment 107

returned gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true); back into function OnDocumentLoad... otherwise it does not work. and amount of Wait/Finished not equals...

> Instead of copying the back surface to the front surface in AnswerFlush, why > not rotate ownership of the buffers? Ok, I'll try to update Plugin API and impl patch using swap buffers... but that will take some time, because I need to check buffers life-time, also some mochitests and reftests which are trying to call SetWindow while pending Paint call (back-buffer owned by plugin) e.t.c

Comment on attachment 470940 [details] [diff] [review] rejtest.js Could you describe what this patch does? The commit message in the patch should be descriptive of what the patch is changing, not just the summary of this bug (which isn't particularly related, since this patch is not to nsObjectFrame). And if a revised commit message wouldn't be enough to tell me what you're fixing, could you also describe it in the bug?

Comment on attachment 470940 [details] [diff] [review] rejtest.js >Bug 556487 - Implement layers for plugins nsObjectFrame. r=roc r=dbaron=? Please make the commit message describe the patch. >+// Some layout frames are painting asynchronously, >+// and to make sure that all our frames have right content >+// we are listening fpr "MozPaintWait" and "MozPaintWaitFinished" events >+// if frame(layer) content is dirty then "MozPaintWait" event fired and gExplicitPendingPaintCounter increased >+// when frame fully painted then "MozPaintWaitFinished" fired and gExplicitPendingPaintCounter decreased. >+// reftest snapshot can be taken only when gExplicitPendingPaintCounter == 0 Please wrap lines evenly at 72-75 characters, begin each sentence with a capital letter, and end each sentence with a period. This is nearly impossible to understand since I can't tell where one sentence ends and where the next begins (although I think perhaps you've used line breaks as sentence separators in some cases). >+ if (!gExplicitPendingPaintCounter) >+ gDumpLog("Error: gExplicitPendingPaintCounter going to be < 0!!!\n"); Please use 4-space indent and bracing of all blocks (even if 1 line) throughout. >- if (stopAfterPaintReceived && !utils.isMozAfterPaintPending) { >+ if (stopAfterPaintReceived && !utils.isMozAfterPaintPending && !gExplicitPendingPaintCounter) { Wrap to the next line. >+ function PaintWaitListener() { >+ gExplicitPendingPaintCounter++; >+ } >+ >+ function PaintWaitFinishedListener() { >+ if (!gExplicitPendingPaintCounter) >+ gDumpLog("Error: gExplicitPendingPaintCounter going to be < 0!!!\n"); >+ gExplicitPendingPaintCounter--; >+ if (gExplicitPendingPaintCounter == 0) >+ setTimeout(setTimeout, 0, DocumentLoaded, 0); >+ } >+ gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true); >+ gBrowser.addEventListener("MozPaintWaitFinished", PaintWaitFinishedListener, true); >+ > // Since we can't use a bubbling-phase load listener from chrome, > // this is a capturing phase listener. So do setTimeout twice, the > // first to get us after the onload has fired in the content, and > // the second to get us after any setTimeout(foo, 0) in the content. > setTimeout(setTimeout, 0, DocumentLoaded, 0); This doesn't make sense to me. You're calling DocumentLoaded from two different places. Shouldn't this setTimeout be replaced with something that increments the same reference count, and then a timeout to decrement it? Or do some of the early returns compensate for this? Why do you have so many different listeners for the same events, all manipulating the same counters? Does something ensure that only one set is registered at once? Why not just have a single listener?

Comment on attachment 470918 [details] [diff] [review] Added DOMEventTarget argument for FireDOMEvent method Do we really need this? Can't you just use nsContentUtils::DispatchTrustedEvent?

+ inline PRBool UseLayers(); + // return FALSE if LayerSurface dirty (newly created and don't have valid plugin content yet) + inline PRBool IsPainted(); Take out 'inline' and just declare them in the class. + // return TRUE if surface was succecefully created or + // the same size surface already created if the surface was successfully created or the same size surface was already created + // fixes test_painting.html test don't mention the test, just explain why this is needed + if (!mInstanceOwner->EnsureLayerSurface(nsIntSize(window->width, + window->height), + PR_TRUE)) + window->CallSetWindow(pi); Why are you only calling SetWindow if EnsureLayerSurface fails? I think nsPluginInstanceOwner::IsPainted should be IsUpToDate? As we discussed on IRC, we should move the CallSetWindow from DidReflow to a post-reflow callback, using the snapped plugin rect for the size. At paint time, nsPluginInstanceOwner::CreatePluginLayer should always use the NPWindow size, don't fit to the painted rectangle. + nsCOMPtr<nsIDOMEventTarget> target = do_QueryInterface(mObjectFrame->PresContext()->Document()->GetWindow()); + mObjectFrame->PresContext()->FireDOMEventAtTarget(target, NS_LITERAL_STRING("MozPaintWaitFinished")); Fit to 80 columns and only call PresContext once. I think we should do what I described in comment #109 so we don't have to copy any surfaces in nsObjectFrame.

Ok, I've fixed all comments up to comment#122, and store latest working version into: http://hg.mozilla.org/users/romaxa_gmail.com/layer-patches/file/latest_non_swap_version And started switching to Gecko:AsyncPluginPainting, hope it will not take so long

Comment on attachment 470918 [details] [diff] [review] Added DOMEventTarget argument for FireDOMEvent method yep, we can use DispatchEvent content utils function

reftests patch divided into reftests.js changes, and test_flush_on_paint.html part.

Comment on attachment 472544 [details] [diff] [review] Get XRenderPictFormat method from xlibSurface >+XRenderPictFormat* >+gfxXlibSurface::XRenderFormat() >+{ >+ return cairo_xlib_surface_get_xrender_format(CairoSurface()); >+} I think there should be an mSurfaceValid check before using CairoSurface(). cairo_xlib_surface_get_xrender_format will crash if CairoSurface() is NULL. Otherwise looks good to me, but I'm not a gfx peer, so giving Jeff a chance to comment.

Here is the stable part of nsObjectFrame -> PluginInstance layers API. SetupLayer - async version of SetWindow NPAPI call. RenderLayer - notify PluginInstanceCh/Par about paint happening on screen IsLayerUpToDate - return state of layer, ready for rendering or not GetLayerSurface - return LayerASurface ready for using as ImageLayer gfxSurface.

Ok, here is the objectFrame part, which is mostly looks fine for review

Comment on attachment 472556 [details] [diff] [review] Add Post reflow callback to better first setWindow call with stabilized frame size + // WMP10 needs an additional SetWindow call here (bug 391261) Delete this comment, we need this for all plugins. Let's land this ASAP. Get yourself added to the landing queue.

Comment on attachment 472567 [details] [diff] [review] Ajust plugin size and reduse amount of SetWindow calls due to pixel snap Actually, we should set window->x and window->y this way as well. GetWindowOriginInPixels is utterly bogus for windowed plugins anyway, and for windowless what we get here should correspond to what we got from GetWindowOriginInPixels (or be more correct).

SetupLayer - async version of SetWindow NPAPI call. RenderLayer - notify PluginInstanceCh/Par about paint happening on screen IsLayerUpToDate - return state of layer, ready for rendering or not GetLayerSurface - return LayerASurface ready for using as ImageLayer gfxSurface. These (or more!) should be comments in the code! Do we need IsLayerUpToDate? Now that we always have something to paint after the first paint, can we just call GetLayerSurface and if we get null, then nothing has been painted yet? I'm not sure why we need RenderLayer.

> I'm not sure why we need RenderLayer. We should notify pluginChild that previous paint/Flush surface has been rendered to the screen, and that plugin can resume rendering and generate next frame.

(In reply to comment #135) > > I'm not sure why we need RenderLayer. > We should notify pluginChild that previous paint/Flush surface has been > rendered to the screen, and that plugin can resume rendering and generate next > frame. Why? Can't the plugin just generate frames at whatever rate it wants? Or would that be bad for performance?

At least we don't need to pass a rectangle, right?

> Why? Can't the plugin just generate frames at whatever rate it wants? Or would > that be bad for performance? yep. (In reply to comment #138) > At least we don't need to pass a rectangle, right? right. that unnecessary I was keeping rect just in case.. but seems there are no case... need to kill that, and find some proper name for that method

Comment on attachment 472544 [details] [diff] [review] Get XRenderPictFormat method from xlibSurface What is this api used for? I didn't see it used in any of the patches?

> What is this api used for? I didn't see it used in any of the patches? this is used in plugin API part which is under development right now. and not in attached patches . http://hg.mozilla.org/users/romaxa_gmail.com/layer-patches/file/896310c1f35f/07_plugin_api_part.diff That is needed for efficient sharing XID Drawables between processes

> What is this api used for? I didn't see it used in any of the patches? Also this is used in remote shadow layers imp http://hg.mozilla.org/users/cjones_mozilla.com/mcmq/file/45ec73998b2f/570625-surfdescx11#l147

http://hg.mozilla.org/mozilla-central/rev/ e28be11c3dc8 -reftests, test_flush_on_paint.html fix 9cec62d02121 - Snap plugin rect to integer pixels. be4524bc2416 - Post Reflow Callback for nsObjectFrame 054ae16dcc13 - gfxASurface GetSize

Comment on attachment 472931 [details] [diff] [review] Plugin child/parent/PluginInstance API part no implementation Just PluginHost <-> nsObjectFrame API

+PluginInstanceParent::GetLayerSurface(gfxASurface* *aSurface) Keep the ** together + * @param aSurface - pointer to gfxASurface There is no surface parameter. + * @param aWindow - the plugin window structure + */ + void setupLayer(in NPWindowPtr aWindow); Actually, I don't see what setupLayer does other than an async SetWindow. Maybe call it asyncSetWindow? + * Call this when plugin layer painted to the screen "Call this each time after the plugin has been painted to the screen" + * This should return valid gfxASurface pointer ready for image layer use "This should return a valid gfxASurface pointer, or null if there is nothing to render yet." What about the size? Is the size always the size passed in in the last setupLayer? Or should we be passing back a size, since gfxASurface doesn't always have a way to get the size?

I kinda wish gfxASurface had a GetSize method, which just returned -1,-1 or something like that for the few cases where we don't know the size...

Oops, I see you just added that :-).

+ /** + * SetupPlugin layer environment, call SetWindow and init + * child plugin backbuffer in case of IPC layers + * + * @param aWindow - the plugin window structure + */ + void setupLayer(in NPWindowPtr aWindow); Shouldn't this have been updated to AsyncSetWindow? And fix the comment to just say it's an async version of setWindow, to be used with async rendering. + /** + * Call this each time after the plugin has been painted to the screen + */ + void notifyLayerPainted(); Let's just call this notifyPainted(). + /** + * This should return a valid gfxASurface pointer, or null if there is nothing to render yet. + */ + void getLayerSurface(out gfxASurfacePtr aSurface); Let's just call this getSurface. I think we shouldn't mention layers here. The plugin library is not really dealing with layers, just surfaces. I suspect though that having nsObjectFrame call getSurface to get a surface and then set it as the CairoImage for a layer's ImageContainer isn't quite right. https://wiki.mozilla.org/Gecko:AsyncPluginPainting assumes that the plugin can change the current surface by calling Show, and that after Show is returned the host no longer uses the old surface. I think probably nsObjectFrame needs to pass the ImageContainer down to the plugin library, and the plugin code needs to set the ImageContainer's Image whenever Show is received from the plugin. Make sense?