Implementations of TextureClient::GetAsDrawTarget are using the preferred Moz2D backend for content drawing, which can be different from the canvas backend.

Texture client defaults to using the content backend to create a drawtarget for data. When using it from canvas with skia enabled in content... boom.

waw gal you reacted fast :) ! I should have assigned myself to the bug right away. I have two different approaches to fix this bug locally. The first one is to specify the backend in GetAsDrawTarget but it makes things awkward because it forces us to add some contracts such as "Do not use several backends with a given texture" and some TextureClient don't support some Moz2D backends. My second attempt specifies the backend when creating the texture, which helps picking the right texture type in the first place

Hey nical, I was actually done with the patch when tor arvid mentioned that you had created this bug, so I just attached what I had then :p I used a slightly different approach though, made a new enum for backend mode that can either be content or canvas. Since gfxPlatform has specific methods to explicitly create either a canvas draw target or content draw target, I figured a mode might be ok. So you can just say give me this for canvas and let the gfxPlatform handle the rest. What do you think? Also the patch won't work without the memleak patch in as well, because CreateDrawTargetForData just leaks a skia draw target anyway and proceeds to create a cg one :/

So we have the luxury to choose between 3 fixes for the same bug after just a few hours. Ali, is there a bug for the skia leak? It's bad, we need to land this fix!

This patch is a mix between Andreas' patch and mine. The difference is that with my previous patch we specify the Moz2D backend, while this version (and Andreas' patch) use an enum that is either content or canvas. I think it is best to decide between the backendMode or backend when creating the texture rather than when asking for a DrawTarget. Then between specifying the backend or the backend mode, it's a matter of finer grained control and the additional checks when creating the Texture (my patch) vs nicer API (gal version). I am happy with both. The two patches happen to e fixing webgl on OMCT Windows + new textures, which is busted since yesterday.

Hey nical, yeah it's probably better to specify the backend while creating the texture. But hehe I think there's a little confusion, the patch with backend mode is mine - I believe gal just came and marked it for review ;). So yeah your merger patch sounds good to me! And no there is not another bug for that memleak. I can create one tomorrow morning though and just attach the patch (gal already r+ed it).

Yes. I just walked by and saw a patch that was ready to be stamped. Ali's work and patch :)

Comment on attachment 8377581 [details] [diff] [review] memleak in CreateDrawTargetForData when backend is skia Added a separate bug 974314 for this patch with r=gal.

Comment on attachment 8377555 [details] [diff] [review] Add BackendMode to get DrawTarget Review of attachment 8377555 [details] [diff] [review]: ----------------------------------------------------------------- The patch looks fine to me! But it'd make me a lot happier if the enum would not be inside Types.h but rather inside gfxTypes.h (or something else outside of Moz2D), as Moz2D really isn't 'content' or 'canvas' aware.

Comment on attachment 8377692 [details] [diff] [review] Gal's patch with BackendMode passed when creating the TextureClient instead of GetAsDrawTarget > aSerializer.cpp b/gfx/layers/ImageDataSerializer.cpp >--- a/gfx/layers/ImageDataSerializer.cpp >+++ b/gfx/layers/ImageDataSerializer.cpp >@@ -123,23 +123,24 @@ ImageDataSerializerBase::GetAsThebesSurf > IntSize size = GetSize(); > return new gfxImageSurface(GetData(), > gfxIntSize(size.width, size.height), > GetStride(), > SurfaceFormatToImageFormat(GetFormat())); > } > > TemporaryRef<DrawTarget> >-ImageDataSerializerBase::GetAsDrawTarget() >+ImageDataSerializerBase::GetAsDrawTarget(gfx::BackendMode aBackendMode) > { > MOZ_ASSERT(IsValid()); > return gfxPlatform::GetPlatform()->CreateDrawTargetForData(GetData(), > GetSize(), > GetStride(), >- GetFormat()); >+ GetFormat(), >+ aBackendMode); > } Since the backend mode is going in when creating a BufferTextureClient, you think maybe it should go into ImageDataSerializerBase as well to match up with BufferTextureClient? >+RefPtr<DrawTarget> >+gfxPlatform::CreateCanvasDrawTargetForData(unsigned char* aData, const IntSize& aSize, int32_t aStride, SurfaceFormat aFormat) >+{ >+ NS_ASSERTION(mPreferredCanvasBackend != BackendType::NONE, "No backend."); >+ if (mPreferredCanvasBackend == BackendType::CAIRO) { >+ nsRefPtr<gfxImageSurface> image = new gfxImageSurface(aData, gfxIntSize(aSize.width, aSize.height), aStride, SurfaceFormatToImageFormat(aFormat)); >+ return Factory::CreateDrawTargetForCairoSurface(image->CairoSurface(), aSize); >+ } >+ RefPtr<DrawTarget> target = Factory::CreateDrawTargetForData(mPreferredCanvasBackend, aData, aSize, aStride, aFormat); >+ if (!target && mFallbackCanvasBackend != BackendType::NONE) { >+ target = Factory::CreateDrawTargetForData(mFallbackCanvasBackend, aData, aSize, aStride, aFormat); I know this part was because of me, but should we check if fallback canvas backend is cairo and call CreateDrawTargetForCairoSurface to match the pattern used in CreateOffscreenCanvasDrawTarget? The thing is that I'm not sure because CreateDrawTargetForData seems to account for the cairo case so it seems like creating a gfxImageSurface and then using CreateDrawTargetForCairoSurface instead of just directly calling CreateDrawTargetForData is unnecessary. Or am I missing something? Another thing, it seems like CompositableClient creates a few other classes that have GetAsDrawTarget(): - TextureClientX11 - DIBTextureClientD3D9 - CairoTextureClientD3D9 - TextureClientD3D11 - GrallocTextureClientOGL They all seem ok in that they do not rely on content or canvas backend within gfxPlatform (afaik). Except GrallocTextureClientOGL, it uses CreateDrawTargetForData as well so would default to the content backend. So I guess that would need a backend mode as well?

