596451 - Asynchronous layer-based plugin painting on Windows

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Benjamin Smedberg]

Asynchronous layer-based plugin painting on Windows

Comment 1

[Benjamin Smedberg]

Attachment: Part A, use bools, fix styling, rev. 1

Comment 2

[Benjamin Smedberg]

Attachment: Make UseAsyncPainting simpler and stop delegating it all around, rev. 1

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

I'm not a peer here, but looks fine to me.

There seems to be a missing agreement from peers on this type of change though. (I've been dinged for using false in dom/plugin code by other reviewers.) Maybe we need to standardize it and document it in out coding guidelines.

https://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Comment 4

[Benjamin Smedberg]

Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

This just aligns the usage with the rest of the code. Josh, can you mark official r+?

Comment 5

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

Don't forget to respect the windowless "opaque" setting to turn off alpha recovery here.

Comment 6

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

We also need to test whether Flash sets the alpha values of the DIB correctly, in which case we don't need alpha recovery at all.

Comment 7

[Oleg Romashin (:romaxa)]

Comment on attachment 477281 [details] [diff] [review]
Make UseAsyncPainting simpler and stop delegating it all around, rev. 1

works fine, and make sense while layers-non-ipc mode not implemented.

Comment 8

[Josh Aas]

Comment on attachment 477151 [details] [diff] [review]
Part A, use bools, fix styling, rev. 1

>-    void UpdateWindowAttributes(PRBool aForceSetWindow = PR_FALSE);
>+    void UpdateWindowAttributes(bool aForceSetWindow = PR_FALSE);

You changed the argument type here but not the default value. Should be "false".

Comment 9

[Benjamin Smedberg]

Attachment: Part A, use bools, fix styling, rev. 1.1

Was that all? No "r+ with this fixed"? :-(

Comment 10

[Tomas]

Any progress?

Comment 11

[Alex]

Pardon my curiosity,
The symptoms in Bug 603040 Comment #3 (TLDR: Plugin gets drawn before page and FF GUI), are they related to this bug (will be fixed w. this)?

Comment 12

[Benjamin Smedberg]

Hera, no, that is unrelated.

Comment 13

[Benjamin Smedberg]

Attachment: Part C - simpify the returning surface in show, rev. 1

Comment 14

[Benjamin Smedberg]

Comment on attachment 483207 [details] [diff] [review]
Part C - simpify the returning surface in show, rev. 1

karlt would be a better reviewer here. Please note that there is one behavior change: I'm no longer calling XSync when returning the old surface. I don't think it should be necessary, because the parent process never writes to that surface, only composites from it... or is it necessary because there might be a pending composite operation?

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 483207 [details] [diff] [review]
Part C - simpify the returning surface in show, rev. 1

>-    if (!SendShow(r, currSurf, &outSurf)) {
>+
>+    // Unused, except to possibly return a shmem to us
>+    SurfaceDescriptor returnSurf;
>+
>+    if (!SendShow(r, currSurf, &returnSurf)) {
>         return false;
>     }
> 
>     nsRefPtr<gfxASurface> tmp = mCurrentSurface;
>     mCurrentSurface = mBackSurface;
>     mBackSurface = tmp;
>     // Outdated back surface... not usable anymore due to changed plugin size.
>     // Dropping obsolete surface

I expect this part is fine, but IIUC outSurf/returnSurf is usually mBackSurface, so it is necessary to ensure that the browser has finished with returnSurf.

(In reply to comment #14)
> Please note that there is one behavior
> change: I'm no longer calling XSync when returning the old surface. I don't
> think it should be necessary, because the parent process never writes to that
> surface, only composites from it... or is it necessary because there might be a
> pending composite operation?

Yes.  The browser-side composite read is pending (in the X server) and XSync ensures that it happens.

The plugin must not delete and should not write to the surface before the X server has completed the composite read requested by the browser.

Comment 16

[Benjamin Smedberg]

Attachment: Async Painting, checkpoint

This is a checkpoint that kinda-works. Our test plugin appears to paint correctly, as does the simple Flash at http://www.webwasp.co.uk/tutorials/a27-transparent/index.php

However, there are the following fairly serious problems:

1) vimeo and hulu refuse to repaint at all. I'm having trouble separating the main video on vimeo (which doesn't work) from the advertising, which works fine.

2) Silverlight doesn't work at all.

3) I haven't hooked up transparency at all yet. I'll be testing shortly to see whether DIBs with transparency work correctly.

