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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Doug Turner (:dougt)]

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

Comment 1

[Matthew Gregan [:kinetik]]

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

Comment 2

[Matthew Gregan [:kinetik]]

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

Comment 3

[Matthew Gregan [:kinetik]]

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.

Comment 4

[Doug Turner (:dougt)]

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?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 6

[Matthew Gregan [:kinetik]]

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

Comment 7

[Doug Turner (:dougt)]

it is unlikely, but a return from wait only signals that a state change "might" have happened.

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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

Comment 9

[Matthew Gregan [:kinetik]]

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

Comment 10

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/9bbfdb7bc9a8

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What actually shuts down the mAudioPlaybackThread? Looks like "nothing", which would then be the cause of bug 621430...

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also, we don't want to create this thread for non-remote-audio builds, I'm guessing...