Remove MediaStreamDeviceThread in favor of using AudioThread.

Remove MediaStreamDeviceThread in favor of using AudioThread.

Doesn't seem to be a good reason for this thread; remove it in favor
of using the thread provided by the AudioManager.

This also ensures we don't issue CoreAudio calls on a thread other
than the main thread for OSX.

Bonus: Making this change required converting several unittests to use
the new TestBrowserThreadBundle instead of manually creating threads.

BUG=158170
TEST=unit tests pass. device changes work.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=208403

[scherkus (not reviewing), 2013-06-20 23:39:25]

https://codereview.chromium.org/17444005/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/media_stream_manager.h (right):

https://codereview.chromium.org/17444005/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/media_stream_manager.h:205:
scoped_refptr<base::MessageLoopProxy> device_loop_;
does this thread do device enumeration?

I know there have been issues in the past w/ accessing devices causing lots of
jank e.g., http://crbug.com/130113

[DaleCurtis, 2013-06-21 00:37:04]

Ah sad, yeah that's a no go for this then as it would mean UI jank....

[DaleCurtis, 2013-06-21 01:03:31]

Though that bug says polling will eventually be removed.  @xians: is this the
case?  Do you expect device enumeration to happen quickly now?

[no longer working on chromium, 2013-06-21 12:20:03]

On 2013/06/21 01:03:31, DaleCurtis wrote:
> Though that bug says polling will eventually be removed.  @xians: is this the
> case?  Do you expect device enumeration to happen quickly now?


Polling was removed a couple of milestones ago, we only do the enumeration after
getting a device changed notification from the monitors.

It is hard to say if enumeration is quick or not, it depends on the OS and the
state of hardware, the worst case is that enumerating some video devices might
take a couple of seconds on Linux, I am not sure if it is the case on other
platforms. But generally it should be quick.

The reason prevented me from doing so was that we used ALSA as the default impl
on Linux, which is a polling scheme and we can't put more burden on the audio
thread, but this has been change after switching to pulse. So this looks good to
me in most of the cases:
Windows: lg
Mac: lg but need some work on media/capture/video. Dale, what thread
audio_manager_->GetMessageLoop() returns on Mac? Is it the UI thread or audio
thread? Due to some crash issues, we have posted the task the main thread for
enumerating/opening/closing video devices, please look at
video_capture_device_qtkit_mac.mm code like performSelectorOnMainThread and
change the code correspondingly there.
Linux: lg for pulse, but not for ALSA. But, should we care about ALSA?

[DaleCurtis, 2013-06-21 20:25:37]

This should be okay in most cases then.  On OSX this will use the main thread,
which it looks like we're already doing for capture device enumeration.

Linux ALSA might glitch during enumeration with this change.  I've added some
UMA statistics for us to see if this is a problem or not.

I also found evidence of crashes during input device enumeration on OSX that
appear to be races with the HAL doing stuff on the main thread:

https://crash.corp.google.com/reportdetail?reportid=474dc41e7a2931be

So I recommend going forward with this change.

[no longer working on chromium, 2013-06-21 20:44:25]

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:942: device_loop_ =
NULL;
will this work?
Previously we join the device_thread when the IO thread is dying, then the
device managers won't post tasks to IO thread anymore, WIth this change, the
audio_thread will outlive the IO thread, so it might post tasks to IO thread
after IO thread dies, what will happen then?

[DaleCurtis, 2013-06-21 20:48:15]

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:942: device_loop_ =
NULL;
On 2013/06/21 20:44:25, xians1 wrote:
> will this work?
> Previously we join the device_thread when the IO thread is dying, then the
> device managers won't post tasks to IO thread anymore, WIth this change, the
> audio_thread will outlive the IO thread, so it might post tasks to IO thread
> after IO thread dies, what will happen then?

Ah... yeah there's a problem here.  I'll see if I can figure out a clean way
around it.

[DaleCurtis, 2013-06-21 22:15:07]

PTAL.

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/17444005/diff/10001/content/browser/renderer_...
content/browser/renderer_host/media/media_stream_manager.cc:942: device_loop_ =
NULL;
On 2013/06/21 20:48:15, DaleCurtis wrote:
> On 2013/06/21 20:44:25, xians1 wrote:
> > will this work?
> > Previously we join the device_thread when the IO thread is dying, then the
> > device managers won't post tasks to IO thread anymore, WIth this change, the
> > audio_thread will outlive the IO thread, so it might post tasks to IO thread
> > after IO thread dies, what will happen then?
> 
> Ah... yeah there's a problem here.  I'll see if I can figure out a clean way
> around it.

Actually this isn't a problem.  I thought it was because the unittests were
falling over, but they were falling over because we weren't actually using a
BrowserThread (which can handle post-death tasks cleanly).  I've fixed this by
switching them over to ajwong's sweet TestBrowserThreadBundle.

At this point the IO thread is dead, so any new tasks posted to it are dropped.
The BrowserThread::PostTask code takes care of this.  Since both
AudioInputDeviceManager and VideoCaptureManager are refcounted, they won't be
destructed until any outstanding AudioManager loop tasks are completed.

[no longer working on chromium, 2013-06-24 16:42:44]

As discussed offline, I will let you decide if you want to clean up the video
code on Mac in this CL or in a separate CL.
And lgtm for the current version.

[DaleCurtis, 2013-06-24 20:10:43]

xians: Regarding our discussion this morning, the OSX docs indicate that calling
performSelectorOnMainThread from the main thread when waitUntilDone == YES will
invoke the task immediately, so there's no need to modify the existing code
right now.  Lets see if this helps first.

scherkus: any comments or defer to xians?

[scherkus (not reviewing), 2013-06-24 23:22:38]

code itself = lgtm / deferring to xians

[commit-bot: I haz the power, 2013-06-24 23:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/17444005/28001

[commit-bot: I haz the power, 2013-06-24 23:46:04]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[DaleCurtis, 2013-06-24 23:49:14]

Whoops +jar for lgtm on histograms.xml

[jar (doing other things), 2013-06-25 00:08:57]

histogtrams.xml LGTM

[commit-bot: I haz the power, 2013-06-25 00:15:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/17444005/28001

[commit-bot: I haz the power, 2013-06-25 04:31:28]

Change committed as 208403