I'll do some more work tomorrow, trying to create better testcases. I'd love assistance where possible, though!

Comment 17

[Benjamin Smedberg]

Attachment: Part D - async painting on Windows (opaque), rev. 1

Comment 18

[Jim Mathies [:jimm]]

(In reply to comment #17)
> Created attachment 485146 [details] [diff] [review]
> Part D - async painting on Windows (opaque), rev. 1

Can you post updated patches? All three fail to apply.

Comment 19

[Benjamin Smedberg]

I'm developing this against projects/maple, all except the latest patch are pushed there, and you should be able to apply the latest on top of it.

Comment 20

[Benjamin Smedberg]

Attachment: Part C review comment 1

Since I'm committing as I go, here is the review comment addressed for part C.

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 485146 [details] [diff] [review]
Part D - async painting on Windows (opaque), rev. 1

> namespace mozilla {
> namespace plugins {
> 
> struct SurfaceDescriptorX11 {
>   int XID;
>   int xrenderPictID;
>   gfxIntSize size;
> };
> 
>+struct SurfaceDescriptorWin {
>+  SharedMemoryHandle handle;
>+  gfxIntSize size;
>+};

While we're in here can we fix the warning on win builds for SurfaceDescriptorX11?

> 
>+static const PRUint32 kBytesPerPixel = 4;
>+
> PRUint32
>-SharedDIBWin::SetupBitmapHeader(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aDepth, BITMAPINFOHEADER *aHeader)
>+SharedDIBWin::SetupBitmapHeader(PRUint32 aWidth, PRUint32 aHeight, BITMAPINFOHEADER *aHeader)


nit, move kBytesPerPixel to the top of the file.


I ran through all of my different windowless tests cases and didn't see any anomalies. Looks good!

Comment 22

[Karl Tomlinson (:karlt)]

Comment on attachment 485735 [details] [diff] [review]
Part C review comment 1

/me notices this correctly moves the XSync to before the reply.

Comment 23

[Benjamin Smedberg]

Attachment: Part E - Implement transparency, rev. 1

Comment 24

[Benjamin Smedberg]

Attachment: Part E - Implement transparency, rev. 1.1

Comment 25

[Benjamin Smedberg]

Attachment: Part E - Implement transparency, rev. 1.2

Comment 26

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

So is part E assuming that all plugins will put alpha values in the DIB when they draw?

Comment 27

[Benjamin Smedberg]

Attachment: Part F - remove silverlight positioning quirk, and make setwindow use the real window coordinates with a translation for painting, rev. 1

Comment 28

[Benjamin Smedberg]

Yes, I am assuming/requiring that the painting sets the alpha values. In practice this works with Flash. I'm working on a silverlight testcase, and I don't know of any other plugins that do transparent windowless.

Comment 29

[Benjamin Smedberg]

I'm having a problem with silverlight, the testcase is online here: http://office.smedbergs.us/sltest/silverlight.html

In this test, we are call AsyncSetWindow with bad coordinates because of the following sequence of events:

We call nsObjectFrame::FixupWindow very early on. At this point, window->type is the default windowed mode, because we haven't initialized it yet. So GetWindowOriginInPixels doesn't do the necessary coordinate transformation at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1184

>	xul.dll!nsObjectFrame::FixupWindow(aSize={...})  Line 1043	C++
 	xul.dll!nsObjectFrame::Instantiate(aChannel=0x09039d68, aStreamListener=0x08fbbeac)  Line 2262	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 706	C++
 	xul.dll!nsBaseChannel::OnStartRequest(request=0x098006d0, ctxt=0x00000000)  Line 712	C++
 	xul.dll!nsInputStreamPump::OnStateStart()  Line 441	C++
 	xul.dll!nsInputStreamPump::OnInputStreamReady(stream=0x09172b20)  Line 397	C++
 	xul.dll!nsInputStreamReadyEvent::Run()  Line 113	C++
 	xul.dll!nsThread::ProcessNextEvent(mayWait=0x00000000, result=0x0030d3c4)  Line 547	C++
 	xul.dll!NS_ProcessNextEvent_P(thread=0x007959e8, mayWait=0x00000000)  Line 250	C++
 	xul.dll!mozilla::ipc::MessagePump::Run(aDelegate=0x00799728)  Line 110	C++
 	xul.dll!MessageLoop::RunInternal()  Line 220	C++
 	xul.dll!MessageLoop::RunHandler()  Line 203	C++
 	xul.dll!MessageLoop::Run()  Line 177	C++
 	xul.dll!nsBaseAppShell::Run()  Line 186	C++
 	xul.dll!nsAppShell::Run()  Line 243	C++
 	xul.dll!nsAppStartup::Run()  Line 191	C++
 	xul.dll!XRE_main(argc=0x00000005, argv=0x00612280, aAppData=0x0078f5c0)  Line 3670	C++

Then almost immediately after, we set mPluginWindow->type to windowless at this stack:

>	xul.dll!nsPluginInstanceOwner::CreateWidget()  Line 6166	C++
 	xul.dll!nsPluginHost::InstantiateEmbeddedPlugin(aMimeType=0x0d3666f8, aURL=0x093a7798, aOwner=0x09711898)  Line 1071	C++
 	xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x09039d68, aContext=0x00000000)  Line 588	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 738	C++

We then send AsyncSetWindow with the bad coordinates here:
>	xul.dll!mozilla::plugins::PluginInstanceParent::AsyncSetWindow(aWindow=0x094c4bf4)  Line 532	C++
 	xul.dll!mozilla::plugins::PluginModuleParent::AsyncSetWindow(instance=0x092a397c, window=0x094c4bf4)  Line 624	C++
 	xul.dll!nsNPAPIPluginInstance::AsyncSetWindow(window=0x094c4bf4)  Line 851	C++
 	xul.dll!nsPluginStreamListenerPeer::OnStartRequest(request=0x09039d68, aContext=0x00000000)  Line 610	C++
 	xul.dll!nsObjectLoadingContent::OnStartRequest(aRequest=0x09039d68, aContext=0x00000000)  Line 738	C++

Should we perhaps be forcing a call to nsObjectFrame::FixupWindow again after nsPluginInstanceOwner::CreateWidget? I suspect that this bug is only visible on <object> silverlight, not <embed>, because of the ordering of how streams are handled.

Comment 30

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

yes, I think so.

Comment 31

[Benjamin Smedberg]

Attachment: Part G - Call FixupWindow again, rev. 1

Comment 32

[Jim Mathies [:jimm]]

Comment on attachment 486125 [details] [diff] [review]
Part F - remove silverlight positioning quirk, and make setwindow use the real window coordinates with a translation for painting, rev. 1

Don't forget to remove QUIRK_SILVERLIGHT_WINLESS_INPUT_TRANSLATION from the header.

Comment 33

[[not reading bugmail]]

Wasn't that silverlight quirk added for OOPP bug 547353?

Comment 34

[Benjamin Smedberg]

Attachment: Part H - make silverlight transparent by default, rev. 1

I had to keep the quirk for transparency, but I renamed it.

Comment 35

[Jim Mathies [:jimm]]

Comment on attachment 486183 [details] [diff] [review]
Part H - make silverlight transparent by default, rev. 1

+        // Silverlight assumes it is transparent in windowless moe.

nit - 'moe'

Comment 36

[Jim Mathies [:jimm]]

So to test the whole series on maple, parts E -> H?

Comment 37

[Benjamin Smedberg]

Attachment: Readback from Windows surfaces, rev. 1

Oops, I forgot to attach this earlier, it implements readback for Windows surfaces so that we don't repaint the entire area each time. It goes between E and F.

Comment 38

[Jim Mathies [:jimm]]

Some issues to be worked out, either here or in another bug:

1) the flash context menu is positioned wrong on hulu, and the main banner isn't painting.
2) we lost mouse cursor changes and the context menu on silverlight.net
3) I still see drawing anomalies on that surface site. They tend to show up down at the bottom edge as drawing artifacts. 

