556487 - Implement layers for plugins nsObjectFrame

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Oleg Romashin (:romaxa)]

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

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Plugin layers imageSurface WIP1

Comment 2

[Oleg Romashin (: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?

Comment 3

[Oleg Romashin (:romaxa)]

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...)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: New wip

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)

Comment 7

[Oleg Romashin (:romaxa)]

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 9

[Oleg Romashin (:romaxa)]

> 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

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: WIP2, Layers + windowles-X IPC async

New version.
Created based on default X-Windowless API.
Tested on Qt Fennec (with X backend). works fine with Desktop flash in Opaque mode.

Still problem with transparent mode (need to implement alpha extraction).

Anyone would like to check it? feedback?

Comment 11

[Benoit Girard (:BenWa)]

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.

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: WIP3, added aplha calculation on plugin process side

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: MAke it works with GTK port

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

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Ups, this is the right patch

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: Add support for transparent alpha

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.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: Updated version, tested with Qt gtk/ IPC/non-IPC...

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

Comment 17

[Oleg Romashin (:romaxa)]

Attachment: Updated version

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.

Comment 18

[Oleg Romashin (:romaxa)]

Attachment: Removed dependency from transparentAlpha patch, added dependency from bug 567065

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)

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Updated patch, need feedback

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

Comment 20

[Oleg Romashin (:romaxa)]

Comment on attachment 447383 [details] [diff] [review]
Updated patch, need feedback

Added common methods for shmem and nativeID (XID/IOSurface) types

Comment 21

[Oleg Romashin (:romaxa)]

Attachment: More cleanup, and fixed rendering with shmem on Qt::Raster backend

Comment 22

[Oleg Romashin (:romaxa)]

Attachment: Here is patch example which is making Image rendering NPAPI (old Maemo5) working with this approach

It would be nice to get new NPAPI approved.

Comment 23

[Karl Tomlinson (:karlt)]

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

Comment 24

[Karl Tomlinson (:karlt)]

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).

Comment 25

