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

RESOLVED FIXED in Firefox 34

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ethan, Assigned: ethan)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 fixed, b2g-v2.2 fixed)

Details

(Whiteboard: p=2)

Attachments

(4 attachments, 4 obsolete attachments)

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).
Assignee: nobody → ettseng
1. Introduce a PAUSED state to the state machine of RTSPSource.
2. Delay a PLAY request if the state is PAUSING.
The current state diagram of RTSPSource.
The updated state diagram according to the patch.
Comment on attachment 8498053 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

Benjamin, could you help to feedback this patch?
Attachment #8498053 - Flags: feedback?(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?
Attachment #8498053 - Flags: feedback?(bechen) → feedback+
(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.
Attachment #8498053 - Attachment is obsolete: true
Attachment #8498719 - Flags: review?(sworkman)
Fix a logic error in the last patch.
Attachment #8498719 - Attachment is obsolete: true
Attachment #8498719 - Flags: review?(sworkman)
Attachment #8498832 - Flags: review?(sworkman)
Fix space alignments.
Attachment #8498832 - Attachment is obsolete: true
Attachment #8498832 - Flags: review?(sworkman)
Attachment #8499411 - Flags: review?(sworkman)
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.
Attachment #8499411 - Flags: review?(sworkman) → review+
Refresh comment "r=sworkman".
Attachment #8499411 - Attachment is obsolete: true
Result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c7426becbe3
Status: NEW → ASSIGNED
Keywords: checkin-needed
Blocks: 1056187
Block the 2.1+.
blocking-b2g: --- → 2.1+
https://hg.mozilla.org/mozilla-central/rev/878c12dad5a1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
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
Attachment #8499949 - Flags: approval-mozilla-aurora?
Whiteboard: p=2
Attachment #8499949 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Unable to verify as it is a back-end issue.
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][QAnalyst-verify-] → [QAnalyst-Triage+][QAnalyst-verify-]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.