If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 34, Firefox OS v2.1

Status

Firefox OS
RTSP
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ethan, Assigned: ethan)

Tracking

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)

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

3 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

3 years ago
Assignee: nobody → ettseng
(Assignee)

Comment 1

3 years ago
Created attachment 8498053 [details] [diff] [review]
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.
(Assignee)

Comment 2

3 years ago
Created attachment 8498057 [details]
RTSPSource State Diagram.svg

The current state diagram of RTSPSource.
(Assignee)

Comment 3

3 years ago
Created attachment 8498058 [details]
RTSPSource State Diagram (updated).svg

The updated state diagram according to the patch.
(Assignee)

Comment 4

3 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

3 years ago
Created attachment 8498659 [details]
play-pause-request.png
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

3 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

3 years ago
Created attachment 8498719 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource
Attachment #8498053 - Attachment is obsolete: true
Attachment #8498719 - Flags: review?(sworkman)
(Assignee)

Comment 9

3 years ago
Created attachment 8498832 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

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

3 years ago
Created attachment 8499411 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

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

3 years ago
Created attachment 8499949 [details] [diff] [review]
Add PAUSED state to state machine of RTSPSource

Refresh comment "r=sworkman".
Attachment #8499411 - Attachment is obsolete: true
(Assignee)

Comment 13

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

Updated

3 years ago
Blocks: 1056187
remote:   https://hg.mozilla.org/integration/b2g-inbound/rev/878c12dad5a1
Keywords: checkin-needed

Comment 15

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

Comment 17

3 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

3 years ago
Whiteboard: p=2

Updated

3 years ago
Attachment #8499949 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcb79b5e2c57
status-b2g-v2.1: --- → fixed
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → fixed
status-firefox35: --- → fixed
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.