913821 - gralloc buffer leaks at ImageBridge

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #901224 +++

The following is a copy from Bug 901224 comment 70. From this it became clear that TextureHost(GrallocTextureHost) is not removed when TextureClient is removed. This causes a gralloc buffer leak.

-video thumbnail generation does not complete.
  + When TextureClient is added and then removed to/from CompositableHost
     before CompositorParent is set to CompositableHost, 
    op.textureID() is 0 within TOpRemoveTexture operation.
    Therefore, ReplyTextureRemoved is not delivered to the client side.

Comment 1

[Sotaro Ikeda [:sotaro]]

I've heard from mikeh about the following. It might also affected to it.
- phone does not work after taking a photo several times on b2g master.

Comment 2

[Sotaro Ikeda [:sotaro]]

I already confirmed that some camera preview frames are rendered before CompositorParent is set to CompositableHost in the past. It is same situation as in comment #0.

Comment 3

[Sotaro Ikeda [:sotaro]]

Invalid texture id is detected at the following in CompositableParentManager. ImageBridgeParent is a derived class from CompositableParentManager.
- CompositableParentManager::ReceiveCompositableUpdate()
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositableTransactionParent.cpp#248

Comment 4

[Sotaro Ikeda [:sotaro]]

During the checking around texture id, I recognized that there are 32bits texture id and 64 bitss texture id in LayersMessages.ipdlh.

struct OpAddTexture {
  PCompositable compositable;
  uint64_t textureID;
  SurfaceDescriptor data;
  uint32_t textureFlags;
};

struct ReplyTextureRemoved {
  PCompositable compositable;
  uint32_t textureId;
};

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#300
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/LayersMessages.ipdlh#395

Comment 5

[Sotaro Ikeda [:sotaro]]

nical, is there a reason of comment 4 ?

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> Invalid texture id is detected at the following in
> CompositableParentManager. ImageBridgeParent is a derived class from
> CompositableParentManager.

FYI, following diagrams show the relationships between classes around ImageBridge.
https://github.com/sotaroikeda/firefox-diagrams/blob/master/gfx/gfx_ImageBridge_FirefoxOS_1_2.pdf?raw=true

Comment 7

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> nical, is there a reason of comment 4 ?

No, it's a mistake, they should bith be 64bit

Comment 8

[Nicolas Silva [:nical]]

It looks like this can be fixed by doing the following thing in GrallocTextureClient's destructor:

if (ShouldDeallocateInDestructor()) {
  if (mBufferLocked) {
    mBufferLocked->Unlock();
  } else {
    // do the deallocation with a ISurfaceAllocator directly
  }
}

Comment 9

[Nicolas Silva [:nical]]

Attachment: Make sure gralloc bufers are deallocated/recycled properly if a texture client is destroyed before being shared

This should probably be incorporated in the patch in bug 901224 since one will not work without the other.

Comment 10

[Nicolas Silva [:nical]]

Attachment: make the uint32 textureID in IPDL uint64 instead.

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #9)
> Created attachment 801510 [details] [diff] [review]
> Make sure gralloc bufers are deallocated/recycled properly if a texture
> client is destroyed before being shared
> 
> This should probably be incorporated in the patch in bug 901224 since one
> will not work without the other.

From the source code, it seems to work for GrallocTextureClientOGL. I am going to check it. But it seems that the corresponding TextureHost continue to exist in b2g process. Is it correct?

