[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)

Assignee

Description

5 years ago
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

Updated

5 years ago
Assignee: nobody → ettseng
Assignee

Comment 1

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

Comment 2

5 years ago
The current state diagram of RTSPSource.
Assignee

Comment 3

5 years ago
The updated state diagram according to the patch.
Assignee

Comment 4

5 years ago
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)
Assignee

Comment 5

5 years ago
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+
Assignee

Comment 7

5 years ago
(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.
Assignee

Comment 8

5 years ago
Attachment #8498053 - Attachment is obsolete: true
Attachment #8498719 - Flags: review?(sworkman)
Assignee

Comment 9

5 years ago
Fix a logic error in the last patch.
Attachment #8498719 - Attachment is obsolete: true
Attachment #8498719 - Flags: review?(sworkman)
Attachment #8498832 - Flags: review?(sworkman)
Assignee

Comment 10

5 years ago
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+
Assignee

Comment 12

5 years ago
Refresh comment "r=sworkman".
Attachment #8499411 - Attachment is obsolete: true
Assignee

Comment 13

5 years ago
Result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1c7426becbe3
Status: NEW → ASSIGNED
Keywords: checkin-needed
Assignee

Updated

5 years ago
Blocks: 1056187
Block the 2.1+.
blocking-b2g: --- → 2.1+
https://hg.mozilla.org/mozilla-central/rev/878c12dad5a1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
Assignee

Comment 17

5 years ago
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?
Assignee

Updated

5 years ago
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.