CopyableCanvasLayer.cpp has a lot of thebes stuff going on. Making it use Moz2D instead of thebes would also let us get rid of the remaining thebes code in TextureClient.

Bas, is this covered by one of the items in the Moz2D roadmap list: https://wiki.mozilla.org/Platform/GFX#Moz2D_.28Azure.29

Not really directly, no. Although it could be seen under some of the things there. And it's very closely related to another.

Is this a good bug for me to look at, maybe? In that case, you can assign it to me, and I'll start working on it.

Great! Nical, Bas, you can figure out who should mentor this?

I've been having trouble converting some stuff in CanvasLayerD3D10::Initialize to Moz2D: ------ * if (mSurface && mSurface->GetType() == gfxSurfaceType::D2D) { * void *data = mSurface->GetData(&gKeyD3D10Texture); * if (data) { * mTexture = static_cast<ID3D10Texture2D*>(data); mIsD2DTexture = true; device()->CreateShaderResourceView(mTexture, nullptr, getter_AddRefs(mSRView)); mHasAlpha = mSurface->GetContentType() == gfxContentType::COLOR_ALPHA; return; } } ------- So as I understand, it gets an ID3D10Texture2D* that gets sent to CreateShaderResourceView. In Moz2D, the SurfaceType enum has three DirectX values (D2D1_BITMAP, D2D1_DRAWTARGET and D2D1_1_IMAGE). I don't really understand which one (if only one) maps to gfxSurfaceType::D2D. But more than that, I don't understand how to get the d2d texture out of a DataSourceSurface (the lines above with asterisks). Has this not been made yet, or have I just not found it? ;)

CC'ing bjacob since he also wanted to get this work done. DrawTarget::GetNativeSurface(D3D10_TEXTURE); should do what you want here.

Awesome --- I hadn't started on it. Do you want to dupe 966455 and update the dependencies?

This file is currently just a helper for doing PremultiplySurface in Moz2D. It corresponds to an existing Thebes one in the gfxUtils class. Upcoming patches will require this PremultiplySurface method. The existing one in gfxUtils has been renamed internally to DeprecatedPremultiplyTables.

This patch deprecates the UpdateSurface and PaintWithOpacity methods in the CCL class. To do this, many other changes were made in the process. BasicImplData::Paint was deprecated, and its mOperator was ported to Moz2D. This caused changes in several *Layer subclasses. GLScreenBuffer::Readback was deprecated. I want to change the usages of the (now) deprecated functions, so that they use the new Moz2D ones: CanvasClient::Update has been updated, but the big one (BasicLayerManager::PaintSelfOrChildren) will have to be its own project.

I have found that there are no uses of CanvasLayer::Data::mSurface in the whole codebase. In the CanvasLayerD3D10 class, I have therefore removed some lines instead of porting them to Moz2D. I'll make another bugzilla entry proposing to remove the Data::mSurface field entirely. (So these patches might not be checkin-ready even if/when they pass review ;) )

Comment on attachment 8370806 [details] [diff] [review] Port CopyableCanvasLayer to Moz2D Review of attachment 8370806 [details] [diff] [review]: ----------------------------------------------------------------- This is a fairly complex patch and I am not sure about the performance implication and on which case we run this code. Adding Matt to help me with the review :) ::: gfx/layers/CopyableCanvasLayer.cpp @@ +55,5 @@ > + mDeprecatedSurface = > + new gfxImageSurface(ms.mData, > + ThebesIntSize(dataSurface->GetSize()), > + ms.mStride, > + SurfaceFormatToImageFormat(dataSurface->GetFormat())); This looks like we are going to use more memory (and cpu) by keeping both the moz2d and the thebes surface. Maybe we should lazily initialize the deprecated surface when we know for sure we are going to use it (if there is any chance we are not going to use it) or lazily initialize the moz2d one depending on which we are most likely to not use.

