984816 - [RTSP] Play/pause does not work for RTSP streaming

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

Reproduction Steps:
1. Enable the preference "media.rtsp.video.enabled" (disabled by default).
2. Click an RTSP URL.
3. When the video app is playing the RTSP streaming, press pause button.
4. Press play button.

Bug: RTSP streaming is not played after step 4.

Note:
1. Although not 100% sure, we guess this bug happens after bug 973408 was landed.
2. I verified RtspController::Play() was not called in step 4.

Comment 1

[Vincent Chang[:vchang][changyihsin]]

The root cause here is similar to the seek issue we found in Bug 947113. The tasks dispatch to mDecodeTaskQueue is pended because function DecodeVideoFrame() is blocked function. 
http://dxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#603 
http://dxr.mozilla.org/mozilla-central/source/content/media/RtspMediaResource.cpp#214

This issue is easy to reproduce especially when network is slow. Still trying to find the better solution for this.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

bechen has a wip patch for this. Reassign to him.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: bug-984816.patch

There are 2 deadlock issues when press pause/play on RTSP streaming.

1. When the streaming is paused, then press play button.

Thread 1 : |MediaDecoderStateMachine::DecodeVideo()| hold a lock then call |EnsureActive();| dispatch a play command to main thread. This thread will be blocked in RtpsMediaResource because there is no more data in it.

Main thread : be blocked |MediaDecoder::PlaybackPositionChanged()| because the lock is held by thread 1, then the play command can not be executed.

solution: release the lock in |EnsureActive|

2. When press pause button.

Thread 1: binder thread hold a lock in OMXCodec then be blocked in RtspMediaResource.

Thread 2: Trying to execute the pause command but it can't get the lock.

solution : reverse the order in RtspOmxReader::SetIdle()

Comment 4

[Benjamin Chen [:bechen]]

Comment on attachment 8397541 [details] [diff] [review]
bug-984816.patch

Hi Chris:
Please help review this patch. Is it safe to release the monitor in EnsureActive?

Hi Vincent and Ethan:
Please help to try this patch to see if there is still deadlock issues.

Comment 5

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #4)
> Hi Vincent and Ethan:
> Please help to try this patch to see if there is still deadlock issues.
I manually tested both RTSP and HTTP streaming with your patch. I performed play/pause for dozens of times and didn't encounter any blocking (deadlock). So far so good. :)

Comment 6

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

Comment on attachment 8397541 [details] [diff] [review]
bug-984816.patch

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

Good catch!

::: content/media/MediaDecoderStateMachine.cpp
@@ +1498,5 @@
>      return;
>    }
>    mIsReaderIdle = false;
> +  {
> +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());

Please also release the monitor while mReader->SetIdle() is called in SetReaderIdle(), the same bug could happen there too.

Comment 7

[Benjamin Chen [:bechen]]

(In reply to Chris Pearce (:cpearce) from comment #6)
> Comment on attachment 8397541 [details] [diff] [review]
> bug-984816.patch
> 
> Review of attachment 8397541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good catch!
> 
> ::: content/media/MediaDecoderStateMachine.cpp
> @@ +1498,5 @@
> >      return;
> >    }
> >    mIsReaderIdle = false;
> > +  {
> > +    ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> 
> Please also release the monitor while mReader->SetIdle() is called in
> SetReaderIdle(), the same bug could happen there too.

I don't think it is necessary to release the monitor in SetReaderIdle() because the caller of SetReaderIdle() would not hold the monitor. (Now we don't have an EnsureIdle function like EnsureActive)

Comment 8

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

Ah yes, very good.

Comment 9

[Benjamin Chen [:bechen]]

Attachment: bug-984816.patch

try server:
https://tbpl.mozilla.org/?tree=Try&rev=79391c484f6c

r=cpearce

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/d0080077a537

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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