Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

This CL adds support for the echoCancellation and deviceId properties to
MediaStreamTrack.getSettings() for audio tracks created with getUserMedia().

The deviceId support for audio tracks was broken due to a placeholder implementation of MediaStreamAudioTrack::GetSettings, which masked the existing correct implementation in WebMediaStreamSource.

deviceId was already supported for video tracks.

BUG=734078
Review-Url: https://codereview.chromium.org/2956063003
Cr-Commit-Position: refs/heads/master@{#484600}
Committed: https://chromium.googlesource.com/chromium/src/+/43855d4106b745fa975172372ece8b0492be5fda

[Guido Urdaneta, 2017-06-27 13:13:41]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-27 13:13:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-27 15:18:44]

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

[commit-bot: I haz the power, 2017-06-27 15:18:45]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Guido Urdaneta, 2017-06-29 08:39:22]

Patchset #1 (id:1) has been deleted

[Guido Urdaneta, 2017-06-29 08:39:29]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 08:39:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-29 08:53:34]

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

[commit-bot: I haz the power, 2017-06-29 08:53:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Guido Urdaneta, 2017-06-29 11:03:58]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 11:04:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Guido Urdaneta, 2017-06-29 12:39:03]

guidou@chromium.org changed reviewers:
+ haraken@chromium.org, mkwst@chromium.org

[Guido Urdaneta, 2017-06-29 12:39:04]

hbos@chromium.org: Please review changes in content/

haraken@chromium.org: Please review changes in WebKit

[Guido Urdaneta, 2017-06-29 12:39:17]

Patchset #1 (id:20001) has been deleted

[Guido Urdaneta, 2017-06-29 12:42:37]

Description was changed from

==========
Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

This CL adds support for the echoCancellation and deviceId properties to
MediaStreamTrack.getSettings() for audio tracks created with getUserMedia().

deviceId was already supported for video tracks.

BUG=734078
==========

to

==========
Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

This CL adds support for the echoCancellation and deviceId properties to
MediaStreamTrack.getSettings() for audio tracks created with getUserMedia().

The deviceId support for audio tracks was broken due to a placeholder
implementation of MediaStreamAudioTrack::GetSettings, which masked the existing
correct implementation in WebMediaStreamSource.

deviceId was already supported for video tracks.

BUG=734078
==========

[commit-bot: I haz the power, 2017-06-29 12:48:21]

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

[commit-bot: I haz the power, 2017-06-29 12:48:22]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-06-29 14:56:31]

guidou@chromium.org changed reviewers:
+ hbos@chromium.org
- mkwst@chromium.org

[Guido Urdaneta, 2017-06-29 14:56:55]

-mkwst@. I intended to have hbos@ as reviewer.

[haraken, 2017-07-02 00:58:35]

LGTM

[hbos_chromium, 2017-07-04 15:26:49]

Looking good!

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:522:
current_audio_input_capabilities_.size();
Hmm.. DCHECK num_pending_audio_input_parameters_ is 0 before this line?

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:540:
current_audio_input_capabilities_[index].parameters = parameters;
Can you [D]CHECK that
current_audio_input_capabilities_[index].parameters.IsValid() after this line?

