801465 - [Azure] Implement DataSourceSurfaceD2D

Status: RESOLVED FIXED

Description

[Nick Cameron [:nrc]]

I did this to try and fix bug 791855, but I figure it might be useful anyway. Not sure how to test the patch at the moment though...

Comment 1

[Nick Cameron [:nrc]]

Attachment: patch

stress testing went well and no crashes or out of control memory etc. I think I remembered to do everything we discussed on IRC.

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 676083 [details] [diff] [review]
patch

Review of attachment 676083 [details] [diff] [review]:
-----------------------------------------------------------------

Start by removing all these releases now that you've changed stuff back to RefPtrs ;)

::: gfx/2d/SourceSurfaceD2D.cpp
@@ +147,5 @@
> +    gfxWarning() << "Failed to create texture. Code: " << hr;
> +    return;
> +  }
> +
> +  IDXGISurface *dxgiSurface;

Make this a RefPtr, you're actually leaking it now.

Comment 3

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=3bc7f59af0ea

Comment 4

[Nick Cameron [:nrc]]

Attachment: patch with changes

+RefPtr, -4*Release

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 676274 [details] [diff] [review]
patch with changes

Review of attachment 676274 [details] [diff] [review]:
-----------------------------------------------------------------

Basically looks good, with a couple of comments.

::: gfx/2d/SourceSurfaceD2D.cpp
@@ +127,5 @@
>  }
>  
> +DataSourceSurfaceD2D::DataSourceSurfaceD2D(const SourceSurfaceD2D& aSourceSurface)
> +  : mTexture(nullptr)
> +  , mSize(aSourceSurface.mSize)

nit: GCC gives compile warnings on initializers not being in the same order as members. I prefer if you pass SourceSurfaceD2D as a pointer here instead of a reference btw. That's more common style for refcounted objects.

@@ +148,5 @@
> +    return;
> +  }
> +
> +  RefPtr<IDXGISurface> dxgiSurface;
> +  hr = sourceTexture->QueryInterface(&dxgiSurface);

Using the & here should not even compile.. but in any case will cause incorrect behaviour.

@@ +186,5 @@
> +  }
> +
> +  aSourceSurface.mDevice->CopyResource(mTexture, sourceTexture);
> +
> +  DrawTargetD2D::mVRAMUsageSS += GetByteSize();

mTexture is not in VRAM. sourceTexture is, but it's lifetime is limited to this function.

@@ +191,5 @@
>  }
> +
> +DataSourceSurfaceD2D::~DataSourceSurfaceD2D()
> +{
> +  if (mTexture) {

&& mMapped

@@ +193,5 @@
> +DataSourceSurfaceD2D::~DataSourceSurfaceD2D()
> +{
> +  if (mTexture) {
> +    mTexture->Unmap(0);
> +    DrawTargetD2D::mVRAMUsageSS -= GetByteSize();

This can go, see comment above.

@@ +202,5 @@
> +DataSourceSurfaceD2D::GetData()
> +{
> +  EnsureMappedTexture();
> +  if (!mMapped) {
> +    return 0;

nit: return nullptr;

@@ +249,5 @@
> +  }
> +}
> +
> +uint32_t
> +DataSourceSurfaceD2D::GetByteSize() const

We can lose this I suppose seeing we don't need to track VRAM.

Comment 6

[Nick Cameron [:nrc]]

Attachment: patch with more changes

Comment 7

[Nick Cameron [:nrc]]

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> @@ +186,5 @@
> > +  }
> > +
> > +  aSourceSurface.mDevice->CopyResource(mTexture, sourceTexture);
> > +
> > +  DrawTargetD2D::mVRAMUsageSS += GetByteSize();
> 
> mTexture is not in VRAM. sourceTexture is, but it's lifetime is limited to
> this function.
> 

So a D3D texture with staging usage is created in CPU memory? I guess that makes sense.

> @@ +191,5 @@
> >  }
> > +
> > +DataSourceSurfaceD2D::~DataSourceSurfaceD2D()
> > +{
> > +  if (mTexture) {
> 
> && mMapped

just if(mMapped) because mTexture should always be non-null if mMapped is true. Doesn't seem worth asserting this though.

Done all the other changes.

Comment 8

[Bas Schouten (:bas.schouten)]

Comment on attachment 676427 [details] [diff] [review]
patch with more changes

Review of attachment 676427 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/2d/SourceSurfaceD2D.cpp
@@ +173,5 @@
> +  renderTarget->DrawBitmap(aSourceSurface->mBitmap,
> +                           D2D1::RectF(0, 0, mSize.width, mSize.height));
> +  renderTarget->EndDraw();
> +
> +  CD3D10_TEXTURE2D_DESC stagingDesc(DXGIFormat(mFormat), mSize.width, mSize.height);

personally I'd just use sourceDesc and modify the CPUAccessFlags, Usage and BindFlags.

@@ +179,5 @@
> +  stagingDesc.CPUAccessFlags = D3D10_CPU_ACCESS_READ;
> +  stagingDesc.Usage = D3D10_USAGE_STAGING;
> +  stagingDesc.BindFlags = 0;
> +  hr = aSourceSurface->mDevice->CreateTexture2D(&stagingDesc, nullptr,
> +                                                byRef(mTexture)); 

nit: whitespace at lineending

Comment 9

[Nick Cameron [:nrc]]

Attachment: patch

with reviewer's changes, carrying r=Bas

Comment 10

[Nick Cameron [:nrc]]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0ab79ed80302

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ab79ed80302