Adds COM init to AudioManager thread for Windows

Adds COM init to AudioManager thread to ensure that we can call COM APIs (e.g. ask for native sample rate in WASAPI) from this thread.

BUG=none
TEST=content_unittests on Windows


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=157225

[henrika (OOO until Aug 14), 2012-09-17 15:48:31]

Move COM initialization "up" to the AM thread and removed duplicate init further
down. Resolves issue on Windows where we could crash asking for sample rate and
buffer size on the AM thread in unit tests.

Question: why did we not fail in Chrome?

[tommi (sloooow) - chröme, 2012-09-17 15:56:45]

lgtm with a couple of comments.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_base.h
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:24: // concurrency model to multi-threaded.
s/concurrency model to multi-threaded/COM threading model on Windows to MTA.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:26: public:
indent (and below)

[DaleCurtis, 2012-09-17 15:57:16]

I don't know anything about COM initialization, so I defer to scherkus, tommi.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:51: using base::win::ScopedCOMInitializer;
This needs an IF (OS_WIN) block right?

[tommi (sloooow) - chröme, 2012-09-17 15:59:36]

ScopedComInitializer exists as a noop on non win platforms, so this is fine
as is, save for the 1 byte allocation that will happen on other platforms.


On Mon, Sep 17, 2012 at 5:57 PM, <dalecurtis@chromium.org> wrote:

> I don't know anything about COM initialization, so I defer to scherkus,
> tommi.
>
>
> https://codereview.chromium.**org/10909271/diff/1/media/**
>
audio/audio_manager_base.cc<https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_base.cc>
> File media/audio/audio_manager_**base.cc (right):
>
> https://codereview.chromium.**org/10909271/diff/1/media/**
>
audio/audio_manager_base.cc#**newcode51<https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_base.cc#newcode51>
> media/audio/audio_manager_**base.cc:51: using
> base::win::**ScopedCOMInitializer;
> This needs an IF (OS_WIN) block right?
>
>
https://codereview.chromium.**org/10909271/<https://codereview.chromium.org/1...
>

[scherkus (not reviewing), 2012-09-17 16:23:48]

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:43: : base::Thread(name) {
fix indent

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:84: audio_thread_.reset(new
media::AudioThread("AudioThread"));
nit: if you don't need to pass in multiple names for the thread I'd simply
remove the ctor param + inline the thread name

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_base.h
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:25: class AudioThread : public base::Thread {
nit: this class can be forward declared + moved to .cc

[henrika (OOO until Aug 14), 2012-09-17 16:43:29]

PTAL Andrew. Would like to land ASAP. Thanks.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:43: : base::Thread(name) {
On 2012/09/17 16:23:48, scherkus wrote:
> fix indent

Done.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.cc:51: using base::win::ScopedCOMInitializer;
No. As Tommi says; only adds one byte on other platforms. It's magic ;-)

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_base.h
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:24: // concurrency model to multi-threaded.
On 2012/09/17 15:56:45, tommi wrote:
> s/concurrency model to multi-threaded/COM threading model on Windows to MTA.

Done.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:25: class AudioThread : public base::Thread {
On 2012/09/17 16:23:48, scherkus wrote:
> nit: this class can be forward declared + moved to .cc

Done.

https://codereview.chromium.org/10909271/diff/1/media/audio/audio_manager_bas...
media/audio/audio_manager_base.h:26: public:
On 2012/09/17 15:56:45, tommi wrote:
> indent (and below)

Done.

[henrika (OOO until Aug 14), 2012-09-17 16:44:08]

https://codereview.chromium.org/10909271/diff/10001/media/audio/audio_manager...
File media/audio/audio_manager_base.h (right):

https://codereview.chromium.org/10909271/diff/10001/media/audio/audio_manager...
media/audio/audio_manager_base.h:15: #include "base/threading/thread.h"
Will fix.

[scherkus (not reviewing), 2012-09-17 16:50:04]

lgtm w/ nits

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:46: public:
indent one space

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:50: protected:
indent one space

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:54: private:
indent one space

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:55:
scoped_ptr<base::win::ScopedCOMInitializer> com_initializer_;
q: this has to be a scoped_ptr<> because we need to CoUninit() in CleanUp() as
opposed to ~AudioThread()?

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:60: : base::Thread("AudioThread") {
nit: can fit on previous line

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:68: using base::win::ScopedCOMInitializer;
nit: move this statement to before "namespace media" -- that way you can also
trim down the declaration inside scoped_ptr<T>

[henrika (OOO until Aug 14), 2012-09-17 17:00:49]

Done. Thanks Andrew.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:46: public:
On 2012/09/17 16:50:04, scherkus wrote:
> indent one space

Done.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:50: protected:
On 2012/09/17 16:50:04, scherkus wrote:
> indent one space

Done.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:54: private:
On 2012/09/17 16:50:04, scherkus wrote:
> indent one space

Done.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:55:
scoped_ptr<base::win::ScopedCOMInitializer> com_initializer_;
Good question. I base it on a similar design in the media stream manager (done
by Shijing, reviewed by Tommi) where this was tried without success. I honestly
don't know why exactly.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:60: : base::Thread("AudioThread") {
On 2012/09/17 16:50:04, scherkus wrote:
> nit: can fit on previous line

Done.

https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_...
media/audio/audio_manager_base.cc:68: using base::win::ScopedCOMInitializer;
Cool. Thanks.

[commit-bot: I haz the power, 2012-09-17 17:01:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/henrika@chromium.org/10909271/6003

[tommi (sloooow) - chröme, 2012-09-17 18:52:24]

Re the scoped_ptr question, yes it need to be allocated and freed on the
thread itself and not in the ctor /dtor since they run on a different
thread (or threads)

Sent from my phone.
On Sep 17, 2012 7:00 PM, <henrika@chromium.org> wrote:

> Done. Thanks Andrew.
>
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc>
> File media/audio/audio_manager_**base.cc (right):
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode46<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode46>
> media/audio/audio_manager_**base.cc:46: public:
> On 2012/09/17 16:50:04, scherkus wrote:
>
>> indent one space
>>
>
> Done.
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode50<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode50>
> media/audio/audio_manager_**base.cc:50: protected:
> On 2012/09/17 16:50:04, scherkus wrote:
>
>> indent one space
>>
>
> Done.
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode54<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode54>
> media/audio/audio_manager_**base.cc:54: private:
> On 2012/09/17 16:50:04, scherkus wrote:
>
>> indent one space
>>
>
> Done.
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode55<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode55>
> media/audio/audio_manager_**base.cc:55:
> scoped_ptr<base::win::**ScopedCOMInitializer> com_initializer_;
> Good question. I base it on a similar design in the media stream manager
> (done by Shijing, reviewed by Tommi) where this was tried without
> success. I honestly don't know why exactly.
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode60<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode60>
> media/audio/audio_manager_**base.cc:60: : base::Thread("AudioThread") {
> On 2012/09/17 16:50:04, scherkus wrote:
>
>> nit: can fit on previous line
>>
>
> Done.
>
> https://codereview.chromium.**org/10909271/diff/8002/media/**
>
audio/audio_manager_base.cc#**newcode68<https://codereview.chromium.org/10909271/diff/8002/media/audio/audio_manager_base.cc#newcode68>
> media/audio/audio_manager_**base.cc:68: using
> base::win::**ScopedCOMInitializer;
> Cool. Thanks.
>
>
https://codereview.chromium.**org/10909271/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2012-09-17 20:30:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/henrika@chromium.org/10909271/12010

[commit-bot: I haz the power, 2012-09-17 22:33:39]

Change committed as 157225