Feed audio constraints over to WebRtcLocalAudioTrack

Feed audio constraints over to WebRtcLocalAudioTrack
and check them to see if audio processing should be applied.
Previously this check was just being based on a device id being
empty or not, which doesn't do the right thing for WebAudio.

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

[henrika (OOO until Aug 14), 2013-08-26 08:04:55]

Guess it is best if xians starts out with this one. In the mean time, can you
add a few more lines on why the latest code by xians breaks the fix that bemasc
did where he enabled all audio components for a WebAudio source. Not crystal
clear to me yet.

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_local_audio_track.cc:79: if
(!need_audio_processing_ && !capturer->device_id().empty()) {
What is a typical case when we can hit this part?

[tommi (sloooow) - chröme, 2013-08-26 08:18:35]

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_local_audio_track.cc:69:
need_audio_processing_(NeedsAudioProcessing(constraints)) {
Henrik: Take a look at this line, see how it was and how it is now.

Shijing added support for "need_audio_processing_" but we hadn't hooked up the
constraints.  So, a workaround was to set needs_audio_processing_ to true for a
non-empty device id.  WebAudio's device id is always empty, so
needs_audio_processing_ would be set to false.  Now we actually check the
constraints.

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_local_audio_track.cc:79: if
(!need_audio_processing_ && !capturer->device_id().empty()) {
On 2013/08/26 08:04:55, henrika wrote:
> What is a typical case when we can hit this part?

apprtc.appspot.com for instance.  No audio processing related constraints are
passed to the call to getUserMedia, yet previous versions of Chrome have had
audio processing enabled despite that.
This is here to not break the functionality as it currently is on beta and
stable but we need to decide if these constraints should be implicitly enabled
or not and if so, set the appropriate constraints if not already present.

[tommi (sloooow) - chröme, 2013-08-26 08:24:19]

Fix build

[henrika (OOO until Aug 14), 2013-08-26 11:34:48]

LGTM. If possible, please add reference to a demo which now works as intended
given the new patch.

[henrika (OOO until Aug 14), 2013-08-26 11:34:55]

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_local_audio_track.cc:69:
need_audio_processing_(NeedsAudioProcessing(constraints)) {
Thanks for pointing out this key change.

https://codereview.chromium.org/23171026/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_local_audio_track.cc:79: if
(!need_audio_processing_ && !capturer->device_id().empty()) {
OK.

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:387: // right now
they're on the source, so we fetch them from there.
TODO()?

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:19: // This helper function
checks if any audio constraints are set that require
Can we set an audio constraint which does not require audio processing?

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:75: // TODO(xians): Remove
this workaround. It is currently here to maintain
Possibly add link to crbug here.

[no longer working on chromium, 2013-08-26 19:15:41]

I knew r216162 would disable the AEC for webaudio when I landed it, so it is
kind of "intentional". There are multiple reasons for doing so:
# We only have 1 APM, if we use the same APM for multiple data streams at the
same time, it will lead to un-expected behaviors.
# Applying audio processing on the stream from WebAudio is not entirely correct,
for example, the data might not from the microphone, or delay value has been
wrong, ..etc.

I will suggest we have a discussion tomorrow before we land this CL.

SX

[tommi (sloooow) - chröme, 2013-08-27 06:10:10]

Yes let's discuss this today.

Some background: I had a little meeting with the ML team and they filed the
bug which has some information. There's more information on why we need to
do this in another bug that I can forward to you.

I'm aware of all the points you raise (and I agree) but this cl doesn't
touch any of them directly. This cl adds support for checking audio
constraints in the audio track, which we need to do regardless of
WebAudio.  What we had before was a workaround which checked if the device
id was empty.

Just look at the code and you should see that there's nothing controversial
going on. The actual constraints for WebAudio are a another issue and they
have been in there for a while.
On Aug 26, 2013 9:15 PM, <xians@chromium.org> wrote:

>
> I knew r216162 would disable the AEC for webaudio when I landed it, so it
> is
> kind of "intentional". There are multiple reasons for doing so:
> # We only have 1 APM, if we use the same APM for multiple data streams at
> the
> same time, it will lead to un-expected behaviors.
> # Applying audio processing on the stream from WebAudio is not entirely
> correct,
> for example, the data might not from the microphone, or delay value has
> been
> wrong, ..etc.
>
> I will suggest we have a discussion tomorrow before we land this CL.
>
> SX
>
>
https://codereview.chromium.**org/23171026/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2013-08-27 10:58:22]

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:387: // right now
they're on the source, so we fetch them from there.
On 2013/08/26 11:34:55, henrika wrote:
> TODO()?

This isn't really a todo but more of an explanation to the reader about the
status quo.  Once the spec has been changed to have the constraints property on
the track and not the source, this will have to be changed.  If we're lucky line
388 will just replace the word 'source' with 'track' in this case, but there's
ton of code all over the place that will need to be changed, starting with
Blink/WebKit.

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
File content/renderer/media/webrtc_local_audio_track.cc (right):

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:19: // This helper function
checks if any audio constraints are set that require
On 2013/08/26 11:34:55, henrika wrote:
> Can we set an audio constraint which does not require audio processing?

yes it is conceivable that constraints be supported that don't require
processing (thinking out loud, stuff like sample rate).  Explicitly setting
audio constraints to False is another way to specify constraints that do not
require audio processing :)

https://codereview.chromium.org/23171026/diff/22001/content/renderer/media/we...
content/renderer/media/webrtc_local_audio_track.cc:75: // TODO(xians): Remove
this workaround. It is currently here to maintain
On 2013/08/26 11:34:55, henrika wrote:
> Possibly add link to crbug here.

Done.

[tommi (sloooow) - chröme, 2013-08-27 10:58:32]

address comments

[no longer working on chromium, 2013-08-27 11:10:51]

lgtm too. two nits

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.cc (right):

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.cc:76: bool 
RTCMediaConstraints::AddMandatory(const std::string& key,
nit, remove the extra space.

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.h (right):

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.h:35: bool override_if_exists);
nit, fix the indentation.

[tommi (sloooow) - chröme, 2013-08-30 16:18:04]

Rebase and update tests from the one-liner cl

[tommi (sloooow) - chröme, 2013-08-30 16:20:46]

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.cc (right):

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.cc:76: bool 
RTCMediaConstraints::AddMandatory(const std::string& key,
On 2013/08/27 11:10:51, xians1 wrote:
> nit, remove the extra space.

Done.

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
File content/renderer/media/rtc_media_constraints.h (right):

https://codereview.chromium.org/23171026/diff/47001/content/renderer/media/rt...
content/renderer/media/rtc_media_constraints.h:35: bool override_if_exists);
On 2013/08/27 11:10:51, xians1 wrote:
> nit, fix the indentation.

Done.

[tommi (sloooow) - chröme, 2013-08-30 16:20:54]

Address comments

[commit-bot: I haz the power, 2013-08-30 16:21:27]

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

[commit-bot: I haz the power, 2013-08-30 19:01:01]

Change committed as 220617