1014360 - Cannot render MP4 video - vdec_open failed

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Ethan Tseng [:ethan]]

I encountered this bug while testing RTSP streaming. However I found it's not RTSP-specific. Playing media through HTTP streaming also has this problem.

Reproduction Steps:
1. Open B2G browser.
2. Navigate to a page contains a list of MP4 media links. (e.g. http://10.247.24.86)
3. Click one MP4 media link. (e.g. http://10.247.24.86/captain.mp4)
4. Press back to the previous page.
5. Click another MP4 media link (e.g. http://10.247.24.86/lance_burton.mp4)
6. Repeat step 3-5 for several times, until the video is not rendered on the screen.
   When this bug is occurring, we can see the audio is playing and the scrubber keeps going.

There are some clues in the logcat:
E/vdec    (  115): vdec: failed to allocate pmem arena (2621440 bytes)
E/omx_vdec(  115): ERROR!!! vdec_open failed

Comment 1

[Ethan Tseng [:ethan]]

Attachment: unagi.log

The logcat output while the bug is happening.

Comment 2

[Ethan Tseng [:ethan]]

Attachment: device-2014-05-22-112115.png

A screenshot while the bug is happening.

Comment 3

[Ethan Tseng [:ethan]]

Hi Bruce,

I can reproduce this bug on the latest v2.0 build on both Unagi and Buri.
It seems to be a critical issue.
Can you kindly help to look into it?

Comment 4

[Jason Smith [:jsmith]]

QA Wanted to check 1.4.

Comment 5

[Bruce Sun [:brsun]]

Test results of PVT builds:
 - hamachi-eng-mozilla-central-20140512160204-ril01.02.00.019.102.zip is good.
 - hamachi-eng-mozilla-central-20140513040202-ril01.02.00.019.102.zip is not good.

Comment 6

[William Hsu [:whsu]]

...a regression bug?!
Thanks Bruce! Have a nice day!

Comment 7

[Jason Smith [:jsmith]]

Actually let's first confirm this doesn't happen on 1.4 before getting the window.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

20140512: 20ca234fd62b
20140513: 4b6d63b05a0a

That gives us a regression range of:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=20ca234fd62b&tochange=4b6d63b05a0a

The only video related changes in that range that stuck and that run on B2G are two pushes by jwwang:

fbae0dc61cd3	JW Wang — Bug 1004669 - Fix leaks in MediaTaskQueue::Dispatch(). r=cpearce

5ceb693e9733	JW Wang — Bug 1001317 - reset |MediaCacheStream::mDidNotifyDataEnded| so that it can notify data ended correctly upon 2nd download. r=roc

You could try reverting them locally to see if that fixes your build.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

We have similar logs in bug 920344 comment 3. Do Unagi and Buri both contain Qualcomm chips?

Comment 10

[Bruce Sun [:brsun]]

(In reply to Chris Pearce (:cpearce) from comment #8)
> 20140512: 20ca234fd62b
> 20140513: 4b6d63b05a0a
> 

Quickly try to backout these two commits locally, but this issue seems still reproducible.

Comment 11

[Bruce Sun [:brsun]]

(In reply to Bruce Sun [:brsun] from comment #10)
> Quickly try to backout these two commits locally, but this issue seems still
> reproducible.

Wrong info... I mean these two commits:

> fbae0dc61cd3	JW Wang — Bug 1004669 - Fix leaks in MediaTaskQueue::Dispatch(). r=cpearce
> 5ceb693e9733	JW Wang — Bug 1001317 - reset |MediaCacheStream::mDidNotifyDataEnded| so that it can notify data ended correctly upon 2nd download. r=roc

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

You could try bisecting the regression range to determine the exact commit in that range where the bug starts occurring.

Comment 13

[Bruce Sun [:brsun]]

Hi Sotaro,

This issue occurs since this commit 7c62af2b1ac9.
Could you help to share some clues of it?

Comment 14

[Jason Smith [:jsmith]]

For the STR here, how many times do you need to repeat step #6 to reproduce this bug?

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Bruce Sun [:brsun] from comment #13)
> Hi Sotaro,
> 
> This issue occurs since this commit 7c62af2b1ac9.
> Could you help to share some clues of it?

I am going to investigate about the problem. Thanks.

Comment 16

[Sotaro Ikeda [:sotaro]]

I seems to regenerate the problem on master hamachi by using the following.
http://people.mozilla.org/~cpeterson/videos/H264_Baseline_Profile_Level_30_640x360p.mp4

STR
-[1] start video playback by browser app
-[2] push the browser app to backbround
-[3] set the browser app to forground again.

continues [2] [3] until the problem happens.

Comment 17

[Sotaro Ikeda [:sotaro]]

I confirmed that this is actually Bug 1000660's regression. It was memory leak:-(

Comment 18

[Sotaro Ikeda [:sotaro]]

Attachment: patch - memory leak when DEALLOCATE_CLIENT is set

Comment 19

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Created attachment 8429402 [details] [diff] [review]
> patch - memory leak when DEALLOCATE_CLIENT is set

I confirmed the fix of the STR in comment16 on master hamachi.

Comment 20

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=b2fd9aaf2d5e

Comment 21

[Nicolas Silva [:nical]]

Comment on attachment 8429402 [details] [diff] [review]
patch - memory leak when DEALLOCATE_CLIENT is set

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

What would be nice, would be to rename this into ShouldDeallocateInFinalize(), add a pure virtual DeallocateSharedData method (to mirror TextureHost's API), and call that in Finalize, rather than relying on all textures to check this and do the right thing in their destructor. I'll file this as a followup.

Comment 22

[Nicolas Silva [:nical]]

Comment on attachment 8429402 [details] [diff] [review]
patch - memory leak when DEALLOCATE_CLIENT is set

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

Wait I reviewed this a bit too quickly! At some point we had a mechanism in place for DEALLOCATE_CLIENT: when this flag was set, the texture data would be held by the transaction until we had received the notification by the compositor that it is safe to delete the data. This patch will work with gralloc but will break shmem textures for instance, since the Compositor might still bu using it when ~TextureClient happens

Comment 23

[Nicolas Silva [:nical]]

The mechanism I am referring to is what we removed in bug 8421991

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #23)
> The mechanism I am referring to is what we removed in bug 8421991

Nical, is it a correct number?

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #22)
> be held by the transaction until we had received the notification by the
> compositor that it is safe to delete the data. This patch will work with
> gralloc but will break shmem textures for instance, since the Compositor
> might still bu using it when ~TextureClient happens

nical, I am not sure about which code is broken by attachment 8429402 [details] [diff] [review]. Can you explain bout it more concretely?

Comment 26

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #24)
> (In reply to Nicolas Silva [:nical] from comment #23)
> > The mechanism I am referring to is what we removed in bug 8421991
> 
> Nical, is it a correct number?

Woops, sorry: I meant bug 971946

Comment 27

[Nicolas Silva [:nical]]

When shared data needs to be deallocated on the client side, we have two options. in both cases the client informs the host that it needs to let go of the shared data before the client deallocates the data.

1) Synchronously: it's the easiest way but it hurst performance so we try to avoid it: in this case we just need to wait for the end of the transaction to deallocate/recycle the shared data.

2) Asynchronously:
 - The client sends a message to the host side
 - The host forgets its references to the data, sends back an async reply
 - The client receives the reply and knows it can safely deallocate the data


originally, we had TextureClientData, which was meant to hold the shared data between the moment the client sends the RemoveTexture message and the moment the reply is received from the host, at which point the shared data would be deallocated.
We removed this in bug 971946, and I forgot why we decided we could do so, because now that I look back I think we shouldn't have removed it (I was the one reviewing the patch, so I am a bit embarrassed that I can't remember the rational behind it).
One way to recover the correct behavior could be to use the AsyncTransactionTracker mechanism you added recently (which IIRC currently only works for ImageBridge) to have an object holding the data and waiting for the async reply to do the destruction.

Anyway, deleting shared data in TextureClient's destructor is not safe because the data is still accessible on the other side.

Comment 28

[Sotaro Ikeda [:sotaro]]

> 
> Anyway, deleting shared data in TextureClient's destructor is not safe
> because the data is still accessible on the other side.

Nical, I am not sure about this part. "mActor->SendClearTextureHostSync()" in TextureClient::ForceRemove() synchronously remove data access from the other side, if TextureFlags::DEALLOCATE_CLIENT is set. Why is it not enough?


void TextureClient::ForceRemove()
{
  if (mValid && mActor) {
    if (GetFlags() & TextureFlags::DEALLOCATE_CLIENT) {
      if (mActor->IPCOpen()) {
        mActor->SendClearTextureHostSync();
        mActor->SendRemoveTexture();
      }
    } else {
      if (mActor->IPCOpen()) {
        mActor->SendRemoveTexture();
      }
    }
  }
  MarkInvalid();
}

Comment 29

[Nicolas Silva [:nical]]

(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> > 
> > Anyway, deleting shared data in TextureClient's destructor is not safe
> > because the data is still accessible on the other side.
> 
> Nical, I am not sure about this part. "mActor->SendClearTextureHostSync()"
> in TextureClient::ForceRemove() synchronously remove data access from the
> other side, if TextureFlags::DEALLOCATE_CLIENT is set. Why is it not enough?
> 
> 
> void TextureClient::ForceRemove()
> {
>   if (mValid && mActor) {
>     if (GetFlags() & TextureFlags::DEALLOCATE_CLIENT) {
>       if (mActor->IPCOpen()) {
>         mActor->SendClearTextureHostSync();
>         mActor->SendRemoveTexture();
>       }
>     } else {
>       if (mActor->IPCOpen()) {
>         mActor->SendRemoveTexture();
>       }
>     }
>   }
>   MarkInvalid();
> }

Ah! that's the important part I forgot about :) r+

Comment 30

[Nicolas Silva [:nical]]

This means we always do synchronous deallocation when we do it on the client side, though. We can do better (we used to be able to have it async with TextureClientData). But at least your patch is correct.

Comment 31

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e8bdfd6198f4

Comment 32

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7469c8431862

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/7469c8431862

Comment 34

[Ethan Tseng [:ethan]]

So glad to see this bug being fixed! ^o^
Thanks for everyone here!

Comment 35

[William Hsu [:whsu]]

Thanks everyone!
Cannot reproduce it on the latest V2.0 build.

* Build Information:
 - Gaia      8d865839d932bfbd5e157f376f74d8cb12bfdd51
 - Gecko     https://hg.mozilla.org/releases/mozilla-aurora/rev/1d4046a8cb6c
 - BuildID   20140610000223
 - Version   32.0a2