Comment 12

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #11)
> (In reply to Nicolas Silva [:nical] from comment #9)
> > Created attachment 801510 [details] [diff] [review]
> > Make sure gralloc bufers are deallocated/recycled properly if a texture
> > client is destroyed before being shared
> > 
> > This should probably be incorporated in the patch in bug 901224 since one
> > will not work without the other.
> 
> From the source code, it seems to work for GrallocTextureClientOGL. I am
> going to check it. But it seems that the corresponding TextureHost continue
> to exist in b2g process. Is it correct?

That's the case where the texture client is being destroyed before it was shared with the compositor (that is, before we sent OpAddTexture which creates the TextureHost). So the TextureHost was never created.

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #12)
> 
> That's the case where the texture client is being destroyed before it was
> shared with the compositor (that is, before we sent OpAddTexture which
> creates the TextureHost). So the TextureHost was never created.


From the following log I took last week, it seems that TextureHost is created. How do you think about it?
------------------------------------------------------------
09-06 20:10:31.309   519   569 I Gecko   :  >>> [Begin] void mozilla::layers::ImageContainer::SetCurrentImage(mozilla::layers::Image*) 0x43db6340
09-06 20:10:31.309   519   569 I Gecko   :  <<< [End]   void mozilla::layers::ImageContainer::SetCurrentImage(mozilla::layers::Image*) 0x43db6340
09-06 20:10:31.309   519   527 I Gecko   :  >>> [Begin] void mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*) 0x0
09-06 20:10:31.309   519   527 I GonkGfx : ImageClientBuffered::UpdateImage() this 0x43c718e0
09-06 20:10:31.309   519   527 I GonkGfx : GrallocTextureClientOGL::GrallocTextureClientOGL() this 0x434516a0
09-06 20:10:31.309   519   527 I GonkGfx : GrallocTextureClientOGL::SetGraphicBufferLocked()_E aBufferLocked 0x43451510 this 0x434516a0
09-06 20:10:31.309   519   527 I GonkGfx : ImageClientSingle::UpdateImage() _E this 0x43c718e0
09-06 20:10:31.309   519   527 I GonkGfx : ImageClientSingle::AddTextureClient() this 0x43c718e0
09-06 20:10:31.309   519   527 I GonkGfx : CompositableClient::AddTextureClient()_E
09-06 20:10:31.309   519   527 I GonkGfx : CompositableClient::AddTextureClient()_X mNextTextureID 3
09-06 20:10:31.309   519   527 I GonkGfx : ImageClientSingle::UpdateImage() _X this 0x43c718e0
09-06 20:10:31.309   519   527 I Gecko   :     >>> [Begin] void mozilla::layers::ImageBridgeChild::EndTransaction() 0x434c66e0
09-06 20:10:31.319   519   569 I GonkGfx : GraphicBufferLocked::GraphicBufferLocked() this 0x434515b0
09-06 20:10:31.319   519   569 I GonkGfx : VideoGraphicBuffer::VideoGraphicBuffer() this 0x434515b0
09-06 20:10:31.319   144   270 I GonkGfx : CompositableParentManager::ReceiveCompositableUpdate() TOpAddTexture_E
09-06 20:10:31.319   144   270 I GonkGfx : CompositableParentManager::ReceiveCompositableUpdate() op.textureID() 3
09-06 20:10:31.319   144   270 I GonkGfx : CompositableParentManager::ReceiveCompositableUpdate() compositable 0x44143340 compositable->GetCompositor() 0x0
09-06 20:10:31.319   144   270 I GonkGfx : CompositableParentManager::ReceiveCompositableUpdate() TOpAddTexture_X
09-06 20:10:31.319   519   527 I Gecko   :     <<< [End]   void mozilla::layers::ImageBridgeChild::EndTransaction() 0x434c66e0
09-06 20:10:31.319   519   527 I Gecko   :  <<< [End]   void mozilla::layers::UpdateImageClientNow(mozilla::layers::ImageClient*, mozilla::layers::ImageContainer*) 0x0

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #10)
> Created attachment 801512 [details] [diff] [review]
> make the uint32 textureID in IPDL uint64 instead.

In current master, I found following 32bit texture ids, aren't they needed to be updated, are they?

struct OpCreatedTexture {
  PCompositable compositable;
  uint32_t textureId;
  SurfaceDescriptor descriptor;
  TextureInfo textureInfo;
};

struct OpPaintTexture {
  PCompositable compositable;
  uint32_t textureId;
  SurfaceDescriptor image;
};

struct OpPaintTextureIncremental {
  PCompositable compositable;
  uint32_t textureId;
  SurfaceDescriptor image;
  nsIntRegion updatedRegion;
  nsIntRect bufferRect;
  nsIntPoint bufferRotation;
};

struct OpTextureSwap {
  PCompositable compositable;
  uint32_t textureId;
  SurfaceDescriptor image;
};

struct ReplyTextureRemoved {
  PCompositable compositable;
  uint32_t textureId;
};

Comment 15

[Nicolas Silva [:nical]]

Attachment: Fix the TextureHost linked list

