Add basic support for "googDucking" to getUserMedia on Windows.

Add basic support for "googDucking" to getUserMedia on Windows.
When enabled, this causes Chrome to open the default communication device instead of the "regular" default device (eConsole).

At the moment this only works for the default device (i.e. when getUserMedia is called _without_ a sourceId constraint).  But it is possible to implement this for other audio devices and I'll do so in a followup CL.

While doing this I also fixed a few other things that I ran into:

* Added support for the 'AudioParameters::DUCKING' effect in the WASAPI audio implementation.
* Refactored WASAPIAudioInputStream:
  Removed HardwareSampleRate and HardwareChannelCount since they duplicate work and added GetInputStreamParameters instead.
* Fixed AudioManager::GetInputStream to return format == linear AudioParameters on XP (previously it would always return 'low latency').
* Fixed AudioManager::GetInputStream to return a 10ms default buffer size for the low latency path instead of a fixed buffer size of 2048.
* Fixed a race issue with how CoreAudio::IsSupported was being used.  Also added documentation/warning about the implementation properties of that function.
   - This race has been causing tests to be flaky (for example the InputVolumeTest has been red for weeks on the Windows audio bot: http://build.chromium.org/p/chromium.gpu.fyi/waterfall?builder=Win7%20Audio).
* Removed the InvalidPacketSize test since it doesn't make sense anymore.  The code that InvalidPacketSize has been testing, has been gone for 6 weeks now and the Windows audio bot has been red that entire time.

BUG=340377, 314654, 295751
TEST=Manually run the "WASAPIAudioInputStreamRecordToFile" test while playing out audio in another application (or a youtube clip).  The playing audio should lower in volume while the test runs.

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

[tommi (sloooow) - chröme, 2014-02-05 16:05:58]

+Andrew fyi

[henrika (OOO until Aug 14), 2014-02-05 16:16:12]

Lovely; almost perfect ;-)

Have you tested it with some test page?

[henrika (OOO until Aug 14), 2014-02-05 16:17:05]

..seems like my comments were not included, hang on...

[henrika (OOO until Aug 14), 2014-02-05 16:19:00]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:265: int frames_per_buffer =
sample_rate / 100;
Have you been able to verify this change somehow? Rather big change.

[henrika (OOO until Aug 14), 2014-02-05 16:19:13]

LGTM

[no longer working on chromium, 2014-02-05 16:43:24]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:265: int frames_per_buffer =
sample_rate / 100;
On 2014/02/05 16:19:01, henrika wrote:
> Have you been able to verify this change somehow? Rather big change.


I did a quick check on the code, speech is using their native buffer size
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/sp....

And AFAIK Pepper should also use their own buffer size as well.
So I think such change is fine.

BTW, should we change the native buffer size to 10ms as well on the output side?

Now it is not exactly 10ms.

[henrika (OOO until Aug 14), 2014-02-05 16:46:06]

> BTW, should we change the native buffer size to 10ms as well on the output
side?

No, it is not as simple on the output side. As an example, for 44.1kHz, best
size can be 441 or 448 depending on audio device.

[no longer working on chromium, 2014-02-05 16:51:16]

On 2014/02/05 16:46:06, henrika wrote:
> > BTW, should we change the native buffer size to 10ms as well on the output
> side?
> 
> No, it is not as simple on the output side. As an example, for 44.1kHz, best
> size can be 441 or 448 depending on audio device.

I guess it is related to the clock accuracy. Honestly I have never really
understood this.

For example, when the sample rate is 44.1kHz
For WebRtc, the best buffer is 441;
For WebAudio, the best buffer is 512;
Since any other value means adding a FIFO.

For other clients like audio element, it is not important since they want larger
buffer size.

[henrika (OOO until Aug 14), 2014-02-05 16:58:38]

We use the default native timing interval given to us by the internal audio
engine. It is 10ms for almost all cases but not all.
For 44.1, it can be 10.158730ms on some devices and it is better to use a buffer
size (448 in this case). The total audio buffer is 2*448 to avoid glitches and
we can schedule the callbacks nicely. If you used 441 always, you can run get
audio glitches on some devices.

[no longer working on chromium, 2014-02-05 20:27:59]

On 2014/02/05 16:58:38, henrika wrote:
> We use the default native timing interval given to us by the internal audio
> engine. It is 10ms for almost all cases but not all.
> For 44.1, it can be 10.158730ms on some devices and it is better to use a
buffer
> size (448 in this case). The total audio buffer is 2*448 to avoid glitches and
> we can schedule the callbacks nicely. If you used 441 always, you can run get
> audio glitches on some devices.

