871753 - Deadlock during YouTube video playback when data connection is relatively slow compared to Wi-Fi

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P2)

Description

[Sotaro Ikeda [:sotaro]]

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

This bug is created as to investigate the youtube video freeze when data connection is relatively slow as in Bug 863441 comment #105.

Comment 1

[Jason Smith [:jsmith]]

This is a platform issue, not a 3rd party app issue.

Comment 2

[Sotaro Ikeda [:sotaro]]

Deadlock happened during youtube video playback like following STR.

- [1] OMXCodec thread is stopped on data read from ChannelMediaResource.
   The thread continues to hold OMXCodec's mutex.

- [2] nsBuiltinDecoderStateMachine's StateMachineThread is stoped to wait OMXCodec's mutex.
   It happens when the thread try to return video buffer to OMXCodec.
   The thread continues to hold nsBuiltinDecoder's ReentrantMonitor.

- [3] The main thread is stopped to wait nsBuiltinDecoder's ReentrantMonitor.

- [4] OMXCodec thread do not receive data forever from hannelMediaResource.
      Data is delivered via main thread.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: sthreads' stack related deadlock

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - fix deadlock during youtube video playback

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #4)
> Created attachment 749474 [details] [diff] [review]
> patch - fix deadlock during youtube video playback

android::ALooper creates a thread for messaging. By using the ALooper, VideoGraphicBuffer::Unlock() prevents to wait OMXCodex's mutex.

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 749474 [details] [diff] [review]
patch - fix deadlock during youtube video playback

doublec, can you review the patch soon? It is tef+ bug. Thanks.

Comment 7

[cajbir (:cajbir)]

