Remove MediaStreamDeviceThread in favor of audio worker TaskRunner.

Remove MediaStreamDeviceThread in favor of audio worker TaskRunner.

Part Deux!  See https://codereview.chromium.org/17444005/ for the
previous attempt.  It was reverted oh so long ago because on OSX we
use the UI thread as the "audio thread" for http://crbug.com/158170.

Now that the AudioManager has the concept of a worker thread for
heavy tasks, this can safely be relanded without fear of blocking
any browser threads.

See http://crbug.com/256945 for more context on the previous revert.

BUG=none
TEST=unittests pass.

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

[DaleCurtis, 2014-02-04 19:10:42]

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:58: class
CONTENT_EXPORT MediaStreamProvider
Note: It looks like this class only has one implementation, time to merge with
the impl?

[tommi (sloooow) - chröme, 2014-02-04 20:45:56]

lgtm

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:58: class
CONTENT_EXPORT MediaStreamProvider
On 2014/02/04 19:10:42, DaleCurtis wrote:
> Note: It looks like this class only has one implementation, time to merge with
> the impl?

Sgtm

[DaleCurtis, 2014-02-04 20:56:05]

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_provider.h (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_provider.h:58: class
CONTENT_EXPORT MediaStreamProvider
On 2014/02/04 20:45:57, tommi wrote:
> On 2014/02/04 19:10:42, DaleCurtis wrote:
> > Note: It looks like this class only has one implementation, time to merge
with
> > the impl?
> 
> Sgtm 

Actually I misread this early.  Both AudioInputDeviceManager and
VideoCaptureDeviceManager are MediaStreamProviders so there are two
implementations.  Disregard me!

[tommi (sloooow) - chröme, 2014-02-04 21:55:53]

Done


On Tue, Feb 4, 2014 at 9:56 PM, <dalecurtis@chromium.org> wrote:

>
> https://codereview.chromium.org/154143002/diff/1/content/
> browser/renderer_host/media/media_stream_provider.h
> File content/browser/renderer_host/media/media_stream_provider.h
> (right):
>
> https://codereview.chromium.org/154143002/diff/1/content/
> browser/renderer_host/media/media_stream_provider.h#newcode58
> content/browser/renderer_host/media/media_stream_provider.h:58: class
> CONTENT_EXPORT MediaStreamProvider
> On 2014/02/04 20:45:57, tommi wrote:
>
>> On 2014/02/04 19:10:42, DaleCurtis wrote:
>> > Note: It looks like this class only has one implementation, time to
>>
> merge with
>
>> > the impl?
>>
>
>  Sgtm
>>
>
> Actually I misread this early.  Both AudioInputDeviceManager and
> VideoCaptureDeviceManager are MediaStreamProviders so there are two
> implementations.  Disregard me!
>
> https://codereview.chromium.org/154143002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[henrika (OOO until Aug 14), 2014-02-05 08:42:03]

Thanks Dale!

LGTM with nits.

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_device_manager.cc:243: bool
AudioInputDeviceManager::IsOnDeviceThread() const {
Are we still OK with names like IsOnDeviceThread()?

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_device_manager.h:11: // device
thread, while GetOpenedDeviceInfoById is synchronous on the IO thread.
Is this comment still valid?

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:1359:
device_task_runner_ = audio_manager_->GetWorkerTaskRunner();
It is actually not clear to me when to use GetWorkerTaskRunner() and when to use
GetTaskRunner(). Any clear "rules"?

[DaleCurtis, 2014-02-05 21:47:05]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2014-02-05 21:47:11]

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_device_manager.cc:243: bool
AudioInputDeviceManager::IsOnDeviceThread() const {
On 2014/02/05 08:42:03, henrika wrote:
> Are we still OK with names like IsOnDeviceThread()?

I think it's fine and possibly useful in the context of testing.

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_device_manager.h (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_device_manager.h:11: // device
thread, while GetOpenedDeviceInfoById is synchronous on the IO thread.
On 2014/02/05 08:42:03, henrika wrote:
> Is this comment still valid?

Yes.  I'm not removing any references to the device thread, just replacing it
with the audio thread.  Probably this can be cleaned up such that it doesn't use
the IO thread at all, but that's work for someone more familiar with this code.

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:1359:
device_task_runner_ = audio_manager_->GetWorkerTaskRunner();
On 2014/02/05 08:42:03, henrika wrote:
> It is actually not clear to me when to use GetWorkerTaskRunner() and when to
use
> GetTaskRunner(). Any clear "rules"?

See the documentation for AudioManager::GetWorkerTaskRunner().  Heavyweight
(long) or high frequency tasks are good candidates.  However, it only really
matters on OSX since we have to use the UI thread as the audio thread there.

[commit-bot: I haz the power, 2014-02-05 21:50:44]

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

[henrika (OOO until Aug 14), 2014-02-05 21:52:04]

LGTM++

[henrika (OOO until Aug 14), 2014-02-05 21:52:21]

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/media/media_stream_manager.cc (right):

https://codereview.chromium.org/154143002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/media/media_stream_manager.cc:1359:
device_task_runner_ = audio_manager_->GetWorkerTaskRunner();
Thanks. The OS X part was not clear to me before.

[commit-bot: I haz the power, 2014-02-06 03:32:54]

Change committed as 249252