893304 - Convert CanvasClientWebGL (and Host) to the new textures

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Comment 1

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientWebGLXP

Hi, Nicolas
I created CanvasClientWebGLXP for WebGL that uses new TextureClient/Host in b2g.

I have a question about buffer sharing.
In non-b2g case, we pass SurfaceStream to compositor thread and content thread will wait for compositor takes the frame.

but in b2g case we pass gralloc buffer directly and some bad things happened.
First is WebGL app would use the buffer which is composited by compositor at the same time. I think this problem could be resolved by some lock/unlock mechanism.
Second is we do not guarantee each frame rendered by WebGL app will be composited by compositor. In other words, content thread would produce many frame but most frame is skip by compositor. Especially in very heavy WebGL app, WebGL app keeps drain gpu power and compositor hardly use gpu resources to compose frame. bug 894847 describes this issue.

Comment 2

[Nicolas Silva [:nical]]

Comment on attachment 799257 [details] [diff] [review]
add-CanvasClientWebGLXP

Review of attachment 799257 [details] [diff] [review]:
-----------------------------------------------------------------

Hey, great to see people are using the new textures. This looks good to me except for one very important point about the ownership model between TextureClient and GrallocBufferActor (see below).

::: gfx/layers/client/CanvasClient.h
@@ +93,5 @@
>  private:
>    RefPtr<TextureClient> mBuffer;
>  };
> +
> +class CanvasClientWebGLXP : public CanvasClient

I am not sure what XP means. In some places XP means cross-platform, but you manipulate gralloc actors directly so I suppose this is B2G-only. I know this is not the final patch but don't forget to add documentation comments to clarify the purpose of this class.

@@ +108,5 @@
> +
> +  virtual void OnDetach() MOZ_OVERRIDE;
> +
> +private:
> +  std::stack<GrallocBufferActor*> mCreatedTextureClient;

This part confuses me a bit, Why not have a stack of TextureClient objects?