https://codereview.chromium.org/2956063003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/2956063003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_getusermedia_browsertest.cc:808:
ExecuteJavascriptAndWaitForOk("GetAudioSettingsDefault()");
JS function names should be lowerCamelCase.

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:235: void
SetHardwareEchoCancellationSetting(blink::WebMediaStreamSource* source) {
Should this be generalized and/or renamed, e.g. SurfaceAudioParamsSettings or
SurfaceHardwareEchoCancellationSetting?
"Set" makes it sound like the value is always updated to reflect the hw value,
when in truth it only sets it to true if it has that effect. It could already be
true due to sw. Please add a comment.

Is this logic specific to ECHO_CANCELLER or would the code look the same for any
AudioParameters that are of interest (e.g. set if true, leave old value
otherwise)?

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:987: // CreateAudioTracks(),
after the track has started.
Could you clarify that the reason we don't set it here is because we don't know
that yet.

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1090:
SetHardwareEchoCancellationSetting(&source);
Add a comment saying the source's current value reflects if it has sw and it is
updated if we have hw (but the old value doesn't change otherwise)... if this
isn't clear from function name change.

https://codereview.chromium.org/2956063003/diff/40001/content/test/data/media...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/2956063003/diff/40001/content/test/data/media...
content/test/data/media/getusermedia.html:560: function
GetAudioSettingsDefault() {
JS function names should be lowerCamelCase.

[Guido Urdaneta, 2017-07-05 09:23:37]

hbos@: PTAL

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
File content/browser/renderer_host/media/media_devices_dispatcher_host.cc
(right):

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:522:
current_audio_input_capabilities_.size();
On 2017/07/04 15:26:48, hbos_chromium wrote:
> Hmm.. DCHECK num_pending_audio_input_parameters_ is 0 before this line?

Done.

https://codereview.chromium.org/2956063003/diff/40001/content/browser/rendere...
content/browser/renderer_host/media/media_devices_dispatcher_host.cc:540:
current_audio_input_capabilities_[index].parameters = parameters;
On 2017/07/04 15:26:48, hbos_chromium wrote:
> Can you [D]CHECK that
> current_audio_input_capabilities_[index].parameters.IsValid() after this line?

Done.

https://codereview.chromium.org/2956063003/diff/40001/content/browser/webrtc/...
File content/browser/webrtc/webrtc_getusermedia_browsertest.cc (right):

https://codereview.chromium.org/2956063003/diff/40001/content/browser/webrtc/...
content/browser/webrtc/webrtc_getusermedia_browsertest.cc:808:
ExecuteJavascriptAndWaitForOk("GetAudioSettingsDefault()");
On 2017/07/04 15:26:48, hbos_chromium wrote:
> JS function names should be lowerCamelCase.

Done.

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:235: void
SetHardwareEchoCancellationSetting(blink::WebMediaStreamSource* source) {
On 2017/07/04 15:26:49, hbos_chromium wrote:
> Should this be generalized and/or renamed, e.g. SurfaceAudioParamsSettings or
> SurfaceHardwareEchoCancellationSetting?
> "Set" makes it sound like the value is always updated to reflect the hw value,
> when in truth it only sets it to true if it has that effect. It could already
be
> true due to sw. Please add a comment.
> 
> Is this logic specific to ECHO_CANCELLER or would the code look the same for
any
> AudioParameters that are of interest (e.g. set if true, leave old value
> otherwise)?

This would probably be the place for future settings that depend on parameters.
However, I prefer to wait until we implement those features before renaming to a
more generic name.
Renamed to SurfaceHardwareEchoCancellationSetting for now.

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:987: // CreateAudioTracks(),
after the track has started.
On 2017/07/04 15:26:48, hbos_chromium wrote:
> Could you clarify that the reason we don't set it here is because we don't
know
> that yet.

Done.

https://codereview.chromium.org/2956063003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1090:
SetHardwareEchoCancellationSetting(&source);
On 2017/07/04 15:26:48, hbos_chromium wrote:
> Add a comment saying the source's current value reflects if it has sw and it
is
> updated if we have hw (but the old value doesn't change otherwise)... if this
> isn't clear from function name change.

Done.

https://codereview.chromium.org/2956063003/diff/40001/content/test/data/media...
File content/test/data/media/getusermedia.html (right):

https://codereview.chromium.org/2956063003/diff/40001/content/test/data/media...
content/test/data/media/getusermedia.html:560: function
GetAudioSettingsDefault() {
On 2017/07/04 15:26:49, hbos_chromium wrote:
> JS function names should be lowerCamelCase.

Done.

[Guido Urdaneta, 2017-07-05 09:51:55]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-05 09:52:08]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-05 11:03:44]

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

[commit-bot: I haz the power, 2017-07-05 11:03:45]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[hbos_chromium, 2017-07-06 12:57:09]

lgtm

[Guido Urdaneta, 2017-07-06 13:03:58]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2017-07-06 13:03:59]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2956063003/#ps80001
(title: "fix DCHECK")

[commit-bot: I haz the power, 2017-07-06 13:04:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-06 15:17:59]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1499346238839180,
"parent_rev": "667c0bb37390dc1f4e336151719e30537c1d746d", "commit_rev":
"43855d4106b745fa975172372ece8b0492be5fda"}

[commit-bot: I haz the power, 2017-07-06 15:18:12]

Description was changed from

==========
Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

This CL adds support for the echoCancellation and deviceId properties to
MediaStreamTrack.getSettings() for audio tracks created with getUserMedia().

The deviceId support for audio tracks was broken due to a placeholder
implementation of MediaStreamAudioTrack::GetSettings, which masked the existing
correct implementation in WebMediaStreamSource.

deviceId was already supported for video tracks.

BUG=734078
==========

to

==========
Add support for echoCancellation and deviceId to MediaStreamTrack.getSettings

This CL adds support for the echoCancellation and deviceId properties to
MediaStreamTrack.getSettings() for audio tracks created with getUserMedia().

The deviceId support for audio tracks was broken due to a placeholder
implementation of MediaStreamAudioTrack::GetSettings, which masked the existing
correct implementation in WebMediaStreamSource.

deviceId was already supported for video tracks.

BUG=734078

Review-Url: https://codereview.chromium.org/2956063003
Cr-Commit-Position: refs/heads/master@{#484600}
Committed:
https://chromium.googlesource.com/chromium/src/+/43855d4106b745fa975172372ece...
==========

[commit-bot: I haz the power, 2017-07-06 15:18:14]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/43855d4106b745fa975172372ece...