Closed Bug 615316 Opened 9 years ago Closed 9 years ago

Should be able to use any ImageContainer with any ImageLayer

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: roc, Assigned: bas.schouten)

References

Details

Attachments

(10 files, 5 obsolete files)

6.05 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.27 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.52 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
4.31 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
6.21 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
2.74 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
602 bytes, patch
jrmuizel
: review+
Details | Diff | Splinter Review
8.65 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
3.67 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
1.93 KB, patch
jrmuizel
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → roc
blocking2.0: --- → ?
Attachment #493884 - Attachment description: Part 1: Refactor Surface to texture conversion a bit → Part 1: Refactor Surface to texture conversion a bit for D3D10
Attachment #493884 - Flags: review?(jmuizelaar)
Attachment #493885 - Flags: review?(jmuizelaar)
Attachment #493886 - Flags: review?(jmuizelaar)
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.
Attachment #493884 - Flags: review?(jmuizelaar) → review+
blocking2.0: ? → betaN+
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
Attachment #493885 - Flags: review?(jmuizelaar) → review+
Attachment #493886 - Flags: review?(jmuizelaar) → review+
(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.
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.
Updated to include the possibility of non-Alpha surfaces.
Attachment #493884 - Attachment is obsolete: true
Attachment #496541 - Flags: review?(jmuizelaar)
These patches also address 611635.
Attachment #493885 - Attachment is obsolete: true
Attachment #493886 - Attachment is obsolete: true
Attachment #496543 - Flags: review?(jmuizelaar)
Blocks: 611635
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 */
Attachment #496541 - Flags: review?(jmuizelaar) → review+
Attachment #496542 - Flags: review?(jmuizelaar) → review+
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?
Attachment #496543 - Flags: review?(jmuizelaar) → review-
(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.
This should do the trick. I haven't tested it yet but it should be straightforward.
Attachment #496543 - Attachment is obsolete: true
Attachment #497307 - Flags: review?(jmuizelaar)
Attachment #497307 - Flags: review?(jmuizelaar) → review+
This will help us deal with changing D3D9 layer managers and asynchronous SetData situations.
Attachment #498054 - Flags: review?(jmuizelaar)
Attachment #498057 - Flags: review?(jmuizelaar)
Attachment #498054 - Flags: review?(jmuizelaar) → review+
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?
(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 on attachment 498055 [details] [diff] [review]
Part 5: Factor out SurfaceToTexture in D3D9 Image layers

Figured this out in person
Attachment #498055 - Flags: review?(jmuizelaar) → review+
Attachment #498056 - Flags: review?(jmuizelaar) → review+
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 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?
Attachment #498058 - Flags: review?(jmuizelaar) → review+
Attachment #498059 - Flags: review?(jmuizelaar) → review+
Attachment #498060 - Flags: review?(jmuizelaar) → review+
Yes, but the generated code should be the same anyway, so it's harmless.
Assignee: roc → 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.
Updated as per review comments.
Attachment #498057 - Attachment is obsolete: true
Attachment #498712 - Flags: review?(jmuizelaar)
Attachment #498057 - Flags: review?(jmuizelaar)
Blocks: 619933
Attachment #498712 - Flags: review?(jmuizelaar) → review+
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.
Assignee: bas.schouten → nobody
Depends on: 620669
Is it possible to have changesets for the D3D part?
If those changes landed ( comment 32) that means this bug is fixed?
Assignee: nobody → bas.schouten
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.