(In reply to Sotaro Ikeda [:sotaro] from comment #6)
> Comment on attachment 749474 [details] [diff] [review]
> patch - fix deadlock during youtube video playback
> 
> doublec, can you review the patch soon? It is tef+ bug. Thanks.

Is there any documentation on the API's you're using? I'm not familiar with them and it's hard to review. Can you provide comments/description in the code explaining what they do if not?

Comment 8

[Sotaro Ikeda [:sotaro]]

I do not know if there is a documentation about it. It mimics android Java's Looper. The way to use is similar to it.

http://developer.android.com/reference/android/os/Looper.html
http://developer.android.com/reference/android/os/Handler.html

Comment 9

[Sotaro Ikeda [:sotaro]]

I am going to add comments tomorrow.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v2 - fix deadlock during youtube video playback

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v2 - add libstagefright_foundation

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #10)
> Created attachment 750088 [details] [diff] [review]
> Part1 v2 - fix deadlock during youtube video playback

Added some comments.

Comment 13

[cajbir (:cajbir)]

Comment on attachment 750090 [details] [diff] [review]
Part2 v2 - add libstagefright_foundation

You'll need to get a reviewer who is a peer for this component to review this, sorry.

Comment 14

[cajbir (:cajbir)]

Comment on attachment 750088 [details] [diff] [review]
Part1 v2 - fix deadlock during youtube video playback

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

Do you know if the same issue is in Android and would need to be made to media/omx-plugin/OmxDecoder.cpp?

::: content/media/omx/OmxDecoder.h
@@ +114,5 @@
>    // OMXCodec does not accept MediaBuffer during seeking. If MediaBuffer is
>    //  returned to OMXCodec during seeking, OMXCodec calls assert.
>    Vector<MediaBuffer *> mPendingVideoBuffers;
> +  // The lock protects mPendingVideoBuffers.
> +  // Do not hold Mutex mPendingVideoBuffersLock -> mSeekLock order.

I don't understand what this comment is saying. Is it saying that you shouldn't hold both locks?

@@ +126,5 @@
>    // Holding time should be minimum.
>    Mutex mSeekLock;
>  
> +  // ALooper is a message loop used in stagefright.
> +  // It creates a thread for messges and handles messages in the thread.

Spelling of 'messges'.

Comment 15

[Sotaro Ikeda [:sotaro]]

(In reply to Chris Double (:doublec) from comment #14)
> Comment on attachment 750088 [details] [diff] [review]
> Part1 v2 - fix deadlock during youtube video playback
> 
> Review of attachment 750088 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you know if the same issue is in Android and would need to be made to
> media/omx-plugin/OmxDecoder.cpp?

omx-plugin does not have this problem. It always copy video data within decode thread and return MediaBuffer to OXMCodec in the thread. So it does not lock StateMachineThread and ImageBridge thread. 

> 
> ::: content/media/omx/OmxDecoder.h
> @@ +114,5 @@
> >    // OMXCodec does not accept MediaBuffer during seeking. If MediaBuffer is
> >    //  returned to OMXCodec during seeking, OMXCodec calls assert.
> >    Vector<MediaBuffer *> mPendingVideoBuffers;
> > +  // The lock protects mPendingVideoBuffers.
> > +  // Do not hold Mutex mPendingVideoBuffersLock -> mSeekLock order.
> 
> I don't understand what this comment is saying. Is it saying that you
> shouldn't hold both locks?

I just say, mSeekLock need to be hold first when the code want to hold both locks. I think, the comment is not necessary. I am going to remove it. 

> 
> @@ +126,5 @@
> >    // Holding time should be minimum.
> >    Mutex mSeekLock;
> >  
> > +  // ALooper is a message loop used in stagefright.
> > +  // It creates a thread for messges and handles messages in the thread.
> 
> Spelling of 'messges'.

I'll fix it in the next patch.

Comment 16

[Sotaro Ikeda [:sotaro]]

Comment on attachment 750090 [details] [diff] [review]
Part2 v2 - add libstagefright_foundation

mwu, can you review the patch?

Comment 17

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #15)
> omx-plugin does not have this problem. It always copy video data within
> decode thread and return MediaBuffer to OXMCodec in the thread. So it does
> not lock StateMachineThread and ImageBridge thread. 

I think omx-plugin has been completely replaced by content/media/omx. What do you guys think of just getting rid of it? It just creates confusion.

Comment 18

[Sotaro Ikeda [:sotaro]]

It is about Firefox browser in android.

Comment 19

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> It is about Firefox browser in android.

Oh, I see. I think it would be nice if Android and B2G used the same thing to avoid duplicating effort, but I guess that's up to you guys.

Comment 20

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v3 - fix deadlock during youtube video playback

Committable patch. Carry "chris.double: review+".

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v3 - add libstagefright_foundation

Committable patch. Carry "mwu: review+ ".

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v3 b2g18 - fix deadlock during youtube video playback

Committable patch. Carry "chris.double: review+".

Comment 23

[Sotaro Ikeda [:sotaro]]

Comment on attachment 750698 [details] [diff] [review]
Part1 v3 b2g18 - fix deadlock during youtube video playback

Committable patch. Carry "chris.double: review+".

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v3 b2g18 - add libstagefright_foundation

Committable patch. Carry "mwu: review+ ".

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: Part1 v3 b2g18-v1.0.1 - fix deadlock during youtube video playback

Committable patch. Carry "chris.double: review+".

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: Part2 v3 b2g18-v1.0.1 - add libstagefright_foundation

Committable patch. Carry "mwu: review+ ".

Comment 27

[Johnny Stenback (:jst)]

https://hg.mozilla.org/projects/birch/rev/b5a42a30e193
https://hg.mozilla.org/projects/birch/rev/2cad9e70289a

Comment 28

[Sotaro Ikeda [:sotaro]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b5a42a30e193
https://hg.mozilla.org/mozilla-central/rev/2cad9e70289a

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/af11c849013a
https://hg.mozilla.org/releases/mozilla-b2g18/rev/88d32aa777f5

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/41ffa95cf01e
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/5927e20d17d6

Comment 31

[Sarah Parsons]

This issue still reproduces on the Inari Device Build ID: 20130529070211

Environmental  Variables:
Kernel Date: Apr 25
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6ca32ed2bbc6
Gaia: 8f5ab7bfd4a2921aab4e2de11e0d79a29c1bb062

Video freezes after a few minutes of watching a youtube video with slow data. I was also able to reproduce bug#863441 where the video will freeze on a green screen. This issue was verified fixed so I can create a new bug or reopen the closed one.

Comment 32

[Jason Smith [:jsmith]]

There's already a bug on this - bug 876843.

Comment 33

[Sotaro Ikeda [:sotaro]]

(In reply to Sarah Parsons from comment #31)
 
> Video freezes after a few minutes of watching a youtube video with slow
> data. I was also able to reproduce bug#863441 where the video will freeze on
> a green screen. This issue was verified fixed so I can create a new bug or
> reopen the closed one.

Green screen might be fixed by Bug 877400. In this case, video playback ended by error in decoding or networking. So it should not be dead lock. Just ended video playback because of error.

Comment 34

[Deepa Subramanian]

Video does not freeze after a few minutes (was able to play a 3 minute video with no interruption) of watching a Youtube video with slow data on Inari (H connectivity). This issue does not reproduces on the Inari v1.0.1 (tested on 2 different builds) 

Inari Build ID:
Build ID: 20130529070211
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/f379dc8c9181
Gaia: ac293ce59acc3bede083fad1b973794fa8bf0253

Inari Build ID: 20130606070202
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e329fbd2d2ed
Gaia: 37b6527c3f73a497f75d057e90386f77ff5a552b

Due to connectivity issue unable to verify for v1.1 on Unagi (only Edge was available. Unable to verify on Leo as Blocked by this bug https://bugzilla.mozilla.org/show_bug.cgi?id=859260.