Closed
Bug 801465
Opened 10 years ago
Closed 10 years ago
[Azure] Implement DataSourceSurfaceD2D
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: nrc, Assigned: nrc)
Details
Attachments
(1 file, 3 obsolete files)
5.99 KB,
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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•10 years ago
|
Attachment #676083 -
Flags: review? → review?(bas)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
Try push: https://tbpl.mozilla.org/?tree=Try&rev=3bc7f59af0ea
Assignee | ||
Comment 4•10 years ago
|
||
+RefPtr, -4*Release
Attachment #676083 -
Attachment is obsolete: true
Attachment #676274 -
Flags: review?(bas)
Comment 5•10 years ago
|
||
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•10 years ago
|
||
Attachment #676274 -
Attachment is obsolete: true
Attachment #676427 -
Flags: review?(bas)
Assignee | ||
Comment 7•10 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 8•10 years ago
|
||
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•10 years ago
|
||
with reviewer's changes, carrying r=Bas
Attachment #676427 -
Attachment is obsolete: true
Attachment #676830 -
Flags: review+
Assignee | ||
Comment 10•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=0ab79ed80302
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0ab79ed80302
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•