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

[RTSP] Avoid unnecessary play/pause requests 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 verified, b2g-v2.2 verified)

Details

(Whiteboard: p=2)

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

3 years ago
Current RTSP implementation could suffer some performance bottleneck by sending unnecessary
RTSP PLAY or PAUSE requests to the server.
It is because the MediaDecoderStateMachine could trigger play-pause operations in a very
short time, such as less than 200 ms.
(More detail was described in https://bugzilla.mozilla.org/show_bug.cgi?id=1021006#c19.)

If we work out some mechanism to avoid sending actual requests for this kind of internal
play/pause operations, the playback of RTSP could be more smooth.
(Assignee)

Updated

3 years ago
Assignee: nobody → ettseng
I have an idea to reduce the unnecessary play/pause commands. Remove play/pause in EnsureActive/SetIdle, instead to send play/pause in RtspOmxDecoder::ApplyStateToStateMachine.

We back to the original designed, I mean the very early version of MediaOmxReader/Decoder. The controller play/pause commands are aligned to the "user's play/pause", but unfortunately we encounter the deadlock issue. So we change the play/pause commands align to OMXDecoder's play/pause or we can say StateMachine's play/pause. Till now, we found the frequently play/pause commands cause some problem here.

The StateMachine's play/pause commands doesn't imply the user's play/pause, we should ignore them because the user never presses pause key but we still perform a pause. And the deadlock issue should not appear again because the thread model is changed.
(Assignee)

Comment 2

3 years ago
(In reply to Benjamin Chen [:bechen] from comment #1)
> The StateMachine's play/pause commands doesn't imply the user's play/pause,
> we should ignore them because the user never presses pause key but we still
> perform a pause. And the deadlock issue should not appear again because the
> thread model is changed.
Great. If feasible, this could avoid many potential nuisances in RTSP stack.
I'll generate a patch to see if it works.
(Assignee)

Comment 3

3 years ago
Created attachment 8498723 [details] [diff] [review]
bug-1074791-wip.patch

A WIP patch.
Benjamin, can you take a look of this patch?
Flags: needinfo?(bechen)
(Assignee)

Updated

3 years ago
Blocks: 1056187
Comment on attachment 8498723 [details] [diff] [review]
bug-1074791-wip.patch

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

As we discussed offline, the patch only works on "MediaCodec preference on". Sad, it can't apply to current code base.
Flags: needinfo?(bechen)

Comment 5

3 years ago
Block the 2.1+.
blocking-b2g: --- → 2.1+
(Assignee)

Comment 6

3 years ago
Created attachment 8500485 [details] [diff] [review]
bug-1074791-fix.patch

1. Put off play and pause by using nsITimer.
2. Cancel the opposite timer in Play() and Pause().
Attachment #8498723 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
Comment on attachment 8500485 [details] [diff] [review]
bug-1074791-fix.patch

Hi Steve,

I would like to have your help to review this patch.

The internal pause/play (not from the user) might be triggered from either
MediaDecoderStateMachine or the Gaia video app.
We found this implicit behavior annoying while working on frame drop problem (bug 1056187).

The rationale of our solution is to eliminate a pause request if a play request follows it
after a very short time, which means the pause-play pair is unlikely originated from the user.
In this case, a play-pause-play sequence can be treated as just a play.

Thus, the solution is some kind of optimization in RTSP controller to prevent sending
unnecessary requests to the server, which cause network latency and could impair
smoothing playback.

BTW, RtspController::Resume() and Suspend() do exactly the same thing as Play() and Pause().
Make them invoke Play()/Pause() directly.
Attachment #8500485 - Flags: review?(sworkman)
Comment on attachment 8500485 [details] [diff] [review]
bug-1074791-fix.patch

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

Looks like a way forward to me. I have one question, so I'll leave the review open for now. Otherwise, it looks good.

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +95,5 @@
> +  // Cancel the pause timer if it is active.
> +  if (mPauseTimer) {
> +    mPauseTimer->Cancel();
> +    mPauseTimer = nullptr;
> +  }

Consider the following:

state is PLAYING
Pause is called; pause time started
play is called; pause timer canceled
play timer fires - is play sent to the server?

So, should you ignore play requests if the pause timer is already set? (I think this case (two PLAYS) may be covered elsewhere - please confirm).

@@ +426,5 @@
> +  MOZ_ASSERT(aClosure);
> +
> +  RtspController *self = static_cast<RtspController*>(aClosure);
> +
> +  self->mRtspSource->play();

MOZ_ASSERT(self->mRtspSource.get());
(Assignee)

Comment 9

3 years ago
(In reply to Steve Workman [:sworkman] from comment #8)
> Review of attachment 8500485 [details] [diff] [review]:
> -----------------------------------------------------------------
> Looks like a way forward to me. I have one question, so I'll leave the
> review open for now. Otherwise, it looks good.
> Consider the following:
> 
> state is PLAYING
> Pause is called; pause time started
> play is called; pause timer canceled
> play timer fires - is play sent to the server?
No.
In this cause, pause is canceled so that RTSPSource stays in PLAYING state.
When RTSPSource receives a play command right now, it just ignore it.
 
> So, should you ignore play requests if the pause timer is already set? (I
> think this case (two PLAYS) may be covered elsewhere - please confirm).
We don't have to worry about repeated play or pause commands.
The class RTSPSource (the member mRtspSource of RtspController) has a state machine to protect from 
invalid state transitions.
Any repeated play or pause requests to RTSPSource will be ignored.

> @@ +426,5 @@
> > +  MOZ_ASSERT(aClosure);
> > +  RtspController *self = static_cast<RtspController*>(aClosure);
> > +  self->mRtspSource->play();
> MOZ_ASSERT(self->mRtspSource.get());
Thanks! I'll add it.
(Assignee)

Comment 10

3 years ago
Created attachment 8500894 [details] [diff] [review]
Delay play and pause using nsITimer

1. Refine code comments.
2. Ensure the timers are stopped on main thread.
Attachment #8500485 - Attachment is obsolete: true
Attachment #8500485 - Flags: review?(sworkman)
Attachment #8500894 - Flags: review?(sworkman)
Comment on attachment 8500894 [details] [diff] [review]
Delay play and pause using nsITimer

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

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +330,5 @@
>  {
>  public:
>    SendOnDisconnectedTask(nsIStreamingProtocolListener *listener,
> +                         nsITimer *playTimer,
> +                         nsITimer *pauseTimer,

The logic is wrong here, we should pass the RtspController reference instead of these two timers.
These two timers will not be updated if the timer fires.

@@ +375,4 @@
>    if (mListener) {
>      nsRefPtr<SendOnDisconnectedTask> task =
> +      new SendOnDisconnectedTask(mListener, mPlayTimer, mPauseTimer,
> +                                 index, reason);

Since RtspController::OnDisconnected is not called on mainthread, we should not access mPlayTimer and mPauseTimer here or we have threading issue.. 
So maybe we should have a mutex lock here.
Attachment #8500894 - Flags: review?(sworkman) → review-
(Assignee)

Comment 12

3 years ago
Comment on attachment 8500894 [details] [diff] [review]
Delay play and pause using nsITimer

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

::: netwerk/protocol/rtsp/controller/RtspController.cpp
@@ +375,4 @@
>    if (mListener) {
>      nsRefPtr<SendOnDisconnectedTask> task =
> +      new SendOnDisconnectedTask(mListener, mPlayTimer, mPauseTimer,
> +                                 index, reason);

Yes. This function could be called on non-main thread.
Then I'll add a mutex lock for the timers.
(Assignee)

Comment 13

3 years ago
Created attachment 8501157 [details] [diff] [review]
Delay play and pause using nsITimer

Add mutex lock to protect contention of timers.
Attachment #8500894 - Attachment is obsolete: true
Attachment #8501157 - Flags: review?(sworkman)
(Assignee)

Comment 14

3 years ago
Created attachment 8501162 [details] [diff] [review]
Delay play and pause using nsITimer

Adjust the declaration location.
Attachment #8501157 - Attachment is obsolete: true
Attachment #8501157 - Flags: review?(sworkman)
Attachment #8501162 - Flags: review?(sworkman)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Whiteboard: p=2
Target Milestone: --- → 2.1 S6 (10oct)
(Assignee)

Comment 15

3 years ago
Additional remarks.  
The timer logic in RtspController guarantees mPlayTimer and mPauseTimer will not be activated
simultaneously. So I think using one mutex lock for contention protection is enough.
Comment on attachment 8501162 [details] [diff] [review]
Delay play and pause using nsITimer

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

Thanks for answering my questions.
Benjamin, good catch with the mutex.
r=me.
Attachment #8501162 - Flags: review?(sworkman) → review+
(Assignee)

Comment 17

3 years ago
Created attachment 8501542 [details] [diff] [review]
Delay play and pause using nsITimer

Refresh comment "r=sworkman, r=bechen".
Add one more check in timer callback to make contention protection more robust.
Attachment #8501162 - Attachment is obsolete: true
(Assignee)

Comment 18

3 years ago
The result of Tree Herder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0cc9ff995fd
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/8a6c8de66a9e
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8a6c8de66a9e
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Please request Aurora approval on this patch when you get a chance :)
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
status-firefox33: --- → wontfix
status-firefox34: --- → affected
status-firefox35: --- → fixed
Flags: needinfo?(ettseng)
(Assignee)

Comment 22

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #21)
> Please request Aurora approval on this patch when you get a chance :)
Yes! I will!
Thanks for reminder. :)
Flags: needinfo?(ettseng)
(Assignee)

Comment 23

3 years ago
Comment on attachment 8501542 [details] [diff] [review]
Delay play and pause using nsITimer

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 #8501542 - Flags: approval-mozilla-aurora?

Updated

3 years ago
Attachment #8501542 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/187ca75356bb
status-b2g-v2.1: affected → fixed
status-firefox34: affected → 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)
Created attachment 8528211 [details]
palypause.MP4
Created attachment 8528212 [details]
logcat of flame 2.1
Created attachment 8528221 [details]
logcat of flame 2.2

Please help to check the video,wheather the response of play/pause is right.
1.Launch browser.
2.Input www.wowza.com/html/mobile.html.
3.Tap the RTSP link.
4.Tap play/pause button continuously.
**RTSP will stop playing and can't recorver.

flame 2.1 buid:
Gaia-Rev        8ae086c39011bc8842b2a19bb5267906fa22345a
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ebbd5c65c3c1
Build-ID        20141124094013
Version         34.0
FLame 2.2 build:
Gaia-Rev        c5bad6d78c5fe168e3bb894fc5cb70902c9b19b1
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/c9cfa9b91dea
Build-ID        20141123040209
Version         36.0a1
Flags: needinfo?(hlu)
(Assignee)

Comment 29

3 years ago
(In reply to Alissa from comment #28)
> 4.Tap play/pause button continuously.
> **RTSP will stop playing and can't recover.

Hmm... This seems to be a kind of stress test.
Hi Alissa,
How about the reproduction rate?
(Assignee)

Comment 30

3 years ago
Hi Jonathan,

Could you help to see if we can reproduce the bug Alissa described in comment 28?
Flags: needinfo?(jhao)
(In reply to Ethan Tseng [:ethan] from comment #29)
> (In reply to Alissa from comment #28)
> > 4.Tap play/pause button continuously.
> > **RTSP will stop playing and can't recover.
> 
> Hmm... This seems to be a kind of stress test.
> Hi Alissa,
> How about the reproduction rate?

Always.5/5.
I can reproduce, though sometimes it starts playing after a long while.
Flags: needinfo?(jhao)
Flags: needinfo?(hlu)
This seems to be another problem. I filed another bug 1105201.
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.