We went through all these bugs when we first implemented ipc drawing. I'm not sure if we want to refile and fix all those issues or try to track as many as we can down here before this lands.

Comment 39

[Benjamin Smedberg]

Hulu homepage is bug 606285.

Context menu stuff I'll deal with here, since we aren't calling the standard SetWindow path any more we're skipping our hooks.

I'm not sure about the drawing anomalies, I fixed the only ones I saw with part H.

Comment 40

[Benjamin Smedberg]

Attachment: Part J - Fix testplugin alpha painting, rev. 1

Comment 41

[Benjamin Smedberg]

Attachment: Part K - set up quirks, rev. 1

Comment 42

[Jim Mathies [:jimm]]

Comment on attachment 486332 [details] [diff] [review]
Readback from Windows surfaces, rev. 1

Normally I'd say someone like roc, joe, or jeff should look this over, but since it's a copy paste I suppose I can approve it.

Comment 43

[Jim Mathies [:jimm]]

Comment on attachment 486395 [details] [diff] [review]
Part J - Fix testplugin alpha painting, rev. 1

per irc conversation, need to confirm everything still succeeds for non-async, and non-ipc enabled test runs.

Comment 44

[Karl Tomlinson (:karlt)]

Comment on attachment 486114 [details] [diff] [review]
Part E - Implement transparency, rev. 1.2

