Closed Bug 1074791 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: ethan, Assigned: ethan)

References

Details

(Whiteboard: p=2)

Attachments

(4 files, 5 obsolete files)

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: 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.
(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.
Attached patch bug-1074791-wip.patch (obsolete) — Splinter Review
A WIP patch.
Benjamin, can you take a look of this patch?
Flags: needinfo?(bechen)
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)
Block the 2.1+.
blocking-b2g: --- → 2.1+
Attached patch bug-1074791-fix.patch (obsolete) — Splinter Review
1. Put off play and pause by using nsITimer.
2. Cancel the opposite timer in Play() and Pause().
Attachment #8498723 - Attachment is obsolete: true
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());
(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.
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-
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.
Add mutex lock to protect contention of timers.
Attachment #8500894 - Attachment is obsolete: true
Attachment #8501157 - Flags: review?(sworkman)
Adjust the declaration location.
Attachment #8501157 - Attachment is obsolete: true
Attachment #8501157 - Flags: review?(sworkman)
Attachment #8501162 - Flags: review?(sworkman)
Status: NEW → ASSIGNED
Whiteboard: p=2
Target Milestone: --- → 2.1 S6 (10oct)
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+
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
https://hg.mozilla.org/mozilla-central/rev/8a6c8de66a9e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Please request Aurora approval on this patch when you get a chance :)
Flags: needinfo?(ettseng)
(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)
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?
Attachment #8501542 - 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)
Attached file 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)
(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?
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.
You need to log in before you can comment on or make changes to this bug.