Closed Bug 625425 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Plug-ins, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

(Whiteboard: [blocks a hardblocker][fixes 613915])

Attachments

(1 file, 4 obsolete files)

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.
A better option would be to change the shared DIB surface to wrap gfxSharedImageSurface, or raw Shmem like gfxSharedImageSurface does.
I don't understand what you mean. We have to have a Windows Shared DIB in order to paint to it with a DC.
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.
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.
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.
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.
I'm going to make a tryserver build people can try. Then we can see what the status of this bug should be.
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.
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/bsmedberg@mozilla.com-d0dceef375bd
Attachment #505505 - Attachment is obsolete: true
Attachment #505943 - Flags: review?(karlt)
Attachment #505943 - Flags: review?(jmathies)
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.
Confirming that the tryserver build referenced in comment 9 fixes bug 613915 for me -- at least with hw accel' disabled (my normal setting).
(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.
http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bsmedberg@mozilla.com-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?
(In reply to comment #13)
> http://stage.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/bsmedberg@mozilla.com-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.
Found it, deleting PPluginSurfaceParent vs. PluginSurfaceParent, PluginSurfaceParent wasn't getting destroyed.
Attachment #505943 - Attachment is obsolete: true
Attachment #506565 - Flags: superreview?(karlt)
Attachment #506565 - Flags: review+
Attachment #505943 - Flags: review?(karlt)
Attachment #505943 - Flags: review?(jmathies)
Most ipdl generated classes have virtual destructors.
What makes PPluginSurfaceParent different?

I assume PPluginSurface.ipdl and PluginSurfaceParent.(h|cpp) were dropped accidentally.
(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.
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.
Whiteboard: [blocks a hardblocker][fixes 613915]
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 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.
Attachment #507135 - Flags: review+
Attachment #506565 - Flags: superreview?(karlt)
Thanks Karl. I'll push this to try first and land if everything looks ok.
Attachment #506565 - Attachment is obsolete: true
Attachment #507135 - Attachment is obsolete: true
Attachment #507491 - Flags: review+
Comment on attachment 507491 [details] [diff] [review]
cleaned up for landing

requesting blocking, this fixes blocker bug 613915.
Attachment #507491 - Flags: approval2.0?
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/jmathies@mozilla.com-0195969b0d2a/try-w32/

Looks good.  Played two windowless flash videos (total of almost 80 minutes) and no problems occurred.
http://hg.mozilla.org/mozilla-central/rev/ecb1a0b69e0b
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.