Closed Bug 947113 Opened 6 years ago Closed 6 years ago

Rtsp: Can't post seek command due to decoder thread is blocked.

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

RESOLVED FIXED
blocking-b2g 1.3+
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed

People

(Reporter: vchang, Assigned: brsun)

References

Details

(Whiteboard: [ft:ril])

Attachments

(1 file, 2 obsolete files)

When perform the seek operation, the decoder thread is blocked caused by network data starvation (caused by Rtsp pause command, the rtsp server stops sending RTP packets when receive pause command). We are failed to post a seek command to decoder thread in this case.(main thread -> state machine thread -> decoder thread)
Assignee: nobody → brsun
Blocks: 898866
This patch can fix the blocking issue of the binder thread and the decoding thread caused by the following scenario:
 1. Play RTSP streaming by clicking the play button.
 2. Pause RTSP streaming by clicking the pause button.
 3. Seek to any position. <-- The binder thread in OMXCodec blocks the decoding thread in MediaDecoderStateMachine
 4. Play RTSP streaming by clicking the play button. <-- MediaDecoder cannot enter play state because the previous seeking operation has not completed.

This patch modify the timeing to trigger RTSP streaming play and pause:
 - Current: Start RTSP streaming right after a user clicks the play button. Stop RTSP streaming right after a user clicks the pause button.
 - Patch: Start RTSP streaming right after the decoding thread in MediaDecoderStateMachine starts and before calling OMXCodec::start(). Stop RTSP streaming right before the decoding thread in MediaDecoderStateMachine stops and after calling OMXCodec::pause().

In current workflow, RTSP streaming stops right after a user clicks the pause button. So the binder thread which calls OMXCodecObserver::onMessage() --> OMXCodec::on_message() --> OMXCodec::drainInputBuffer() --> RtspMediaSource::read() will be blocked due to network data starvation. Since OMXCodec::mLock is held by this binder thread at this moment, the decoding thread in MediaDecoderStateMachine will be blocked in OMXCodec::read() as well due to OMXCodec::mLock. Once MediaDecoder enters the seeking state in this scenario, MediaDecoder cannot leave the seeking state even MediaDecoder::Play() is called because the decoding thread in MediaDecoderStateMachine is blocked by OMXCodec::mLock and no chance to handle the seeking operation. As a result, there is no data coming from network anymore, and both the decoding thread and the binder thread block forever.

In the new workflow, RTSP streaming stops right after OMXDecoder::Pause() has been called when the decoding thread in MediaDecoderStateMachine is about to be destroyed. By doing so, the decoding thread won't be blocked by the binder thread in OMXCodec. Similarly, RTSP streaming starts right before OMXDecoder::Play() will be called when the decoding thread in MediaDecoderStateMachine has just been created. By doing so, the network data have chance to be retrieved when the binder thread in OMXCodec calls RtspMediaSource::read().

Influence: After the pause button has been clicked, the network data is still streaming until the decode buffers are full. There will be a little bit more network usage after applying this patch comparing to the original workflow. But the amount of extra network data usage would be small since the decode buffers in the state machine is very limited currently (video is 3 frames in B2G, 10 frames in other platforms; audio is 1000000 microseconds)
Attachment #8344446 - Flags: review?(chris.double)
This is a very important fix for Rtsp seek function. It would be better if we could land it on 1.3. 
Nominate it for madai?
blocking-b2g: --- → madai?
Declarations of override functions in the header file were missing somehow. Add these declarations back.
Attachment #8344446 - Attachment is obsolete: true
Attachment #8344446 - Flags: review?(chris.double)
Attachment #8345118 - Flags: review?(chris.double)
Rtsp should be landed to 1.3.
blocking-b2g: madai? → 1.3?
Whiteboard: [ft:ril]
Comment on attachment 8345118 [details] [diff] [review]
bug947113_rtsp_seek_deadlock.v2.patch

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

::: content/media/omx/RtspOmxReader.cpp
@@ +289,5 @@
>    return MediaOmxReader::Seek(aTime, aStartTime, aEndTime, aCurrentTime);
>  }
>  
> +void RtspOmxReader::OnDecodeThreadStart() {
> +  // Start RTSP streaming right starting the decoding thread in

This comment doesn't parse for me. Is the word 'starting' supposed to be something else or is a word missing?
Attachment #8345118 - Flags: review?(chris.double) → review+
(In reply to Chris Double (:doublec) from comment #5)
> Comment on attachment 8345118 [details] [diff] [review]
> bug947113_rtsp_seek_deadlock.v2.patch
> 
> Review of attachment 8345118 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/RtspOmxReader.cpp
> @@ +289,5 @@
> >    return MediaOmxReader::Seek(aTime, aStartTime, aEndTime, aCurrentTime);
> >  }
> >  
> > +void RtspOmxReader::OnDecodeThreadStart() {
> > +  // Start RTSP streaming right starting the decoding thread in
> 
> This comment doesn't parse for me. Is the word 'starting' supposed to be
> something else or is a word missing?

Thanks. One word is missing indeed:
+  // Start RTSP streaming right *after* starting the decoding thread in
+  // MediaDecoderStateMachine and before starting OMXCodec decoding.
This is important bug fix for RTSP feature in 1.3.
blocking-b2g: 1.3? → 1.3+
This patch is ready to be check-in:
 - based on attachment 8345118 [details] [diff] [review]
 - add one missing word in the comment
 - add reviewer information in the commit log
Keywords: checkin-needed
Attachment #8345118 - Attachment is obsolete: true
Blocks: 904177
No longer blocks: 904177
You need to log in before you can comment on or make changes to this bug.