This bug is about making the code in bug 651192 work again. It was backed out in bug 1072528. All I've done so far is to rebase the old patches so that they compile on Linux. Most of the meat has been commented out. David and I discussed an alternate design for actually getting the surfaces to the screen, so there doesn't seem to be much point in retaining those aspects.

Same as Bill's patch, with a few changes: (1) #if 0'd code removed, Async API stubbed out. (2) Removed old gfx implementation code. (3) Querying the drawing model now calls into the parent. (4) #ifdef XP_WIN checks have been moved into gfxPlatform callbacks.

This version is more minimal. It also renames "async drawing" to "direct drawing". Not the best name but I'd rather have that than be ambiguous with "async rendering".

This is the old test plugin, cleaned up just a little and will now fail to load if it doesn't get its direct drawing model. It doesn't really test didComposite yet, I'll do that in another iteration.

correct patch

This makes async and direct drawing mutually exclusive. When direct drawing is enabled, we won't fire paint events since the plugin is responsible for painting off DidComposite. We also assert that the plugin process does not attempt to present a surface using the async rendering path.

This patch implements NPDrawingModelAsyncBitmapSurface. It's very basic since this is still a prototype: each SetCurrent call will synchronously copy the entire surface to avoid ownership issues with the compositor. We also update the image on the main thread, meaning we won't see it until the next paint and then the next composite. That said, it works with the test plugin.

New version fixes GetImageSize() for direct drawing.

This version uses async ImageContainers for direct drawing, which means we should be able to avoid the InvalidateRect call and should get a faster transaction to the compositor via the ImageBridge thread. Originally I thought we couldn't do this, but since we're copying the image on the main thread it should be fine.

This patch allows us to synchronously open another protocol rather than having to wait for a message to echo back in our own queue. It's not finished.

This patch creates a separate PluginImageBridge. When we set up direct drawing, this new protocol is created. In the child, it runs on the main thread and can be accessed via pluginInstanceChild->mImageBridge. In the parent, it's hooked up to the image bridge thread. However, the code in PluginImageBridgeParent::Create runs on the main thread. I think that would be a good place to get the ImageContainer so that we can save it for later. The PluginImageBridgeParent::RecvShowDirectBitmap runs on the image bridge thread, so we can use the image container there and do whatever synchronous layer transactions we need. There are probably a lot of bugs here, especially in teardown. Hopefully it's enough to unblock you on the graphics side David.

All the API pieces needed, with the endpoints left unimplemented. For reference: https://wiki.mozilla.org/NPAPI:AsyncDrawing

This ensures that when we're direct rendering, we don't call async rendering callbacks (such as sending a WM_PAINT).

Nical, the goal of this patch (and the next) is to make TextureClient+Image pairing easier. Right now you have to decide a TextureClient derivative ahead of time, and then hardcode the type of Image you'll use. This, for example, is what the media code does. Instead, I'd like to have TextureClient::CreateForDrawing be able to return the optimal compositable surface, and then have an Image type that directly wraps that. This greatly simplifies working with drawable surfaces and Images on the main thread.

This patch adds a new TextureAllocationFlag, called ALLOC_FOR_OUT_OF_BAND_CONTENT. The intent is twofold: (1) Make a TextureClient that can used to send layers transactions on the main thread, outside of ShadowLayerForwarder, so we don't have to wait for a content paint to update an ImageContainer. This basically entails making sure the texture can be locked without a SyncObject. (2) Bypass Moz2D backend checks when deciding what kind of textures to use on the main thread. This is so we can get D3D11 textures when D2D is not available.

This patch implements the direct bitmap drawing model. The surface for this is allocated on the child side in Shmem. When the plugin wants to present it as the current surface, we grab a TextureClient, upload the Shmem, then ship it off in a TextureWrapperImage.