Oh my! It turns out that one GrallocTextureHostOGL that was kept alive because of me failing at implementing a link list :(

The last gralloc texture host was never removed from the list, so it was kept alive until the Compositable itself got destroyed (much later, not sure why). The shared buffer was handled properly but the TextureHost object itself would just stay empty in the list. Not harmful but not pretty.

Comment 16

[Nicolas Silva [:nical]]

Attachment: Make the uint32 textureID in OpRemoveTexture uint64 instead.

Silly me, I changed uint32 -> uint64 for the the wrong struct in my previous patch.

Comment 17

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #14)
> In current master, I found following 32bit texture ids, aren't they needed
> to be updated, are they?
> 
> struct OpCreatedTexture {
>   PCompositable compositable;
>   uint32_t textureId;
>   SurfaceDescriptor descriptor;
>   TextureInfo textureInfo;
> };
> 
> struct OpPaintTexture {
>   PCompositable compositable;
>   uint32_t textureId;
>   SurfaceDescriptor image;
> };
> 
> struct OpPaintTextureIncremental {
>   PCompositable compositable;
>   uint32_t textureId;
>   SurfaceDescriptor image;
>   nsIntRegion updatedRegion;
>   nsIntRect bufferRect;
>   nsIntPoint bufferRotation;
> };
> 
> struct OpTextureSwap {
>   PCompositable compositable;
>   uint32_t textureId;
>   SurfaceDescriptor image;
> };

Actually no, Only OpRemoveTexture, OpAddTexture, OpUseTexture, OpUpdateTexture and ReplyTextureRemoved need uint64 textureIDs. Those are the messages used with new textures.

The others messages with "textureId" members are from the deprecated world and, confusingly enough, textureId here means "Front buffer" or "texture on Black", etc. They don't refer to IDs as in "the key for a hash-table". Those are 32 bits and I verified that they are meant to be so.

Comment 18

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=d1dba3964b9d

Comment 19

[Nicolas Silva [:nical]]

Attachment: Ensure we always clear the textures at the end of a video.

Comment 20

[Nicolas Silva [:nical]]

Sorry Sotaro I just gave you a lot of reviews and you are probably busy, don't hesitate to reassign them to someone else if you don't have the time.

Comment 21

[Sotaro Ikeda [:sotaro]]

Comment on attachment 801683 [details] [diff] [review]
Fix the TextureHost linked list

> void
> CompositableHost::RemoveTextureHost(uint64_t aTextureID)
> {
>+  if (mFirstTexture && mFirstTexture->GetID() == aTextureID) {
>+    RefPtr<TextureHost> toRemove = mFirstTexture;
>+    mFirstTexture = mFirstTexture->GetNextSibling();
>+    toRemove->SetNextSibling(nullptr);
>+  }
>   RefPtr<TextureHost> it = mFirstTexture;
>   while (it) {
>     if (it->GetNextSibling() &&
>         it->GetNextSibling()->GetID() == aTextureID) {
>+      RefPtr<TextureHost> toRemove = it;
>       it->SetNextSibling(it->GetNextSibling()->GetNextSibling());
>+      it->SetNextSibling(nullptr);
>     }
>     it = it->GetNextSibling();
>   }
> }

Pointer handling seems not correct. How about following? By this change, H.264 video playback's problem was fixed. It happened especially when seeking.

void
CompositableHost::RemoveTextureHost(uint64_t aTextureID)
{
  if (mFirstTexture && mFirstTexture->GetID() == aTextureID) {
    RefPtr<TextureHost> toRemove = mFirstTexture;
    mFirstTexture = mFirstTexture->GetNextSibling();
    toRemove->SetNextSibling(nullptr);
    return;
  }
  RefPtr<TextureHost> it = mFirstTexture;
  while (it) {
    if (it->GetNextSibling() &&
        it->GetNextSibling()->GetID() == aTextureID) {
      RefPtr<TextureHost> toRemove = it->GetNextSibling();
      it->SetNextSibling(it->GetNextSibling()->GetNextSibling());
      toRemove->SetNextSibling(nullptr);
    }
    it = it->GetNextSibling();
  }
}

Comment 22

[Sotaro Ikeda [:sotaro]]

Sorry, return seems not necessary. The following is changed version. How about it?
void
CompositableHost::RemoveTextureHost(uint64_t aTextureID)
{
  if (mFirstTexture && mFirstTexture->GetID() == aTextureID) {
    RefPtr<TextureHost> toRemove = mFirstTexture;
    mFirstTexture = mFirstTexture->GetNextSibling();
    toRemove->SetNextSibling(nullptr);
  }
  RefPtr<TextureHost> it = mFirstTexture;
  while (it) {
    if (it->GetNextSibling() &&
        it->GetNextSibling()->GetID() == aTextureID) {
      RefPtr<TextureHost> toRemove = it->GetNextSibling();
      it->SetNextSibling(it->GetNextSibling()->GetNextSibling());
      toRemove->SetNextSibling(nullptr);
    }
    it = it->GetNextSibling();
  }
}

Comment 23

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #22)
> CompositableHost::RemoveTextureHost(uint64_t aTextureID)
> {
>   if (mFirstTexture && mFirstTexture->GetID() == aTextureID) {
>     RefPtr<TextureHost> toRemove = mFirstTexture;
>     mFirstTexture = mFirstTexture->GetNextSibling();
>     toRemove->SetNextSibling(nullptr);
>   }
>   RefPtr<TextureHost> it = mFirstTexture;
>   while (it) {
>     if (it->GetNextSibling() &&
>         it->GetNextSibling()->GetID() == aTextureID) {
>       RefPtr<TextureHost> toRemove = it->GetNextSibling();
>       it->SetNextSibling(it->GetNextSibling()->GetNextSibling());
>       toRemove->SetNextSibling(nullptr);
>     }
>     it = it->GetNextSibling();
>   }
> }

Heh, yes, good catch!

Comment 24

[Nicolas Silva [:nical]]

Landed the uint64 fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f406d02bf24

Comment 25

[Nicolas Silva [:nical]]

Attachment: Fix the TextureHost linked list

Comment 26

[Nicolas Silva [:nical]]

Landed the linked list fix:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19918a47a06f

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2f406d02bf24
https://hg.mozilla.org/mozilla-central/rev/19918a47a06f

Comment 28

[Nicolas Silva [:nical]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f49d97642d4c

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f49d97642d4c

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

We should not try and verify this until after bug 916112 is verified because the preference change completely removed some code paths from active use.