Comment on attachment 8370808 [details] [diff] [review] Implement BasicColorLayer::Paint Review of attachment 8370808 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicColorLayer.cpp @@ +46,4 @@ > > virtual void Paint(DrawTarget* aTarget, SourceSurface* aMaskSurface) > { > + if (IsHidden()) nit: please add braces here even though it wasn't in the original code. @@ +57,5 @@ > + ColorPattern pattern(ToColor(mColor)); > + aTarget->MaskSurface(pattern, > + aMaskSurface, > + ToIntRect(GetBounds()).TopLeft(), > + opts); looks like you are forgetting to restore the operation at the end, as the deprecated version is doing with AutoSetOperator.

Comment on attachment 8370810 [details] [diff] [review] Implement BasicImageLayer::Paint Review of attachment 8370810 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/basic/BasicImageLayer.cpp @@ +114,5 @@ > + mContainer->SetImageFactory(mManager->IsCompositingCheap() ? > + nullptr : > + BasicManager()->GetImageFactory()); > + IntSize size; > + Image* image; even if LockCurrentAsSourceSurface will initialize image, please initialize the variable to nullptr here to avoid warnings, people having to check later that things do get initialized, and ultimately a bug if the behavior of LockCurrentAsSourceSurface changes eventually. @@ +122,5 @@ > + if (!surf) { > + return; > + } > + > + SurfacePattern pat(surf, ExtendMode::CLAMP, Matrix(), ToFilter(mFilter)); There seems to be missing logic here. In PaintContext there is some special logic with MOZ_X11 and cairo. Is that intentional?

Comment on attachment 8370806 [details] [diff] [review] Port CopyableCanvasLayer to Moz2D Review of attachment 8370806 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/CopyableCanvasLayer.cpp @@ +52,5 @@ > + RefPtr<DataSourceSurface> dataSurface = mSurface->GetDataSurface(); > + DataSourceSurface::MappedSurface ms; > + dataSurface->Map(DataSourceSurface::MapType::READ, &ms); > + mDeprecatedSurface = > + new gfxImageSurface(ms.mData, dataSurface will be destroyed at the end of this scope and its memory will probably die along with it, so you can't have the gfxImageSurface outlive the dataSurface.

> @@ +122,5 @@ > > + if (!surf) { > > + return; > > + } > > + > > + SurfacePattern pat(surf, ExtendMode::CLAMP, Matrix(), ToFilter(mFilter)); > > There seems to be missing logic here. In PaintContext there is some special > logic with MOZ_X11 and cairo. Is that intentional? I think we need some input from others here (Matt Woodrow?) The old code both checks if we have ifdef'ed MOZ_X11 .. and then checks if the: gfxASurface->GetType() == gfxSurfaceType::Xlib && static_cast<gfxXlibSurface*>(target.get())->IsPadSlow()) I have no idea how to translate that into sensible Moz2D code :-/

This file is currently just a helper for doing PremultiplySurface in Moz2D. It corresponds to an existing Thebes one in the gfxUtils class. An upcoming patch will require this PremultiplySurface method. The existing one in gfxUtils has been renamed internally to DeprecatedPremultiplyTables.

(In reply to Nicolas Silva [:nical] from comment #16) > Comment on attachment 8370806 [details] [diff] [review] > Port CopyableCanvasLayer to Moz2D > > Review of attachment 8370806 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/CopyableCanvasLayer.cpp > @@ +52,5 @@ > > + RefPtr<DataSourceSurface> dataSurface = mSurface->GetDataSurface(); > > + DataSourceSurface::MappedSurface ms; > > + dataSurface->Map(DataSourceSurface::MapType::READ, &ms); > > + mDeprecatedSurface = > > + new gfxImageSurface(ms.mData, > > dataSurface will be destroyed at the end of this scope and its memory will > probably die along with it, so you can't have the gfxImageSurface outlive > the dataSurface. When Bug 968746 is merged, this code path will be removed after the rebase, and we won't have to worry about it

(In reply to Tor Arvid Lund [:torarvid] from comment #17) > > @@ +122,5 @@ > > > + if (!surf) { > > > + return; > > > + } > > > + > > > + SurfacePattern pat(surf, ExtendMode::CLAMP, Matrix(), ToFilter(mFilter)); > > > > There seems to be missing logic here. In PaintContext there is some special > > logic with MOZ_X11 and cairo. Is that intentional? > > I think we need some input from others here (Matt Woodrow?) The old code > both checks if we have ifdef'ed MOZ_X11 > > .. and then checks if the: > > gfxASurface->GetType() == gfxSurfaceType::Xlib && > static_cast<gfxXlibSurface*>(target.get())->IsPadSlow()) > > I have no idea how to translate that into sensible Moz2D code :-/ No sensible Moz2D code exists :) Moz2D intentionally doesn't offer an equivalent to EXTEND_NONE. Previously we were using EXTEND_NONE (even though it gives incorrect results) on old X servers, since it was much faster. I think we might be able to do this conversion within DrawTargetCairo, but not sure what exactly the conditions for it should be. Jeff? Also, the fixed version of x-server shipped in 2009, do we still care about optimizing for it?

(In reply to Matt Woodrow (:mattwoodrow) from comment #20) > No sensible Moz2D code exists :) Moz2D intentionally doesn't offer an > equivalent to EXTEND_NONE. > > Previously we were using EXTEND_NONE (even though it gives incorrect > results) on old X servers, since it was much faster. > > I think we might be able to do this conversion within DrawTargetCairo, but > not sure what exactly the conditions for it should be. Jeff? So, do I understand you correctly in thinking that: If this should be done at all, it should be a general/separate thing inside DrawTargetCairo, and not something specific for this one case in BasicImageLayer?

Exactly

This file is currently just a helper for doing PremultiplySurface in Moz2D. It corresponds to an existing Thebes one in the gfxUtils class. An upcoming patch will require this PremultiplySurface method. The existing one in gfxUtils has been renamed internally to DeprecatedPremultiplyTables.

This patch deprecates the UpdateSurface and PaintWithOpacity methods in the CCL class. To do this, many other changes were made in the process. BasicImplData::Paint was deprecated, and its mOperator was ported to Moz2D. This caused changes in several *Layer subclasses. GLScreenBuffer::Readback was deprecated. I want to change the usages of the (now) deprecated functions, so that they use the new Moz2D ones: CanvasClient::Update has been updated, but the big one (BasicLayerManager::PaintSelfOrChildren) will have to be its own project.

Thanks for the reviews so far. According to the try server (https://tbpl.mozilla.org/?tree=Try&rev=cd9a745a4f19) I have a few failing reftests on Windows XP. It is related to CanvasLayerD3D9. I'll take a whack at that some time next week.

Well, my last message can be disregarded. I have now verified that the reftests in question were failing before my patches, and is indeed failing on vanilla Firefox 27 using Windows + D3D9 (typically Win XP users). Nical, whenever you find the time, you can maybe look at my last remaining patch and then say whether or not you think the series is ready for checkin.

There seemed to be some duplication of code in the UpdateSurface method, as well as an issue with updating that caused some reftests to fail. In this patch, I remove the mSurface member variable from CanvasLayerD3D9.h, and instead call mDrawTarget->Snapshot() as needed. I also reuse the code to draw into the D3DLOCKED_RECT so that we don't duplicate that portion anymore. It seems now to be more similar to the way CanvasLayerD3D10 works. The reftests now pass.

Comment on attachment 8373307 [details] [diff] [review] Simplify (and fix issue) with CanvasLayerD3D9 Review of attachment 8373307 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d9/CanvasLayerD3D9.cpp @@ +69,5 @@ > return; > Painted(); > > + if (mDrawTarget) { > + mDrawTarget->Flush(); Why are we flushing here? I don't imply that it is a mistake, I just want to understand. Flush is an expensive operation so it should only be done when needed. @@ +94,2 @@ > } else { > + surface = mDrawTarget->Snapshot().drop(); Why not place it in a RefPtr? It looks like you are leaking the snapshot. TemporaryRef points to an object that has its refcount already incremented, and if you call drop() manually the ref count is not decremented.

(In reply to Nicolas Silva [:nical] from comment #31) > Comment on attachment 8373307 [details] [diff] [review] > Simplify (and fix issue) with CanvasLayerD3D9 > > Review of attachment 8373307 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/d3d9/CanvasLayerD3D9.cpp > @@ +69,5 @@ > > return; > > Painted(); > > > > + if (mDrawTarget) { > > + mDrawTarget->Flush(); > > Why are we flushing here? I don't imply that it is a mistake, I just want to > understand. Flush is an expensive operation so it should only be done when > needed. Good catch! This was a remnant from my testing when I tried to track down the cause of the isse. And indeed it seems that it's not needed. > @@ +94,2 @@ > > } else { > > + surface = mDrawTarget->Snapshot().drop(); > > Why not place it in a RefPtr? > It looks like you are leaking the snapshot. > TemporaryRef points to an object that has its refcount already incremented, > and if you call drop() manually the ref count is not decremented. Good note. I've added a new revision using a RefPtr now.

https://tbpl.mozilla.org/?tree=Try&rev=40e8860511c3 The reftests look good now

(In reply to Matt Woodrow (:mattwoodrow) from comment #20) > (In reply to Tor Arvid Lund [:torarvid] from comment #17) > > > @@ +122,5 @@ > > > > + if (!surf) { > > > > + return; > > > > + } > > > > + > > > > + SurfacePattern pat(surf, ExtendMode::CLAMP, Matrix(), ToFilter(mFilter)); > > > > > > There seems to be missing logic here. In PaintContext there is some special > > > logic with MOZ_X11 and cairo. Is that intentional? > > > > I think we need some input from others here (Matt Woodrow?) The old code > > both checks if we have ifdef'ed MOZ_X11 > > > > .. and then checks if the: > > > > gfxASurface->GetType() == gfxSurfaceType::Xlib && > > static_cast<gfxXlibSurface*>(target.get())->IsPadSlow()) > > > > I have no idea how to translate that into sensible Moz2D code :-/ > > No sensible Moz2D code exists :) Moz2D intentionally doesn't offer an > equivalent to EXTEND_NONE. > > Previously we were using EXTEND_NONE (even though it gives incorrect > results) on old X servers, since it was much faster. > > I think we might be able to do this conversion within DrawTargetCairo, but > not sure what exactly the conditions for it should be. Jeff? > > Also, the fixed version of x-server shipped in 2009, do we still care about > optimizing for it? This basically seems like bug 951383

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff8f9be1a46 https://hg.mozilla.org/integration/mozilla-inbound/rev/a0862cfdec39 https://hg.mozilla.org/integration/mozilla-inbound/rev/14131014b9a6 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cf4af2659e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/9b7c28e40c1c

https://hg.mozilla.org/mozilla-central/rev/dff8f9be1a46 https://hg.mozilla.org/mozilla-central/rev/a0862cfdec39 https://hg.mozilla.org/mozilla-central/rev/14131014b9a6 https://hg.mozilla.org/mozilla-central/rev/8cf4af2659e9 https://hg.mozilla.org/mozilla-central/rev/9b7c28e40c1c