Asynchronous layer-based plugin painting on Windows

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

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

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

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

Any progress?

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

Hera, no, that is unrelated.

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

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!

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

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.

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

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 on attachment 485735 [details] [diff] [review] Part C review comment 1 /me notices this correctly moves the XSync to before the reply.

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

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.

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.

yes, I think so.

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.

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

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

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'

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

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.

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.

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

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

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?

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.

Could this have caused bug 617480?