I'm assuming you just wanted my review on the mochitest so r+ on that.
I'm not really familiar with the other code, so haven't studied that.
I'd tend to prefer passing an ImageFormat or other enum instead the bool parameter, but I assume you didn't ask my review on that.

Comment 45

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/67f89755fa94 (part A)
http://hg.mozilla.org/mozilla-central/rev/cda406eada98 (part B)
http://hg.mozilla.org/mozilla-central/rev/f409549bea96 (part C)
http://hg.mozilla.org/mozilla-central/rev/94324cad0457 (part D)
http://hg.mozilla.org/mozilla-central/rev/866e9298ee15 (part D bustage)
http://hg.mozilla.org/mozilla-central/rev/425bd6be7289 (part D bustage)
http://hg.mozilla.org/mozilla-central/rev/37188e297b07 (part E)
http://hg.mozilla.org/mozilla-central/rev/88f740a74afc (part I)
http://hg.mozilla.org/mozilla-central/rev/eda1909e1416 (part F)
http://hg.mozilla.org/mozilla-central/rev/900cdb9c8952 (part G)
http://hg.mozilla.org/mozilla-central/rev/c6a351a89dde (part H)
http://hg.mozilla.org/mozilla-central/rev/5d416c488505 (part I bustage)
http://hg.mozilla.org/mozilla-central/rev/ac385c4d7b37 (part J)
http://hg.mozilla.org/mozilla-central/rev/005779cf3e41 (part K)
http://hg.mozilla.org/mozilla-central/rev/9adc8f43d987 (part J painting fix)
http://hg.mozilla.org/mozilla-central/rev/167349934e8d (part E test fix)

And pushed along with bug 583109 and several others at http://hg.mozilla.org/mozilla-central/rev/bdbef533364f

Comment 46

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

If we created a way to pass D3D surfaces via IPC, then we could improve this further by having the plugin-container responsible for uploading the plugin content to D3D when acceleration is enabled --- which would be a big step towards implementing our proposal for D3D plugin rendering with NPAPI. Should we have a followup bug for that?

Comment 47

[Benjamin Smedberg]

Yes. I'm unlikely to implement it, though. I'm also going to file a followup on refactoring this so that in-process plugins use the asynchronous rendering path also.

Comment 48

[Sylvain Pasche]

Could this have caused bug 617480?