Turn audio ducking on by default on Windows again.

Turn audio ducking on by default on Windows again.

This changes the behavior for opting output devices into ducking so that ducking
is only enabled if an input device has already been opened with ducking enabled.

Entering the ducking session is now done for local streams as well, including
HTMLMediaElement audio rendering.  This allows the communication application to
also play back unducked ambient sounds.

On the browser side, when we open the default communication device(s) (input and
output), we now initialize them into a session that is separate from other
audio.  This makes it much easier to observe, control and troubleshoot what is
happening during a comm session.  It also fixes a problem whereby opening up an
output communications device before opening up an input communications device
would cause the output device to get ducked for some reason (and not opted out
of ducking).

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

[tommi (sloooow) - chröme, 2014-07-03 14:51:16]

rebase

[tommi (sloooow) - chröme, 2014-07-04 12:16:13]

Only turn on ducking for output if an input stream already has it enabled.

[tommi (sloooow) - chröme, 2014-07-04 12:24:42]

Rebased on the cl that disables ducking

[tommi (sloooow) - chröme, 2014-07-04 12:46:04]

Update documentation and clean up logging

[tommi (sloooow) - chröme, 2014-07-07 10:57:59]

Rebase, update tests, logging and documentation

[tommi (sloooow) - chröme, 2014-07-07 11:04:59]

Turn audio ducking on by default on Windows again.

This changes the behavior for opting output devices into ducking so that ducking
is only enabled if an input device has already been opened with ducking enabled.

Entering the ducking session is now done for local streams as well, including
HTMLMediaElement audio rendering.  This allows the communication application to
also play back unducked ambient sounds.

On the browser side, when we open the default communication device(s) (input and
output), we now initialize them into a session that is separate from other
audio.  This makes it much easier to observe, control and troubleshoot what is
happening during a comm session.  It also fixes a problem whereby opening up an
output communications device before opening up an input communications device
would cause the output device to get ducked for some reason (and not opted out
of ducking).

[no longer working on chromium, 2014-07-08 10:42:35]

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_dispatcher.cc:420: if
(device_it->device.input.effects && media::AudioParameters::DUCKING)
should it be & instead of && ?

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_dispatcher_unittest.cc:435:
audio_device_info.device.input.effects = media::AudioParameters::DUCKING;
do you mind adding a few lines of code to check ducking is off when the
|effects| is not setup for ducking. 
The existing test already looks great, it is fine if you don't think it is
worthy.

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:105:
constraints->AddOptional(kConstraintEffectMap[i].constraint,
FYI, this HarmonizeConstraintsAndEffects() method is used to modify
|constraints|, which will be passed to libjingle. Since ducking is handled by
chrome, there is no use to modify the value here.
But Setting the value here does not hurt anything. Feel free to keep what it is.

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_renderer.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_renderer.cc:285: base::AutoLock
auto_lock(thread_lock_);
with your move, I believe |source_params_| and |sink_params_| are only accessed
on the main thread, so we can move the thread_lock_ down before
loopback_fifo_.reset(new media::AudioFifo(...

https://codereview.chromium.org/367923004/diff/90001/media/audio/win/core_aud...
File media/audio/win/core_audio_util_win.h (right):

https://codereview.chromium.org/367923004/diff/90001/media/audio/win/core_aud...
media/audio/win/core_audio_util_win.h:205: static HRESULT
SharedModeInitialize(IAudioClient* client,
it seems that this method is not used in production code any more. Consider
removing it? or add a TODO to remove it?

[tommi (sloooow) - chröme, 2014-07-08 12:29:58]

Address comments

[tommi (sloooow) - chröme, 2014-07-08 12:30:56]

All done. Ptal

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_dispatcher.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_dispatcher.cc:420: if
(device_it->device.input.effects && media::AudioParameters::DUCKING)
On 2014/07/08 10:42:35, xians1 wrote:
> should it be & instead of && ?

Ah, good catch!

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
File content/renderer/media/media_stream_dispatcher_unittest.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/m...
content/renderer/media/media_stream_dispatcher_unittest.cc:435:
audio_device_info.device.input.effects = media::AudioParameters::DUCKING;
On 2014/07/08 10:42:35, xians1 wrote:
> do you mind adding a few lines of code to check ducking is off when the
> |effects| is not setup for ducking. 
> The existing test already looks great, it is fine if you don't think it is
> worthy.

I think that's a good idea given my blunder in the other file! Done.

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
File content/renderer/media/webrtc/peer_connection_dependency_factory.cc
(right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
content/renderer/media/webrtc/peer_connection_dependency_factory.cc:105:
constraints->AddOptional(kConstraintEffectMap[i].constraint,
On 2014/07/08 10:42:35, xians1 wrote:
> FYI, this HarmonizeConstraintsAndEffects() method is used to modify
> |constraints|, which will be passed to libjingle. Since ducking is handled by
> chrome, there is no use to modify the value here.
> But Setting the value here does not hurt anything. Feel free to keep what it
is.

This actually is also for Chrome via CreateAudioCapturer (see below).

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_renderer.cc (right):

https://codereview.chromium.org/367923004/diff/90001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_renderer.cc:285: base::AutoLock
auto_lock(thread_lock_);
On 2014/07/08 10:42:35, xians1 wrote:
> with your move, I believe |source_params_| and |sink_params_| are only
accessed
> on the main thread, so we can move the thread_lock_ down before
> loopback_fifo_.reset(new media::AudioFifo(...

Great. thanks for pointing that out.

https://codereview.chromium.org/367923004/diff/90001/media/audio/win/core_aud...
File media/audio/win/core_audio_util_win.h (right):

https://codereview.chromium.org/367923004/diff/90001/media/audio/win/core_aud...
media/audio/win/core_audio_util_win.h:205: static HRESULT
SharedModeInitialize(IAudioClient* client,
On 2014/07/08 10:42:35, xians1 wrote:
> it seems that this method is not used in production code any more. Consider
> removing it? or add a TODO to remove it?

I removed it and renamed the other one to SharedModeInitialize since there's
only one such function.

[no longer working on chromium, 2014-07-08 12:38:55]

Great work!

lgtm

https://codereview.chromium.org/367923004/diff/110001/content/renderer/media/...
File content/renderer/media/webrtc_local_audio_renderer.h (right):

https://codereview.chromium.org/367923004/diff/110001/content/renderer/media/...
content/renderer/media/webrtc_local_audio_renderer.h:138: // Must only be
touched on the main thread.
thanks.

[tommi (sloooow) - chröme, 2014-07-08 12:39:27]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-07-08 12:40:08]

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

[commit-bot: I haz the power, 2014-07-08 14:24:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-07-08 20:21:01]

Change committed as 281814

[DaleCurtis, 2014-07-09 21:12:04]

Late drive-by.  Thanks for fixing this!

https://codereview.chromium.org/367923004/diff/110001/media/audio/win/core_au...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/367923004/diff/110001/media/audio/win/core_au...
media/audio/win/core_audio_util_win.cc:28: const GUID kCommunicationsSessionId =
{
static const?

[tommi (sloooow) - chröme, 2014-07-10 02:57:52]

https://codereview.chromium.org/367923004/diff/110001/media/audio/win/core_au...
File media/audio/win/core_audio_util_win.cc (right):

https://codereview.chromium.org/367923004/diff/110001/media/audio/win/core_au...
media/audio/win/core_audio_util_win.cc:28: const GUID kCommunicationsSessionId =
{
On 2014/07/09 21:12:04, DaleCurtis wrote:
> static const?

I need the constant exported from the object file so I can use it from the input
code as well.