Normally the idea is to hold strong references to TextureClients, which manage the lifetime of shared data such as shmem or GrallocBufferActor. In your patch it looks like you did the other way around: you added a strong reference to a TextureClient in the GrallocBufferActor and you manipulate this through the actor. It is not great because the layers and textures are designed so that it is the TextureClient that controls the lifetime of the shared data (and not the opposite.

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.h
@@ +112,5 @@
>    // used only for hacky fix in gecko 23 for bug 862324
>    // see bug 865908 about fixing this.
>    DeprecatedTextureHost* mDeprecatedTextureHost;
>  
> +  RefPtr<TextureClient> mTextureClient;

As I mentioned in the comment in CanvasClient.h, I think this is not the right way to do it. It has to be the TextureClient that manages the the actor. If you add a strong reference to the texture client in the gralloc actor the lifetime of the texture client becomes dependent on the actor's and the actor has to be managed manually.
Note that the mDeprecatedTextureHost above is a hack that was needed to solve a tricky bug, but we don't need to do that with the new textures.

Comment 3

[Nicolas Silva [:nical]]

(In reply to Nicolas Silva [:nical] from comment #2)
> Comment on attachment 799257 [details] [diff] [review]
> add-CanvasClientWebGLXP
> 
> Review of attachment 799257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey, great to see people are using the new textures. This looks good to me
> except for one very important point about the ownership model between
> TextureClient and GrallocBufferActor (see below).
> 
> ::: gfx/layers/client/CanvasClient.h
> @@ +93,5 @@
> >  private:
> >    RefPtr<TextureClient> mBuffer;
> >  };
> > +
> > +class CanvasClientWebGLXP : public CanvasClient
> 
> I am not sure what XP means. In some places XP means cross-platform, but you
> manipulate gralloc actors directly so I suppose this is B2G-only. I know
> this is not the final patch but don't forget to add documentation comments
> to clarify the purpose of this class.

Oh! silly me. It means "cross-process" I suppose.
If this is going to be B2G specific, we should give it a more explicit name.

Comment 4

[Nicolas Silva [:nical]]

(In reply to Morris Tseng [:mtseng] from comment #1)
> I have a question about buffer sharing.
> In non-b2g case, we pass SurfaceStream to compositor thread and content
> thread will wait for compositor takes the frame.
> 
> but in b2g case we pass gralloc buffer directly and some bad things happened.
> First is WebGL app would use the buffer which is composited by compositor at
> the same time. I think this problem could be resolved by some lock/unlock
> mechanism.

We have somewhat similar problems with video. The idea is that we need to wait until the gralloc buffer that is being composited is unlocked before reusing it on the client side. The hard part is that Unlocking doesn't happen explicitly. My understanding is that GrallocBuffer N-1 is unlocked when GrallocBuffer N has been Composited using the same OpenGL texture. This means that we can't just swap the textures during the layer transaction, we need to wait a bit longer. We haven't really found a good solution for that yet with video.

> Second is we do not guarantee each frame rendered by WebGL app will be
> composited by compositor. In other words, content thread would produce many
> frame but most frame is skip by compositor. Especially in very heavy WebGL
> app, WebGL app keeps drain gpu power and compositor hardly use gpu resources
> to compose frame. bug 894847 describes this issue.

I think Vlad was talking about using a separate thread on the compositor process to just wait for the WebGL frame to be realized and then post a message to the compositor thread saying "now use this frame it is ready for you".
I don't know very well how that would work in term of the APIs we use for OpenGL/Gralloc (Is it actually possible to use the same context on separate threads? Would we need a different context?).

Comment 5

[Peter Chang[:pchang]]

(In reply to Nicolas Silva [:nical] from comment #3)
> (In reply to Nicolas Silva [:nical] from comment #2)
> > Comment on attachment 799257 [details] [diff] [review]
> > add-CanvasClientWebGLXP
> > 
> > Review of attachment 799257 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Hey, great to see people are using the new textures. This looks good to me
> > except for one very important point about the ownership model between
> > TextureClient and GrallocBufferActor (see below).
> > 
> > ::: gfx/layers/client/CanvasClient.h
> > @@ +93,5 @@
> > >  private:
> > >    RefPtr<TextureClient> mBuffer;
> > >  };
> > > +
> > > +class CanvasClientWebGLXP : public CanvasClient
> > 
> > I am not sure what XP means. In some places XP means cross-platform, but you
> > manipulate gralloc actors directly so I suppose this is B2G-only. I know
> > this is not the final patch but don't forget to add documentation comments
> > to clarify the purpose of this class.
> 
> Oh! silly me. It means "cross-process" I suppose.
> If this is going to be B2G specific, we should give it a more explicit name.

I think original DeprecatedCanvasClientSurfaceStream could handle cross-thread and cross process cases.
And not only b2g uses it.

Comment 6

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Nicolas Silva [:nical] from comment #2)
> Comment on attachment 799257 [details] [diff] [review]
> add-CanvasClientWebGLXP
> 
> Review of attachment 799257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey, great to see people are using the new textures. This looks good to me
> except for one very important point about the ownership model between
> TextureClient and GrallocBufferActor (see below).
> 
> ::: gfx/layers/client/CanvasClient.h
> @@ +93,5 @@
> >  private:
> >    RefPtr<TextureClient> mBuffer;
> >  };
> > +
> > +class CanvasClientWebGLXP : public CanvasClient
> 
> I am not sure what XP means. In some places XP means cross-platform, but you
> manipulate gralloc actors directly so I suppose this is B2G-only. I know
> this is not the final patch but don't forget to add documentation comments
> to clarify the purpose of this class.
> 
This means cross-process. I'll replace it to a better name.
> @@ +108,5 @@
> > +
> > +  virtual void OnDetach() MOZ_OVERRIDE;
> > +
> > +private:
> > +  std::stack<GrallocBufferActor*> mCreatedTextureClient;
> 
> This part confuses me a bit, Why not have a stack of TextureClient objects?
> 
> Normally the idea is to hold strong references to TextureClients, which
> manage the lifetime of shared data such as shmem or GrallocBufferActor. In
> your patch it looks like you did the other way around: you added a strong
> reference to a TextureClient in the GrallocBufferActor and you manipulate
> this through the actor. It is not great because the layers and textures are
> designed so that it is the TextureClient that controls the lifetime of the
> shared data (and not the opposite.
> 
The initial idea is store GrallocBufferActor and TextureClient relationship to a map structure (e.g. std::map<GrallocBufferActor*, RefPtr<TextureClient> >). But since I could get GrallocBufferActor from SharedSurface in this case. If I store TextureClient inside GrallocBufferActor, I could save a query call from map. I agree with you about lifetime problem and I'll fix this.

> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.h
> @@ +112,5 @@
> >    // used only for hacky fix in gecko 23 for bug 862324
> >    // see bug 865908 about fixing this.
> >    DeprecatedTextureHost* mDeprecatedTextureHost;
> >  
> > +  RefPtr<TextureClient> mTextureClient;
> 
> As I mentioned in the comment in CanvasClient.h, I think this is not the
> right way to do it. It has to be the TextureClient that manages the the
> actor. If you add a strong reference to the texture client in the gralloc
> actor the lifetime of the texture client becomes dependent on the actor's
> and the actor has to be managed manually.
> Note that the mDeprecatedTextureHost above is a hack that was needed to
> solve a tricky bug, but we don't need to do that with the new textures.

as above mentioned, I'll fix this as well.

Comment 7

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientWebGLXP

fix TextureClient ownership from GrallocBufferActor to CanvasClientWebGLXP base on Comment 2

Comment 8

[Nicolas Silva [:nical]]

Sorry for the review lag, I am pretty busy these days.
I am not an expert on the SurfaceStream stuff so you'll need an additional reviewer like snorp or nrc.

I am wondering, is this subject to a pressing deadline? If not I would strongly prefer that we implement directly a solution that is backend-independent just like the current deprecated implem. If it is really important to get this landed asap, we can probably cope with having a b2g-specific implementation here temporarily but only if we really need to.

I will still need to spend more time reviewing this patch but I do have one remark so far: it is important that we don't manipulate GrallocBufferActors by hands when possible. Here for instance you should use a map<uint64_t, RefPtr<TextureClient> > and use textureClient->GetID() as the key.

apart from the glue between SharedSurface and TextureClient there doesn't seem to be any backend specific code so making it backend independent should not be hard and we can sit on the TextureClient abstracion level instead of gralloc for most things.

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to Nicolas Silva [:nical] from comment #8)
> Sorry for the review lag, I am pretty busy these days.
> I am not an expert on the SurfaceStream stuff so you'll need an additional
> reviewer like snorp or nrc.
> 
> I am wondering, is this subject to a pressing deadline? If not I would
> strongly prefer that we implement directly a solution that is
> backend-independent just like the current deprecated implem. If it is really
> important to get this landed asap, we can probably cope with having a
> b2g-specific implementation here temporarily but only if we really need to.
I'm working on backend-independent implementation now. It should be finished soon. I'll upload patch when I finish.
> 
> I will still need to spend more time reviewing this patch but I do have one
> remark so far: it is important that we don't manipulate GrallocBufferActors
> by hands when possible. Here for instance you should use a map<uint64_t,
> RefPtr<TextureClient> > and use textureClient->GetID() as the key.
> 
The main purpose of this map is keep associating between the buffers retrieved from SurfaceStream and TextureClient. Thus I need buffer infomations as my map key. If I use textureClient->GetID() as the map key, then I can't found corresponding TextureClient from buffer. Right now I know I shouldn't manipulate GrallocBufferActor directly. How about use pointer of SharedSurface as the key?

> apart from the glue between SharedSurface and TextureClient there doesn't
> seem to be any backend specific code so making it backend independent should
> not be hard and we can sit on the TextureClient abstracion level instead of
> gralloc for most things.

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

Implement backend-independent solution.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 801383 [details] [diff] [review]:
-----------------------------------------------------------------

Overall looks pretty good to me, but I'm not an expert.

::: gfx/layers/client/CanvasClient.cpp
@@ +155,5 @@
> +    // Bug 894405
> +    //
> +    // Ref this so the SurfaceStream doesn't disappear unexpectedly. The
> +    // Compositor will need to unref it when finished.
> +    aLayer->mGLContext->AddRef();

Do we still need this awful hack? My understanding was that the new texture stuff somehow fixes this?

Comment 12

[Nick Cameron [:nrc]]

Comment on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 801383 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, but I'm not an expert on either the canvas surface streams or the new textures, so I don't think my feedback is necessary.

Comment 13

[Nicolas Silva [:nical]]

Comment on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 801383 [details] [diff] [review]:
-----------------------------------------------------------------

I am so sorry for the late feedbacks. This looks good to me.

Beware, turning on skia-GL canvas seems to be making a lot of waves these days on B2G, and the skia-GL stuff is using surface stream, so you should make sure this well tested and bullet proof if you want to get it checked in soon. If you think this patch is risky it's worth thinking about whether or not to wait for the canvas situation to stabilize a bit (your call).

Various trivial nits: You should add some documentation comments, at least one for each new class. Also there are some trailing spaces to fix.

Sorry again that I took so long to get back to you, I have been focusing on the high priority stuff lately.

Comment 14

[Nicolas Silva [:nical]]

Comment on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 801383 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/CanvasClient.cpp
@@ +144,5 @@
> +#ifndef MOZ_WIDGET_GONK
> +    if (!mBuffer) {
> +      StreamTextureClientOGL* textureClient = new StreamTextureClientOGL(mTextureInfo.mTextureFlags);
> +      textureClient->InitWith(stream, gfx::IntSize());
> +      AddTextureClient(textureClient);

You should check for the return value of AddTextureClient.
If it fails (returns false) you need to not call subsequent messages on it (like UseTexture or RemoveTextureClient) and not set mBuffer to it.

It's rather recent (when you wrote that patch AddTextureClient did not have a return value). But we ran into crashes caused by trying to send messages for textures which serialization had failed (see bug 923917).

Comment 15

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

I should probably check in on the SurfaceStream bits.

Comment 16

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

Rebase to latest m-c and update patch to address comment

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8342176 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8342176 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/opengl/TextureHostOGL.h
@@ +359,5 @@
> +    , mTextureHandle(0)
> +    , mTextureTarget(LOCAL_GL_TEXTURE_2D)
> +    , mUploadTexture(0)
> +    , mFormat(gfx::FORMAT_UNKNOWN)
> +  {}

nit: MOZ_COUNT_CTOR/DTOR stuff

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8342176 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8342176 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good modulo one nit

::: gfx/layers/opengl/TextureClientOGL.h
@@ +79,5 @@
> +
> +  virtual TextureClientData* DropTextureData() MOZ_OVERRIDE { return nullptr; }
> +
> +  void InitWith(gfx::SurfaceStream* aStream,
> +                gfx::IntSize aSize);

Do we ever use this size? The actual size is that of the consumed surface, right, so we shouldn't ever need this. Maybe drop the aSize argument here to avoid confusion.

Comment 19

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+, snorp: r+)

Update to address comment. Still fighting with try server. right now I had some reftest failures on B2G emulator. 
https://tbpl.mozilla.org/?tree=Try&rev=f2d90b1b31f7

Comment 20

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

I think this version pass all test on try server. https://tbpl.mozilla.org/?tree=Try&rev=6c8d3a80a063

Previous try failure on emulator because emulator isn't running cross process. So some logic which exclude by some #ifndef should be executed but not executed.

nical, snorp, could you take a look those change and review it again? Thanks

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8345782 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8345782 [details] [diff] [review]:
-----------------------------------------------------------------

I am r+ing provided you file a bug to fix the mBuffers[nullptr] situation :)

::: gfx/layers/client/CanvasClient.cpp
@@ +161,5 @@
> +    MOZ_ASSERT(false);
> +#endif
> +  } else {
> +    void *nullTemp = nullptr;
> +    if (mBuffers.find(nullTemp) == mBuffers.end()) {

This looks really hacky to me. Didn't we just have mBuffer before for the non-gonk case?

Comment 22

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #21)
> Comment on attachment 8345782 [details] [diff] [review]
> add-CanvasClientSurfaceStream
> 
> Review of attachment 8345782 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am r+ing provided you file a bug to fix the mBuffers[nullptr] situation :)
> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +161,5 @@
> > +    MOZ_ASSERT(false);
> > +#endif
> > +  } else {
> > +    void *nullTemp = nullptr;
> > +    if (mBuffers.find(nullTemp) == mBuffers.end()) {
> 
> This looks really hacky to me. Didn't we just have mBuffer before for the
> non-gonk case?

Yes, but b2g emulator reftest also run into this code path and b2g is gonk case so I cannot put those code in #ifndef MOZ_WIDGET_GONK. Otherwise, I can keep both mBuffer and mBuffers but I think keep both data means some redundent. So I re-use mBuffers by store a nullptr to simulate mBuffer.

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 8345782 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8345782 [details] [diff] [review]:
-----------------------------------------------------------------

I agree with snorp about the mBuffers hack. We need at least a good comment explaining what's going on. If you really want to not have a redundant mBuffer, you should pick a another name for nullTemp which would better describe that we are in a scenario where we have only one slot. In any case it's hard to understand at first glance right now. Otherwise, an extra mBuffer doesn't bother me that much.

Comment 24

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+, snorp: r+)

I think keep both mBuffer and mBuffers is better idea. This patch keep both data!

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8345956 [details] [diff] [review]
add-CanvasClientSurfaceStream (carries nical: r+, snorp: r+)

Review of attachment 8345956 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/CanvasClient.cpp
@@ +132,5 @@
> +      return;
> +    }
> +
> +    if (surf->Type() != SharedSurfaceType::Gralloc) {
> +      printf_stderr("Unexpected non-Gralloc SharedSurface in IPC path!");

assert(false) here, because DEBUG builds should probably break if this happens.

::: gfx/layers/opengl/TextureHostOGL.h
@@ +370,5 @@
> +  }
> +
> +  virtual TextureSourceOGL* AsSourceOGL() { return this; }
> +
> +  virtual void BindTexture(GLenum activetex) MOZ_OVERRIDE;

I usually call this 'texUnit', but this works too.

Comment 26

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+, snorp: r+, jgilbert: r+)

