Hook up AudioDeviceListenerWin to monitor all default device changes.

Hook up AudioDeviceListenerWin to monitor all default device changes.
In addition to the current functionality, this adds:
* Detection of default capture devices (communication and console).
* Detection of changes to the default communication render device.

While doing this, I decided to also make 'effects' visible in the media-internals page which was very helpful in tracking down the source of the problem I was running into.

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

[tommi (sloooow) - chröme, 2014-03-05 15:55:02]

Rebase to head

[tommi (sloooow) - chröme, 2014-03-05 16:19:08]

Remove unused variable (doh!).

[DaleCurtis, 2014-03-06 01:46:32]

lgtm

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
File media/audio/win/audio_device_listener_win.cc (right):

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:47: DVLOG(1)  << "Failed to
retrieve the device id: " << std::hex << hr;
Extra space? Above too. I'm on mobile, so maybe font :)

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:140: // Only listen for console and
communication device changes.
Are there any other possibilities?

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:146: std::string& current_device_id
=
Const.  Probably cleaner to abstract into a method.

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:176: current_device_id =
new_device_id;
No point now?

[tommi (sloooow) - chröme, 2014-03-06 08:15:21]

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
File media/audio/win/audio_device_listener_win.cc (right):

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:47: DVLOG(1)  << "Failed to
retrieve the device id: " << std::hex << hr;
On 2014/03/06 01:46:32, DaleCurtis wrote:
> Extra space? Above too. I'm on mobile, so maybe font :)

Ah, good eyes :)  Fixed. (above too)

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:140: // Only listen for console and
communication device changes.
On 2014/03/06 01:46:32, DaleCurtis wrote:
> Are there any other possibilities?

Yes, there's also eMultimedia. In case more get added I decided to check for the
ones we support instead of just for (not) eMultimedia.

http://msdn.microsoft.com/en-us/library/windows/desktop/dd370842(v=vs.85).aspx

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:146: std::string& current_device_id
=
On 2014/03/06 01:46:32, DaleCurtis wrote:
> Const.  Probably cleaner to abstract into a method.

I need to modify it below.  Maybe this was hard to follow on the phone, I'll
wait until you get back today.

Here, I'm grabbing a reference to the appropriate id that I want to compare
against the new one and then update.  The logic is the same as it was before but
now I've just added a selector for the 4 supported IDs.

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:176: current_device_id =
new_device_id;
On 2014/03/06 01:46:32, DaleCurtis wrote:
> No point now?

Yes, since this is a reference, it updates the appropriate member variable.

[tommi (sloooow) - chröme, 2014-03-06 08:15:29]

Fix whitespace

[DaleCurtis, 2014-03-06 18:47:35]

Still looks good, I trust your judgement for commit.

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
File media/audio/win/audio_device_listener_win.cc (right):

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:146: std::string& current_device_id
=
On 2014/03/06 08:15:21, tommi wrote:
> On 2014/03/06 01:46:32, DaleCurtis wrote:
> > Const.  Probably cleaner to abstract into a method.
> 
> I need to modify it below.  Maybe this was hard to follow on the phone, I'll
> wait until you get back today.
> 
> Here, I'm grabbing a reference to the appropriate id that I want to compare
> against the new one and then update.  The logic is the same as it was before
but
> now I've just added a selector for the 4 supported IDs.

Oy that's subtle.  I wonder if it's nicer to move most of this function into a
new "void OnDefaultDeviceChanged(XXX, std::string* current_device_id);"

You can then reflow this if and the above if into an easier to read switch
statement.  WDYT?

If that ends up being uglier, at least add a comment to the assignment below to
call out what's happening.

[tommi (sloooow) - chröme, 2014-03-06 19:22:09]

Improve readability.

[tommi (sloooow) - chröme, 2014-03-06 19:22:10]

Improve readability.

[tommi (sloooow) - chröme, 2014-03-06 19:23:20]

Fix whitespace again! (not sure how it snuck back in!?)

[tommi (sloooow) - chröme, 2014-03-06 19:23:20]

Fix whitespace again! (not sure how it snuck back in!?)

[tommi (sloooow) - chröme, 2014-03-06 19:24:13]

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
File media/audio/win/audio_device_listener_win.cc (right):