+1 on what Bas said as well :) Nical, feel free to mix and match whatever way you see fit and check in the merged patch. If you want me to take over and make changes then that's cool, just let me know (I have my hands full today and tomorrow though, so I can take over after that if you want me to).

(In reply to Ali Ak from comment #14) > I know this part was because of me, but should we check if fallback canvas > backend is cairo and call CreateDrawTargetForCairoSurface to match the > pattern used in CreateOffscreenCanvasDrawTarget? > > The thing is that I'm not sure because CreateDrawTargetForData seems to > account for the cairo case so it seems like creating a gfxImageSurface and > then using CreateDrawTargetForCairoSurface instead of just directly calling > CreateDrawTargetForData is unnecessary. Or am I missing something? If we want to go down that road I would rather just pass the BackendType to CreateTextureClientForDrawing instead of a SurfaceMode (like my other patch does). > > Another thing, it seems like CompositableClient creates a few other classes > that have GetAsDrawTarget(): > - TextureClientX11 > - DIBTextureClientD3D9 > - CairoTextureClientD3D9 > - TextureClientD3D11 > - GrallocTextureClientOGL > > They all seem ok in that they do not rely on content or canvas backend > within gfxPlatform (afaik). Except GrallocTextureClientOGL, it uses > CreateDrawTargetForData as well so would default to the content backend. So > I guess that would need a backend mode as well? That was the advantage of using the BackendType instead of BackendMode (at the expense of a slightly less "nice" API).

Talked with Bas and opted for specifying the BackendType when creating a TextureClient. https://hg.mozilla.org/integration/mozilla-inbound/rev/6364146ddb19 https://hg.mozilla.org/integration/mozilla-inbound/rev/2784838dbc94

This caused gtest failures because my patch was initializing the textues in TestTextures.cpp with an invalid backend (gfx::BackendType::NONE) but we actually used GetAsDrawTarget in the test so it failed. Fixed it on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/5ab4f97a3220

I also had forgotten to fixup GrallocTextureClient's constructor... fixed on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/a6831c02d8cf

Backed out for B2G bustage (after the last bustage fix). https://hg.mozilla.org/integration/mozilla-inbound/rev/995cfd42e045 https://tbpl.mozilla.org/php/getParsedLog.php?id=34925863&tree=Mozilla-Inbound

(In reply to Nicolas Silva [:nical] from comment #16) > > The thing is that I'm not sure because CreateDrawTargetForData seems to > > account for the cairo case so it seems like creating a gfxImageSurface and > > then using CreateDrawTargetForCairoSurface instead of just directly calling > > CreateDrawTargetForData is unnecessary. Or am I missing something? > > If we want to go down that road I would rather just pass the BackendType to > CreateTextureClientForDrawing instead of a SurfaceMode (like my other patch > does). Sure thing. It was more of a general question for CreateDrawTargetForData for a cairo backend though - I still don't understand the check, but I'll bring it up on irc at some point so someone can explain it to me :) > > They all seem ok in that they do not rely on content or canvas backend > > within gfxPlatform (afaik). Except GrallocTextureClientOGL, it uses > > CreateDrawTargetForData as well so would default to the content backend. So > > I guess that would need a backend mode as well? > > That was the advantage of using the BackendType instead of BackendMode (at > the expense of a slightly less "nice" API). Right, though, if we used mode or type, GrallocTextureClient would still need to know about whichever right? (that's what I meant I guess I should've been a bit more clear) ... but it looks like you caught it anyway! > Talked with Bas and opted for specifying the BackendType when creating a TextureClient. > > https://hg.mozilla.org/integration/mozilla-inbound/rev/6364146ddb19 > https://hg.mozilla.org/integration/mozilla-inbound/rev/2784838dbc94 Good stuff!

fixed the issues locally, try push https://tbpl.mozilla.org/?tree=Try&rev=8a5b6370d71d

rebased the patches, new try push https://tbpl.mozilla.org/?tree=Try&rev=117a6ddd7eed