update to address comment

Comment 27

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Since PTexture has landed, I need rebase my patch to latest m-c. So remove checkin-needed

Comment 28

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+, snorp: r+, jgilbert: r+)

Rebase to latest m-c, wait for try server result
https://tbpl.mozilla.org/?tree=Try&rev=6f763b6f1268

Comment 29

[Sotaro Ikeda [:sotaro]]

Set to 1.3? based on Bug 950079.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/96b0ddfc63f7

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/96b0ddfc63f7

Comment 32

[Sotaro Ikeda [:sotaro]]

It seems better not to uplift this bug to b2g v1.3 only for fence handling. Clear "1.3?" flag.

Comment 33

[Jason Smith [:jsmith]]

FYI - Either this bug or bug 929513 has just caused a large array of gfx layers crashes in bug 950589 that are blocking multiple smoketests.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/b4ccf273d273

Comment 35

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

In bug 950589, app crashes because SurfaceDescriptor delete twice. First delete is at TextureClient::Finalize() and second one is SharedSurface_Gralloc's destructor. So I add a flag indicate the ownership of SurfaceDescriptor has transferred. Once bug 941400 is landed, I'll remove this flag because SharedSurface_Gralloc will hold a TextureClient directly.

Comment 36

[Jason Smith [:jsmith]]

