593268 - CanvasLayerD3D9 should use D2D Interop

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

We should allow CanvasLayerD3D9 to interop while Canvas is drawing to a D2D surface. We decided to do this by exposing a CreateOptimalSurface from LayerManager, so that layer managers can choose to expose surfaces specifically suited to be used by the layer manager.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Expose CreateOptimalSurface API on LayerManager

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Make Canvas use CreateOptimalSurface

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Implement and use CreateOptimalSurface for CanvasLayerD3D9

Comment 4

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

I think it would be a little better to have a method LayerManager::CreateCanvasSurface. It's more clear exactly what that means.

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to comment #4)
> I think it would be a little better to have a method
> LayerManager::CreateCanvasSurface. It's more clear exactly what that means.

I think, as discussed on IRC, that there's no real use case for different types. I cannot envision a LayerManager wanting different kinds of optimal surfaces for subtly different users. I believe a general, re-usable Optimal surface is a better idea.

Comment 6

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

Canvas surfaces are read/write. For images you could easily imagine having a write-once-only surface that's optimized by caching the rendered image somewhere where there's no protocol to update it.

Comment 7

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

Comment on attachment 471749 [details] [diff] [review]
Part 1: Expose CreateOptimalSurface API on LayerManager

I think it's a minor point though, so I'll let this go.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add surface types

Comment 9

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 471750 [details] [diff] [review]
Part 2: Make Canvas use CreateOptimalSurface

Hmm, this has a chance of crashing if content is null or if there's no owner document (LayerManagerForDocument assumes a non-null arg).  We can hit that in various cases, including rob arnold's aero peek crazyland.  So need to null check everything along the way and fall back to CreateOffscreenSurface if anything along the way is null.

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Implement and use CreateOptimalSurface for CanvasLayerD3D9 v2

Comment 11

[Bas Schouten (:bas.schouten)]

Attachment: Add surface types v2

Updated to be ready to land, r=me, patch is by jrmuizel.

Comment 12

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Make Canvas use CreateOptimalSurface v2

Updated per review comments and ready to land.

Comment 13

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Make Canvas use CreateOptimalSurface v3

Corrected copy-paste mistake.

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Expose CreateOptimalSurface API on LayerManager - Ready To Land

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 471906 [details] [diff] [review]
Part 3: Implement and use CreateOptimalSurface for CanvasLayerD3D9 v2


>+cairo_user_data_key_t gKeyD3D9Texture;
>+
>+void ReleaseTexture(void *texture)
>+{
>+  ((IDirect3DTexture9*)texture)->Release();
>+}

c++ style cast would be better.


>+  nsRefPtr<gfxD2DSurface> surface =
>+    new gfxD2DSurface(sharedHandle, aFormat == gfxASurface::ImageFormatRGB24 ?
>+      gfxASurface::CONTENT_COLOR : gfxASurface::CONTENT_COLOR_ALPHA);

Maybe CreateOptimalSurface should take a content instead of a surface. (I haven't thought about this...)?

Comment 17

[Bas Schouten (:bas.schouten)]

http://hg.mozilla.org/mozilla-central/rev/87d1bedd44d9
http://hg.mozilla.org/mozilla-central/rev/a2f13b7d1a62
http://hg.mozilla.org/mozilla-central/rev/7ad078323b52
http://hg.mozilla.org/mozilla-central/rev/9f8e55caeca1

Comment 18

[ojab]

These patchset brakes --enable-system-cairo: https://bugzilla.mozilla.org/show_bug.cgi?id=594322