Closed Bug 893304 Opened 7 years ago Closed 6 years ago

Convert CanvasClientWebGL (and Host) to the new textures

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: nical, Assigned: mtseng)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 14 obsolete files)

38.18 KB, patch
Details | Diff | Splinter Review
No description provided.
Blocks: NewTextures
Attached patch add-CanvasClientWebGLXP (obsolete) — Splinter Review
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.
Attachment #799257 - Flags: feedback?(nical.bugzilla)
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.
(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.
(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?).
(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.
(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.
Attached patch add-CanvasClientWebGLXP (obsolete) — Splinter Review
fix TextureClient ownership from GrallocBufferActor to CanvasClientWebGLXP base on Comment 2
Attachment #799257 - Attachment is obsolete: true
Attachment #799257 - Flags: feedback?(nical.bugzilla)
Attachment #799387 - Flags: feedback?(nical.bugzilla)
Assignee: nobody → mtseng
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.
(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.
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
Implement backend-independent solution.
Attachment #801383 - Flags: feedback?(snorp)
Attachment #801383 - Flags: feedback?(nical.bugzilla)
Attachment #801383 - Flags: feedback?(ncameron)
Attachment #799387 - Attachment is obsolete: true
Attachment #799387 - Flags: feedback?(nical.bugzilla)
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?
Attachment #801383 - Flags: feedback?(snorp) → feedback+
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.
Attachment #801383 - Flags: feedback?(ncameron)
Attachment #801383 - Flags: feedback?(vladimir)
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.
Attachment #801383 - Flags: feedback?(nical.bugzilla) → feedback+
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 on attachment 801383 [details] [diff] [review]
add-CanvasClientSurfaceStream

I should probably check in on the SurfaceStream bits.
Attachment #801383 - Flags: feedback?(jgilbert)
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
Rebase to latest m-c and update patch to address comment
Attachment #801383 - Attachment is obsolete: true
Attachment #801383 - Flags: feedback?(vladimir)
Attachment #801383 - Flags: feedback?(jgilbert)
Attachment #8342176 - Flags: review?(snorp)
Attachment #8342176 - Flags: review?(nical.bugzilla)
Attachment #8342176 - Flags: review?(jgilbert)
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
Attachment #8342176 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8342176 - Flags: review?(snorp) → review+
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
Attachment #8342176 - Attachment is obsolete: true
Attachment #8342176 - Flags: review?(jgilbert)
Attachment #8345143 - Flags: review?(jgilbert)
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
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
Attachment #8345143 - Attachment is obsolete: true
Attachment #8345143 - Flags: review?(jgilbert)
Attachment #8345782 - Flags: review?(snorp)
Attachment #8345782 - Flags: review?(nical.bugzilla)
Attachment #8345782 - Flags: review?(jgilbert)
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?
Attachment #8345782 - Flags: review?(snorp) → review+
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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 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.
Attachment #8345782 - Flags: review?(nical.bugzilla) → review+
Blocks: 925444
I think keep both mBuffer and mBuffers is better idea. This patch keep both data!
Attachment #8345782 - Attachment is obsolete: true
Attachment #8345782 - Flags: review?(jgilbert)
Attachment #8345956 - Flags: review?(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.
Attachment #8345956 - Flags: review?(jgilbert) → review+
update to address comment
Attachment #8345956 - Attachment is obsolete: true
Since PTexture has landed, I need rebase my patch to latest m-c. So remove checkin-needed
Blocks: 950079
Set to 1.3? based on Bug 950079.
blocking-b2g: --- → 1.3?
https://hg.mozilla.org/mozilla-central/rev/96b0ddfc63f7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
No longer blocks: 925444
It seems better not to uplift this bug to b2g v1.3 only for fence handling. Clear "1.3?" flag.
blocking-b2g: 1.3? → ---
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.
Depends on: 950589
Backed out.
https://hg.mozilla.org/integration/b2g-inbound/rev/b4ccf273d273
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla29 → ---
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
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.
Attachment #8347114 - Attachment is obsolete: true
Attachment #8348574 - Flags: review?(nical.bugzilla)
Attachment #8348574 - Flags: review?(bjacob)
Attachment #8348574 - Flags: review?(nical.bugzilla) → review+
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.
Attachment #8348574 - Flags: review?(bjacob) → review+
Attachment #8348574 - Flags: review-
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.
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)
After context-loss, it's probably a new SurfaceStream, though. Maybe that's enough?
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?
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.
(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.
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.
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
Replaced SurfaceDescriptor by TextureClient inside ShreadSurfaceGralloc to resolved leak problem
Attachment #8348574 - Attachment is obsolete: true
Attachment #8350511 - Flags: review?(jgilbert)
Attachment #8350511 - Flags: review?(bjacob)
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.
Attachment #8350511 - Flags: review?(bjacob) → review+
update to address comment
Attachment #8350511 - Attachment is obsolete: true
Attachment #8350511 - Flags: review?(jgilbert)
Attachment #8351128 - Flags: review?(jgilbert)
Attachment #8351128 - Flags: review?(jgilbert) → review+
The compilation errors look like this was recently bitrotted by some moz2dification changes, perhaps by bug 877115.
BTW, can you make sure that the headers are being included alphabetically? I noticed a few instances where they aren't.
I wasn't aware of such a coding style rule; could you share a link?
(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.
Attached patch add-CanvasClientSurfaceStream (obsolete) — Splinter Review
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
Attachment #8351128 - Attachment is obsolete: true
Attachment #8356938 - Flags: review?(nical.bugzilla)
Attachment #8356938 - Flags: review?(bjacob)
Attachment #8356938 - Flags: review?(nical.bugzilla) → review+
fix compile error in previous patch.
new try push: https://tbpl.mozilla.org/?tree=Try&rev=b9641e1bbe1d
Attachment #8356938 - Attachment is obsolete: true
Attachment #8356938 - Flags: review?(bjacob)
Attachment #8357523 - Flags: review?(bjacob)
Attachment #8357523 - Flags: review?(bjacob) → review+
rebase again.
try push: https://tbpl.mozilla.org/?tree=Try&rev=a35761649912
once all green, this patch is ready to land
Attachment #8357523 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8dcb1a549395
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 941400
Depends on: 980582
You need to log in before you can comment on or make changes to this bug.