http://hg.mozilla.org/mozilla-central/rev/b4ccf273d273

Comment 37

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8348574 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8348574 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/client/CanvasClient.cpp
@@ +138,5 @@
> +                                                                     grallocSurf->Size(),
> +                                                                     mTextureInfo.mTextureFlags);
> +
> +      grallocSurf->TransferBufferOwnership();
> +      mBuffers[surf] = grallocTC;

I am concerned that if, for whatever reason, the surface stream was getting new SharedSurfaces instead of reusing the old ones, we would be keeping stale entries in the mBuffers map. That would be a memory leak.

If you are certain that this doesn't happen, you could add an assertion,

  MOZ_ASSERT(mBuffers.size() <= 3);

But I am not so sure that it can't ever happen. So a way to handle this gracefully would be to clear the map when we exceed 3 elements there, before we insert the new grallocTC:

  if (mBuffers.size() >= 3) {
    mBuffers.clear();
  }
  mBuffers[surf] = grallocTC;

r=me with either solution. Ask :jgilbert if you need more details on this, he knows this stuff better than I do.

Comment 38

[Kelsey Gilbert [:jgilbert]]

Resizes will mean you'll get new buffers, so if mBuffers lives across resize boundaries, it will leak. I'm going to r- until we have enough assertions and/or safeguards here to be reasonably sure we're ok.

