1074791 - [RTSP] Avoid unnecessary play/pause requests to RTSP server

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RTSP, defect)

Description

[Ethan Tseng [:ethan]]

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.

Comment 1

[Benjamin Chen [:bechen]]

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.

Comment 2

[Ethan Tseng [:ethan]]

(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.

Comment 3

[Ethan Tseng [:ethan]]

Attachment: bug-1074791-wip.patch

A WIP patch.
Benjamin, can you take a look of this patch?

Comment 4

[Benjamin Chen [:bechen]]

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.

Comment 5

[Ken Chang[:kenkai]]

Block the 2.1+.

Comment 6

[Ethan Tseng [:ethan]]

Attachment: bug-1074791-fix.patch

1. Put off play and pause by using nsITimer.
2. Cancel the opposite timer in Play() and Pause().

Comment 7

[Ethan Tseng [:ethan]]

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.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

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());

Comment 9

[Ethan Tseng [:ethan]]

(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.

Comment 10

[Ethan Tseng [:ethan]]

Attachment: Delay play and pause using nsITimer

1. Refine code comments.
2. Ensure the timers are stopped on main thread.

Comment 11

[Benjamin Chen [:bechen]]

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.

Comment 12

[Ethan Tseng [:ethan]]

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.

Comment 13

[Ethan Tseng [:ethan]]

Attachment: Delay play and pause using nsITimer

Add mutex lock to protect contention of timers.

Comment 14

[Ethan Tseng [:ethan]]

Attachment: Delay play and pause using nsITimer

Adjust the declaration location.

Comment 15

[Ethan Tseng [:ethan]]

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 16

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 17

[Ethan Tseng [:ethan]]

Attachment: 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.

Comment 18

[Ethan Tseng [:ethan]]

The result of Tree Herder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0cc9ff995fd

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8a6c8de66a9e

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a6c8de66a9e

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this patch when you get a chance :)

Comment 22

[Ethan Tseng [:ethan]]

(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. :)

Comment 23

[Ethan Tseng [:ethan]]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/187ca75356bb

Comment 25

[Yeojin Chung [:YeojinC]]

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

Comment 26

[Alissa（leave from Mozilla）]

Attachment: palypause.MP4

Comment 27

[Alissa（leave from Mozilla）]

Attachment: logcat of flame 2.1

Comment 28

[Alissa（leave from Mozilla）]

Attachment: 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

Comment 29

[Ethan Tseng [:ethan]]

(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?

Comment 30

[Ethan Tseng [:ethan]]

Hi Jonathan,

Could you help to see if we can reproduce the bug Alissa described in comment 28?

Comment 31

[Alissa（leave from Mozilla）]

(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.

Comment 32

[Jonathan Hao (inactive) [:jhao]]

I can reproduce, though sometimes it starts playing after a long while.

Comment 33

[Jonathan Hao (inactive) [:jhao]]

This seems to be another problem. I filed another bug 1105201.