Closed Bug 556487 Opened 10 years ago Closed 9 years ago

Implement layers for plugins nsObjectFrame

Categories

(Core :: Plug-ins, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7

People

(Reporter: romaxa, Assigned: romaxa)

References

Details

Attachments

(17 files, 71 obsolete files)

8.28 KB, patch
joe
: review+
Details | Diff | Splinter Review
1.99 KB, patch
karlt
: review+
jrmuizel
: review+
Details | Diff | Splinter Review
1.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.61 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.18 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
11.30 KB, patch
cjones
: review+
roc
: review+
Details | Diff | Splinter Review
26.88 KB, text/html
Details
838 bytes, patch
roc
: review+
Details | Diff | Splinter Review
7.94 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
34.94 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.86 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.36 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.89 KB, patch
roc
: review+
Details | Diff | Splinter Review
We should create simple layers implementation for plugins, and make it works at least for NPP_DrawImage NPAPI, and probably for HandleEvent.
Attached patch Plugin layers imageSurface WIP1 (obsolete) — Splinter Review
Assignee: nobody → romaxa
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?
Attachment #436652 - Flags: review?(roc)
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.
Attached patch New wip (obsolete) — Splinter Review
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)
Attachment #436652 - Attachment is obsolete: true
Attachment #440216 - Flags: review?(roc)
Attachment #436652 - Flags: review?(roc)
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
Depends on: 567065
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?
Attachment #440216 - Attachment is obsolete: true
Attachment #440216 - Flags: review?(roc)
Attachment #446512 - Flags: feedback?(b56girard)
Attachment #446512 - Flags: feedback?(b56girard) → 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.
Attachment #446512 - Attachment is obsolete: true
Attached patch MAke it works with GTK port (obsolete) — Splinter Review
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
Attachment #446759 - Attachment is obsolete: true
Attached patch Ups, this is the right patch (obsolete) — Splinter Review
Attachment #446796 - Attachment is obsolete: true
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.
Attachment #446797 - Attachment is obsolete: true
I think this is more clear implementation, less X dependencies, easier porting to another platforms.
Attached patch Updated version (obsolete) — Splinter Review
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.
Attachment #446970 - Attachment is obsolete: true
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)
Attached patch Updated patch, need feedback (obsolete) — Splinter Review
Fixed rendering to gfxImageSurface (default offscreen)
Need feedback about current hacks (see comments in patch)
Attachment #446968 - Attachment is obsolete: true
Attachment #447067 - Attachment is obsolete: true
Attachment #447149 - Attachment is obsolete: true
Attachment #447383 - Flags: feedback?(roc)
Attachment #447383 - Flags: feedback?(karlt)
Comment on attachment 447383 [details] [diff] [review]
Updated patch, need feedback