Comment 39

[Kelsey Gilbert [:jgilbert]]

Actually, I think context-loss can give us a new set of buffers without resizing, so I don't think layers can actually predict which buffers are no longer used. (This seems really bad)

Comment 40

[Kelsey Gilbert [:jgilbert]]

After context-loss, it's probably a new SurfaceStream, though. Maybe that's enough?

Comment 41

[Benoit Jacob [:bjacob] (mostly away)]

Context loss is implemented by nulling the WebGLContext's reference to the GLContext, so we have a guarantee that nothing under the old GLContext is still referenced by the WebGL context afterward. Since the SurfaceStream is among the things that is held by the GLContext, there is no way that the WebGLContext could still reference the old SurfaceStream after a context loss.

In itself that doesn't prevent other things (the ClientCanvasLayer...) from still referencing the old GLContext, but these things wouldn't be getting any new frames from the WebGLContext.

Is that enough to make you feel good about this code?

Comment 42

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

I think SharedSufaceGralloc store TextureClient instead SurfaceDescriptor resolved leak issue. This kind of thing I have done in bug 941400. Maybe merge that patch to here is good idea? Although SharedSurface will be replaced by TextureClient in bug 941390 very soon. I think bug 941400 is quick solution for now.

Comment 43

[Kelsey Gilbert [:jgilbert]]

