734404 - Implement NPAPI Async accelerated DXGI model

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

Attachment: Add support for DXGI shared surface handles to image layers

We've implemented the NPAPI Async bitmap model, we should also implement the accelerated DXGI model.

Comment 1

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

Comment on attachment 604426 [details] [diff] [review]
Add support for DXGI shared surface handles to image layers

Review of attachment 604426 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ImageLayers.cpp
@@ +519,5 @@
>  }
> +
> +TextureD3D10BackendData*
> +RemoteDXGITextureImage::GetD3D10TextureBackendData(ID3D10Device *aDevice)
> +{

Does this code really need to live in ImageLayers.cpp? Why can't it live in ImageLayerD3D10?

@@ +527,5 @@
> +    
> +    nsRefPtr<ID3D10Device> device;
> +    data->mTexture->GetDevice(getter_AddRefs(device));
> +
> +    if(device == aDevice) {

if (

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #1)
> Comment on attachment 604426 [details] [diff] [review]
> Add support for DXGI shared surface handles to image layers
> 
> Review of attachment 604426 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ImageLayers.cpp
> @@ +519,5 @@
> >  }
> > +
> > +TextureD3D10BackendData*
> > +RemoteDXGITextureImage::GetD3D10TextureBackendData(ID3D10Device *aDevice)
> > +{
> 
> Does this code really need to live in ImageLayers.cpp? Why can't it live in
> ImageLayerD3D10?

I more or less followed the example of the OGL code, I'm not sure if there's a real 'need' for it to live here.

Comment 3

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

Which OGL code? For MacIOSurfaceImage, we have a few stub functions that call functionality in another file only built on Mac. That seems like a better way to go than putting all the code in ImageLayers.cpp, which is what you're doing.

Comment 4

[Bas Schouten (:bas.schouten)]

Attachment: Add support for DXGI shared surface handles to image layers v2

Comment 5

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

Comment on attachment 621612 [details] [diff] [review]
Add support for DXGI shared surface handles to image layers v2

Review of attachment 621612 [details] [diff] [review]:
-----------------------------------------------------------------

What about the other layer managers supported on Windows --- BasicLayers and D3D9? Seems to me that if someone sends us a DXGI surface with those layer managers, we'll go ahead and create the D3D10 Image object and then weird things will happen...

::: gfx/layers/ImageLayers.cpp
@@ +310,5 @@
>        mActiveImage = newImg;
>      }
> +#ifdef XP_WIN
> +    else if (mRemoteData->mType == RemoteImageData::DXGI_TEXTURE_HANDLE &&
> +               mRemoteData->mTextureHandle && !mActiveImage) {

Fix indent

::: gfx/layers/ImageLayers.h
@@ +270,5 @@
> +    RAW_BITMAP,
> +
> +    /**
> +     * This is a format that uses a pointer to a texture do draw directly
> +     * from a shared texture.

Can you say a bit more about what this handle is, how one is obtained and how the lifetime of the underlying resource is managed?

::: gfx/layers/d3d10/ImageLayerD3D10.cpp
@@ +458,5 @@
> +  desc.BindFlags = 0;
> +  desc.Usage = D3D10_USAGE_STAGING;
> +
> +  nsRefPtr<ID3D10Texture2D> softTexture;
> +  device->CreateTexture2D(&desc, NULL, getter_AddRefs(softTexture));

Can this fail?

@@ +466,5 @@
> +
> +  nsRefPtr<gfxImageSurface> surface =
> +    new gfxImageSurface(mSize, mFormat == RemoteImageData::BGRX32 ?
> +                                          gfxASurface::ImageFormatRGB24 :
> +                                          gfxASurface::ImageFormatARGB32);

This can fail, need to handle that

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> Comment on attachment 621612 [details] [diff] [review]
> Add support for DXGI shared surface handles to image layers v2
> 
> Review of attachment 621612 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> What about the other layer managers supported on Windows --- BasicLayers and
> D3D9? Seems to me that if someone sends us a DXGI surface with those layer
> managers, we'll go ahead and create the D3D10 Image object and then weird
> things will happen...


If D3D10 is switched off we no longer have a D3D10 device. Being unable to create that device is also the only reason we would switch off D3D10 ourselves. Once this happens plugins will continue functioning but we fundamentally have no way of drawing it.

When D2D is not enabled we report unsupported when the plugin requests DXGI support.

If this image is drawn through basiclayers while Direct2D -is- enabled GetAsSurface will just do its job and readback into an image surface, and it should work fine.

> ::: gfx/layers/ImageLayers.h
> @@ +270,5 @@
> > +    RAW_BITMAP,
> > +
> > +    /**
> > +     * This is a format that uses a pointer to a texture do draw directly
> > +     * from a shared texture.
> 
> Can you say a bit more about what this handle is, how one is obtained and
> how the lifetime of the underlying resource is managed?

Yes, where would you like me to add those comments?

> ::: gfx/layers/d3d10/ImageLayerD3D10.cpp
> @@ +458,5 @@
> > +  desc.BindFlags = 0;
> > +  desc.Usage = D3D10_USAGE_STAGING;
> > +
> > +  nsRefPtr<ID3D10Texture2D> softTexture;
> > +  device->CreateTexture2D(&desc, NULL, getter_AddRefs(softTexture));
> 
> Can this fail?

Realistically I haven't seen it but I'll add a check.

Comment 7

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

(In reply to Bas Schouten (:bas) from comment #6)
> > ::: gfx/layers/ImageLayers.h
> > @@ +270,5 @@
> > > +    RAW_BITMAP,
> > > +
> > > +    /**
> > > +     * This is a format that uses a pointer to a texture do draw directly
> > > +     * from a shared texture.
> > 
> > Can you say a bit more about what this handle is, how one is obtained and
> > how the lifetime of the underlying resource is managed?
> 
> Yes, where would you like me to add those comments?

In the code I quoted is probably as good a place as any.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add support for DXGI shared surface handles to image layers v3

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Add support for DXGI shared surface handles to image layers v4

Properly qref!

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Integrate DXGI shared surface model into plugin code.

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add code to test plugin to support DXGI async drawing.

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Add reftests for DXGI async drawing model

Comment 13

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

Comment on attachment 626651 [details] [diff] [review]
Part 2: Integrate DXGI shared surface model into plugin code.

Review of attachment 626651 [details] [diff] [review]:
-----------------------------------------------------------------

I'm a bit concerned that we have no way for a plugin to clean up AsyncSurfaces that it's not using anymore.

::: dom/plugins/ipc/PluginInstanceParent.h
@@ +336,5 @@
>      HWND               mPluginHWND;
>      WNDPROC            mPluginWndProc;
>      bool               mNestedEventState;
> +
> +    nsRefPtrHashtable<nsPtrHashKey<void>, ID3D10Texture2D> mTextureMap;

Comment that this is used to automatically release the texture(s) when this object goes away.

Comment 14

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

Comment on attachment 626653 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing.

Review of attachment 626653 [details] [diff] [review]:
-----------------------------------------------------------------

You need to add something to dom/plugins/test/testplugin/README to explain the async rendering mode parameters.

Also, the fact that you only support solid-color drawing and not the plugin user-agent-text drawing option needs to be documented. Possibly also checked in code.

::: dom/plugins/test/testplugin/nptest_windows.cpp
@@ +133,5 @@
> +      NPN_MemFree(instanceData->backBuffer);
> +    }
> +
> +    if (!instanceData->platformData->device) {
> +

Move the get-device code into a helper function

Comment 15

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Integrate DXGI shared surface model into plugin code. v2

Re-requesting review as this update addresses both review concerns, and so adds a little bit of IPC code.

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add code to test plugin to support DXGI async drawing. v2

Updated to address review comments.

Comment 17

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

Comment on attachment 626841 [details] [diff] [review]
Part 2: Integrate DXGI shared surface model into plugin code. v2

Review of attachment 626841 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +2457,5 @@
>          }
> +#ifdef XP_WIN
> +    case NPDrawingModelAsyncWindowsDXGISurface: {
> +            
> +            {

Remove blank line above

Comment 18

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

Comment on attachment 626851 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v2

Review of attachment 626851 [details] [diff] [review]:
-----------------------------------------------------------------

I think pluginInstanceShutdown needs to finalize surfaces and release the device.

::: dom/plugins/test/testplugin/README
@@ +26,5 @@
> +* asyncmodel="bitmap"
> +The plugin will use the NPAPI Async Bitmap drawing model extension.
> +
> +* asyncmodel="dxgi"
> +The plugin will use the NPAPI Async DXGI drawing mdoel extension. Only

model

@@ +27,5 @@
> +The plugin will use the NPAPI Async Bitmap drawing model extension.
> +
> +* asyncmodel="dxgi"
> +The plugin will use the NPAPI Async DXGI drawing mdoel extension. Only
> +supported on Windows Vista or higher.

Explain what happens on platforms that don't support the specified model (i.e., we fall back to non-async rendering ... right?)

::: dom/plugins/test/testplugin/nptest.cpp
@@ +944,5 @@
> +#ifdef XP_WIN
> +  else if (instanceData->asyncDrawing == AD_DXGI) {
> +    NPBool supportsAsyncBitmap = false;
> +    if ((NPN_GetValue(instance, NPNVsupportsAsyncWindowsDXGISurfaceBool, &supportsAsyncBitmap) == NPERR_NO_ERROR) &&
> +        supportsAsyncBitmap) {

this bool should not be called supportsAsyncBitmap!

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add code to test plugin to support DXGI async drawing. v3

This fixes a small issue with the former patch.

Comment 20

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add code to test plugin to support DXGI async drawing. v4

Address review comments.

Comment 21

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Add code to test plugin to support DXGI async drawing. v5

Address cleanup concerns.

Comment 22

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0131d14e2c91
https://hg.mozilla.org/integration/mozilla-inbound/rev/671dee55dab0
https://hg.mozilla.org/integration/mozilla-inbound/rev/93908fff3b0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/96b85fac1243

Comment 23

[Ed Morley [:emorley]]

Please can you set the milestone when landing. Thanks :-)

https://hg.mozilla.org/mozilla-central/rev/0131d14e2c91
https://hg.mozilla.org/mozilla-central/rev/671dee55dab0
https://hg.mozilla.org/mozilla-central/rev/93908fff3b0c
https://hg.mozilla.org/mozilla-central/rev/96b85fac1243