Added common methods for shmem and nativeID (XID/IOSurface) types
Attachment #447383 - Flags: feedback?(b56girard)
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).
Attachment #447383 - Flags: feedback?(karlt)
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.
Attachment #447383 - Attachment is obsolete: true
Attachment #447824 - Attachment is obsolete: true
Attachment #447383 - Flags: feedback?(roc)
Attachment #447383 - Flags: feedback?(b56girard)
Attachment #447828 - Attachment is obsolete: true
Attachment #452925 - Attachment is obsolete: true
Attachment #452926 - Attachment is obsolete: true
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
Status: NEW → ASSIGNED
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.
blocking2.0: --- → ?
> 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.
blocking2.0: ? → beta4+
(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+ → ---
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
blocking2.0: --- → ?
blocking2.0: ? → beta4+
(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
Attached patch 546071 fail result (obsolete) — Splinter Review
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.
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...
Attachment #461538 - Flags: feedback?(roc)
(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).
Blocks: 586162
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.
blocking2.0: beta4+ → beta5+
Attached patch Better version works for cases (obsolete) — Splinter Review
Attachment #465365 - Attachment is obsolete: true
This version mostly working everywhere and using new API.
Still need to accumulate feedback about reftests and fix it according to comments.
Attachment #465563 - Attachment is obsolete: true
Attached patch v5 async reftest with autowait (obsolete) — Splinter Review
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.
Attachment #465670 - Attachment is obsolete: true
Attachment #465867 - Flags: feedback?(roc)
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.
Attached patch Green tests version (obsolete) — Splinter 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?
Attachment #460979 - Attachment is obsolete: true
Attachment #461538 - Attachment is obsolete: true
Attachment #461543 - Attachment is obsolete: true
Attachment #465867 - Attachment is obsolete: true
Attachment #465867 - Flags: feedback?(roc)
Attachment #467268 - Flags: feedback?(joshmoz)
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.
Attachment #467268 - Flags: feedback?(karlt)
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)
Attachment #467268 - Attachment is obsolete: true
Attachment #467268 - Flags: feedback?(karlt)
Attachment #467268 - Flags: feedback?(joshmoz)
Blocks: 583135
Attached patch v1 (obsolete) — Splinter Review
Attachment #467965 - Attachment is obsolete: true
Attachment #468112 - Flags: review?(roc)
Attachment #468112 - Flags: feedback?(karlt)
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
Blocks: 581725
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?
Blocks: 589389
Attached patch Fixed comments (obsolete) — Splinter Review
> +  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...
Attachment #468112 - Attachment is obsolete: true
Attachment #468757 - Flags: review?(roc)
Attachment #468112 - Flags: review?(roc)
Attachment #468112 - Flags: feedback?(karlt)
(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?
Duplicate of this bug: 591174
Attached patch v4. (obsolete) — Splinter Review
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.
Attachment #468757 - Attachment is obsolete: true
Attachment #469805 - Flags: review?(roc)
Attachment #468757 - Flags: review?(roc)
Blocks: 588591
Feature freeze is now beta6, so pushing this out.
blocking2.0: beta5+ → beta6+
Comment on attachment 469805 [details] [diff] [review]
v4.

Ok, I'll try to separate API's, implementation, and usage parts for easier review
Attachment #469805 - Flags: review?(roc)
Attached patch gfxUtils for plugin layers (obsolete) — Splinter Review
Attachment #469805 - Attachment is obsolete: true
Attachment #470248 - Flags: review?(roc)
Attachment #470253 - Flags: review?(roc)
Attachment #470253 - Flags: review?(roc)
Attachment #470253 - Attachment is obsolete: true
Attachment #470256 - Flags: review?(roc)
Attached patch ObjectFrame part (obsolete) — Splinter Review
Attachment #470258 - Flags: review?(roc)
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...
Attachment #470248 - Attachment is obsolete: true
Attachment #470457 - Flags: review?
Attachment #470248 - Flags: review?(roc)
Attachment #470457 - Flags: review? → review?(joe)
Attached patch reftests api, updated. v2 (obsolete) — Splinter Review
Attachment #470249 - Attachment is obsolete: true
Attachment #470458 - Flags: review?(roc)
Attachment #470249 - Flags: review?(roc)
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?
Attachment #470457 - Flags: review?(joe) → review-
New bug created:
https://bugzilla.mozilla.org/show_bug.cgi?id=592037
Attachment #470457 - Attachment is obsolete: true
Attachment #470542 - Flags: review?
Attachment #470542 - Flags: review? → review?(joe)
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.
Attachment #470542 - Flags: review?(joe) → review+
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.
Attachment #470458 - Flags: review?(roc) → review+
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
Attachment #470769 - Flags: review?(dbaron)
Comment on attachment 470458 [details] [diff] [review]
reftests api, updated. v2

this seems broken
Attachment #470458 - Flags: review+
Blocks: 552512
Blocks: 591409
(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.
Attachment #470458 - Attachment is obsolete: true
Attachment #470769 - Attachment is obsolete: true
Attachment #470918 - Flags: review?(roc)
Attached patch Updated for comment 107 (obsolete) — Splinter Review
Ok, I hope I fix all comments from comment 107
Attachment #470256 - Attachment is obsolete: true
Attachment #470511 - Attachment is obsolete: true
Attachment #470922 - Flags: review?(roc)
Attachment #470256 - Flags: review?(roc)
Attachment #470511 - Flags: review?(roc)
Attached patch rejtest.js (obsolete) — Splinter Review
returned
gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true);

back into function OnDocumentLoad... otherwise it does not work. and amount of Wait/Finished not equals...
Attachment #470940 - Flags: review?(dbaron)
Attachment #470940 - Flags: review?(roc)
> 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?
Attachment #470940 - Flags: review?(dbaron) → review-
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?
Attached patch Fixed comment 119 (obsolete) — Splinter Review
Attachment #470940 - Attachment is obsolete: true
Attachment #471018 - Flags: review?(dbaron)
Attachment #470940 - Flags: review?(roc)
+  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
No longer blocks: 589389
No longer blocks: 583135
Comment on attachment 470918 [details] [diff] [review]
Added DOMEventTarget argument for FireDOMEvent method

yep, we can use DispatchEvent content utils function
Attachment #470918 - Flags: review?(roc)
Depends on: 592866
reftests patch divided into reftests.js changes, and test_flush_on_paint.html part.
Attachment #470918 - Attachment is obsolete: true
Attachment #472547 - Flags: review?(roc)
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.
Attachment #472544 - Flags: review?(karlt)
Attachment #472544 - Flags: review?(jmuizelaar)
Attachment #472544 - Flags: review+
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.
Attachment #472569 - Flags: review?(roc)
Attached patch ObjectFrame part. (obsolete) — Splinter Review
Ok, here is the objectFrame part, which is mostly looks fine for review
Attachment #470922 - Attachment is obsolete: true
Attachment #472681 - Flags: review?(roc)
Attachment #470922 - Flags: review?(roc)
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.
Attachment #472556 - Flags: review?(roc) → review+
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).
Attachment #472567 - Flags: review+ → review-
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.
Attachment #472567 - Attachment is obsolete: true
Attachment #472863 - Flags: review?(roc)
(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?
Attachment #472569 - Attachment is obsolete: true
Attachment #472569 - Flags: review?(roc)
> 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
Attachment #472544 - Flags: review?(jmuizelaar) → review+
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
Attachment #472931 - Flags: review?(roc)
Attachment #472931 - Flags: review?(joshmoz)
Attachment #472681 - Attachment is obsolete: true
Attachment #473382 - Flags: review?(roc)
Attachment #472681 - Flags: review?(roc)
+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 :-).
Attachment #472931 - Attachment is obsolete: true
Attachment #473421 - Flags: review?(roc)
Attachment #472931 - Flags: review?(roc)
Attachment #472931 - Flags: review?(joshmoz)
Attachment #473424 - Flags: review?(karlt)
+    /**
+     * 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?
Attachment #473421 - Attachment is obsolete: true
Attachment #473610 - Flags: review?(roc)
Attachment #473421 - Flags: review?(roc)
Attachment #473664 - Flags: review?(jones.chris.g)
Attachment #473382 - Attachment is obsolete: true
Attachment #473667 - Flags: review?(roc)
Attachment #473382 - Flags: review?(roc)
(In reply to comment #156)
> Created attachment 473667 [details] [diff] [review]
> nsObjectFrame v4, added SetCurrentImage immideately after AnswerSwap

This looks like the wrong patch; no changes to nsObjectFrame here
Attachment #473667 - Attachment is obsolete: true
Attachment #473738 - Flags: review?(roc)
Attachment #473667 - Flags: review?(roc)
Attached patch Reftest fix 580160-1 (obsolete) — Splinter Review
Same as border_padding reftest... wait for paint after plugin resize....
Another way to fix this and border_padding test is to detect globally that we are in reftest system mode... and somehow teach plugin fire "MozPaintWait" event to layout
Attachment #473744 - Flags: review?(roc)
IsUpToDate check also layer size.
If layer size != NPWindow size then it is old, and we should fire MozPaintWait event
Attachment #473738 - Attachment is obsolete: true
Attachment #473744 - Attachment is obsolete: true
Attachment #473800 - Flags: review?(roc)
Attachment #473738 - Flags: review?(roc)
Attachment #473744 - Flags: review?(roc)
+  if (mInstanceOwner->UseLayers())
+    pi->AsyncSetWindow(window);
+  else
+    window->CallSetWindow(pi);

{}

Instead of GetCairoImage returning an Image, just have it call SetCurrentImage on the container and you don't need to return the image. It should probably return a boolean to say if we got a surface though (see below).

+  nsRect area = GetContentRect() + aBuilder->ToReferenceFrame(GetParent());

You need to add aItem->ToReferenceFrame()

+  nsPresContext* presContext = PresContext();
+  gfxRect r = gfxRect(presContext->AppUnitsToGfxUnits(area.x),
+                      presContext->AppUnitsToGfxUnits(area.y),
+                      presContext->AppUnitsToGfxUnits(area.width),
+                      presContext->AppUnitsToGfxUnits(area.height));

Use nsLayoutUtils::RectToGfxRect()

+  if (!layer || layer->GetType() != Layer::TYPE_IMAGE) {

You should just be able to assert that the type is correct. We aren't going to create any other kind of layer for a plugin.

+    nsRefPtr<ImageLayer> imglayer = static_cast<ImageLayer*>(layer.get());

Doesn't need to be an nsRefPtr since we're already holding a ref earlier

+  if (pi)
+    pi->NotifyPainted();

{}

+  // We don't want to see dirty plugin layer surface content
+  nsRefPtr<gfxASurface> layerSurface;
+  pi->GetSurface(getter_AddRefs(layerSurface));
+  if (!layer || !layerSurface)
+    return nsnull;

Why don't we move the NotifyPainted call further up and return nsnull earlier if !layer and then again if mInstanceOwner->GetCairoImage fails?

+  if (IsOpaque())
+    layer->SetContentFlags(layer->GetContentFlags() |
+                           Layer::CONTENT_OPAQUE);

{}

If IsOpaque changes, this will leave CONTENT_OPAQUE in the flags. Instead, just call SetContentFlags(IsOpaque() ? Layer::CONTENT_OPAQUE : 0)

+  transform.Scale(r.Width() / layerSurface->GetSize().width,
+                  r.Height() / layerSurface->GetSize().height);
+  layer->SetTransform(gfx3DMatrix::From2D(transform));
+  nsRefPtr<Layer> result = layer.forget();
+  return result.forget();

Personally I think we shouldn't scale to fit. I think we should center the layer or something like that. Scaling to fit will mean that for a moment the plugin image will be smeared out a bit and that will also be slower.

+  // Plugin Swap always calling InvalidateRect and we should
+  // notify reftests about paint finished, and update ImageContainer
+  // And we do it this way, because there are
+  // no legal way to generate DOM event from PluginInstanceParent or make call to nsObjectFrame

I think "Show" is a better name than "Swap" for this. I would just say
// Each time an asynchronously-drawing plugin sends a new surface to display,
// InvalidateRect is called. We notify reftests that painting is up to
// date and update our ImageContainer with the new surface.

+  // We must remove container image and frontSurface reference
+  // that we can return safetly front surface to plugin child

I'd just say
// Drop image reference because the child may destroy the surface after we return.
Comment on attachment 473424 [details] [diff] [review]
Don't crash silently if visual == 0

A null visual during painting indicates a bug in the browser AFAIK.

It's unfortunate that we do pass null visuals sometimes, but we should be giving the plugin a good visual before it needs to use it for the paint.

If we don't have a visual to give the plugin at this point, then we shouldn't be asking the plugin to paint.
Attachment #473424 - Flags: review?(karlt) → review-
+    const char *surfaceTypeOverride = PR_GetEnv("PL_SURF_TYPE");
+    if (surfaceTypeOverride && *surfaceTypeOverride)
+        sSurfacePrefferedType = (gfxASurface::gfxSurfaceType)atoi(surfaceTypeOverride);

Are we actually going to ship with this?

+static gfxASurface::gfxSurfaceType sSurfacePrefferedType = gfxASurface::SurfaceTypeMax;

Try to avoid static initializers. Also, "preferred".

+    if (mFrontSurface) {
+#ifdef MOZ_X11
+        if (mFrontSurface->GetType() == gfxASurface::SurfaceTypeXlib) {
+            gfxXlibSurface *xsurf = static_cast<gfxXlibSurface*>(mFrontSurface.get());
+            *rtnsurf = SurfaceDescriptorX11(xsurf->XDrawable(), xsurf->XRenderFormat()->id,
+                                            mFrontSurface->GetSize().width,
+                                            mFrontSurface->GetSize().height);
+        } else

What is rtnsurf for? With https://wiki.mozilla.org/Gecko:AsyncPluginPainting you don't need it.

+    } else if (mPendingPaintRequest) {
+        mPendingPaintRequest = PR_FALSE;
+        rv = SendFlushFinished();

I'd call this "SendPaintFinished". Actually I don't know why we can't have PaintFinished and ForcePaint be the same method. The plugin knows if the size has changed and if it's sent a buffer before.

+    // Cached plugin transparent mode value
+    PRPackedBool          mIsTransparent;

Why do we need to cache this? Seems like we could just ask the plugin every time. That would allow for dynamic changes.

+    // Params of pending AsyncSetWindow call
+    gfxASurface::gfxSurfaceType        mSurfaceType;
+    gfxASurface::gfxContentType        mContentType;

You mean params of *last* AsyncSetWindow call?

+    PRPackedBool          mDoAlphaExtraction;

What's this for?

+    CancelableTask       *mCurrentInvalidateTask;

What's this?

In PluginInstanceChild::CreateOptSurface, for MOZ_X11, can't you just call CreateSimilarSurface?

+    bool EnsureFallbackSurfaceFor(gfxASurface* aSurface);

What's this for? I'm not sure what "Fallback" means here. In general I think we need more documentation about how the plugin side works.
(In reply to comment #164)
> +static gfxASurface::gfxSurfaceType sSurfacePrefferedType =
> gfxASurface::SurfaceTypeMax;
> 
> Try to avoid static initializers.

Never mind, that initializer is fine.
(In reply to comment #162)
> You need to add aItem->ToReferenceFrame()

No, I was being stupid. Never mind.
Attachment #473800 - Attachment is obsolete: true
Attachment #473843 - Flags: review?(roc)
Attachment #473800 - Flags: review?(roc)
+  if (container)
+    SetCurrentImage(container);

{}

One more thing: you need to override nsDisplayPlugin::GetBounds to return the bounds of the layer, if we're painting using layers.
Comment on attachment 473745 [details] [diff] [review]
Final part, plugin child/parent implementation v2, swap

>+PluginInstanceChild::CreateOptSurface(gfxASurface::gfxSurfaceType& aType,

>+        XSync(dpy, False);

Only one XSync is needed before access returns to the other process.

This XSync is unnecessary because it will be followed by
PaintRectWithAlphaExtraction or PaintRectToSurface, which call XSync,
before CallSwap.

Sometimes PaintRectToSurface is called when there is no need to sync.  I'm not
sure we care to much about that case, but the point of SurfaceDescriptorX11
construction would be a logical single place to call XSync.

>+        Visual* visual = gfxXlibSurface::FindVisual(DefaultScreenOfDisplay(dpy), format);
>+        if (!visual) {
>+            NS_ERROR("Need X falback surface, but visual failed");

There won't be an ARGB32 visual on displays without the Composite extension.
I'm guessing FindRenderFormat is what you want here.
(Falling back to image surfaces is also an option, but Xlib surfaces may be
better for browser composites.)

In EnsureFallbackSurfaceFor, colormap is set but not used.
The XSync in EnsureFallbackSurfaceFor is unnecessary because mFallBackSurface
is not passed between processes.

>+PluginInstanceChild::EnsureFallbackSurfaceFor

>+    return mFallBackSurface ? true : false;

>+bool
>+PluginInstanceChild::EnsureCurrentBuffer()

>+    return EnsureFallbackSurfaceFor(mCurrentSurface);

>+bool
>+PluginInstanceChild::PaintRectWithFlush()

>+    if (!EnsureCurrentBuffer())
>+        return false;

IIUC, when a fallback surface is not required, EnsureFallbackSurfaceFor
returns false and PaintRectWithFlush does nothing?
Attachment #473664 - Attachment is obsolete: true
Attachment #473916 - Flags: review?(roc)
Attachment #473664 - Flags: review?(jones.chris.g)
+  // Plugin parent notify child that previous Invalidate has been painted to the screen
+  // and plugin-child can start next backsurface update
+  // @param forceRepaint - initiate force plugin repaint
+  async PaintFinished(bool forceRepaint);

Why is forceRepaint needed? As I said above, the plugin always knows whether it has painted before and whether the size has changed.

(In reply to comment #139)
> > Why? Can't the plugin just generate frames at whatever rate it wants? Or would
> > that be bad for performance?
> yep.

Thinking again, I don't actually understand this reason. Is the idea to have the plugin repaint at the same rate as the browser? I think for now it might be easier to not try to control that. Instead, let the plugin produce frames at whatever frame rate it wants. Hopefully the browser can keep up.

+  // Child send to parent surface with valid content
+  // if parent has surface received previous swap
+  // then parent should return that surface back to child
+  // @param rect - backbuffer rectangle which need to be flushed to front buffer (speedup compositing)
+  // @param newSurface - remotable surface
+  // @param rtnsurface - return surface for recycling
+  rpc Swap(NPRect rect, SurfaceDescriptor newSurface)
+    returns (SurfaceDescriptor rtnsurface);

Why is 'rect' needed? I thought the parent doesn't copy any buffers anymore.

You said that rtnsurface is not needed for X, but is needed for shared memory. I'm still not sure why it's needed for shared memory. As I outlined on the wiki page, we shouldn't need to explicitly return any buffers here; the plugin just knows that after Swap (which I think would be better called Show) has returned, the previous buffer is no longer being used by the host.
Attached patch GetBounds correction to center (obsolete) — Splinter Review
Attachment #473424 - Attachment is obsolete: true
Attachment #473964 - Flags: review?(roc)
Comment on attachment 473916 [details] [diff] [review]
Plugin Child/Parent async API (no implementation) Removed force paint method

Drive-by review, I guess ;).

>diff --git a/dom/plugins/PPluginInstance.ipdl b/dom/plugins/PPluginInstance.ipdl
>--- a/dom/plugins/PPluginInstance.ipdl
>+++ b/dom/plugins/PPluginInstance.ipdl
>+struct SurfaceDescriptorX11 {
>+  int xID;
>+  int pictID;
>+  int width;
>+  int height;
>+};
>+

You can define this in C++ (PLayers does), no particular benefit to
defining this as an IPDL struct except auto-serialization, I guess.
Prefer gfxIntSize or nsIntSize to width/height.

>+union SurfaceDescriptor {
>+  null_t;
>+  Shmem;
>+  SurfaceDescriptorX11;
>+};

Please document why a surface descriptor can be null.

>+  // ********************** Async plugins rendering
>+  // see https://wiki.mozilla.org/Gecko:AsyncPluginPainting
>+  // **********************
>+
>+  // Plugin parent notify child that previous Invalidate has been painted to the screen
>+  // and plugin-child can start next backsurface update
>+  // @param forceRepaint - initiate force plugin repaint
>+  async PaintFinished(bool forceRepaint);
>+

I'm afraid I haven't been following this bug closely, but it's not
clear to me why we need this notification.  I assume the intention is
to try to use the browser's refresh driver to throttle Swap()s sent by
the plugin process?  You're not passing this notification along to the
plugin itself?  If so, why not throttle the Swap()s to the
refresh-driver Hz in the plugin process itself?

>+  // Async version of SetWindow call
>+  // @param surfaceType - gfxASurface::gfxSurfaceType
>+  //        plugin child must create offscreen buffer
>+  //        with type equals surfaceType
>+  async AsyncSetWindow(int surfaceType, NPRemoteWindow window);
>+

Please add a validating serializer for gfxSurfaceType in
ipc/glue/IPCMessageUtils.h rather than sending raw |int|.  See the
serializer for gfxPattern::GraphicsFilter in IPCMessageUtils.h

>+  // Child send to parent surface with valid content
>+  // if parent has surface received previous swap
>+  // then parent should return that surface back to child

Maybe

 // Give |newSurface|, containing this instance's updated pixels, to
 // the browser for compositing.  Get back |prevSurface|, containing
 // old pixels, to be recycled.

>+  // @param rect - backbuffer rectangle which need to be flushed to front buffer (speedup compositing)
>+  // @param newSurface - remotable surface
>+  // @param rtnsurface - return surface for recycling
>+  rpc Swap(NPRect rect, SurfaceDescriptor newSurface)
>+    returns (SurfaceDescriptor rtnsurface);
>+

I prefer the name |prevSurface|, but don't care overmuch.

Are you using |rect| to invalidate the minimum area in the
nsObjectFrame or are you actually using it to copy
newSurface->prevSurface?  The former sounds like a potentially good
idea, but I don't think you'll gain much from the copy new->prev.
Remember too that this copy would be on the browser main thread, so it
would block the processing of UI events etc.
(In reply to comment #171)
> > > Why? Can't the plugin just generate frames at whatever rate it wants? Or would
> > > that be bad for performance?
> > yep.
> 
> Thinking again, I don't actually understand this reason. Is the idea to have
> the plugin repaint at the same rate as the browser? I think for now it might be
> easier to not try to control that. Instead, let the plugin produce frames at
> whatever frame rate it wants. Hopefully the browser can keep up.

Currently we don't ask plugins to paint when the window is minimized, they are in a hidden tab or they are scrolled offscreen.  AFAIK, if we don't do something to limit painting, we'll have a regression in the amount of CPU used by hidden plugins.
Disable GetBounds centering for non-layer mode.
Also fixed bounds correction when layer size == 0
Attachment #473964 - Attachment is obsolete: true
Attachment #474072 - Flags: review?(roc)
Attachment #473964 - Flags: review?(roc)
Attached patch child/parent async API v3. (obsolete) — Splinter Review
Fixed roc comments, and most of cjones comments.
gfxIntSize added to messagetils, SurfaceDescriptorX11 still not native.
Show(updatedRect - can explain to layerManager/widget which part of layer has really new pixels, and widget can do partial update/paint
Attachment #473916 - Attachment is obsolete: true
Attachment #474089 - Flags: review?(jones.chris.g)
Attachment #473916 - Flags: review?(roc)
sorry, wrong enum type
Attachment #474089 - Attachment is obsolete: true
Attachment #474128 - Flags: review?(jones.chris.g)
Attachment #474089 - Flags: review?(jones.chris.g)
Fixed typo
-  r.pos.x = (r.Width() - container->GetCurrentSize().width) / 2;
+  r.pos.y += (r.Height() - container->GetCurrentSize().height) / 2;
and style from comment 168
Attachment #473843 - Attachment is obsolete: true
Attachment #474160 - Flags: review?(roc)
Attachment #473843 - Flags: review?(roc)
Attachment #474128 - Attachment is obsolete: true
Attachment #474163 - Flags: review?(jones.chris.g)
Attachment #474128 - Flags: review?(jones.chris.g)
Comment on attachment 474163 [details] [diff] [review]
rpc->sync, child/parent async API v5

roc check it also plsz
Attachment #474163 - Flags: review?(roc)
Attachment #474163 - Flags: review?(jones.chris.g) → review+
Ok, Fixed all roc and karl comments.
mIsTransparent still in use, because plugins don't remember their values and getvalue return always transparent false...
Attachment #473745 - Attachment is obsolete: true
Attachment #474221 - Flags: review?(roc)
Attachment #473745 - Flags: review?(roc)
This test page makes current firefox scrolling almost impossible.
But with this bug fixed scrolling works always
Comment on attachment 474072 [details] [diff] [review]
ObjectFrame GetBounds v.2

+    if (size.width && size.height) {

I don't think you want to check this. If we happen to have a zero-size image, that's what we'll paint.
Attachment #474072 - Flags: review?(roc) → review+
Comment on attachment 474160 [details] [diff] [review]
nsObjectFrame v7 Fixed all comments and typo

+  if (mInstanceOwner->UseLayers())
+    pi->AsyncSetWindow(window);
+  else
+    window->CallSetWindow(pi);

Add the {}
Attachment #474160 - Flags: review?(roc) → review+
Comment on attachment 474163 [details] [diff] [review]
rpc->sync, child/parent async API v5

+    case gfxASurface::SurfaceTypeImage:
+    case gfxASurface::SurfaceTypePDF:
<etc>

Why do we need all that? Why can't we just cast to int32 in all cases? r=me if you just get rid of the unnecessary switch.
Attachment #474163 - Flags: review?(roc) → review+
+    // This must create surface as close as possible to aType and aFormat.

These aren't parameters here. I guess you just mean mSurfaceType?

+        return mCurrentSurface ? PR_TRUE : PR_FALSE;

return mCurrentSurface != nsnull;

+    bool PaintRectWithFlush(void);
+    bool PaintRectWithAlphaExtraction(const gfxRect& aRect, gfxASurface* aSurface);
+    bool PaintRectToSurface(const gfxRect& aRect, gfxASurface* aSurface);
+    void AsyncPaintRectWithFlush(PRUint32 aMsec = 0);
+    bool DrawToImageUsingHelperSurface(const gfxRect& aRect,
+                                       gfxImageSurface* aImage,
+                                       float aColor);
+    PRBool UpdateWindowAttributes(PRBool aForceSetWindow = PR_FALSE);
+    void InvalidateRectDelayed(void);
+    PRBool CreateOptSurface(void);
+
+    bool EnsureCurrentBuffer(void);
+    bool EnsureHelperSurfaceFor(gfxASurface* aSurface);

These need documentation.

I think aRect in all these methods should be a const nsIntRect&, since it is always integers. A simple helper function to convert an nsIntRect to a gfxRect would be helpful when you need to pass the rect to Thebes.

+    // X - plugins does not support transparent rendering with alpha,
+    // also there is no API to render directly to image buffer
+    // To make it possible and don't make composition on parent process slower
+    // we create cached temporary surface and use it as temporary buffer
+    // for rendering transparent plugins and rendering to image surface

This comment should be improved. How about
  // On some platforms, plugins may not support rendering to a surface with
  // alpha, or not support rendering to an image surface.
  // In those cases we need to draw to a temporary platform surface; we cache
  // that surface here.

+        // Check if we need can create helper surface with non-default visual

delete "need"

+    PRBool haveHelper = EnsureHelperSurfaceFor(mCurrentSurface) && mHelperSurface;

If EnsureHelperSurfaceFor is non-null, mHelperSurface can't be null, so just assert that.

+    clipRect.x = mWindow.x;
+    clipRect.y = mWindow.y;
+    clipRect.width = mWindow.width;
+    clipRect.height = mWindow.height;

Better to do clipRect = nsIntRect(mWindow.x, ...);

+    PRBool winUpdated = UpdateWindowAttributes();
+
+

Stray blank line

+    mPendingPluginCall = PR_TRUE;
+    if (mPluginIface->setwindow)
+        mPluginIface->setwindow(&mData, &mWindow);
+    mPendingPluginCall = PR_FALSE;

Should we assert !mPendingPluginCall before we set it to true? I'm not really sure what situation it's guarding against.

Why doesn't the xlib-surface path of PaintRectToSurface need to clear the surface before asking the plugin to draw into it? Don't we need to do that, if the plugin is transparent and we've got an RGBA visual?

+        // Render using temporary X surface, with copy to image surface
+        if (mIsTransparent) {
+            // Clear transparent plugins surface

I think you should check the content type of aSurface instead of mIsTransparent.

+            nsRefPtr<gfxContext> ctx = new gfxContext(mHelperSurface);
+            ctx->SetOperator(gfxContext::OPERATOR_CLEAR);
+            ctx->Rectangle(aRect);
+            ctx->Clip();
+            ctx->Fill();

Don't clip, just fill.

+        gfxIntSize clipSize(aRect.Width(), aRect.Height());

clipSize is not used.

+        gfxRect plPaintRect(0, 0, aRect.XMost(), aRect.YMost());
+        if (!PaintRectToSurface(mIsTransparent ? plPaintRect : aRect, mHelperSurface))

Why are you passing 0,0 as the top-left for transparent plugins? If we're working around a Flash bug, say so. (And the workaround should only happen on X11 if that's where the bug is.)

+            return false;
+        nsRefPtr<gfxContext> ctx = new gfxContext(aSurface);
+        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
+        ctx->SetSource(mHelperSurface);
+        ctx->Rectangle(aRect, PR_TRUE);
+        ctx->Clip();
+        ctx->Paint();

Why not just Fill()?

+PluginInstanceChild::DrawToImageUsingHelperSurface(const gfxRect& aRect,
+                                                   gfxImageSurface* aImage,
+                                                   float aColor)

A float isn't a color, so I think an nscolor or a const gfxRGBA& would be better here.

+    ctx->SetColor(gfxRGBA(aColor, aColor, aColor));
+    ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
+    ctx->Rectangle(aRect, PR_TRUE);
+    ctx->Clip();
+    ctx->Paint();

Again, this can just be Fill.

+    if (!PaintRectToSurface(gfxRect(0, 0, aRect.XMost(), aRect.YMost()),
+                            mHelperSurface))

Again, why not aRect?

It's probably worth separating out the "Paint to platform surface" functionality of PaintRectToSurface, say into PaintRectToPlatformSurface. Then PaintRectToSurface would call that along two different paths, instead of calling itself recursively. DrawToImageUsingHelperSurface would also call that. This would make the code clearer.

The non-Xlib-surface code path in PaintRectToSurface seems like it should be outside the #ifdef MOZ_X11.

+    ctxImg->SetSource(mHelperSurface, -aRect.pos);
+    ctxImg->SetOperator(gfxContext::OPERATOR_SOURCE);
+    ctxImg->Paint();
+    return true;

Need a comment somewhere, maybe in header for DrawToImageUsingHelperSurface, that this function assumes aImage is the size of aRect, so we're extracting a subrectangle of the plugin into aImage.

Actually I'd like to remove DrawToImageUsingHelperSurface and have it just call PaintRectToSurface, passing in the image surface. Then on platforms where the plugin *can* paint directly into an image surface (but not with alpha), we could eliminate a copy just by implementing PaintRectToSurface properly. I think we would need to pass the desired background color as an extra parameter for PaintRectToSurface. Use NS_RGBA(0,0,0,0) if you just want to clear.

+    // Paint onto black image
+    nsRefPtr<gfxImageSurface> blackImage =
+        new gfxImageSurface(clipSize, gfxASurface::ImageFormatARGB32);

Should be RGB24

In PaintRectWithAlphaExtraction, if gfxASurface is an image surface, we can avoid a copy by using it as the black surface directly instead of copying the black surface into it. This should be easy by creating a gfxImageSurface that points at the right offset of the data in gfxASurface, with the right size set and stride set to gfxASurface's stride. If it doesn't matter right now, at least add an XXX comment explaining that it should be done. This probably will matter for Windows.

+    // Just repaint whole plugin
+    gfxRect renderRect(0, 0, mWindow.width, mWindow.height);
+    if (mDoAlphaExtraction) {
+        handled = PaintRectWithAlphaExtraction(renderRect, mCurrentSurface);
+    } else {
+        handled = PaintRectToSurface(renderRect, mCurrentSurface);
+    }

Why repaint the whole plugin when we know it hasn't all changed? Seems like only processing the invalid rect would be a big win for alpha extraction.

+PluginInstanceChild::AsyncPaintRectWithFlush(PRUint32 aMsec)

Why have an aMsec parameter when no-one uses it? I'm not even sure what it means.
(In reply to comment #186)
> Comment on attachment 474163 [details] [diff] [review]
> rpc->sync, child/parent async API v5
> 
> +    case gfxASurface::SurfaceTypeImage:
> +    case gfxASurface::SurfaceTypePDF:
> <etc>
> 
> Why do we need all that? Why can't we just cast to int32 in all cases? r=me if
> you just get rid of the unnecessary switch.

I asked Oleg to write a checked serializer for surface type, so we don't send garbage values over IPC or pull them off the wire.  The switch isn't necessary, this works too

  SurfaceTypeImage <= type && type < SurfaceTypeMax
Depends on: 595697
Ok, I hope I fixed remarks from comment 187.

>+        new gfxImageSurface(clipSize, gfxASurface::ImageFormatARGB32);
>Should be RGB24
I don;t think so... blackImage is ARGB32 image and that is final ARGB32 transparent surface.
Attachment #474221 - Attachment is obsolete: true
Attachment #474576 - Flags: review?(roc)
Attachment #474221 - Flags: review?(roc)
Added missing surface clear Qt -Raster backend (image rendering)
Also fixed fill with rectangle.
Attachment #474576 - Attachment is obsolete: true
Attachment #474586 - Flags: review?(roc)
Attachment #474576 - Flags: review?(roc)
(In reply to comment #190)
> >+        new gfxImageSurface(clipSize, gfxASurface::ImageFormatARGB32);
> >Should be RGB24
> I don;t think so... blackImage is ARGB32 image and that is final ARGB32
> transparent surface.

Ah, you're quite right.

+    // This must create surface if mSurfaceType
+    // To make composite operation on screen as fast as possible

This comment is incomplete.

CreatePlatformHelperSurface should be called MaybeCreatePlatformHelperSurface or CreatePlatformHelperSurfaceIfNeeded, since it doesn't always create a surface.

+    // Workaround for Flash bug
+    if (mIsTransparent)
+        plPaintRect.SetRect(0, 0, aRect.XMost(), aRect.YMost());

You should describe what the bug is (referencing a bug number, ideally), and put {} around the if body

+    if (!(renderSurface->GetContentType() == gfxASurface::CONTENT_COLOR
+          && aColor.a == 0)) {
+       // Don't fill color to non-alpha surface and when color.a == 0
+       FillSurface(renderSurface, aColor);

Shouldn't you just check !mIsTransparent here? The comment would be "// Don't apply background color if the plugin will just draw over it". I'm not sure why you want to check aColor.a.

+        if (aSurface->GetContentType() != gfxASurface::CONTENT_COLOR)
+            FillSurface(aSurface, aColor);

{}

Why do you need to do this again? We already did FillSurface(renderSurface).

+    if (aColor.a == 0) {
+        ctx->SetOperator(gfxContext::OPERATOR_CLEAR);
+    } else {
+        ctx->SetColor(aColor);
+        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
+    }

Is using OPERATOR_CLEAR known to be faster than painting rgba(0,0,0,0) with OPERATOR_SOURCE? If you're not sure, don't add the special a == 0 path.

+    // Paint plugin content rectangle to surface with bg color filling
+    PRBool PaintRectToSurface(const nsIntRect& aRect,
+                              gfxASurface* aSurface,
+                              const gfxRGBA& aColor);
+
+    // Render plugin content to surface using
+    // white/black image alpha extraction algorithm
+    PRBool PaintRectWithAlphaExtraction(const nsIntRect& aRect,
+                                        gfxASurface* aSurface);
+
+    // Call plugin NPAPI function to render plugin content to surface
+    // @param - aSurface - should be compatible with current platform plugin rendering
+    // @return - FALSE if plugin not painted to surface
+    PRBool PaintRectToPlatformSurface(const nsIntRect& aRect,
+                                      gfxASurface* aSurface);

In these comments, it's not clear what is actually drawn in aSurface. Are the drawing results put in (aRect.x, aRect.y, aRect.width, aRect.height) in aSurface? Or are they in (0, 0, width, height)?
I found that we are continue using dead XID after plugin crash...
I've checked and noticed that actor destroy is coming too late... but we can detect dead plugin-child process by checking NotifyPaint return value (SendIPCMethod will always fail if target process is dead)
Attachment #474628 - Flags: review?(roc)
Only flash bug number is unknown. sorry cannot find BMO related bug.
Attachment #474586 - Attachment is obsolete: true
Attachment #474636 - Flags: review?(roc)
Attachment #474586 - Flags: review?(roc)
Fixed comment about flash bug
Attachment #474636 - Attachment is obsolete: true
Attachment #474644 - Flags: review?(roc)
Attachment #474636 - Flags: review?(roc)
+    if (mIsTransparent) {
+       // Clear surface content for transparent rendering
+       PaintColorRectToSurface(plPaintRect, renderSurface, aColor);
+       if (renderSurface != aSurface)
+           PaintColorRectToSurface(plPaintRect, aSurface, aColor);

{}

Why do we need to paint the color to aSurface here? We're going to do a SOURCE copy from renderSurface to aSurface anyway, if they're not equal. This second PaintColorRect should not be necessary.

If you get rid of it, you can inline PaintColorRect into this call site.

+        // Copy helper surface content to target
+        nsRefPtr<gfxContext> ctx = new gfxContext(aSurface);
+        ctx->SetSource(renderSurface);
+        ctx->SetOperator(gfxContext::OPERATOR_SOURCE);
+        ctx->Rectangle(gfxRect(aRect.x, aRect.y, aRect.width, aRect.height), PR_TRUE);

You can leave off PR_TRUE since plPaintRect is already an intrect.

I think PaintRectToPlatformSurface should just assert that aSurface is a surface that the plugin can draw directly into. So for MOZ_X11, just assert that the surface is an Xlib surface, and go.

I also think that PaintRectToPlatformSurface and PaintRectToSurface should never return failure. Can mPluginIface->event ever really be null? If so, we should check that much earlier, during initialization.

PaintRectWithAlphaExtraction is still doing alpha extraction on the whole plugin area, not just the changed area. Why?

+    // In the PaintRect functions, aSurface is the size of the full plugin window. Each PaintRect function
+    // renders into the subrectangle aRect of aSurface and leaves the rest of aSurface unchanged.

I realized the last sentence isn't actually true. For the Flash workaround, we can render outside of aRect. Just say that "Each PaintRect function renders into the subrectangle aRect of aSurface (possibly more if we're working around a Flash bug)".

+    // Just repaint whole plugin, because we cannot read back from Shmem which is owned by another process

This comment belongs after the #endif.

+    // Cached rectangle for reading back previously rendered content
+    nsIntRect             mPreviousRenderedRect;

This comment needs to be better. Explain that this is a rectangle that contains the differences between mBackSurface and mCurrentSurface. Maybe change the name to mSurfaceDifferenceRect.

+    mPreviousRenderedRect.SetRect(0, 0, mWindow.width, mWindow.height);

Put {} around this

ShowPluginFrame is a bit confusing.

You need a comment to explain that in general there are three things we need to do here: 1) update mCurrentSurface to be a complete copy of mBackSurface and 2) draw the invalidated plugin area into mCurrentSurface and 3) send it.

For step 1 with X, you can copy mPreviousRenderedRect from mBackSurface to mCurrentSurface. You do that. However, if 'rect' contains mPreviousRenderedRect (which is likely!), you don't need to do this, since the plugin's own drawing we're going to do next will replace the results of this copy. I think it's worth adding that optimization. You might even want to use nsIntRegion to subtract rect from mPreviousRenderedRect and copy each rectangle in the result.

For step 1 without X, you just force step 2 to redraw the entire plugin so step 1 is not needed. I'm not sure this is the best approach but it's certainly fine for now. However, you're doing this:
+    mPreviousRenderedRect.SetRect(0, 0, mWindow.width, mWindow.height);
Setting mPreviousRenderedRect seems wrong here. That should always be set to rect by the time we exit ShowPluginFrame. Instead here you want to set the "rectangle of plugin to draw into mCurrentSurface", which I think means you should be setting "rect" and using "rect" in part 2, instead of mPreviousRenderedRect which you have now. mPreviousRenderedRect should mean what it says until you update it with "rect" at the end of ShowPluginFrame.

ctx->Rectangle does not need to pass a snapping parameter.

+        mCurrentSurface = nsnull;
+        handled = SendShow(r, currSurf, &outSurf);
+        mCurrentSurface = mBackSurface;

Why does mCurrentSurface need to be null during the SendShow?
Attachment #474644 - Attachment is obsolete: true
Attachment #474851 - Flags: review?(roc)
Attachment #474644 - Flags: review?(roc)
Comment on attachment 471018 [details] [diff] [review]
Fixed comment 119

>+    function PaintWaitListener() {
>+        gExplicitPendingPaintCounter++;
>+    }
>+    gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true);

As far as I can tell, this readds the listener every time a new
document loads in the browser.

>+                gBrowser.addEventListener("MozPaintWaitFinished", PaintWaitFinishedListenerRefWait, true);

Likewise, this adds a listener every time we load a reftest-wait test.

>+        gBrowser.addEventListener("MozPaintWaitFinished", PaintWaitFinishedListener, true);

And, likewise, this adds the listener every time we load a
non-reftest-wait test.

As far as I can tell, nothing ever removes any of these listeners, so
they stay registered from that point on.  (Since addEventListener
coalesces duplicate registrations, they never get notified twice.)

This is extremely confusing.  It also seems like it could be harmful in
a number of cases.

First, waiting to register the MozPaintWait listener until the document
load has completed could produce an incorrect counter (too low) if
MozPaintWait events are fired before the first document finishes
loading.  It seems like this could happen if there's a plugin paint that
happens while the test is still waiting for other network loads.

Second, it seems like having both MozPaintWaitFinished listeners running
at the same time could confuse things.  I haven't investigated this
possibility in detail.

If you're going to use permanently-registered listeners (which seems
like a reasonable thing to do), you should just register them in
OnRefTestLoad and keep them registered, just have one for each event,
and make what they do depend on the current state.

Or am I misunderstanding what you've done?


How did you test this patch?
Attachment #471018 - Flags: review?(dbaron) → review-
Comment on attachment 474851 [details] [diff] [review]
plugin child/parent implementation v8. Updated to comment 196

>+            NS_ERROR("Need X falback surface, but FindRenderFormat failed");
>+            return PR_FALSE;

I don't know whether we need to care about this case where the display doesn't have render, but could this fall back to the shared image surfaces?

>+        if (visual && defaultVisual != visual && !supportNonDefaultVisual) {

I think these lines should test
  (!visual || (defaultVisual != visual && !supportNonDefaultVisual))

>+        if (!visual) {
>+            NS_ERROR("Need X falback surface, but visual failed");
>+            return PR_FALSE;
>+        }

so that this never happens.
Add missing PaintColorRect fix.
Added check for null description.
Attachment #474851 - Attachment is obsolete: true
Attachment #474931 - Flags: review?(roc)
Attachment #474851 - Flags: review?(roc)
In PaintRectWithAlphaExtraction, when we create temporary surfaces, you can create a surface that's the size of aRect and set the device offset to -aRect.TopLeft() so we don't have to allocate as much memory. But you'll have to be careful in gfxAlphaRecovery::RecoverAlpha (don't call GetSubImage when we do that).

GetSubImage should be called GetSubimage ("sub" isn't a word :-) )

+    if (handled) {

Don't bother with 'handled'; just do "if (!PaintRectWithAlphaExtraction(...)) return PR_FALSE;" etc.
BTW you didn't address these comments:
> I think PaintRectToPlatformSurface should just assert that aSurface is a
> surface that the plugin can draw directly into. So for MOZ_X11, just assert
> that the surface is an Xlib surface, and go.

> I also think that PaintRectToPlatformSurface and PaintRectToSurface should
> never return failure. Can mPluginIface->event ever really be null? If so, we
> should check that much earlier, during initialization.
Attached patch SubImage API (obsolete) — Splinter Review
Attachment #474949 - Flags: review?
Attachment #474949 - Flags: review? → review?(joe)
Use subimage API, and remove return value for PaintRect functions
Attachment #474931 - Attachment is obsolete: true
Attachment #474953 - Flags: review?(roc)
Attachment #474931 - Flags: review?(roc)
Attached patch Subimage API, v2 (obsolete) — Splinter Review
Added note about life time of sub image relatively to parent image.
rename to s/SubImage/Subimage/
Attachment #474949 - Attachment is obsolete: true
Attachment #474959 - Flags: review?
Attachment #474949 - Flags: review?(joe)
+#ifdef MOZ_X11
+    if (aSurface->GetType() == gfxASurface::SurfaceTypeXlib) {

This should be an assertion.

You haven't addressed the first paragraph of comment #201 yet.
Listeners moved to OnReftestLoad function.
Added hacky pointer to AttrListener function which is not global (what is the better way to do that?)
Attachment #471018 - Attachment is obsolete: true
Attachment #475081 - Flags: review?(dbaron)
Attachment #474953 - Attachment is obsolete: true
Attachment #475084 - Flags: review?(roc)
Attachment #474953 - Flags: review?(roc)
Attachment #474959 - Attachment is obsolete: true
Attachment #475086 - Flags: review?(roc)
Attachment #474959 - Flags: review?
(In reply to comment #207)
> Created attachment 475081 [details] [diff] [review]
> reftests.js v3, address comment 198
> 
> Listeners moved to OnReftestLoad function.
> Added hacky pointer to AttrListener function which is not global (what is the
> better way to do that?)

How did you test this patch?
> 
> How did you test this patch?

hmm... like this:
/usr/bin/python2.5 ../../_tests/reftest/runreftest.py  --setpref=mozilla.plugins.use_layers=true  --symbols-path=./dist/crashreporter-symbols  /mnt/SBOXT61P/scratchbox/users/romaxa/home/romaxa/microbcomponent/mozilla-central/modules/plugin/test/reftest/reftest.list | grep 'UNEXPECTED-FAIL'

without this patch, most of plugin tests are failing because snapshot taken before plugin has valid layer surface (content).

with patch, reftests.js waiting for nsObjectFrame notifications and don't take snapshot if there are some objectFrame without layer surface created.
Comment on attachment 475081 [details] [diff] [review]
reftests.js v3, address comment 198

Could you rename the following variables:

 gDoRefWaitTest -> gRunningReftestWaitTest
 gDefaultReftestContainAsyncObjects -> gTestContainsAsyncPaintObjects

r=dbaron with that
Attachment #475081 - Flags: review?(dbaron) → review+
Depends on: 596379
Reopening due to non-ipc orange
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Disable layers of non-ipc (obsolete) — Splinter Review
Attachment #475256 - Flags: review?(roc)
no idea why it is failing without my patch, but it is passes always with my patch applied...
Roc any ideas?
Attachment #475281 - Flags: review?(roc)
Comment on attachment 475281 [details] [diff] [review]
make SVG reftest pass

Let's just do this. Too bad we don't know why it started passing, but at least we'll know if it starts failing again.
Attachment #475281 - Flags: review?(roc) → review+
(In reply to comment #215)
> Created attachment 475256 [details] [diff] [review]
> Disable layers of non-ipc

We need a new function, say UseAsyncPainting, that we can call instead of checking the IPC pref.
Attachment #475256 - Attachment is obsolete: true
Attachment #475311 - Flags: review?(roc)
Attachment #475256 - Flags: review?(roc)
Blocks: 596451
Is there a particular reason this uses an outparam instead of just 'virtual bool UseAsyncPainting()' ? Death to unnecessary outparams!
It has to be defined in nsIPluginInstance.idl.
Depends on: 596769
No longer blocks: 586162
Depends on: 598112
Blocks: 598425
Blocks: 586162
Blocks: 583475
Depends on: 603397
Depends on: 606285
Target Milestone: --- → mozilla2.0b7
Depends on: 609444
No longer depends on: 609444
Depends on: 615025
No longer blocks: 588591
Blocks: 588591
Depends on: 629472
Depends on: 657874
Depends on: 723659
You need to log in before you can comment on or make changes to this bug.