625425 - Don't re-share the plugin surfaces every time we flip front/back buffers (on Windows at least)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

For async plugins, the current rendering model has an inefficiency. We create a front and back buffer on the plugin side. Every time we send a frame, we share that buffer with the parent. This involves creating a handle and apparently quite a few page faults, and may be implicated in the odd VM space issues in bug 613915.

I think with only a little extra work we can share the buffers when they are created, and just keep an actor around to track them until we resize.

Comment 1

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

A better option would be to change the shared DIB surface to wrap gfxSharedImageSurface, or raw Shmem like gfxSharedImageSurface does.

Comment 2

[Benjamin Smedberg]

I don't understand what you mean. We have to have a Windows Shared DIB in order to paint to it with a DC.

Comment 3

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

The SharedDIB class is functionally equivalent to Shmem except for ownership checking and automatic cleanup.  It looks to me like this code http://mxr.mozilla.org/mozilla-central/source/dom/plugins/PluginInstanceChild.cpp#2783 runs on paints, and it's totally unnecessary with Shmem because the mapping (=HANDLE) is held by each process until the Shmem is destroyed.

Yes, of course you need to wrap win32 painting things around the underlying memory segment.

Comment 4

[Benjamin Smedberg]

When I first wrote this code, we talked about adding features to Shmem such that you could get the HANDLE out of it to pass to ::CreateDIBSection, and we decided that it would torture the system too much. I'm not sure I want to revisit that decision now.

Comment 5

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

I don't remember that Shmem existed when this code was originally written, or at least I don't remember discussing it wrt SharedDIB.  No worries there.  I'm not against handing out a low-level ref; we do it for SysV shmem http://mxr.mozilla.org/mozilla-central/source/ipc/glue/Shmem.h#200.  Would probably want to hand out a dup HANDLE there though, if we did it.

Comment 6

[Jim Mathies [:jimm]]

I suspect if we eliminate the repetitive duplicate handle behavior, bug 613915 goes away. This would match up with our older sync painting model which didn't have the problem. Are we considering blocking on this? If so, it should probably block 613915 so we don't waste more time on that until this after lands.

Comment 7

[Benjamin Smedberg]

I'm going to make a tryserver build people can try. Then we can see what the status of this bug should be.

Comment 8

[Benjamin Smedberg]

Attachment: Cache windows DIBs when the size doesn't change, WIP

This plays, but crashes when you navigate away from youtube. I'm trying to record this, but I'm not sure I'll have time before the weekend.

Comment 9

[Benjamin Smedberg]

Attachment: Cache windows DIBs when the size doesn't change, rev. 1

Stupid error, s/delete this;/delete s;/

Anyway, I'm running this through try now, and I'm going to try to get people to see if it fixes or at least mitigates bug 613915 and bug 612113. They will be available at http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d0dceef375bd

Comment 10

[:Felipe Gomes (needinfo for replies!)]

I built the previous patch plus the s/delete this/delete s/ change on WinXP and have been playing windowless video for 2 hours now. So far, copy and paste still works! I could easily reproduce the bug with about 38min of video playback on a VM.

Comment 11

[IU]

Confirming that the tryserver build referenced in comment 9 fixes bug 613915 for me -- at least with hw accel' disabled (my normal setting).

Comment 12

[IU]