https://codereview.chromium.org/187593004/diff/40001/media/audio/win/audio_de...
media/audio/win/audio_device_listener_win.cc:146: std::string& current_device_id
=
On 2014/03/06 18:47:36, DaleCurtis wrote:
> On 2014/03/06 08:15:21, tommi wrote:
> > On 2014/03/06 01:46:32, DaleCurtis wrote:
> > > Const.  Probably cleaner to abstract into a method.
> > 
> > I need to modify it below.  Maybe this was hard to follow on the phone, I'll
> > wait until you get back today.
> > 
> > Here, I'm grabbing a reference to the appropriate id that I want to compare
> > against the new one and then update.  The logic is the same as it was before
> but
> > now I've just added a selector for the 4 supported IDs.
> 
> Oy that's subtle.  I wonder if it's nicer to move most of this function into a
> new "void OnDefaultDeviceChanged(XXX, std::string* current_device_id);"
> 
> You can then reflow this if and the above if into an easier to read switch
> statement.  WDYT?
> 
> If that ends up being uglier, at least add a comment to the assignment below
to
> call out what's happening.

I gave it a shot and ended up with a similar approach to this one.
Other approaches involve more code and don't really add much to readability.

Here's what I did though:

* Add documentation.
* Instead of using a reference, I use a pointer.  This makes it imho more
obvious, especially to chrome devs that are more used to either 'const &' or a
pointer (and a non-const reference is a rare thing).

[tommi (sloooow) - chröme, 2014-03-06 19:34:19]

Rebase

[DaleCurtis, 2014-03-06 19:36:54]

lgtm

[tommi (sloooow) - chröme, 2014-03-06 21:45:47]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-03-06 22:21:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/187593004/120001

[commit-bot: I haz the power, 2014-03-06 22:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 22:46:49]

Try jobs failed on following builders: mac_chromium_rel

[tommi (sloooow) - chröme, 2014-03-06 23:00:09]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-03-06 23:03:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/187593004/120001

[commit-bot: I haz the power, 2014-03-07 07:40:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-07 07:40:59]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[tommi (sloooow) - chröme, 2014-03-07 17:25:06]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-03-07 17:27:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/187593004/120001

[commit-bot: I haz the power, 2014-03-07 19:42:40]

Change committed as 255687

[Nico, 2014-07-26 04:43:52]

https://codereview.chromium.org/187593004/diff/120001/media/audio/win/audio_d...
File media/audio/win/audio_device_listener_win.cc (right):

https://codereview.chromium.org/187593004/diff/120001/media/audio/win/audio_d...
media/audio/win/audio_device_listener_win.cc:149: role == eRender ? (
this has to be eConsole,

https://codereview.chromium.org/187593004/diff/120001/media/audio/win/audio_d...
media/audio/win/audio_device_listener_win.cc:150: flow == eConsole ?
and this

https://codereview.chromium.org/187593004/diff/120001/media/audio/win/audio_d...
media/audio/win/audio_device_listener_win.cc:154: flow == eConsole ?
and this should be eRender, right?

(both have the numeric value of 0, and both enums have the same number of
elements, so it shouldn't make a difference in practice, but that's only due to
luck)

found by clang:

..\..\media\audio\win\audio_device_listener_win.cc(149,12) :  warning(clang):
comparison of two values with different enumeration types ('ERole' (aka
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0002') and
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0001') [-Wenum-compare]
      role == eRender ? (
      ~~~~ ^  ~~~~~~~
..\..\media\audio\win\audio_device_listener_win.cc(150,16) :  warning(clang):
comparison of two values with different enumeration types ('EDataFlow' (aka
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0001') and
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0002') [-Wenum-compare]
          flow == eConsole ?
          ~~~~ ^  ~~~~~~~~
..\..\media\audio\win\audio_device_listener_win.cc(154,16) :  warning(clang):
comparison of two values with different enumeration types ('EDataFlow' (aka
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0001') and
'__MIDL___MIDL_itf_mmdeviceapi_0000_0000_0002') [-Wenum-compare]
          flow == eConsole ?
          ~~~~ ^  ~~~~~~~~
3 warnings generated.