973892 - Make TextureClient::GetAsDrawTarget work with canvas

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

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

Comment 1

[Ali Ak]

Attachment: memleak in CreateRecordingDrawTarget when backend is skia

Comment 2

[Ali Ak]

Attachment: Add BackendMode to get DrawTarget

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.

Comment 3

[Ali Ak]

Attachment: memleak in CreateDrawTargetForData when backend is skia

Comment 4

[Nicolas Silva [:nical]]

Attachment: expose backend in ImageDataSerializer

Comment 5

[Nicolas Silva [:nical]]

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

Comment 6

[Ali Ak]

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 :/

Comment 7

[Nicolas Silva [:nical]]

Attachment: part 2) Specify the moz2d backend when creating a TextureClient

Comment 8

[Nicolas Silva [:nical]]

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!

Comment 9

[Nicolas Silva [:nical]]

Attachment: Gal's patch with BackendMode passed when creating the TextureClient instead of GetAsDrawTarget

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.

Comment 10

[Ali Ak]

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).

Comment 11

[Andreas Gal :gal]

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

Comment 12

[Ali Ak]

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 13

[Bas Schouten (:bas.schouten)]

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 14

[Ali Ak]

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?

Comment 15

[Ali Ak]

+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).

Comment 16

[Nicolas Silva [:nical]]

(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).

Comment 17

[Nicolas Silva [:nical]]

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

Comment 18

[Nicolas Silva [:nical]]

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

Comment 19

[Nicolas Silva [:nical]]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Ali Ak]

(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!

Comment 22

[Nicolas Silva [:nical]]

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

Comment 23

[Nicolas Silva [:nical]]

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

Comment 24

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b26312bcf46

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5b26312bcf46