MediaStreamTrack: Add echo-cancellation and device-id to getSettings

MediaStreamTrack: Add echo-cancellation and device-id to getSettings

This CL lets MediaStreamTrack.getSettings() return the echoCancellation
and deviceId settings.
This involves making a call path so that one can get from a
MediaStreamTrack to the WebMediaStreamSource associated with it, which
means adding a specific API to the ExtraData objects involved.

BUG=682245

[hta - Chromium, 2017-01-19 08:27:13]

Description was changed from

==========
MediaStreamTrack: Add echo-cancellation and device-id to getSettings

This CL lets MediaStreamTrack.getSettings() return the echoCancellation
and deviceId settings.
This involves making a call path so that one can get from a
MediaStreamTrack to the WebMediaStreamSource associated with it, which
means adding a specific API to the ExtraData objects involved.

BUG=682245
==========

to

==========
MediaStreamTrack: Add echo-cancellation and device-id to getSettings

This CL lets MediaStreamTrack.getSettings() return the echoCancellation
and deviceId settings.
This involves making a call path so that one can get from a
MediaStreamTrack to the WebMediaStreamSource associated with it, which
means adding a specific API to the ExtraData objects involved.

BUG=682245
==========

[hta - Chromium, 2017-01-19 08:27:14]

hta@chromium.org changed reviewers:
+ guidou@chromium.org, jochen@chromium.org

[hta - Chromium, 2017-01-19 08:27:45]

I think this is ready for review now.

[hta - Chromium, 2017-01-19 12:52:36]

On 2017/01/19 08:27:45, hta - Chromium wrote:
> I think this is ready for review now.

No, it isn't. Mocks haven't been updated to do the Right Thing.
Working on it.

[Guido Urdaneta, 2017-01-19 15:28:07]

Missed the comment about not being ready for review yet.
A couple of comments below anyway.

https://codereview.chromium.org/2645613005/diff/1/content/renderer/media/webr...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/2645613005/diff/1/content/renderer/media/webr...
content/renderer/media/webrtc/processed_local_audio_source.cc:352: // TODO(hta):
Give the correct value.
nit: add crbug.com reference

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h (right):

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/mediastream/MediaStreamSource.h:56: class
ExtraData {
nit: Maybe rename to SourceData, as it was done with TrackData when similar
changes were made?

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaStreamSource.h (right):

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaStreamSource.h:53: class ExtraData {
Maybe rename to SourceData as it was done with TrackData?

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/public/p...
File third_party/WebKit/public/platform/WebMediaStreamTrack.h (right):

https://codereview.chromium.org/2645613005/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/WebMediaStreamTrack.h:62: bool
m_hasEchoCancellation = false;
Given that this has private fields now, change to class?

[miu, 2017-01-19 20:04:38]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2017-01-19 20:04:39]

not lgtm (general approach)

We shouldn't be adding a second, reverse-direction flow of data by adding
interfaces to what were meant to be opaque ExtraData objects. Instead, how about
adding the settings object to blink::WebMediaStreamSource/Track, and adding a
mutable accessor to it? Then, content/renderer/media code can alter the settings
with something like this (maybe in EnsureSourceIsStarted()?):

  web_ms_source.settings().setEchoCancellation(true);

Also, let's undo these changes to all the *AudioSource/Track code except for
ProcessedLocalAudioSource since echo cancellation is not relevant to the
non-WebRTC media streams. The default settings should already come with the bool
set to false.

[hta - Chromium, 2017-01-19 20:12:08]

On 2017/01/19 20:04:39, miu wrote:
> not lgtm (general approach)
> 
> We shouldn't be adding a second, reverse-direction flow of data by adding
> interfaces to what were meant to be opaque ExtraData objects. Instead, how
about
> adding the settings object to blink::WebMediaStreamSource/Track, and adding a
> mutable accessor to it? Then, content/renderer/media code can alter the
settings
> with something like this (maybe in EnsureSourceIsStarted()?):
> 
>   web_ms_source.settings().setEchoCancellation(true);
> 
> Also, let's undo these changes to all the *AudioSource/Track code except for
> ProcessedLocalAudioSource since echo cancellation is not relevant to the
> non-WebRTC media streams. The default settings should already come with the
bool
> set to false.

I'll take a stab at that approach. You're suggesting that the lower layer
objects inform the blink-layer objects about what the settings are, and that the
getSettings() call terminates inside the blink layer, just reporting back what
has been previously reported up, right?

Will be a new CL. Hopefully with fewer changes.

[miu, 2017-01-19 21:10:26]

On 2017/01/19 20:12:08, hta - Chromium wrote:
> I'll take a stab at that approach. You're suggesting that the lower layer
> objects inform the blink-layer objects about what the settings are, and that
the
> getSettings() call terminates inside the blink layer, just reporting back what
> has been previously reported up, right?

Yep. :) Looks like folks did this for other things
(https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebMe...)
like setReadyState() and constraints() (not sure if this is one is mutable,
though) and add/removeAudioConsumer().

[miu, 2017-02-18 21:35:53]

Closed issue. IIRC, we landed a change w/ a different approach.