Last Comment Bug 734404 - Implement NPAPI Async accelerated DXGI model
: Implement NPAPI Async accelerated DXGI model
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla15
Assigned To: Bas Schouten (:bas.schouten)
:
Mentors:
Depends on: 759643 821392
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-09 08:57 PST by Bas Schouten (:bas.schouten)
Modified: 2012-12-13 10:42 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add support for DXGI shared surface handles to image layers (12.24 KB, patch)
2012-03-09 08:57 PST, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Add support for DXGI shared surface handles to image layers v2 (13.28 KB, patch)
2012-05-07 08:26 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 1: Add support for DXGI shared surface handles to image layers v3 (13.35 KB, patch)
2012-05-23 13:06 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 1: Add support for DXGI shared surface handles to image layers v4 (13.96 KB, patch)
2012-05-23 13:59 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 2: Integrate DXGI shared surface model into plugin code. (14.04 KB, patch)
2012-05-23 17:33 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 3: Add code to test plugin to support DXGI async drawing. (11.62 KB, patch)
2012-05-23 17:34 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 4: Add reftests for DXGI async drawing model (3.75 KB, patch)
2012-05-23 17:38 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 2: Integrate DXGI shared surface model into plugin code. v2 (17.05 KB, patch)
2012-05-24 09:08 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review
Part 3: Add code to test plugin to support DXGI async drawing. v2 (12.84 KB, patch)
2012-05-24 09:30 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 3: Add code to test plugin to support DXGI async drawing. v3 (12.89 KB, patch)
2012-05-24 16:14 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 3: Add code to test plugin to support DXGI async drawing. v4 (13.02 KB, patch)
2012-05-24 17:00 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Review
Part 3: Add code to test plugin to support DXGI async drawing. v5 (13.35 KB, patch)
2012-05-24 17:28 PDT, Bas Schouten (:bas.schouten)
roc: review+
Details | Diff | Review

Description Bas Schouten (:bas.schouten) 2012-03-09 08:57:14 PST
Created attachment 604426 [details] [diff] [review]
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) (Exited; email my personal email if necessary) 2012-03-09 09:12:26 PST
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) 2012-03-09 09:53:23 PST
(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) (Exited; email my personal email if necessary) 2012-03-09 13:28:31 PST
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) 2012-05-07 08:26:13 PDT
Created attachment 621612 [details] [diff] [review]
Add support for DXGI shared surface handles to image layers v2
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-07 16:02:50 PDT
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) 2012-05-11 13:19:18 PDT
(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) (Exited; email my personal email if necessary) 2012-05-11 15:57:58 PDT
(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) 2012-05-23 13:06:31 PDT
Created attachment 626567 [details] [diff] [review]
Part 1: Add support for DXGI shared surface handles to image layers v3
Comment 9 Bas Schouten (:bas.schouten) 2012-05-23 13:59:52 PDT
Created attachment 626591 [details] [diff] [review]
Part 1: Add support for DXGI shared surface handles to image layers v4

Properly qref!
Comment 10 Bas Schouten (:bas.schouten) 2012-05-23 17:33:48 PDT
Created attachment 626651 [details] [diff] [review]
Part 2: Integrate DXGI shared surface model into plugin code.
Comment 11 Bas Schouten (:bas.schouten) 2012-05-23 17:34:46 PDT
Created attachment 626653 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing.
Comment 12 Bas Schouten (:bas.schouten) 2012-05-23 17:38:12 PDT
Created attachment 626654 [details] [diff] [review]
Part 4: Add reftests for DXGI async drawing model
Comment 13 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-23 19:46:44 PDT
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) (Exited; email my personal email if necessary) 2012-05-23 20:00:48 PDT
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) 2012-05-24 09:08:51 PDT
Created attachment 626841 [details] [diff] [review]
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) 2012-05-24 09:30:43 PDT
Created attachment 626851 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v2

Updated to address review comments.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-05-24 15:37:05 PDT
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) (Exited; email my personal email if necessary) 2012-05-24 15:41:18 PDT
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) 2012-05-24 16:14:37 PDT
Created attachment 627004 [details] [diff] [review]
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) 2012-05-24 17:00:42 PDT
Created attachment 627033 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v4

Address review comments.
Comment 21 Bas Schouten (:bas.schouten) 2012-05-24 17:28:43 PDT
Created attachment 627044 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v5

Address cleanup concerns.

Note You need to log in before you can comment on or make changes to this bug.