The default bug view has changed. See this FAQ.

Implement NPAPI Async accelerated DXGI model

RESOLVED FIXED in mozilla15

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bas, Assigned: bas)

Tracking

unspecified
mozilla15
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

13.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
17.05 KB, patch
roc
: review+
Details | Diff | Splinter Review
13.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
Attachment #604426 - Flags: review?(roc)
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 (
(Assignee)

Comment 2

5 years ago
(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.
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.
(Assignee)

Comment 4

5 years ago
Created attachment 621612 [details] [diff] [review]
Add support for DXGI shared surface handles to image layers v2
Attachment #604426 - Attachment is obsolete: true
Attachment #621612 - Flags: review?(roc)
Attachment #604426 - Flags: review?(roc)
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
(Assignee)

Comment 6

5 years ago
(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.
(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.
(Assignee)

Comment 8

5 years ago
Created attachment 626567 [details] [diff] [review]
Part 1: Add support for DXGI shared surface handles to image layers v3
Attachment #621612 - Attachment is obsolete: true
Attachment #621612 - Flags: review?(roc)
Attachment #626567 - Flags: review?(roc)
(Assignee)

Comment 9

5 years ago
Created attachment 626591 [details] [diff] [review]
Part 1: Add support for DXGI shared surface handles to image layers v4

Properly qref!
Attachment #626567 - Attachment is obsolete: true
Attachment #626567 - Flags: review?(roc)
Attachment #626591 - Flags: review?(roc)
Attachment #626591 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 626651 [details] [diff] [review]
Part 2: Integrate DXGI shared surface model into plugin code.
Attachment #626651 - Flags: review?(roc)
(Assignee)

Comment 11

5 years ago
Created attachment 626653 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing.
Attachment #626653 - Flags: review?(roc)
(Assignee)

Comment 12

5 years ago
Created attachment 626654 [details] [diff] [review]
Part 4: Add reftests for DXGI async drawing model
Attachment #626654 - Flags: review?(roc)
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.
Attachment #626651 - Flags: review?(roc) → review+
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
Attachment #626654 - Flags: review?(roc) → review+
(Assignee)

Comment 15

5 years ago
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.
Attachment #626651 - Attachment is obsolete: true
Attachment #626841 - Flags: review?(roc)
(Assignee)

Comment 16

5 years ago
Created attachment 626851 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v2

Updated to address review comments.
Attachment #626653 - Attachment is obsolete: true
Attachment #626653 - Flags: review?(roc)
Attachment #626851 - Flags: review?(roc)
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
Attachment #626841 - Flags: review?(roc) → review+
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!
(Assignee)

Comment 19

5 years ago
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.
Attachment #626851 - Attachment is obsolete: true
Attachment #626851 - Flags: review?(roc)
Attachment #627004 - Flags: review?(roc)
(Assignee)

Comment 20

5 years ago
Created attachment 627033 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v4

Address review comments.
Attachment #627004 - Attachment is obsolete: true
Attachment #627004 - Flags: review?(roc)
Attachment #627033 - Flags: review?(roc)
(Assignee)

Comment 21

5 years ago
Created attachment 627044 [details] [diff] [review]
Part 3: Add code to test plugin to support DXGI async drawing. v5

Address cleanup concerns.
Attachment #627033 - Attachment is obsolete: true
Attachment #627033 - Flags: review?(roc)
Attachment #627044 - Flags: review?(roc)
Attachment #627044 - Flags: review?(roc) → review+
(Assignee)

Updated

5 years ago
Depends on: 759643
(Assignee)

Comment 22

5 years ago
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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Depends on: 821392
You need to log in before you can comment on or make changes to this bug.