This implements the DXGI model. It works the same as the Bitmap model, except we special case TextureClientD3D11 to get an optimal surface upload. Also different from the Bitmap model: the parent process is responsible for allocating and finalizing the surface. This patch requires D3D10 for creating the surface, but if it's okay to share a D3D11 texture with D3D10, I can drop that restriction. (I assume when we drop D2D 1.0 we will stop creating a D3D10 device.)

This implements the DidComposite callback. As far as I can tell, Adobe does not use it. If we end up confirming that they won't, I'll drop this patch and excise all the relevant pieces from part 1.

Same as the previous patch, except now we don't try to get a borrowed DrawTarget in TextureClientD3D11::Lock() for out-of-band content textures. This is to decouple TextureClientD3D11 from needing D2D1 on the main thread. (It's worth nothing that gfxWindowsPlatform will only create a D3D11 content device if we use D2D1, but I'll file a separate bug for that.)

Simpler version of the previous patch: removes parent-process dependence on D3D10.

Comment on attachment 8683508 [details] [diff] [review] part 4, introduce TextureWrapperImage Review of attachment 8683508 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/TextureWrapperImage.h @@ +18,5 @@ > +class TextureWrapperImage final : public Image > +{ > +public: > + TextureWrapperImage(TextureClient* aClient, > + const gfx::IntSize& aSize, Do we need to support mSize being different from aClient->GetSize() ? If not please remove this parameter and use aClient->GetSize instead

Comment on attachment 8683517 [details] [diff] [review] part 6, bitmap model Review of attachment 8683517 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/ipc/PPluginInstance.ipdl @@ +183,5 @@ > async NPN_InvalidateRect(NPRect rect); > > + sync RevokeCurrentDirectSurface(); > + > + sync ShowDirectBitmap(Shmem buffer, Comments! @@ +186,5 @@ > + > + sync ShowDirectBitmap(Shmem buffer, > + SurfaceFormat format, > + IntSize size, > + IntRect damage); I'd prefer 'dirty' or 'invalid' since that's more common terminology for gecko. ::: dom/plugins/ipc/PluginInstanceChild.cpp @@ +2717,5 @@ > + : IntRect(IntPoint(0, 0), bitmap->mSize); > + > + // Need a holder since IPDL zaps the object for mysterious reasons. > + Shmem shmemHolder = bitmap->mShmem; > + SendShowDirectBitmap(shmemHolder, bitmap->mFormat, bitmap->mSize, damage); Assert that the stride is what we expect, or add a stride parameter (see PluginInstanceParent comments). ::: dom/plugins/ipc/PluginInstanceParent.cpp @@ +610,5 @@ > + if (!container) { > + return true; > + } > + > + gfx::IntRect invalid(gfx::IntPoint(0, 0), container->GetCurrentSize()); What is this used for? @@ +621,5 @@ > +bool > +PluginInstanceParent::RecvShowDirectBitmap(Shmem&& buffer, > + const SurfaceFormat& format, > + const IntSize& size, > + const IntRect& rect) dirty/invalid again. @@ +624,5 @@ > + const IntSize& size, > + const IntRect& rect) > +{ > + // Validate format. > + if (format != SurfaceFormat::B8G8R8A8 && format != SurfaceFormat::B8G8R8X8) { Can we assert this too? @@ +637,5 @@ > + { > + return false; > + } > + > + uint32_t stride = size.width * bytesPerPixel; We can't usually assume that the stride is the same as width*bpp, since callers sometimes want to align their rows and add padding. Given that we control the only caller, and bpp is always 4 this probably isn't important, but we should make sure that we document it clearly (or add a stride parameter to the ipdl call). ::: gfx/thebes/gfxUtils.h @@ +361,5 @@ > + * multiply is stored in the outparam and true is returned. Otherwise, > + * returns false. > + */ > +static inline bool > +BytesForSizeWidthDepth(int aWidth, int aHeight, unsigned aBytesPerPixel, size_t* aOut) I don't really understand this name; size, width and depth? How about SafeBytesForBitmap? We also have CheckedInt<T> in mfbt which you can use to implement the overflow checks. http://mxr.mozilla.org/mozilla-central/source/dom/canvas/CanvasRenderingContext2D.cpp#5165 Maybe CheckedInt<uint32_t> BytesForBitmap(width, height, bpp); ::: gfx/thebes/gfxWindowsPlatform.h @@ +292,5 @@ > static mozilla::Atomic<size_t> sD3D9MemoryUsed; > static mozilla::Atomic<size_t> sD3D9SharedTextureUsed; > > void GetDeviceInitData(mozilla::gfx::DeviceInitData* aOut) override; > + Trailing whitespace!

Comment on attachment 8683521 [details] [diff] [review] part 8, DidComposite Review of attachment 8683521 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/ipc/CompositorParent.cpp @@ +2159,5 @@ > TimeStamp& aCompositeStart, > TimeStamp& aCompositeEnd) > { > sIndirectLayerTreesLock->AssertCurrentThreadOwns(); > + if (LayerTransactionParent *layerTree = sIndirectLayerTrees[aId].mLayerTree) { Do we want the same change to CompositorParent::DidComposite for things running in the parent process? ::: layout/generic/nsPluginFrame.cpp @@ +1504,5 @@ > + mInstanceOwner->UseAsyncRendering()) > + { > + RefPtr<ClientLayerManager> lm = aBuilder->GetWidgetLayerManager()->AsClientLayerManager(); > + if (!mDidCompositeObserver) { > + mDidCompositeObserver = new PluginFrameDidCompositeObserver(mInstanceOwner, lm); If lm has changed, then we'll add the observer to the new one, but leave mLayerManager holding a ref to the old one. Compare them and remove the old observer and update mLayerManager to the new one.

Comment on attachment 8683959 [details] [diff] [review] part 7, dxgi model Review of attachment 8683959 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/ipc/D3DSurfaceHolder.cpp @@ +18,5 @@ > +D3DSurfaceHolder::D3DSurfaceHolder(ID3D11Device* device11, > + ID3D11Texture2D* back, > + SurfaceFormat format, > + const IntSize& size) > + : mDevice11(device11), Could just initialize this using GetD3D11ContentDevice() instead of having a param, since that's what we verify with below. @@ +51,5 @@ > + return false; > + } > + > + RefPtr<ID3D11DeviceContext> context; > + mDevice11->GetImmediateContext(getter_AddRefs(context)); Assert NS_IsMainThread since we're using the d3d11 content device. @@ +76,5 @@ > + NS_WARNING("Could not acquire DXGI surface lock - plugin forgot to release?"); > + return false; > + } > + > + context->CopyResource(client->GetD3D11Texture(), mBack); Since CopyResource doesn't return an error code, it might be worth verifying some of the restrictions manually. We could check that aClient matches the size we expect etc (assertions seems fine) @@ +78,5 @@ > + } > + > + context->CopyResource(client->GetD3D11Texture(), mBack); > + > + if (mutex) { 'mutex' is guaranteed to be non-null here, right? ::: dom/plugins/ipc/PPluginInstance.ipdl @@ +193,5 @@ > SurfaceFormat format, > IntSize size, > IntRect damage); > + sync ShowDirectD3D10Surface(WindowsHandle handle, > + IntRect damage); invalid/dirty again. ::: dom/plugins/ipc/PluginInstanceChild.cpp @@ +2776,5 @@ > + } > + > + IntRect damage = changed > + ? IntRect(changed->left, changed->top, > + changed->right - changed->left, changed->bottom - changed->top) Helper for NPRect -> IntRect since we're doing this twice now? ::: gfx/layers/client/TextureClient.h @@ +480,5 @@ > /** > * Track how much of this texture is wasted. > * For example we might allocate a 256x256 tile but only use 10x10. > */ > + void SetWaste(int aWasteArea) { What changed in this section? It looks identical.. line endings?

(In reply to Matt Woodrow (:mattwoodrow) from comment #26) > ::: gfx/layers/client/TextureClient.h > @@ +480,5 @@ > > /** > > * Track how much of this texture is wasted. > > * For example we might allocate a 256x256 tile but only use 10x10. > > */ > > + void SetWaste(int aWasteArea) { > > What changed in this section? It looks identical.. line endings? Indent was off by one space.

w/ comments addressed

The NPAPI changes need to be run through the plugin-futures gauntlet again. Please submit the proposal as per https://wiki.mozilla.org/NPAPI

(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #29) > The NPAPI changes need to be run through the plugin-futures gauntlet again. > Please submit the proposal as per https://wiki.mozilla.org/NPAPI Why? This feature was already accepted. It's just not implemented in our tree.

(In reply to David Anderson [:dvander] from comment #30) > (In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #29) > > The NPAPI changes need to be run through the plugin-futures gauntlet again. > > Please submit the proposal as per https://wiki.mozilla.org/NPAPI > > Why? This feature was already accepted. It's just not implemented in our > tree. Last week I asked Josh whether or not your patch required it. I'll forward you his responses.

This patch marshals DXGI_ADAPTER_DESC through ParamTraits instead of an IPDL struct. This makes it easier to send it verbatim. Unfortunately it's also a little annoying, since we can't include <dxgi.h> in many places (it leaks random weirdness into the global namespace), and IPDL is very #include-happy for protocol headers. So instead I duplicated the struct with a different name.

Dual-GPU systems can cause the plugin process to bind to a different DXGI adapter than the parent process. In theory this kind of cross-GPU texture sharing is supposed to work, but mostly it just causes havoc. This patch adds a new NPN_GetValue query, NPNVpreferredDXGIAdapter, which returns the DXGI_ADAPTER_DESC of the browser's DXGI adapter. Plugins are responsible for using IDXGIFactory1::EnumAdapters to find an adapter with a matching LUID, and if none is found, to request a different drawing model.

Comment on attachment 8687629 [details] [diff] [review] part 9, marshal DXGI_ADAPTER_DESC Review of attachment 8687629 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/ipc/D3DMessageUtils.cpp @@ +15,5 @@ > +} > + > +#if defined(XP_WIN) > +static_assert(sizeof(DxgiAdapterDesc) == sizeof(DXGI_ADAPTER_DESC), > + "DXGI_ADAPTER_DESC doe snot match DxgiAdapterDesc"); Space in the wrong place.

In bug 1183910 (or something) we stopped creating a D3D11 content device if D2D was blocked or unavailable. The direct drawing API wants to be able to allocate a DXGI surface regardless of D2D support, so I'd like to revert this change, and always create the D3D11 content device. Alternately we could do it on demand the first time a plugin requests a DXGI surface, maybe that would be better.

Comment on attachment 8683503 [details] [diff] [review] part 2, testplugin changes Review of attachment 8683503 [details] [diff] [review]: ----------------------------------------------------------------- OK with comments addressed. ::: dom/plugins/test/testplugin/nptest.cpp @@ +622,5 @@ > + memcpy(&premultiplied, subpixels, sizeof(premultiplied)); > + > + for (uint32_t* lastPixel = pixelData + instanceData->backBuffer->size.width * instanceData->backBuffer->size.height; > + pixelData < lastPixel; > + ++pixelData) Style nit: curly braces please @@ +2909,5 @@ > + drawAsyncBitmapColor(id); > + } > +#ifdef XP_WIN > + else if (id->asyncDrawing == AD_DXGI) { > + //pluginDrawAsyncDxgiColor(id); This looks wrong. Please fix.

Comment on attachment 8683502 [details] [diff] [review] part 1, NPAPI stub code Review of attachment 8683502 [details] [diff] [review]: ----------------------------------------------------------------- r=me provided that SDK pull request is accepted and comments addressed ::: dom/plugins/base/nsNPAPIPlugin.cpp @@ +2788,5 @@ > +NPError > +_initasyncsurface(NPP instance, NPSize *size, NPImageFormat format, void *initData, NPAsyncSurface *surface) > +{ > + nsNPAPIPluginInstance *inst = (nsNPAPIPluginInstance *)instance->ndata; > + if (!inst) Curly braces please @@ +2798,5 @@ > +NPError > +_finalizeasyncsurface(NPP instance, NPAsyncSurface *surface) > +{ > + nsNPAPIPluginInstance *inst = (nsNPAPIPluginInstance *)instance->ndata; > + if (!inst) ditto @@ +2808,5 @@ > +void > +_setcurrentasyncsurface(NPP instance, NPAsyncSurface *surface, NPRect *changed) > +{ > + nsNPAPIPluginInstance *inst = (nsNPAPIPluginInstance *)instance->ndata; > + if (!inst) ditto ::: dom/plugins/base/nsNPAPIPluginInstance.cpp @@ +1637,5 @@ > +NPError > +nsNPAPIPluginInstance::InitAsyncSurface(NPSize *size, NPImageFormat format, > + void *initData, NPAsyncSurface *surface) > +{ > + if (mOwner) Curly braces please @@ +1646,5 @@ > + > +NPError > +nsNPAPIPluginInstance::FinalizeAsyncSurface(NPAsyncSurface *surface) > +{ > + if (mOwner) ditto @@ +1655,5 @@ > + > +void > +nsNPAPIPluginInstance::SetCurrentAsyncSurface(NPAsyncSurface *surface, NPRect *changed) > +{ > + if (mOwner) ditto ::: dom/plugins/ipc/PluginInstanceParent.cpp @@ +441,5 @@ > + break; > +#endif > + case NPDrawingModelAsyncBitmapSurface: > + allowed = AllowDirectBitmapSurfaceDrawing(); > + break; Please add a default case to this switch statement.

Whoops, wrong patch.

Comment on attachment 8692165 [details] [diff] [review] part 10, adapter negotiation API Matt, could you check the gfx code in PluginInstanceParent?

Comment on attachment 8688115 [details] [diff] [review] part 11, always create a D3D11ContentDevice Review of attachment 8688115 [details] [diff] [review]: ----------------------------------------------------------------- Should be ok.

Comment on attachment 8692165 [details] [diff] [review] part 10, adapter negotiation API Review of attachment 8692165 [details] [diff] [review]: ----------------------------------------------------------------- r+ provided that NPNVpreferredDXGIAdapter is accepted as part of SDK pull request.

https://hg.mozilla.org/integration/mozilla-inbound/rev/857633d4a2b5 https://hg.mozilla.org/integration/mozilla-inbound/rev/71328ca2c9c0 https://hg.mozilla.org/integration/mozilla-inbound/rev/9e77cc23c491 https://hg.mozilla.org/integration/mozilla-inbound/rev/1079b6b580c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/1b245f8b7fea https://hg.mozilla.org/integration/mozilla-inbound/rev/ea55c2c5a81d https://hg.mozilla.org/integration/mozilla-inbound/rev/86b8d0183c5d https://hg.mozilla.org/integration/mozilla-inbound/rev/5c38d2f6fb93 https://hg.mozilla.org/integration/mozilla-inbound/rev/7c063ffd2e22 https://hg.mozilla.org/integration/mozilla-inbound/rev/65f787c9fd4e

https://hg.mozilla.org/mozilla-central/rev/857633d4a2b5 https://hg.mozilla.org/mozilla-central/rev/71328ca2c9c0 https://hg.mozilla.org/mozilla-central/rev/9e77cc23c491 https://hg.mozilla.org/mozilla-central/rev/1079b6b580c2 https://hg.mozilla.org/mozilla-central/rev/1b245f8b7fea https://hg.mozilla.org/mozilla-central/rev/ea55c2c5a81d https://hg.mozilla.org/mozilla-central/rev/86b8d0183c5d https://hg.mozilla.org/mozilla-central/rev/5c38d2f6fb93 https://hg.mozilla.org/mozilla-central/rev/7c063ffd2e22 https://hg.mozilla.org/mozilla-central/rev/65f787c9fd4e