Closed Bug 1080470 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox OS Graveyard :: RTSP, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.1 S7 (24Oct)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: ethan, Assigned: ethan)

References

Details

(Whiteboard: p=1)

Attachments

(1 file, 1 obsolete file)

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: nobody → ettseng
Whiteboard: p=1
Target Milestone: --- → 2.1 S6 (10oct)
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
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+
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!
Addressed the issues raised in the review comment.
Attachment #8502555 - Attachment is obsolete: true
(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.
Blocks: 1056187
https://hg.mozilla.org/mozilla-central/rev/b0d430fb34b7
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 S6 (10oct) → 2.1 S7 (24Oct)
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?
[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+
Attachment #8503920 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
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.