Improved CoreAudioUtil::IsSupported()

Improved CoreAudioUtil::IsSupported().

This patch adds one more test in CoreAudioUtil::IsSupported() and that is to be able to create an IMMDeviceEnumerator. We have seen some crashes where it does not seem to be sufficient to check that Audioses.dll can be loaded. I hope we are fine now.

BUG=166397
TEST=media_unittests and different audio clients in Chrome.

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

[henrika (OOO until Aug 14), 2013-03-04 10:32:33]

Tommi, perhaps you could take a first look. Thanks.

[DaleCurtis, 2013-03-04 17:18:26]

lgtm % the (presumably) typo is fixed.

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:103: if (g_audioses_dll_available)
!g_audioses_dll_available ?

[tommi (sloooow) - chröme, 2013-03-04 17:29:01]

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:110: static bool
g_can_create_device_enumerator = CanCreateDeviceEnumerator();
If this function is called on a non-COM initialized thread, then it will fail. 
Also, since we're using the MTA generally, then there's a further requirement
that threads that call this function be initialized into the MTA since the
DLL(s) will potentially make assumptions during initialization about the
apartment model being used.

So, is there a way to guarantee that we are currently in an MTA thread here? 
Otherwise we can erroneously fail and I suspect that that false positive would
cause problems for a lot more users than we're trying to fix the problem for.

[henrika (OOO until Aug 14), 2013-03-05 09:09:10]

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:103: if (g_audioses_dll_available)
My bad. Will fix. Thanks!

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:110: static bool
g_can_create_device_enumerator = CanCreateDeviceEnumerator();
Tommi, great comments as usual, please see replies inline:

On 2013/03/04 17:29:01, tommi wrote:
> If this function is called on a non-COM initialized thread, then it will fail.

Yes it will but why is that a problem? Then we know that Core Audio is not
supported, right?

> Also, since we're using the MTA generally, then there's a further requirement
> that threads that call this function be initialized into the MTA since the
> DLL(s) will potentially make assumptions during initialization about the
> apartment model being used.

Yes, but I don't understand how that is related to this patch (see below).
> 
> So, is there a way to guarantee that we are currently in an MTA thread here? 
> Otherwise we can erroneously fail and I suspect that that false positive would
> cause problems for a lot more users than we're trying to fix the problem for.

I added logging of all calls and included thread ID + HRESULT from
CoInitializeEx(COINIT_MULTITHREADED) =>

[2376:3928:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 3928
[2376:3928:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 80010106
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1
[2376:3928:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 3928
[2376:3928:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 80010106
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1
[2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
GetCurrentThreadId: 6636
[2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] # COINIT_MULTITHREADED
=> hr: 1

where RPC_E_CHANGED_MODE <=> 80010106

Thread ID 6636 is the AudioThread and 3928 is the CrBrowserMain thread hence it
seems like the main thread is COM STA. Hence, we do ask for support under two
different models but we never use Core Audio under other than MTA.

Sound OK?

[tommi (sloooow) - chröme, 2013-03-05 09:32:10]

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:86: return SUCCEEDED(hr);
add a check here:
CHECK_NE(hr, CO_E_NOTINITIALIZED);

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:110: static bool
g_can_create_device_enumerator = CanCreateDeviceEnumerator();
Yes, that sounds OK with me but ask that we add the CHECK_NE() above so that we
will catch false positives.

On 2013/03/05 09:09:11, henrika wrote:
> Tommi, great comments as usual, please see replies inline:
> 
> On 2013/03/04 17:29:01, tommi wrote:
> > If this function is called on a non-COM initialized thread, then it will
fail.
> 
> Yes it will but why is that a problem? Then we know that Core Audio is not
> supported, right?
> 
> > Also, since we're using the MTA generally, then there's a further
requirement
> > that threads that call this function be initialized into the MTA since the
> > DLL(s) will potentially make assumptions during initialization about the
> > apartment model being used.
> 
> Yes, but I don't understand how that is related to this patch (see below).
> > 
> > So, is there a way to guarantee that we are currently in an MTA thread here?

> > Otherwise we can erroneously fail and I suspect that that false positive
would
> > cause problems for a lot more users than we're trying to fix the problem
for.
> 
> I added logging of all calls and included thread ID + HRESULT from
> CoInitializeEx(COINIT_MULTITHREADED) =>
> 
> [2376:3928:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 3928
> [2376:3928:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 80010106
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> [2376:3928:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 3928
> [2376:3928:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 80010106
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> [2376:6636:0305/095554:VERBOSE1:core_audio_util_win.cc(92)] #
> GetCurrentThreadId: 6636
> [2376:6636:0305/095554:ERROR:core_audio_util_win.cc(111)] #
COINIT_MULTITHREADED
> => hr: 1
> 
> where RPC_E_CHANGED_MODE <=> 80010106
> 
> Thread ID 6636 is the AudioThread and 3928 is the CrBrowserMain thread hence
it
> seems like the main thread is COM STA. Hence, we do ask for support under two
> different models but we never use Core Audio under other than MTA.
> 
> Sound OK?

[henrika (OOO until Aug 14), 2013-03-05 09:49:34]

Tommi,

hit me again.

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:86: return SUCCEEDED(hr);
On 2013/03/05 09:32:10, tommi wrote:
> add a check here:
> CHECK_NE(hr, CO_E_NOTINITIALIZED);

Done.

https://codereview.chromium.org/12378066/diff/1/media/audio/win/core_audio_ut...
media/audio/win/core_audio_util_win.cc:110: static bool
g_can_create_device_enumerator = CanCreateDeviceEnumerator();
Got it, thanks. Part of my comments above were invalid since I forgot that we
only check once. Never mind. Will make change as proposed.

[tommi (sloooow) - chröme, 2013-03-05 09:58:57]

lgtm

[commit-bot: I haz the power, 2013-03-05 10:35:10]

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

[commit-bot: I haz the power, 2013-03-05 13:08:37]

Change committed as 186166

[DaleCurtis, 2013-03-05 18:36:19]

https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/cor...
File media/audio/win/core_audio_util_win.cc (left):

https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/cor...
media/audio/win/core_audio_util_win.cc:146: CHECK(SUCCEEDED(hr));
I was thinking you'd keep this CHECK() since we shouldn't be hitting this code
if IsSupported() returns false.

And if we still saw CHECK() failures here we've got an even weirder problem
where CoCreateInstance eventually fails after previously succeeding.

[henrika (OOO until Aug 14), 2013-03-05 18:58:51]

https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/cor...
File media/audio/win/core_audio_util_win.cc (left):

https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/cor...
media/audio/win/core_audio_util_win.cc:146: CHECK(SUCCEEDED(hr));
I can fix that in a separate CL. OK?

[tommi (sloooow) - chröme, 2013-03-05 19:01:55]

Ok with me as long as we keep the check.
On Mar 5, 2013 7:58 PM, <henrika@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/12378066/diff/**
>
7001/media/audio/win/core_**audio_util_win.cc<https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/core_audio_util_win.cc>
> File media/audio/win/core_audio_**util_win.cc (left):
>
> https://chromiumcodereview.**appspot.com/12378066/diff/**
>
7001/media/audio/win/core_**audio_util_win.cc#oldcode146<https://chromiumcodereview.appspot.com/12378066/diff/7001/media/audio/win/core_audio_util_win.cc#oldcode146>
> media/audio/win/core_audio_**util_win.cc:146: CHECK(SUCCEEDED(hr));
> I can fix that in a separate CL. OK?
>
>
https://chromiumcodereview.**appspot.com/12378066/<https://chromiumcodereview...
>