Fix getUserMedia so that failure is reported for invalid audio sources.

Fix getUserMedia so that failure is reported for invalid audio sources.
This changes getUserMedia to wait for initialization of local
audio sources before issuing the completion callback (either success or
failure).
Previously, if an error occurs between attempting to start a local
audio source and the render side OnStreamCreated callback, getUserMedia
would report successful completion with an audio track but no audio
would actually be delivered for that track.

BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2623443002
Cr-Commit-Position: refs/heads/master@{#442323}
Committed: https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbcaabf6d5c3c04

[tommi (sloooow) - chröme, 2017-01-08 17:46:02]

Description was changed from

==========
Fix getUserMedia so that failure is reported for invalid audio sources.
This changes getUserMedia to wait for initialization of local
audio sources before issuing the completion callback (either success or
failure).
Previously, if an error occurs between attempting to start a local
audio source and the render side OnStreamCreated callback, getUserMedia
would report successful completion with an audio track but no audio
would actually be delivered for that track.

BUG=679210
==========

to

==========
Fix getUserMedia so that failure is reported for invalid audio sources.
This changes getUserMedia to wait for initialization of local
audio sources before issuing the completion callback (either success or
failure).
Previously, if an error occurs between attempting to start a local
audio source and the render side OnStreamCreated callback, getUserMedia
would report successful completion with an audio track but no audio
would actually be delivered for that track.

BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[tommi (sloooow) - chröme, 2017-01-08 17:50:51]

tommi@chromium.org changed reviewers:
+ guidou@chromium.org, jochen@chromium.org, xhwang@chromium.org

[tommi (sloooow) - chröme, 2017-01-08 17:50:51]

guidou - main reviewer (everything please)

owners:

xhwang - media/base/audio_capturer_source.h
jochen - blink_test_runner.cc

[tommi (sloooow) - chröme, 2017-01-08 17:51:02]

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

[commit-bot: I haz the power, 2017-01-08 17:51:07]

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-01-08 21:21:34]

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

[commit-bot: I haz the power, 2017-01-08 21:21:35]

Dry run: This issue passed the CQ dry run.

[jochen (gone - plz use gerrit), 2017-01-09 08:31:53]

is it possible to add a layout test or browser test for this?

blink_test_runner.cc lgtm

[tommi (sloooow) - chröme, 2017-01-09 09:08:47]

On 2017/01/09 08:31:53, jochen wrote:
> is it possible to add a layout test or browser test for this?
> 
> blink_test_runner.cc lgtm

I'll take a look. At the moment I think it could get quite a bit hairy with how
things are wired.

[Guido Urdaneta, 2017-01-09 11:08:40]

lgtm % nits

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:499: for (auto it =
pending_local_sources_.begin();
DCHECK that this is running on the correct thread?

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:874: return NULL;
nit: since you're rewriting this to C++11 style, use nullptr here.

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:881: DVLOG(1) << "Source
already exist. Reusing source with id "
nit: exist -> exists.

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:1080: // OnAudioSourceStarted
notification.
Add crbug.com reference

[tommi (sloooow) - chröme, 2017-01-09 11:39:14]

Address comments

[tommi (sloooow) - chröme, 2017-01-09 11:39:30]

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
File content/renderer/media/user_media_client_impl.cc (right):

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:499: for (auto it =
pending_local_sources_.begin();
On 2017/01/09 11:08:40, Guido Urdaneta wrote:
> DCHECK that this is running on the correct thread?

Done.

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:874: return NULL;
On 2017/01/09 11:08:40, Guido Urdaneta wrote:
> nit: since you're rewriting this to C++11 style, use nullptr here.

Done.

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:881: DVLOG(1) << "Source
already exist. Reusing source with id "
On 2017/01/09 11:08:40, Guido Urdaneta wrote:
> nit: exist -> exists.

Done.

https://codereview.chromium.org/2623443002/diff/1/content/renderer/media/user...
content/renderer/media/user_media_client_impl.cc:1080: // OnAudioSourceStarted
notification.
On 2017/01/09 11:08:40, Guido Urdaneta wrote:
> Add http://crbug.com reference

Done.

[tommi (sloooow) - chröme, 2017-01-09 11:39:51]

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

[commit-bot: I haz the power, 2017-01-09 11:39:58]

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-01-09 13:01:56]

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

[commit-bot: I haz the power, 2017-01-09 13:01:57]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-01-09 17:40:36]

rs lgtm

[tommi (sloooow) - chröme, 2017-01-09 20:01:44]

On 2017/01/09 17:40:36, xhwang wrote:
> rs lgtm

Thanks all.

Jochen - I think that I can write up a browser test for this.  I'm continuing
work in this area so I'll include that in upcoming CLs.

[tommi (sloooow) - chröme, 2017-01-09 20:01:52]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2017-01-09 20:01:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/2623443002/#ps20001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-09 20:03:26]

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-01-09 20:18:21]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1483992112503080,
"parent_rev": "94146b3fa89acb43b24182b30258f3fb5bb62047", "commit_rev":
"81cb66acf3f8099e416c7fc5ecbcaabf6d5c3c04"}

[commit-bot: I haz the power, 2017-01-09 20:19:12]

Description was changed from

==========
Fix getUserMedia so that failure is reported for invalid audio sources.
This changes getUserMedia to wait for initialization of local
audio sources before issuing the completion callback (either success or
failure).
Previously, if an error occurs between attempting to start a local
audio source and the render side OnStreamCreated callback, getUserMedia
would report successful completion with an audio track but no audio
would actually be delivered for that track.

BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Fix getUserMedia so that failure is reported for invalid audio sources.
This changes getUserMedia to wait for initialization of local
audio sources before issuing the completion callback (either success or
failure).
Previously, if an error occurs between attempting to start a local
audio source and the render side OnStreamCreated callback, getUserMedia
would report successful completion with an audio track but no audio
would actually be delivered for that track.

BUG=679210
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2623443002
Cr-Commit-Position: refs/heads/master@{#442323}
Committed:
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...
==========

[commit-bot: I haz the power, 2017-01-09 20:19:14]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/81cb66acf3f8099e416c7fc5ecbc...

[dewittj, 2017-01-09 22:23:11]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2626533002/ by dewittj@chromium.org.

The reason for reverting is: Revert due to WebRTC tests failing:

https://uberchromegw.corp.google.com/i/chromium.win/builders/Win7%20Tests%20%...

[4832:364:0109/131325.673:4351226:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
  Actual: false
Expected: true
Failed to execute javascript callAndRenegotiateToVideo({audio: true}, {audio:
true, video:true});.
From javascript: (nothing)
When executing 'callAndRenegotiateToVideo({audio: true}, {audio: true,
video:true});'
c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed

and

[4704:4156:0109/131352.840:4378402:ERROR:browser_test_utils.cc(147)] Cannot
communicate with DOMMessageQueue.
c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(66):
error: Value of: ExecuteScriptAndExtractString(shell(), javascript, &result)
  Actual: false
Expected: true
Failed to execute javascript getUserMediaInIframeAndCloseInSuccessCb({audio:
true});.
From javascript: (nothing)
When executing 'getUserMediaInIframeAndCloseInSuccessCb({audio: true});'
c:\c\win\src\contentrowser\webrtc\webrtc_content_browsertest_base.cc(90):
error: Failed.