1075400 - [RTSP] Ensure PausedDone completes before sending a PLAY request to RTSP server

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

Consider the case when RTSP client sends PAUSE and PLAY requests to the server.
  C->S: PAUSE rtsp://<some-url>
  S->C: RTSP/1.0 200 OK
  C->S: PLAY rtsp://<some-url>
        Range: npt=<playTimeBegin>-<playTimeEnd>

When paused, RTSPSource remembers the frame time from the latest received RTP packet
(stored in RTSPSource::mLatestPausedUnit).
Then when it transits from PAUSING to PLAY state, it uses mLatestPausedUnit as the
playTimeBegin parameter in the Range field of PLAY request.

The tricky part is, the update of mLatestPausedUnit is done in PausedDone event.
If the play command comes earlier than this event, it would send a incorrect
playTimeBegin value the server.

Note: This bug partially deals with the issue raised in bug 984295 ([Rtsp] Serialize Rtsp controller requests).

Comment 1

[Ethan Tseng [:ethan]]

Attachment: Add PAUSED state to state machine of RTSPSource

1. Introduce a PAUSED state to the state machine of RTSPSource.
2. Delay a PLAY request if the state is PAUSING.

Comment 2

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram.svg

The current state diagram of RTSPSource.

Comment 3

[Ethan Tseng [:ethan]]

Attachment: RTSPSource State Diagram (updated).svg

The updated state diagram according to the patch.

Comment 4

[Ethan Tseng [:ethan]]

Comment on attachment 8498053 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

Benjamin, could you help to feedback this patch?

Comment 5

[Ethan Tseng [:ethan]]

Attachment: play-pause-request.png

Comment 6

[Benjamin Chen [:bechen]]

Comment on attachment 8498053 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

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

Since we will have |mPlayPending|, do we need |mPausePending| ?

::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
@@ +402,5 @@
> +            if (mPlayPending) {
> +              mPlayPending = false;
> +              int64_t playTimeUs;
> +              if (!msg->findInt64("timeUs", &playTimeUs)) {
> +                playTimeUs = 0;

That means we will seek to zero if we can't find the timestamp in msg? Do we have a better value not 0?

Comment 7

[Ethan Tseng [:ethan]]

(In reply to Benjamin Chen [:bechen] from comment #6)
> Review of attachment 8498053 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Since we will have |mPlayPending|, do we need |mPausePending| ?
We need mPlayPending because we want to put off PLAY request until a preceding PAUSE is done.
By contrast, we don't need to delay PAUSE request, which doesn't carry timing parameter as PLAY request does.
So I think mPausePending is unnecessary.

> ::: netwerk/protocol/rtsp/rtsp/RTSPSource.cpp
> > +            if (mPlayPending) {
> > +              mPlayPending = false;
> > +              int64_t playTimeUs;
> > +              if (!msg->findInt64("timeUs", &playTimeUs)) {
> > +                playTimeUs = 0;
> That means we will seek to zero if we can't find the timestamp in msg? Do we
> have a better value not 0?
Thanks for pointing out this.
I should use mLatestPausedUnit as the playTimeUs directly here.

Comment 8

[Ethan Tseng [:ethan]]

Attachment: Add PAUSED state to state machine of RTSPSource

Comment 9

[Ethan Tseng [:ethan]]

Attachment: Add PAUSED state to state machine of RTSPSource

Fix a logic error in the last patch.

Comment 10

[Ethan Tseng [:ethan]]

Attachment: Add PAUSED state to state machine of RTSPSource

Fix space alignments.

Comment 11

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8499411 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

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

Looks fine to me. r=me.

Comment 12

[Ethan Tseng [:ethan]]

Attachment: Add PAUSED state to state machine of RTSPSource

Refresh comment "r=sworkman".

Comment 13

[Ethan Tseng [:ethan]]

Result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c7426becbe3

Comment 14

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/878c12dad5a1

Comment 15

[Ken Chang[:kenkai]]

Block the 2.1+.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/878c12dad5a1

Comment 17

[Ethan Tseng [:ethan]]

Comment on attachment 8499949 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

Approval Request Comment
[Feature/regressing bug #]: Reduce frame drop rate over RTSP (bug 1056187)
[User impact if declined]: RTSP streaming playback is not smooth
[Describe test coverage new/current, TBPL]: On m-c
[Risks and why]: None
[String/UUID change made/needed]: None

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcb79b5e2c57

Comment 19

[Yeojin Chung [:YeojinC]]

Unable to verify as it is a back-end issue.