(In reply to Benoit Jacob [:bjacob] from comment #41)
> Context loss is implemented by nulling the WebGLContext's reference to the
> GLContext, so we have a guarantee that nothing under the old GLContext is
> still referenced by the WebGL context afterward. Since the SurfaceStream is
> among the things that is held by the GLContext, there is no way that the
> WebGLContext could still reference the old SurfaceStream after a context
> loss.
> 
> In itself that doesn't prevent other things (the ClientCanvasLayer...) from
> still referencing the old GLContext, but these things wouldn't be getting
> any new frames from the WebGLContext.
> 
> Is that enough to make you feel good about this code?

Maybe, but words are wind. Give me reasonable asserts, please. :) `count <= 3` should do it.

Comment 44

[Sotaro Ikeda [:sotaro]]

I tried current patch by using CrystalSkull on master hamachi. The CrystalSkull crashed when I changed the app state between foreground and background continuously several times.

Comment 45

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

Replaced SurfaceDescriptor by TextureClient inside ShreadSurfaceGralloc to resolved leak problem

Comment 46

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 8350511 [details] [diff] [review]
add-CanvasClientSurfaceStream

Review of attachment 8350511 [details] [diff] [review]:
-----------------------------------------------------------------

I really like the approach taken here: rather than have to deal with a fragile and complex mapping of SharedSurfaceGralloc to GrallocTextureClient, you got a IMO very good idea here to decide that this is the right time to actually replace the internals of SharedSurfaceGralloc by a GrallocTextureClient, which goes one step in the right direction anyway (since eventually we do want to replace SharedSurfaceGralloc by GrallocTextureClient).

