[Azure] Implement DataSourceSurfaceD2D

RESOLVED FIXED in mozilla19

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

15 Branch
mozilla19
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

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

Comment 1

7 years ago
Posted patch patch (obsolete) — Splinter Review
stress testing went well and no crashes or out of control memory etc. I think I remembered to do everything we discussed on IRC.
Attachment #676083 - Flags: review?
Assignee

Updated

7 years ago
Attachment #676083 - Flags: review? → review?(bas)
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.
Attachment #676083 - Flags: review?(bas) → review-
Assignee

Comment 4

7 years ago
Posted patch patch with changes (obsolete) — Splinter Review
+RefPtr, -4*Release
Attachment #676083 - Attachment is obsolete: true
Attachment #676274 - Flags: review?(bas)
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.
Attachment #676274 - Flags: review?(bas) → review-
Assignee

Comment 6

7 years ago
Posted patch patch with more changes (obsolete) — Splinter Review
Attachment #676274 - Attachment is obsolete: true
Attachment #676427 - Flags: review?(bas)
Assignee

Comment 7

7 years ago
(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 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
Attachment #676427 - Flags: review?(bas) → review+
Assignee

Comment 9

7 years ago
Posted patch patchSplinter Review
with reviewer's changes, carrying r=Bas
Attachment #676427 - Attachment is obsolete: true
Attachment #676830 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0ab79ed80302
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.