[Karl Tomlinson (: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.

Comment 26

[Karl Tomlinson (:karlt)]

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

Comment 27

[Oleg Romashin (:romaxa)]

Attachment: Patch updated to trunk, fixed multiple Image/Layer creation (optimized perf)

Comment 28

[Oleg Romashin (:romaxa)]

Attachment: Make it works with roc patch queue

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Updated to latest trunk with retained layers landed

Comment 30

[Benjamin Smedberg]

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

Comment 31

[Oleg Romashin (:romaxa)]

(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.

Comment 32

[Oleg Romashin (:romaxa)]

Attachment: Updated to latest m-c 2335c842c06c, fixed compilation bustage (tested on Try)

Comment 33

[Oleg Romashin (:romaxa)]

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

Comment 34

[Oleg Romashin (:romaxa)]

Attachment: Fixed oranges caused by SetWindow while pending plugin->event() call (re-enter problem)

Comment 35

[Oleg Romashin (:romaxa)]

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?

Comment 36

[Benjamin Smedberg]

No, we should wait for it somehow.

Comment 37

[Benjamin Smedberg]

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.

Comment 38

[Oleg Romashin (:romaxa)]

> 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.

Comment 39

[Johnny Stenback (:jst)]

Blocking per comment 37.

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 41

[Oleg Romashin (:romaxa)]

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?

Comment 42

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 43

[Oleg Romashin (:romaxa)]

(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...

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 45

[Oleg Romashin (:romaxa)]

restoring blocking request

Comment 46

[Karl Tomlinson (:karlt)]

(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.

Comment 47

[Oleg Romashin (:romaxa)]

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

Comment 48

[Oleg Romashin (:romaxa)]

Attachment: add reftest-async style for taking screenshot on second invalidate

Comment 49

[Oleg Romashin (:romaxa)]

Attachment: 546071 fail result

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?

Comment 50

[Karl Tomlinson (:karlt)]

(In reply to comment #49)
> Is it something related to pixel-snap?

Yes, looks like it is.

Comment 51

[Oleg Romashin (:romaxa)]

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

Comment 52

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 53

[Oleg Romashin (:romaxa)]

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!"?

Comment 54

[Oleg Romashin (:romaxa)]

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

Comment 55

[Oleg Romashin (:romaxa)]

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

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes

Comment 57

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 58

[Oleg Romashin (:romaxa)]

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...

Comment 59

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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 60

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 61

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 63

[Benjamin Smedberg]

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

Comment 64

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 66

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 68

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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).

Comment 70

[Oleg Romashin (:romaxa)]

Attachment: Switch to new objectFrame-PluginParent API. WIP1

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

Comment 71

[Johnny Stenback (:jst)]

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.

Comment 72

[Oleg Romashin (:romaxa)]

Attachment: Better version works for cases

Comment 73

[Oleg Romashin (:romaxa)]

Attachment: Working version with new API, mostly ready for pre-review

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

Comment 74

[Oleg Romashin (:romaxa)]

Attachment: v5 async reftest with autowait

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.

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 76

[Oleg Romashin (:romaxa)]

> 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.

Comment 77

[Oleg Romashin (:romaxa)]

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.

Comment 78

[Oleg Romashin (:romaxa)]

Attachment: Green tests version

This version has fix for PluginLayers when ipc.enabled=false. (implementing async layers rendering for non-ipc plugins).

Does anybody want to look at this patch? feedback review?

Comment 79

[Oleg Romashin (:romaxa)]

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.

Comment 80

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 81

[Oleg Romashin (:romaxa)]

Attachment: Update comments and some tests problems

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

Comment 82

[Oleg Romashin (:romaxa)]

Attachment: v1

Comment 83

[Oleg Romashin (:romaxa)]

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

Comment 84

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 85

[Oleg Romashin (:romaxa)]

Attachment: Fixed comments

> +  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...

Comment 86

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 88

[Oleg Romashin (:romaxa)]

Attachment: v4.

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.

Comment 89

[Johnny Stenback (:jst)]

Feature freeze is now beta6, so pushing this out.

Comment 90

[Oleg Romashin (:romaxa)]

Comment on attachment 469805 [details] [diff] [review]
v4.

Ok, I'll try to separate API's, implementation, and usage parts for easier review

Comment 91

[Oleg Romashin (:romaxa)]

Attachment: gfxUtils for plugin layers

Comment 92

[Oleg Romashin (:romaxa)]

Attachment: reftests fixes to make boxes green with new async rendering model

Comment 93

[Oleg Romashin (:romaxa)]

Attachment: Plugin module API part + implementation

Comment 94

[Oleg Romashin (:romaxa)]

Attachment: Plugin module API part + implementation

Comment 95

[Oleg Romashin (:romaxa)]

Attachment: ObjectFrame part

Comment 96

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 97

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 98

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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?

Comment 99

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // 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.

Comment 100

[Oleg Romashin (:romaxa)]

Attachment: Updated, create virtual ASurface method

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

Comment 101

[Oleg Romashin (:romaxa)]

Attachment: reftests api, updated. v2

Comment 102

[Oleg Romashin (:romaxa)]

Attachment: ObjectFrame part. Updated to new GetSize, DOMEvent API.

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

Comment 103

[Joe Drew (not getting mail)]

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?

Comment 104

[Oleg Romashin (:romaxa)]

Attachment: Fixed GetSize method

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

Comment 105

[Joe Drew (not getting mail)]

Comment on attachment 470542 [details] [diff] [review]
Fixed GetSize method


>diff --git a/gfx/thebes/gfxUtils.cpp b/gfx/thebes/gfxUtils.cpp

> #if defined(XP_WIN) || defined(WINCE)
> #include "gfxWindowsPlatform.h"
> #endif
>+#include "gfxImageSurface.h"
>+#ifdef MOZ_X11
>+#include "gfxXlibSurface.h"
>+#endif

These includes aren't needed anymore.

Great otherwise.

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 107

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 108

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // 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.

Comment 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 110

[Oleg Romashin (:romaxa)]

Attachment: reftest.js updated to comment 106, and added smal doc about new functionality

Comment 111

[Oleg Romashin (:romaxa)]

Comment on attachment 470769 [details] [diff] [review]
reftest.js updated to comment 106, and added smal doc about new functionality

ups, something wrong with this reftest.js version.. reftests hang need to check what is going on

Comment 112

[Oleg Romashin (:romaxa)]

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

this seems broken

Comment 113

[Karl Tomlinson (:karlt)]

(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.

Comment 114

[Oleg Romashin (:romaxa)]

Attachment: Added DOMEventTarget argument for FireDOMEvent method

Comment 115

[Oleg Romashin (:romaxa)]

Attachment: Updated for comment 107

Ok, I hope I fix all comments from comment 107

Comment 116

[Oleg Romashin (:romaxa)]

Attachment: rejtest.js

returned
gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true);

back into function OnDocumentLoad... otherwise it does not work. and amount of Wait/Finished not equals...

Comment 117

[Oleg Romashin (:romaxa)]

> 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 118

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 119

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 470940 [details] [diff] [review]
rejtest.js

>Bug 556487 - Implement layers for plugins nsObjectFrame. r=roc r=dbaron=?

Please make the commit message describe the patch.

>+// Some layout frames are painting asynchronously,
>+// and to make sure that all our frames have right content
>+// we are listening fpr  "MozPaintWait" and "MozPaintWaitFinished" events
>+// if frame(layer) content is dirty then "MozPaintWait" event fired and gExplicitPendingPaintCounter increased
>+// when frame fully painted then "MozPaintWaitFinished" fired and gExplicitPendingPaintCounter decreased.
>+// reftest snapshot can be taken only when gExplicitPendingPaintCounter == 0

Please wrap lines evenly at 72-75 characters, begin each sentence with a capital letter, and end each sentence with a period.  This is nearly impossible to understand since I can't tell where one sentence ends and where the next begins (although I think perhaps you've used line breaks as sentence separators in some cases).

>+            if (!gExplicitPendingPaintCounter)
>+              gDumpLog("Error: gExplicitPendingPaintCounter going to be < 0!!!\n");

Please use 4-space indent and bracing of all blocks (even if 1 line) throughout.

>-            if (stopAfterPaintReceived && !utils.isMozAfterPaintPending) {
>+            if (stopAfterPaintReceived && !utils.isMozAfterPaintPending && !gExplicitPendingPaintCounter) {

Wrap to the next line.

>+        function PaintWaitListener() {
>+            gExplicitPendingPaintCounter++;
>+        }
>+
>+        function PaintWaitFinishedListener() {
>+            if (!gExplicitPendingPaintCounter)
>+              gDumpLog("Error: gExplicitPendingPaintCounter going to be < 0!!!\n");
>+            gExplicitPendingPaintCounter--;
>+            if (gExplicitPendingPaintCounter == 0)
>+              setTimeout(setTimeout, 0, DocumentLoaded, 0);
>+        }
>+        gBrowser.addEventListener("MozPaintWait", PaintWaitListener, true);
>+        gBrowser.addEventListener("MozPaintWaitFinished", PaintWaitFinishedListener, true);
>+
>         // Since we can't use a bubbling-phase load listener from chrome,
>         // this is a capturing phase listener.  So do setTimeout twice, the
>         // first to get us after the onload has fired in the content, and
>         // the second to get us after any setTimeout(foo, 0) in the content.
>         setTimeout(setTimeout, 0, DocumentLoaded, 0);

This doesn't make sense to me.  You're calling DocumentLoaded from two different places.  Shouldn't this setTimeout be replaced with something that increments the same reference count, and then a timeout to decrement it?

Or do some of the early returns compensate for this?


Why do you have so many different listeners for the same events, all manipulating the same counters?  Does something ensure that only one set is registered at once?  Why not just have a single listener?

Comment 120

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 121

[Oleg Romashin (:romaxa)]

Attachment: Fixed comment 119

Comment 122

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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.

Comment 123

[Oleg Romashin (:romaxa)]

Ok, I've fixed all comments up to comment#122, and store latest working version into:
http://hg.mozilla.org/users/romaxa_gmail.com/layer-patches/file/latest_non_swap_version

And started switching to Gecko:AsyncPluginPainting, hope it will not take so long

Comment 124

[Oleg Romashin (:romaxa)]

Attachment: Get XRenderPictFormat method from xlibSurface

Comment 125

[Oleg Romashin (:romaxa)]

Comment on attachment 470918 [details] [diff] [review]
Added DOMEventTarget argument for FireDOMEvent method

yep, we can use DispatchEvent content utils function

Comment 126

[Oleg Romashin (:romaxa)]

Attachment: reftests, test_flush_on_paint.html part, just need r+ flag reset

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

Comment 127

[Karl Tomlinson (:karlt)]

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.

Comment 128

[Oleg Romashin (:romaxa)]

Attachment: Add Post reflow callback to better first setWindow call with stabilized frame size

Comment 129

[Oleg Romashin (:romaxa)]

Attachment: Ajust plugin size and reduse amount of SetWindow calls due to pixel snap

Comment 130

[Oleg Romashin (:romaxa)]

Attachment: Plguins API stabble part, no implementation

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.

Comment 131

[Oleg Romashin (:romaxa)]

Attachment: ObjectFrame part.

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

Comment 132

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 472556 [details] [diff] [review]
Add Post reflow callback to better first setWindow  call with stabilized frame size

+  // WMP10 needs an additional SetWindow call here (bug 391261)

Delete this comment, we need this for all plugins.

Let's land this ASAP. Get yourself added to the landing queue.

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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).

Comment 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 135

[Oleg Romashin (:romaxa)]

> 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.

Comment 136

[Oleg Romashin (:romaxa)]

Attachment: Removed GetWindowOriginInPixels from CallSetWindow

Comment 137

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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?

Comment 138

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 139

[Oleg Romashin (:romaxa)]

> 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 140

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 141

[Oleg Romashin (:romaxa)]

Attachment: Plugin child/parent/PluginInstance API part no implementation

Comment 142

[Oleg Romashin (:romaxa)]

> 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

Comment 143

[Oleg Romashin (:romaxa)]

> 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

Comment 144

[Oleg Romashin (:romaxa)]

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 145

[Oleg Romashin (:romaxa)]

Comment on attachment 472931 [details] [diff] [review]
Plugin child/parent/PluginInstance API part no implementation

Just PluginHost <-> nsObjectFrame API

Comment 146

[Oleg Romashin (:romaxa)]

Attachment: ObjectFrame part. Fixed transform.Scale part and use layerSurface size instead window...

Comment 147

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+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?

Comment 148

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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...

Comment 149

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 150

[Oleg Romashin (:romaxa)]

Attachment: Plugin child/parent/PluginInstance API part no implementation. v3

Comment 151

[Oleg Romashin (:romaxa)]

Attachment: make test waiting for paint happening after setColor

Comment 152

[Oleg Romashin (:romaxa)]

Attachment: Don't crash silently if visual == 0

Comment 153

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    /**
+     * 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?

Comment 154

[Oleg Romashin (:romaxa)]

Attachment: Plugin child/parent/PluginInstance API part no implementation. v4

Comment 155

[Oleg Romashin (:romaxa)]

Attachment: Plugin Child/Parent async API (no implementation)

Comment 156

[Oleg Romashin (:romaxa)]

Attachment: nsObjectFrame v4, added SetCurrentImage immideately after AnswerSwap

Comment 157

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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

Comment 158

[Oleg Romashin (:romaxa)]

Attachment: nsObjectFrame v4, added SetCurrentImage immideately after AnswerSwap

Comment 159

[Oleg Romashin (:romaxa)]

Attachment: Reftest fix 580160-1

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

Comment 160

[Oleg Romashin (:romaxa)]

Attachment: Final part, plugin child/parent implementation v2, swap

Comment 161

[Oleg Romashin (:romaxa)]

Attachment: nsObjectFrame.v5, fixed 580160-1 reftest

IsUpToDate check also layer size.
If layer size != NPWindow size then it is old, and we should fire MozPaintWait event

Comment 162

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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 163

[Karl Tomlinson (:karlt)]

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.

Comment 164

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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.

Comment 165

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #164)
> +static gfxASurface::gfxSurfaceType sSurfacePrefferedType =
> gfxASurface::SurfaceTypeMax;
> 
> Try to avoid static initializers.

Never mind, that initializer is fine.

Comment 166

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #162)
> You need to add aItem->ToReferenceFrame()

No, I was being stupid. Never mind.

Comment 167

[Oleg Romashin (:romaxa)]

Attachment: nsObjectFrame v6 Updated up to comment 162

Comment 168

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  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 169

[Karl Tomlinson (:karlt)]

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?

Comment 170

[Oleg Romashin (:romaxa)]

Attachment: Plugin Child/Parent async API (no implementation) Removed force paint method

Comment 171

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // 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.

Comment 172

[Oleg Romashin (:romaxa)]

Attachment: GetBounds correction to center

Comment 173

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 174

[Karl Tomlinson (:karlt)]

(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.

Comment 175

[Oleg Romashin (:romaxa)]

Attachment: ObjectFrame GetBounds v.2

Disable GetBounds centering for non-layer mode.
Also fixed bounds correction when layer size == 0

Comment 176

[Oleg Romashin (:romaxa)]

Attachment: child/parent async API v3.

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

Comment 177

[Oleg Romashin (:romaxa)]

Attachment: ups, SurfaceType not ContentType child/parent async API v4

sorry, wrong enum type

Comment 178

[Oleg Romashin (:romaxa)]

Attachment: nsObjectFrame v7 Fixed all comments and typo

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

Comment 179

[Oleg Romashin (:romaxa)]

Attachment: rpc->sync, child/parent async API v5

Comment 180

[Oleg Romashin (:romaxa)]

Comment on attachment 474163 [details] [diff] [review]
rpc->sync, child/parent async API v5

roc check it also plsz

Comment 181

[Oleg Romashin (:romaxa)]

Attachment: Final part, plugin child/parent implementation v3

Ok, Fixed all roc and karl comments.
mIsTransparent still in use, because plugins don't remember their values and getvalue return always transparent false...

Comment 182

[Oleg Romashin (:romaxa)]

border_padding_test
http://hg.mozilla.org/mozilla-central/rev/a1cbee847ec5
ObjectFrame-PluginHost API
http://hg.mozilla.org/mozilla-central/rev/5caf82356f86

Comment 183

[Oleg Romashin (:romaxa)]

Attachment: Test page with a lot of plugins

This test page makes current firefox scrolling almost impossible.
But with this bug fixed scrolling works always

Comment 184

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 185

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 {}

Comment 186

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 187

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // 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.

Comment 188

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(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

Comment 189

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Ah, that sounds good.

Comment 190

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v5. Updated to comment 187.

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.

Comment 191

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v6. Updated to comment 187 + tested on maemo

Added missing surface clear Qt -Raster backend (image rendering)
Also fixed fill with rectangle.

Comment 192

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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)?

Comment 193

[Oleg Romashin (:romaxa)]

Attachment: Don't proceed with layers if NotifyPainted return NS_ERROR...

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)

Comment 194

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v7. Updated to comment 192

Only flash bug number is unknown. sorry cannot find BMO related bug.

Comment 195

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v7.5. Updated to comment 192

Fixed comment about flash bug

Comment 196

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    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?

Comment 197

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v8. Updated to comment 196

Comment 198

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 199

[Karl Tomlinson (:karlt)]

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.

Comment 200

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v8.5. Updated to comment 196

Add missing PaintColorRect fix.
Added check for null description.

Comment 201

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 202

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 203

[Oleg Romashin (:romaxa)]

Attachment: SubImage API

Comment 204

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v9. Updated to comment 196

Use subimage API, and remove return value for PaintRect functions

Comment 205

[Oleg Romashin (:romaxa)]

Attachment: Subimage API, v2

Added note about life time of sub image relatively to parent image.
rename to s/SubImage/Subimage/

Comment 206

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+#ifdef MOZ_X11
+    if (aSurface->GetType() == gfxASurface::SurfaceTypeXlib) {

This should be an assertion.

You haven't addressed the first paragraph of comment #201 yet.

Comment 207

[Oleg Romashin (:romaxa)]

Attachment: 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?)

Comment 208

[Oleg Romashin (:romaxa)]

Attachment: plugin child/parent implementation v10. Updated to comment 201/206

Comment 209

[Oleg Romashin (:romaxa)]

Attachment: Subimage v3. class with refpointer to parent class

Comment 210

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?

Comment 211

[Oleg Romashin (:romaxa)]

> 
> 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 212

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 213

[Oleg Romashin (:romaxa)]

Pushed
http://hg.mozilla.org/mozilla-central/rev/e67204601bc4
http://hg.mozilla.org/mozilla-central/rev/4aa0d49ee39b
http://hg.mozilla.org/mozilla-central/rev/67c4d8600d8a
http://hg.mozilla.org/mozilla-central/rev/6bf95d58032e
http://hg.mozilla.org/mozilla-central/rev/0ba703ded5c8
http://hg.mozilla.org/mozilla-central/rev/f513e1ef23dd
http://hg.mozilla.org/mozilla-central/rev/e80ae0ee06d7

Comment 214

[Oleg Romashin (:romaxa)]

Reopening due to non-ipc orange

Comment 215

[Oleg Romashin (:romaxa)]

Attachment: Disable layers of non-ipc

Comment 216

[Oleg Romashin (:romaxa)]

Attachment: make SVG reftest pass

no idea why it is failing without my patch, but it is passes always with my patch applied...
Roc any ideas?

Comment 217

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 218

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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.

Comment 219

[Oleg Romashin (:romaxa)]

Attachment: Disable layers of non-ipc v2.

Comment 220

[Benjamin Smedberg]

Is there a particular reason this uses an outparam instead of just 'virtual bool UseAsyncPainting()' ? Death to unnecessary outparams!

Comment 221

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It has to be defined in nsIPluginInstance.idl.

Comment 222

[Oleg Romashin (:romaxa)]

Landed API's , async API, implementation and subimage...
http://hg.mozilla.org/mozilla-central/rev/7e449cbc19df
http://hg.mozilla.org/mozilla-central/rev/8e61317e16e5
http://hg.mozilla.org/mozilla-central/rev/25fb7f219724
http://hg.mozilla.org/mozilla-central/rev/e4f55fb43e0b

Comment 223

[Oleg Romashin (:romaxa)]

Second try... 
http://hg.mozilla.org/mozilla-central/rev/34d351e0d832
http://hg.mozilla.org/mozilla-central/rev/83bf3f557dff
http://hg.mozilla.org/mozilla-central/rev/249f4f1cbe3d
http://hg.mozilla.org/mozilla-central/rev/d10496466977
http://hg.mozilla.org/mozilla-central/rev/8966ece73994
http://hg.mozilla.org/mozilla-central/rev/31d7bc6c5c4a