I have one important comment I want to see addressed before this would land: unless you have a specific reason not to, it is important to hold strong references to ISurfaceAllocator's to avoid a pattern of crashes we've had in the past. See comment below about RefPtr<ISurfaceAllocator>. Another comment that I would like to see addressed is the one on the (mCompositable ? mCompositable->GetForwarder() : mAllocator) logic that should be unified in one place.

As I explain below, the introduction of the mAllocator member goes in the right direction (see bug 952540) so I think that we should r+ that aspect despite the redundancy with mCompositable, as we really want to remove the mCompositable field.

::: gfx/layers/opengl/GrallocTextureClient.cpp
@@ +137,5 @@
>                                                        nsIntSize(0,0), false, false);
> +
> +      ISurfaceAllocator *allocator = mCompositable ?
> +                                     mCompositable->GetForwarder() :
> +                                     mAllocator;

This logic,

   mCompositable ? mCompositable->GetForwarder() : mAllocator;

Should be abstracted in a GetAllocator() method, so that you don't have to write it multiple times (I can see it at least 3 times).

::: gfx/layers/opengl/GrallocTextureClient.h
@@ +112,5 @@
>    RefPtr<GraphicBufferLocked> mBufferLocked;
>  
>    android::sp<android::GraphicBuffer> mGraphicBuffer;
>  
> +  ISurfaceAllocator* mAllocator;

This should be a RefPtr<ISurfaceAllocator> instead of a ISurfaceAllocator*, so that we won't have again the same kind of crash as in bug 914823 when the allocator is dead.

Notice that likewise, SharedSurfaceGralloc now has a RefPtr<ISurfaceAllocator>.

Here is another remark on a more meta level, and not blocking r+ here: it is never good to have redundant members in a class (here mAllocator and mCompositable), but here, clearly mAllocator is the right thing to have, and mCompositable is a legacy thing that should go away: since PTexture landed, TextureClient's should no longer have to keep a reference to a compositable. I filed bug 952540 for that. Again, this is just a meta remark, and does not block r+ on the present patch.

Comment 47

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries bjacob: r+)

update to address comment

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/403ab3213879

Comment 49

[Ryan VanderMeulen [:RyanVM]]

Looks like this needed a 5th reviewer. Backed out for bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e281ab880710

https://tbpl.mozilla.org/php/getParsedLog.php?id=32637462&tree=Mozilla-Inbound

Comment 50

[Benoit Jacob [:bjacob] (mostly away)]

The compilation errors look like this was recently bitrotted by some moz2dification changes, perhaps by bug 877115.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

BTW, can you make sure that the headers are being included alphabetically? I noticed a few instances where they aren't.

Comment 52

[Benoit Jacob [:bjacob] (mostly away)]

I wasn't aware of such a coding style rule; could you share a link?

Comment 53

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #51)
> BTW, can you make sure that the headers are being included alphabetically? I
> noticed a few instances where they aren't.

The ordering of the headers can be significant, we would never do it alphabetically.

Comment 54

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream

rebase to latest m-c. And I add bjacob and nical as reviewers because you are reviewers in bug 877115. 

Here is try push: https://tbpl.mozilla.org/?tree=Try&rev=bb36e4a0aad0

Comment 55

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+)

fix compile error in previous patch.
new try push: https://tbpl.mozilla.org/?tree=Try&rev=b9641e1bbe1d

Comment 56

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Attachment: add-CanvasClientSurfaceStream (carries nical: r+, bjacob: r+)

rebase again.
try push: https://tbpl.mozilla.org/?tree=Try&rev=a35761649912
once all green, this patch is ready to land

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8dcb1a549395

Comment 58

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8dcb1a549395