(In reply to comment #11)
> at least with hw accel' disabled (my normal setting).

Apparently with it enabled.  The pref to enable/disable hw accel' was changed, so it was actually enabled, ignoring my previous setting.

Comment 13

[Benjamin Smedberg]

http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d0dceef375bd/try-w32-dbg/tryserver_win7-debug_test-reftest-build304.txt.gz Shows a leak in the chrome process of PPluginSurfaceParent and gfxASurface which will need to be examined? felipe or jimm, is one of you able to take this while I'm away on leave?

Comment 14

[Jim Mathies [:jimm]]

(In reply to comment #13)
> http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d0dceef375bd/try-w32-dbg/tryserver_win7-debug_test-reftest-build304.txt.gz
> Shows a leak in the chrome process of PPluginSurfaceParent and gfxASurface
> which will need to be examined? felipe or jimm, is one of you able to take this
> while I'm away on leave?

Sure, we'll figure it out.

Comment 15

[Jim Mathies [:jimm]]

Attachment: Cache windows DIBs when the size doesn't change, rev. 2

Found it, deleting PPluginSurfaceParent vs. PluginSurfaceParent, PluginSurfaceParent wasn't getting destroyed.

Comment 16

[Karl Tomlinson (:karlt)]

Most ipdl generated classes have virtual destructors.
What makes PPluginSurfaceParent different?

I assume PPluginSurface.ipdl and PluginSurfaceParent.(h|cpp) were dropped accidentally.

Comment 17

[Jim Mathies [:jimm]]

(In reply to comment #16)
> Most ipdl generated classes have virtual destructors.
> What makes PPluginSurfaceParent different?


I made the wrong assumption. The casting isn't needed, the difference is which implementation of delete gets invoked. The code in the header is calling the mozalloc library. In the cpp the std library implementation gets invoked which calls the dtor.

> I assume PPluginSurface.ipdl and PluginSurfaceParent.(h|cpp) were dropped
> accidentally.

Sorry, forgot to add those, new patch coming up.

Comment 18

[Jim Mathies [:jimm]]

Attachment: Cache windows DIBs when the size doesn't change, rev. 3

Comment 19

[Jim Mathies [:jimm]]

I think we could also move everything into the header like PluginSurfaceChild and avoid this as well. Doesn't make much difference though, the two just need to be matched up sp they get the same allocator.

Comment 20

[Karl Tomlinson (:karlt)]

cjones explained to me that the problem was that, because PPluginSurfaceParent.h wasn't included in PluginInstanceParent.h, PPluginSurfaceParent was an incomplete type (forward declared) and so the destructor was not called from delete.
Moving to PluginInstanceParent.cpp, which does include PPluginSurfaceParent.h, as you have done, seems the appropriate solution then.

Comment 21

[Karl Tomlinson (:karlt)]

Comment on attachment 507135 [details] [diff] [review]
Cache windows DIBs when the size doesn't change, rev. 3

>  ~PluginSurfaceParent() { printf("~PluginSurfaceParent()\n"); }

printf left behind.

>     void InvalidateRectDelayed(void);
> 
>     // Set as true when SetupLayer called
>     // and go with different path in InvalidateRect function
>     bool mLayersRendering;
> 
>+    // Clear mCurrentSurface/mCurrentSurfaceActor/mHelperSurface
>+    void ClearCurrentSurface();
>+
>+    // Swap mCurrentSurface/mBackSurface and their associated actors
>+    void SwapSurfaces();
>+
>+    // Clear all surfaces in response to NPP_Destroy
>+    void ClearAllSurfaces();

Can we move the new method declarations together with the other methods before
the mLayersRendering variable, please?

Also in PluginInstanceChild.cpp can we move the ClearAllSurfaces definition to
before AnswerNPP_Destroy to help with the history here.  (hg diff has taken
one of its bad turns trying to describe these changes.)  ClearCurrentSurface
could probably logically be before ClearAllSurfaces.

Comment 22

[Jim Mathies [:jimm]]

Attachment: cleaned up for landing

Thanks Karl. I'll push this to try first and land if everything looks ok.

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 507491 [details] [diff] [review]
cleaned up for landing

requesting blocking, this fixes blocker bug 613915.

Comment 24

[IU]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-0195969b0d2a/try-w32/

Looks good.  Played two windowless flash videos (total of almost 80 minutes) and no problems occurred.

Comment 25

[Jim Mathies [:jimm]]

http://hg.mozilla.org/mozilla-central/rev/ecb1a0b69e0b