615316 - Should be able to use any ImageContainer with any ImageLayer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=613744#c6

The current documentation (and implementation) in ImageLayer::SetContainer is bogus. We need to be able to associate any image container with any image layer, to ensure that layer manager changes and fallback BasicLayer rendering works correctly.

We need to change that comment and modify ImageLayerOGL/ImageLayerD3D9/ImageLayerD3D10 to ensure that they can handle situations where the image container is not the expected type. In such situations they should just call GetCurrentAsSurface() and proceed using the surface.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Refactor Surface to texture conversion a bit for D3D10

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Support GetAsSurface for CairoImageD3D10

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Make ImageLayerD3D10 work with any image container

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 493884 [details] [diff] [review]
Part 1: Refactor Surface to texture conversion a bit for D3D10

This looks fine. It would be good if you had mentioned that this added support for using D2D surfaces or done that as a separate change.

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 493885 [details] [diff] [review]
Part 2: Support GetAsSurface for CairoImageD3D10

Is this correct if we've disabled D2D but not D3D10? Or do we never run in that situation?

Also, please include a comment about why it's safe to assume that the texture is of type COLOR_ALPHA

Comment 6

[Bas Schouten (:bas.schouten)]

(In reply to comment #5)
> Comment on attachment 493885 [details] [diff] [review]
> Part 2: Support GetAsSurface for CairoImageD3D10
> 
> Is this correct if we've disabled D2D but not D3D10? Or do we never run in that
> situation?
> 
> Also, please include a comment about why it's safe to assume that the texture
> is of type COLOR_ALPHA

D3D10 layers cannot be used without D2D.

The assumption that the texture and original source surface is of type COLOR_ALPHA has always been there. I don't know the why and this might be faulty but I'm not sure, I'll double check this with roc.

Comment 7

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

CairoImageD3D10::SetData can't assume that the incoming surface is COLOR_ALPHA. But you could easily fix that by taking the copy path for non-COLOR_ALPHA surfaces. Maybe you can do something clever with D3D10 to avoid the extra copy.

It seems like CairoImageD3D10::SetData should be calling gfxASurface::GetAsImageSurface by the way.

It could also do something clever for D2D surfaces, although that probably doesn't matter yet.

Comment 8

[Bas Schouten (:bas.schouten)]

Attachment: Part 1: Refactor Surface to texture conversion a bit for D3D10 v2

Updated to include the possibility of non-Alpha surfaces.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Part 2: Support GetAsSurface for CairoImageD3D10 v2

Comment 10

[Bas Schouten (:bas.schouten)]

Attachment: Part 3: Make ImageLayerD3D10 work with any image container v2

These patches also address 611635.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 496541 [details] [diff] [review]
Part 1: Refactor Surface to texture conversion a bit for D3D10 v2

># HG changeset patch
># Parent a778b53ab83a18bd615f34057bb5a97540ee62dd
>Bug 615316 - Part 1: Factor out SurfaceToTexture conversion for D3D10 Image Layers.
>
>diff --git a/gfx/layers/d3d10/ImageLayerD3D10.cpp b/gfx/layers/d3d10/ImageLayerD3D10.cpp
>--- a/gfx/layers/d3d10/ImageLayerD3D10.cpp
>+++ b/gfx/layers/d3d10/ImageLayerD3D10.cpp
>@@ -32,23 +32,69 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "ImageLayerD3D10.h"
> #include "gfxImageSurface.h"
>+#include "gfxWindowsSurface.h"
> #include "yuv_convert.h"
> 
> namespace mozilla {
> namespace layers {
> 
> using mozilla::MutexAutoLock;
> 
>+static already_AddRefed<ID3D10Texture2D>
>+SurfaceToTexture(ID3D10Device *aDevice,
>+                 gfxASurface *aSurface,
>+                 const gfxIntSize &aSize)
>+{
>+  if (aSurface && aSurface->GetType() == gfxASurface::SurfaceTypeD2D) {
>+    void *data = aSurface->GetData(&gKeyD3D10Texture);
>+    if (data) {
>+      nsRefPtr<ID3D10Texture2D> texture = static_cast<ID3D10Texture2D*>(data);
>+      ID3D10Device *dev;
>+      texture->GetDevice(&dev);
>+      if (dev == aDevice) {
>+        return texture.forget();
>+      }
>+    }
>+  }
>+
>+  nsRefPtr<gfxImageSurface> imageSurface = aSurface->GetAsImageSurface();
>+
>+  if (!imageSurface) {
>+    imageSurface = new gfxImageSurface(aSize,
>+                                       gfxASurface::ImageFormatARGB32);
>+    
>+    nsRefPtr<gfxContext> context = new gfxContext(imageSurface);
>+    context->SetSource(aSurface);
>+    context->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    context->Paint();
>+  }
>+
>+  D3D10_SUBRESOURCE_DATA data;
>+  
>+  CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM,
>+                             imageSurface->GetSize().width,
>+                             imageSurface->GetSize().height,
>+                             1, 1);
>+  desc.Usage = D3D10_USAGE_IMMUTABLE;
>+  
>+  data.pSysMem = imageSurface->Data();
>+  data.SysMemPitch = imageSurface->Stride();
>+
>+  nsRefPtr<ID3D10Texture2D> texture;
>+  aDevice->CreateTexture2D(&desc, &data, getter_AddRefs(texture));
>+  return texture.forget();
>+}
>+
> ImageContainerD3D10::ImageContainerD3D10(LayerManagerD3D10 *aManager)
>   : ImageContainer(aManager)
>   , mDevice(aManager->device())
>   , mActiveImageLock("mozilla.layers.ImageContainerD3D10.mActiveImageLock")
> {
> }
> 
> already_AddRefed<Image>
>@@ -182,20 +228,28 @@ ImageLayerD3D10::RenderLayer()
>         (float)0,
>         (float)yuvImage->mSize.width,
>         (float)yuvImage->mSize.height)
>       );
>   } else if (image->GetFormat() == Image::CAIRO_SURFACE) {
>     CairoImageD3D10 *cairoImage =
>       static_cast<CairoImageD3D10*>(image.get());
> 
>-    if (mFilter == gfxPattern::FILTER_NEAREST) {
>-      technique = effect()->GetTechniqueByName("RenderRGBALayerPremulPoint");
>+    if (cairoImage->mHasAlpha) {
>+      if (mFilter == gfxPattern::FILTER_NEAREST) {
>+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremulPoint");
>+      } else {
>+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremul");
>+      }
>     } else {
>-      technique = effect()->GetTechniqueByName("RenderRGBALayerPremul");
>+      if (mFilter == gfxPattern::FILTER_NEAREST) {
>+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremulPoint");
>+      } else {
>+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremul");
>+      }
>     }
> 
>     if (cairoImage->mSRView) {
>       effect()->GetVariableByName("tRGB")->AsShaderResource()->SetResource(cairoImage->mSRView);
>     }
> 
>     effect()->GetVariableByName("vLayerQuad")->AsVector()->SetFloatVector(
>       ShaderConstantRectD3D10(
>@@ -351,40 +405,27 @@ PlanarYCbCrImageD3D10::GetAsSurface()
> CairoImageD3D10::~CairoImageD3D10()
> {
> }
> 
> void
> CairoImageD3D10::SetData(const CairoImage::Data &aData)
> {
>   mSize = aData.mSize;
>+  NS_ASSERTION(aData.mSurface->GetContentType() != gfxASurface::CONTENT_ALPHA,
>+               "Invalid content type passed to CairoImageD3D10.");
> 
>-  nsRefPtr<gfxImageSurface> imageSurface;
>+  mTexture = SurfaceToTexture(mDevice, aData.mSurface, mSize);
> 
>-  if (aData.mSurface->GetType() == gfxASurface::SurfaceTypeImage) {
>-    imageSurface = static_cast<gfxImageSurface*>(aData.mSurface);
>+  if (aData.mSurface->GetContentType() == gfxASurface::CONTENT_COLOR) {
>+    mHasAlpha = false;
>   } else {
>-    imageSurface = new gfxImageSurface(aData.mSize,
>-                                       gfxASurface::ImageFormatARGB32);
>-    
>-    nsRefPtr<gfxContext> context = new gfxContext(imageSurface);
>-    context->SetSource(aData.mSurface);
>-    context->SetOperator(gfxContext::OPERATOR_SOURCE);
>-    context->Paint();
>+    mHasAlpha = true;
>   }
> 
>-  D3D10_SUBRESOURCE_DATA data;
>-  
>-  CD3D10_TEXTURE2D_DESC desc(DXGI_FORMAT_B8G8R8A8_UNORM, mSize.width, mSize.height, 1, 1);
>-  desc.Usage = D3D10_USAGE_IMMUTABLE;
>-  
>-  data.pSysMem = imageSurface->Data();
>-  data.SysMemPitch = imageSurface->Stride();
>-
>-  mDevice->CreateTexture2D(&desc, &data, getter_AddRefs(mTexture));
>   mDevice->CreateShaderResourceView(mTexture, NULL, getter_AddRefs(mSRView));
> }
> 
> already_AddRefed<gfxASurface>
> CairoImageD3D10::GetAsSurface()
> {
>   return nsnull;
> }
>diff --git a/gfx/layers/d3d10/ImageLayerD3D10.h b/gfx/layers/d3d10/ImageLayerD3D10.h
>--- a/gfx/layers/d3d10/ImageLayerD3D10.h
>+++ b/gfx/layers/d3d10/ImageLayerD3D10.h
>@@ -133,24 +133,26 @@ public:
> 
> class THEBES_API CairoImageD3D10 : public CairoImage,
>                                    public ImageD3D10
> {
> public:
>   CairoImageD3D10(ID3D10Device1 *aDevice)
>     : CairoImage(static_cast<ImageD3D10*>(this))
>     , mDevice(aDevice)
>+    , mHasAlpha(true)
>   { }
>   ~CairoImageD3D10();
> 
>   virtual void SetData(const Data &aData);
> 
>   virtual already_AddRefed<gfxASurface> GetAsSurface();
> 
>   nsRefPtr<ID3D10Device1> mDevice;
>   nsRefPtr<ID3D10Texture2D> mTexture;
>   nsRefPtr<ID3D10ShaderResourceView> mSRView;
>   gfxIntSize mSize;
>+  bool mHasAlpha;
> };
> 
> } /* layers */
> } /* mozilla */
> #endif /* GFX_IMAGELAYERD3D10_H */

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 496543 [details] [diff] [review]
Part 3: Make ImageLayerD3D10 work with any image container v2


>-  if (image->GetFormat() == Image::PLANAR_YCBCR) {
>+  if (GetContainer()->Manager() != Manager()) {
>+    GetContainer()->SetLayerManager(Manager());
>+  }
>+
>+  if (GetContainer()->Manager() != Manager()) {
>+    gfxIntSize size;
>+    nsRefPtr<gfxASurface> surf = GetContainer()->GetCurrentAsSurface(&size);
>+    nsRefPtr<ID3D10Texture2D> texture =
>+      SurfaceToTexture(device(), surf, size);
>+
>+    nsRefPtr<ID3D10ShaderResourceView> srView;
>+    device()->CreateShaderResourceView(texture, NULL, getter_AddRefs(srView));
>+
>+    if (surf->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA) {
>+      if (mFilter == gfxPattern::FILTER_NEAREST) {
>+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremulPoint");
>+      } else {
>+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremul");
>+      }
>+    } else {
>+      if (mFilter == gfxPattern::FILTER_NEAREST) {
>+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremulPoint");
>+      } else {
>+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremul");
>+      }
>+    }
>+
>+    if (srView) {
>+      effect()->GetVariableByName("tRGB")->AsShaderResource()->SetResource(srView);
>+    }
>+
>+    effect()->GetVariableByName("vLayerQuad")->AsVector()->SetFloatVector(
>+      ShaderConstantRectD3D10(
>+        (float)0,
>+        (float)0,
>+        (float)size.width,
>+        (float)size.height)
>+      );

Can we avoid duplicating this hunk of code with what part 1 does?

Comment 13

[Bas Schouten (:bas.schouten)]

(In reply to comment #12)
> Comment on attachment 496543 [details] [diff] [review]
> Part 3: Make ImageLayerD3D10 work with any image container v2
> 
> 
> >-  if (image->GetFormat() == Image::PLANAR_YCBCR) {
> >+  if (GetContainer()->Manager() != Manager()) {
> >+    GetContainer()->SetLayerManager(Manager());
> >+  }
> >+
> >+  if (GetContainer()->Manager() != Manager()) {
> >+    gfxIntSize size;
> >+    nsRefPtr<gfxASurface> surf = GetContainer()->GetCurrentAsSurface(&size);
> >+    nsRefPtr<ID3D10Texture2D> texture =
> >+      SurfaceToTexture(device(), surf, size);
> >+
> >+    nsRefPtr<ID3D10ShaderResourceView> srView;
> >+    device()->CreateShaderResourceView(texture, NULL, getter_AddRefs(srView));
> >+
> >+    if (surf->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA) {
> >+      if (mFilter == gfxPattern::FILTER_NEAREST) {
> >+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremulPoint");
> >+      } else {
> >+        technique = effect()->GetTechniqueByName("RenderRGBALayerPremul");
> >+      }
> >+    } else {
> >+      if (mFilter == gfxPattern::FILTER_NEAREST) {
> >+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremulPoint");
> >+      } else {
> >+        technique = effect()->GetTechniqueByName("RenderRGBLayerPremul");
> >+      }
> >+    }
> >+
> >+    if (srView) {
> >+      effect()->GetVariableByName("tRGB")->AsShaderResource()->SetResource(srView);
> >+    }
> >+
> >+    effect()->GetVariableByName("vLayerQuad")->AsVector()->SetFloatVector(
> >+      ShaderConstantRectD3D10(
> >+        (float)0,
> >+        (float)0,
> >+        (float)size.width,
> >+        (float)size.height)
> >+      );
> 
> Can we avoid duplicating this hunk of code with what part 1 does?

I'll try to combine the two.

Comment 14

[Bas Schouten (:bas.schouten)]

Attachment: Make ImageLayerD3D10 work with any image container v3

This should do the trick. I haven't tested it yet but it should be straightforward.

Comment 15

[Bas Schouten (:bas.schouten)]

Attachment: Part 4: Store device on CairoImageD3D9 instead of manager

This will help us deal with changing D3D9 layer managers and asynchronous SetData situations.

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Part 5: Factor out SurfaceToTexture in D3D9 Image layers

Comment 17

[Bas Schouten (:bas.schouten)]

Attachment: Part 6: Support device changing for CairoImageD3D9

Comment 18

[Bas Schouten (:bas.schouten)]

Attachment: Part 7: Support surfaces with Alpha for CairoImageD3D9

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Part 8: Support GetAsSurface for CairoImageD3D9

Comment 20

[Bas Schouten (:bas.schouten)]

Attachment: Part 9: Support changing devices for YCbCrImageD3D9

Comment 21

[Bas Schouten (:bas.schouten)]

Attachment: Part 10: Support different layer managers in ImageLayerD3D9

Comment 22

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 498055 [details] [diff] [review]
Part 5: Factor out SurfaceToTexture in D3D9 Image layers


>+
>+  nsRefPtr<IDirect3DTexture9> texture;
>+  nsRefPtr<IDirect3DDevice9Ex> deviceEx;
>+  aDevice->QueryInterface(__uuidof(IDirect3DDevice9),
>+                          (void**)getter_AddRefs(deviceEx));

Why the change to IDirect3DDevice9 from IDirect3DDevice9Ex?

Comment 23

[Bas Schouten (:bas.schouten)]

(In reply to comment #22)
> Comment on attachment 498055 [details] [diff] [review]
> Part 5: Factor out SurfaceToTexture in D3D9 Image layers
> 
> 
> >+
> >+  nsRefPtr<IDirect3DTexture9> texture;
> >+  nsRefPtr<IDirect3DDevice9Ex> deviceEx;
> >+  aDevice->QueryInterface(__uuidof(IDirect3DDevice9),
> >+                          (void**)getter_AddRefs(deviceEx));
> 
> Why the change to IDirect3DDevice9 from IDirect3DDevice9Ex?

By checking if the device supports that interface we know if this is a D3D9Ex device. We store this in a boolean on the manager but we can't access that from here.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 498055 [details] [diff] [review]
Part 5: Factor out SurfaceToTexture in D3D9 Image layers

Figured this out in person

Comment 25

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 498057 [details] [diff] [review]
Part 7: Support surfaces with Alpha for CairoImageD3D9

Can you just reuse mCachedSurface->GetContentType() == gfxASurface::CONTENT_COLOR_ALPHA instead of storing additional state? If you want something shorter feel free to create a helper method.

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 498058 [details] [diff] [review]
Part 8: Support GetAsSurface for CairoImageD3D9

When reviewing this I noticed:
  return imageSurface.forget().get();
int PlanarYCbCrImageD3D10::GetAsSurface()

That should probably be:
  return imageSurface.forget();

shouldn't it?

Comment 27

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

Yes, but the generated code should be the same anyway, so it's harmless.

Comment 28

[Bas Schouten (:bas.schouten)]

(In reply to comment #26)
> Comment on attachment 498058 [details] [diff] [review]
> Part 8: Support GetAsSurface for CairoImageD3D9
> 
> When reviewing this I noticed:
>   return imageSurface.forget().get();
> int PlanarYCbCrImageD3D10::GetAsSurface()
> 
> That should probably be:
>   return imageSurface.forget();
> 
> shouldn't it?

Note how this has only been possible since the conversion template was added in http://hg.mozilla.org/mozilla-central/rev/a7160db7abc5.

The D3D9 layers code was written in May, and predates this possibility :-).

But yes, we can change that now if we want.

Comment 29

[Bas Schouten (:bas.schouten)]

Attachment: Part 7: Support surfaces with Alpha for CairoImageD3D9 v2

Updated as per review comments.

Comment 30

[Bas Schouten (:bas.schouten)]

Unassigning this from me. This may need OpenGL work (I'm not sure) but all the D3D work has been done. If this is already fine with OGL we can resolve it.

Comment 31

[Scoobidiver (away)]

Is it possible to have changesets for the D3D part?

Comment 32

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/2d4b1d635e8d
http://hg.mozilla.org/mozilla-central/rev/6cfc50d8629c
http://hg.mozilla.org/mozilla-central/rev/7d05c8f6c4da
http://hg.mozilla.org/mozilla-central/rev/b6e4e41dbee0
http://hg.mozilla.org/mozilla-central/rev/5c0fb0080a5a
http://hg.mozilla.org/mozilla-central/rev/56caf266b854
http://hg.mozilla.org/mozilla-central/rev/f8a49ff98d77
http://hg.mozilla.org/mozilla-central/rev/b71a05bcd2f5

(In reply to comment #30)
> Unassigning this from me. This may need OpenGL work (I'm not sure) but all the
> D3D work has been done. If this is already fine with OGL we can resolve it.

Who could assess if more work is needed?

Comment 33

[Tomas]

If those changes landed ( comment 32) that means this bug is fixed?