I am not a windows guy, so likely my opinion is not well considered.
My concerns are:
# 10ms works for standalone webrtc.
# Previously we had glitches, but probably the glitches were not caused by the
buffer size. And lots of improvement has been done since then, for example, our
sync socket is much robust on delivering data.

Anyway, I am fine with keeping the existing way since the audio works fine
nowadays.

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

Note that 10ms is in fact returned today in most cases also for output. The
examples I give are not common but Dale and I have identified them and verified
that the existing solution is required.

[DaleCurtis, 2014-02-05 22:15:43]

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
File content/common/media/media_stream_options.cc (right):

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
content/common/media/media_stream_options.cc:22: const char
kMediaStreamAudioDucking[] = "googDucking";
Is this a web facing attribute?  I'm not familiar with this code.

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
Won't this blow up on multichannel input systems?  Though do we even support
those?  Should you instead use GuessChannelLayout?

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:137:
core_audio_supported_(CoreAudioUtil::IsSupported()) {
Heh, I just removed this from the startup path last week... Avoiding any
unnecessary LoadLibrary we can there seems like a good idea.

Also there are other users of that call, it'd be better to make IsSupported()
thread safe than have every other caller figure out how to deal with the lack of
thread safety.

It should be possible to just use an atomic set in IsSupported() to fix this.

[no longer working on chromium, 2014-02-06 09:02:55]

On 2014/02/05 21:31:40, henrika wrote:
> Note that 10ms is in fact returned today in most cases also for output. The
> examples I give are not common but Dale and I have identified them and
verified
> that the existing solution is required.

Thanks for the information.

[tommi (sloooow) - chröme, 2014-02-06 11:50:33]

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
File content/common/media/media_stream_options.cc (right):

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
content/common/media/media_stream_options.cc:22: const char
kMediaStreamAudioDucking[] = "googDucking";
On 2014/02/05 22:15:43, DaleCurtis wrote:
> Is this a web facing attribute?  I'm not familiar with this code.

Yes, these are constraints that can be passed to getUserMedia.
Here are more audio constraints as an example, but these constants are different
in the respect that they are currently implemented in libjingle (in the process
of being moved over to Chrome):
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...

The constant just above this new one, kMediaStreamRenderToAssociatedSink, is
another example but it uses the 'chrome' prefix which has been decided is not a
good thing since it introduces a new namespace.

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
On 2014/02/05 22:15:43, DaleCurtis wrote:
> Won't this blow up on multichannel input systems?  Though do we even support
> those?  Should you instead use GuessChannelLayout?

Henrik - I guess this is rather a question for you I think.

This is not new code in this CL but just moved from the implementation from
AudioManagerWin::GetInputStreamParameters to here. (and saved one call to
GetMixFormat() by doing so).

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:265: int frames_per_buffer =
sample_rate / 100;
On 2014/02/05 16:19:01, henrika wrote:
> Have you been able to verify this change somehow? Rather big change.

In practice it's actually not a big change for two reasons:
1. As Shijing found out as well, it turns out nobody currently uses this value! 
Previously it was 2048 which would have caused noticeable latency but all
callers of GetInputStreamParameters currently use their own buffer size and as
has been discussed, it is most of the time exactly this value which is why I
chose it as the default.

2. There is one place that checks this value and compares it against another
value (inside WebRtcAudioCapturer::EnablePeerConnectionMode) that this might
affect.  If it does affect it (which is what I've consulted Shijing on), then
there's a bug in WebRtcAudioCapturer::EnablePeerConnectionMode.  If it does not
affect it, then we have improved WebRtcAudioCapturer::EnablePeerConnectionMode
by avoiding to have to inject an extra "capturer source" for a stream.
However, practially speaking that special case is there only for WebAudio which
uses a non-default buffer size that is much smaller than 10ms, so this is not an
issue.

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:137:
core_audio_supported_(CoreAudioUtil::IsSupported()) {
On 2014/02/05 22:15:43, DaleCurtis wrote:
> Heh, I just removed this from the startup path last week... Avoiding any
> unnecessary LoadLibrary we can there seems like a good idea.
> 
> Also there are other users of that call, it'd be better to make IsSupported()
> thread safe than have every other caller figure out how to deal with the lack
of
> thread safety.
> 
> It should be possible to just use an atomic set in IsSupported() to fix this.

Did you measure this impact?  It's not clear to me if the LoadLibrary calls are
there to actually load the DLLs or just to pin them (in which case they were
already loaded)/check for their existence.

In the former, most common case), the cost of the LoadLibrary call would be
negligible.
In the rare case where these DLLs are actually missing (again, if my assumption
is correct), then this isn't exactly fast, but imho that's fine and it allows us
to fail immediately.

I can implement some sort of an atomic set that may call LoadLibrary and/or
CanCreateDeviceEnumerator more than once while still avoiding the race issue,
but whether or not that gives us better or worse performance is not clear to me.
 My gut feeling right now is that it will be a net-negative for the most common
case.

In any case, this is a regression so we need to fix it one way or another so if
you have data to support that this was impacting the startup path, then that
would be good to know.

[henrika (OOO until Aug 14), 2014-02-06 12:07:55]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
Well, as Tommi states, nothing is changed here. I must admit that I have never
encountered a multi-channel input device and have never seen and crash reports
related trying to open such a device in stereo.

Guess the best answer I can give is: I don't know but would have to test on a
multi-channel input device to provide a correct answer.

Feel free to add a TODO on me here.

[tommi (sloooow) - chröme, 2014-02-06 12:55:33]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
On 2014/02/06 12:07:56, henrika wrote:
> Well, as Tommi states, nothing is changed here. I must admit that I have never
> encountered a multi-channel input device and have never seen and crash reports
> related trying to open such a device in stereo.
> 
> Guess the best answer I can give is: I don't know but would have to test on a
> multi-channel input device to provide a correct answer.
> 
> Feel free to add a TODO on me here.

Multichannel devices that I've tried, all support stereo so I think this is
fine.  Those sorts of devices are actually fairly common (think gamers with 5.1
setups), so we would have heard a lot of screaming by now if it didn't work.

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:137:
core_audio_supported_(CoreAudioUtil::IsSupported()) {
Henrik - maybe you can weigh in here as well with some background?

[henrika (OOO until Aug 14), 2014-02-06 13:11:28]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
Hope I am not repeating anything here but we are talking about multi-channel
input; right?

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:137:
core_audio_supported_(CoreAudioUtil::IsSupported()) {
Link to original issue which triggered the change:
https://code.google.com/p/chromium/issues/detail?id=166397.

[tommi (sloooow) - chröme, 2014-02-06 13:28:33]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_low_la...
media/audio/win/audio_low_latency_input_win.cc:255: channel_layout =
audio_engine_mix_format->nChannels == 1 ?
On 2014/02/06 13:11:28, henrika wrote:
> Hope I am not repeating anything here but we are talking about multi-channel
> input; right?

Ah, sorry, brainfart :)  Yes we are.  I haven't tested with multi-channel input
devices.  I have tested with devices that have multiple inputs but none that has
a single input device with more than two channels.  Even binaural recording only
uses two sources (and even then they are typically consumed as two mono
sources).

Having said that, we might be looking at a hybrid case of this (>2 channel
input) soon so we'll see how this is handled then.  But for now and for the
purposes of this CL I don't think we should be rocking the boat too much.

[DaleCurtis, 2014-02-06 19:09:20]

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
File media/audio/win/audio_manager_win.cc (right):

https://codereview.chromium.org/155863003/diff/1/media/audio/win/audio_manage...
media/audio/win/audio_manager_win.cc:137:
core_audio_supported_(CoreAudioUtil::IsSupported()) {
On 2014/02/06 13:11:28, henrika wrote:
> Link to original issue which triggered the change:
> https://code.google.com/p/chromium/issues/detail?id=166397.

Here's the CL I moved it off in: https://codereview.chromium.org/144613003/
which was triggered by discussion on http://crbug.com/311259 where I noted
upgrading audio drivers resolved some issues for my home machine.  Despite some
unclear interactions with the GPU it seemed a reasonable change to make.

I haven't found any perf benefits on the bots though, so I can't back up my
claims.  It seems reasonable that there are users out there which may have a
slow LoadLibrary call here though.

Apologies for breaking the tests!  For this race condition is it not enough to
have the unittest pump the audio message loop once so that initialization is
allowed to complete?  Note the changes I made to the enumeration tests to do
this in the above CL.

Is there another caller that you're worried about?

[DaleCurtis, 2014-02-06 19:19:58]

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
File content/common/media/media_stream_options.cc (right):

https://codereview.chromium.org/155863003/diff/1/content/common/media/media_s...
content/common/media/media_stream_options.cc:22: const char
kMediaStreamAudioDucking[] = "googDucking";
On 2014/02/06 11:50:34, tommi wrote:
> On 2014/02/05 22:15:43, DaleCurtis wrote:
> > Is this a web facing attribute?  I'm not familiar with this code.
> 
> Yes, these are constraints that can be passed to getUserMedia.
> Here are more audio constraints as an example, but these constants are
different
> in the respect that they are currently implemented in libjingle (in the
process
> of being moved over to Chrome):
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libjin...
> 
> The constant just above this new one, kMediaStreamRenderToAssociatedSink, is
> another example but it uses the 'chrome' prefix which has been decided is not
a
> good thing since it introduces a new namespace.

Do you need to make any FYI / PSA announcements on blink-dev / chromium-dev or
what not for this change then?  See what VP9 did here:

https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/omm_XzZ7mMg/WB...

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

Thanks for making the enumeration changes.  With that I'm pretty confident
Chrome won't have this problem as all the calls to IsSupported happen on the
same thread (I'm going to double check that tomorrow).  I'll just update the
test accordingly if it hasn't already been updated.

Regarding PSA I don't think there's a need for that since the effect of this
change is that Chrome on Windows will use the default communication device for
voice instead of the default 'console' device and not doing so (as we've been
doing), is simply a bug.  Note that in most cases the default console and
communications device maps to the exact same hardware with the only difference
being that ducking is enabled when opened as a communications device and that's
really the bug.

[tommi (sloooow) - chröme, 2014-02-12 10:26:56]

> Thanks for making the enumeration changes.  With that I'm pretty confident
> Chrome won't have this problem as all the calls to IsSupported happen on the
> same thread (I'm going to double check that tomorrow).  I'll just update the
> test accordingly if it hasn't already been updated.
> 

On second thought, I think we should keep this as is right now.  We got reports
that the GPU folks are running into this race this week so the race is still
there.
I feel that since this is a solid fix, we should keep it like this for M34 and
implement thread safety after the cut and allow IsSupported to be called later.

Another thing that needs to be fixed is that the intent to open up a default
device is not being sent from the renderer.  We always send the full device id
so the browser side cannot know if the intent is to open the default device or
that specific device based on the user's selection (that just happens to be
default).  There's a subtle difference there but I'll need to fix that in a
separate cl.
in_reply_to:
ahdzfmNocm9taXVtY29kZXJldmlldy1ocnIdCxIFSXNzdWUY24-pSgwLEgdNZXNzYWdlGKHgFAw
send_mail: 1
subject: Add basic support for "googDucking" to getUserMedia on Windows.

[tommi (sloooow) - chröme, 2014-02-12 10:28:15]

I'm going to clean up the cl and back out a few changes.  Will ping back when
it's ready for review again.

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

PTAL.

The "intent bug" turned out to be a pebkac.  My test profile was set up to
override the default device so that's why I was never getting the intent
correctly sent over to the browser process.  The current patch works as
expected.
Please take a look and let me know if there's something you don't agree with. 
Regarding the thread safety of IsSupported I would like to go with this approach
for now since it fixes a regression that happened a few weeks back (goes back to
the old behavior) and if we want to implement thread safety and address a
performance problem, I'd like to do that in a dedicated cl.

[DaleCurtis, 2014-02-12 19:06:46]

I think we should still move the initialization off the UI thread, but since we
only have anecdotal evidence such a change might be premature.

How about drafting a followup CL to add a UMA stat to AudioManager startup so we
can get some numbers? Essentially AudioManager::Create() should have a
media/base/scoped_histogram_timer.

In the meantime you might consider moving the enumeration_type setting back to
the constructor as well in this CL.

lgtm

[tommi (sloooow) - chröme, 2014-02-12 19:13:18]

On 2014/02/12 19:06:46, DaleCurtis wrote:
> I think we should still move the initialization off the UI thread, but since
we
> only have anecdotal evidence such a change might be premature.
> 
> How about drafting a followup CL to add a UMA stat to AudioManager startup so
we
> can get some numbers? Essentially AudioManager::Create() should have a
> media/base/scoped_histogram_timer.
> 
> In the meantime you might consider moving the enumeration_type setting back to
> the constructor as well in this CL.
> 
> lgtm

Thanks Dale.  Yes, that's a good plan.  I'll update the enumeration_type as
well, upload and cq later tonight.

[tommi (sloooow) - chröme, 2014-02-12 19:53:42]

Initialize enumeration flag in constructor

[tommi (sloooow) - chröme, 2014-02-12 19:56:24]

The CQ bit was checked by tommi@chromium.org

[commit-bot: I haz the power, 2014-02-12 19:59:03]

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

[commit-bot: I haz the power, 2014-02-12 23:38:53]

Change committed as 250844