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

RESOLVED FIXED in Firefox 35, 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 S7 (24Oct)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

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

Details

(Whiteboard: p=1)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
We tried to avoid unnecessary play/pause requests to the RTSP server in bug 1074791.

However, the solution is not ideal enough because the timers were added in RtspController, which is in parent process.
The delay between play or pause commands will be magnified after passing through IPC.
If we implement command timers in RtspControllerChild, which is in content process,
we can better remedy the problem and even reduce IPC overhead of RTSP commands.
(Assignee)

Updated

3 years ago
Assignee: nobody → ettseng
Whiteboard: p=1
Target Milestone: --- → 2.1 S6 (10oct)
(Assignee)

Comment 1

3 years ago
Created attachment 8502555 [details] [diff] [review]
Delay play and pause using nsITimer
(Assignee)

Comment 2

3 years ago
Hi Benjamin and Steve,

This patch does exactly the same thing as bug 1074791.
I just moved the play and pause timers from RtspController to RtspControllerChild.
One thing to notice is, RtspControllerChild could be run on the decoder thread, which is from
nsIThreadPool and cannot be the target of nsITimer (the callback will never be executed on such thread).
So we need to dispatch the timer callback to the main thread.

Putting these timers in RtspControllerChild instead of in RtspController can more effectively eliminate
the repeating play/pause issue. Furthermore, it can reduce a lot of IPC overhead caused by frequent play
requests from MediaDecoder. The overall RTSP streaming quality could be improved.

I should do it right in the first place. Sorry to bother you to review it again.
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Attachment #8502555 - Flags: review?(sworkman)
Attachment #8502555 - Flags: review?(bechen)
Comment on attachment 8502555 [details] [diff] [review]
Delay play and pause using nsITimer

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

Code looks generally fine - some cleanups needed. Do you have any data to show that this is significantly better? It seems better in theory, but I'd like to make sure.

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ +375,4 @@
>      }
> +    // We have to dispatch the timer callback to the main thread because the
> +    // decoder thread is a thread from nsIThreadPool and cannot be the timer
> +    // target.

// ... and IPC Send functions should only be called from the main thread.

@@ -343,5 @@
>    NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED);
>  
>    if (!--mSuspendCount) {
> -    if (NS_IsMainThread()) {
> -      if (!OKToSendIPC() || !SendResume()) {

Are you calling SendResume anywhere else? You might be able to get rid of it and SendSuspend. Also, SendResumeEvent and SendSuspendEvent.

@@ +593,5 @@
> +
> +  MutexAutoLock lock(self->mTimerLock);
> +  if (self->mPlayTimer) {
> +    if (self->OKToSendIPC()) {
> +      self->SendPlay();

Put early return condition first.

if (!self->mPlayTimer || !self->OKToSendIPC()) {
  return;
}
self->SendPlay();
self->mPlayTimer = nullptr;
Attachment #8502555 - Flags: review?(sworkman) → review+
Attachment #8502555 - Flags: review?(bechen) → review+
(Assignee)

Comment 4

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

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

::: netwerk/protocol/rtsp/controller/RtspControllerChild.cpp
@@ -343,5 @@
>    NS_ENSURE_TRUE(mSuspendCount > 0, NS_ERROR_UNEXPECTED);
>  
>    if (!--mSuspendCount) {
> -    if (NS_IsMainThread()) {
> -      if (!OKToSendIPC() || !SendResume()) {

No. Both SendResume and SendSuspend are not used anymore. I'll remove them.
The upshot is that RTSP controller treats resume as play and suspend as pause.
It seems we should also remove resume/suspend from PRtspController.ipdl and the
corresponding Recv functions in parent. I tend to file a follow-up to do it.

@@ +593,5 @@
> +
> +  MutexAutoLock lock(self->mTimerLock);
> +  if (self->mPlayTimer) {
> +    if (self->OKToSendIPC()) {
> +      self->SendPlay();

Good advice. Thanks!
(Assignee)

Comment 5

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

Addressed the issues raised in the review comment.
Attachment #8502555 - Attachment is obsolete: true
(Assignee)

Comment 6

3 years ago
(In reply to Steve Workman [:sworkman] from comment #3)
> Code looks generally fine - some cleanups needed. Do you have any data to
> show that this is significantly better? It seems better in theory, but I'd
> like to make sure.

Hi Steve,

I have the following data shared here.

1. Eliminate the overhead caused by sending play requests
The frequent play requests are triggered by MediaDecoder and will call into RtspOmxReader::EnsureActive(),
which calls RtspControllerChild::Play().
RtspController::Play() is called more than 70 times per second. The average interval is around 14 ms.
Before this patch, these play requests will all be passed to RtspControllerParent through IPDL. And this
patch could eliminate this unnecessary overhead.

2. Cancel the pause requests effectively
Most importantly, the interval of successive requests might be amplified after passing through IPC.
We observe normally RtspControllerChild::Pause() is called twice within 1~2 ms in the beginning of playback,
but the interval of these two pauses become 200~400 ms for RtspController::Pause().
Thus, the timer mechanism might fail to eliminate unnecessary requests if implemented in the parent side.
(Assignee)

Comment 7

3 years ago
The result of Treeherder:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=d1dfffc81094
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Blocks: 1056187
https://hg.mozilla.org/mozilla-central/rev/b0d430fb34b7
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
(Assignee)

Comment 10

3 years ago
Comment on attachment 8503920 [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 #8503920 - Flags: approval-mozilla-b2g34?
(Assignee)

Comment 11

3 years ago
[Blocking Requested - why for this release]:
This bug fixes an issue related to bug 1056187 (frame drop over RTSP).
In short, it completes our solution of reducing frame drops.
blocking-b2g: --- → 2.1?
triage: Without this fix, user still may see video lag even if network environment is good. We should uplift this to 2.1.
blocking-b2g: 2.1? → 2.1+

Updated

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