Remoting Audio - Thread per stream to avoid problems with blocking drain/write calls

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: dougt, Assigned: kinetik)

Tracking

Trunk
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
+++ This bug was initially created as a clone of Bug #599089 +++

Fallout from review comments:

we need a thread per stream to avoid problems with blocking
drain/write calls affecting other AudioStream's operations.
(Reporter)

Updated

8 years ago
tracking-fennec: 2.0b3+ → ?
(Reporter)

Updated

8 years ago
Depends on: 612799
tracking-fennec: ? → 2.0+
Assignee: nobody → doug.turner
(Assignee)

Comment 1

8 years ago
There's two issues here.  The local stream calls to Write and Drain may block (Drain, in particular, may block for up to two seconds).  This means that we need a thread per AudioStreamLocal so that blocking Write/Drain calls for one stream don't affect other streams.  That's this bug.

The second issue here is that the remote stream Drain needs to block until the local stream Drain completes.  Right now, the IPDL protocol marks Drain as sync, but I think this is the wrong approach since we have to go via the main thread in both processes, so we're going to end up blocking everything for long periods of time.  I don't think there's any point solving the first issue without also solving this one, so this bug should also cover the second issue.

Remote, audio thread:
  - Send drain to main thread
  - Wait for response

Remote, main thread:
  - IPC drain to chrome process

Local, main thread:
  - Shunt drain to audio thread

Local, audio thread:
  - Drain (waits for 2s, for example)
  - Send drain_done to main thread

Local, main thread:
  - IPC drain_done to content process

Remote, main thread:
  - Shunt drain_done to audio thread

Remote, audio thread:
  - Response received, return to caller
(Assignee)

Comment 2

8 years ago
Created attachment 493627 [details] [diff] [review]
possible solution v0

Make Drain() IPC async, then send a DrainDone event back when the local Drain() completes.  The remote Drain waits on a monitor which is signalled when DrainDone() reaches the remote.

Slightly gross, will ask around to see if there's a nicer way to do this.
(Assignee)

Comment 3

8 years ago
Comment on attachment 493627 [details] [diff] [review]
possible solution v0

Had a quick chat to cjones about this.  The nicer way to do this would use direct audio<->audio thread IPC (then drain could be a simple sync IPC), but support for that is awaiting review and it may not be usable for this after the media decoder dethreadification that cpearce is working on anyway.  So this'll do for now.  The post-FF4 audio rewrite will have an async drain, so this problem will disappear when that lands.
Attachment #493627 - Flags: review?(doug.turner)
(Reporter)

Comment 4

8 years ago
Comment on attachment 493627 [details] [diff] [review]
possible solution v0

when you wait on a mozilla::Monitor you must check for a state change when you are notified.  Spurious interrupts can and do happen.

+
+protected:
+  nsRefPtr<nsIThread> mAudioPlaybackThread;
 };

nsCOMPtr<>


AudioParent::RecvDrain()

why not just call Drain directly in this method, and then post a AudioDrainDoneEvent? What is the purpose of AudioDrainEvent.


-sync protocol PAudio
+protocol PAudio
 {

I think all protocols need a default type, don't they?
(In reply to comment #4)
> Comment on attachment 493627 [details] [diff] [review]
> -sync protocol PAudio
> +protocol PAudio
>  {
> 
> I think all protocols need a default type, don't they?

Default is |async|.  This is fine.
(Assignee)

Comment 6

8 years ago
(In reply to comment #4)
> when you wait on a mozilla::Monitor you must check for a state change when you
> are notified.  Spurious interrupts can and do happen.

I know about this in general, but not sure what could possibly cause a spurious wakeup with this particular use.  I can change it to check a state, though.

> nsCOMPtr<>

Hm, yes, but I think it'd be better to make nsAudioStream a simple refcounted class instead of inheriting from nsISupports.  Will fix.

> AudioParent::RecvDrain()
> 
> why not just call Drain directly in this method, and then post a
> AudioDrainDoneEvent? What is the purpose of AudioDrainEvent.

Drain blocks, so it needs to run on the audio thread.
(Reporter)

Comment 7

8 years ago
it is unlikely, but a return from wait only signals that a state change "might" have happened.
(In reply to comment #6)
> (In reply to comment #4)
> > when you wait on a mozilla::Monitor you must check for a state change when you
> > are notified.  Spurious interrupts can and do happen.
> 
> I know about this in general, but not sure what could possibly cause a spurious
> wakeup with this particular use.  I can change it to check a state, though.
> 

Spurious wakeups are part of the pthreads API contract.  Spurious wakeups won't happen on linux/glibc to my knowledge, but it's not great to rely on that.  Also I'm not sure about mac/windows.
(Assignee)

Comment 9

8 years ago
Created attachment 493911 [details] [diff] [review]
patch v1

Ignore the nsAudioStream refcount comment, I was thinking of a different piece of code.

Sets and checks mDrained for monitor state.  Uses nsCOMPtr to hold nsIThread reference.
Assignee: doug.turner → kinetik
Attachment #493627 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #493911 - Flags: review?(doug.turner)
Attachment #493627 - Flags: review?(doug.turner)
(Reporter)

Updated

8 years ago
Attachment #493911 - Flags: review?(doug.turner) → review+
(Reporter)

Comment 10

8 years ago
http://hg.mozilla.org/mozilla-central/rev/9bbfdb7bc9a8
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Blocks: 616504

Updated

8 years ago
Depends on: 620359
What actually shuts down the mAudioPlaybackThread? Looks like "nothing", which would then be the cause of bug 621430...
Also, we don't want to create this thread for non-remote-audio builds, I'm guessing...
You need to log in before you can comment on or make changes to this bug.