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

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

This is a reland:
* Original CL https://codereview.chromium.org/2623443002/
* The originally reviewed CL is in the first patch.
* A fix for the issue discovered on the "Win7 Tests (dbg)(1)" bot is in the second patch set.

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/2622073003
Cr-Commit-Position: refs/heads/master@{#443660}
Committed: https://chromium.googlesource.com/chromium/src/+/5d0910b9e09533bf6dfe128adcfb882bc2d5e845

[tommi (sloooow) - chröme, 2017-01-11 12:53:37]

Don't delete a request object that's on the stack, from inside a callback from
that request object (plus more DCHECKs)

[tommi (sloooow) - chröme, 2017-01-11 13:06:08]

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

[tommi (sloooow) - chröme, 2017-01-11 13:06:08]

miu - main reviewer.

jochen - owner of blink_test_runner.c (already reviewed), I'm working on a
browser test.
xhwang - owner of audio_capturer_source.h (already reviewed)

miu - adding you explicitly to cc (I'm assuming you have a watch filter)
dewittj - cc-ed to keep me honest


Re the discussion here: https://codereview.chromium.org/2622643006/
I think that changing the EnsureSourceIsStarted contract will result in a lot
more changes than this approach has.  If you think it's worth doing that though,
we can.  If so, I'd like to propose that we land this approach first and allow
it some bake time before the 57 that's not that far into the future. I'm
expecting that the other approach would touch much more code and then should
probably be landed for M58.

[tommi (sloooow) - chröme, 2017-01-11 13:06:19]

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

[commit-bot: I haz the power, 2017-01-11 13:06:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-01-11 13:07:46]

blink test runner lgtm

[commit-bot: I haz the power, 2017-01-11 13:32:46]

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

[commit-bot: I haz the power, 2017-01-11 13:32:46]

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...)

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

Change approach to be more conservative. Instead adapt OnAudioSourceStarted to
the current design and add documentation.

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

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

[commit-bot: I haz the power, 2017-01-11 14:37:28]

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-11 14:48:50]

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

[commit-bot: I haz the power, 2017-01-11 14:48:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[xhwang, 2017-01-11 17:28:10]

media/base/audio_capturer_source.h lgtm

[miu, 2017-01-11 20:51:12]

Comments on PS3 (mostly just moving a few things around to help clarify the code
paths):

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.cc:43: //
OnCaptureStarted() is expected to be called synchronously by this
I don't think it's synchronous. The callback originates from
CastReceiverSessionDelegate::StartAudio(), which is running on the IO thread.
The code here is running on the UI thread.

Taking a step back: Note that ExternalMediaStreamAudioSource is already
constructed with a source. So, we don't have to worry about whether the source
exists. Therefore, it seems this class needs no changes since it should never
result in a device-not-found type of failure.

(Also, IMHO, you should define the
AudioCapturerSource::CaptureCallback::OnCaptureStarted() to be an
empty-by-default virtual method instead of a pure virtual method, to indicate
that handling this event is optional.)

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:505: if (source_extra_data ==
source) {
style nit:

  if (source_extra_data != source)
    continue;
  ...rest of code one indent level less...

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:509: // Since a request object
might get deleted and removed from the
This comment leads me to believe the pointer may become invalid. How do we know
all the pointers we're copying to the temp vector will be valid? Or, is it that
*only* the object being notified could be deleted (i.e., inside our
OnAudioSourceStarted() call here)? If that's the case, let's clarify that in the
comment.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:513:
std::vector<UserMediaRequestInfo*> requests;
Suggest simplifying by just using the vector copy ctor here:

  const std::vector<UserMediaRequestInfo*> notifyees(user_media_requests_);
  for (auto* request : notifyees)
    request->OnAudioSourceStarted(...);

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:517: for (const auto& request :
requests)
nit: The elements are pointers (plain old data types), so just "auto*" is all
that's needed here.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:721:
request->StartAudioTrack((*webkit_tracks)[i], source_initialized);
It's a bit confusing to pass in a bool turning "StartAudioTrack" into a "maybe
start the audio track, if the bool is true, except for Android." So, could we
move that decision here, and make "StartAudioTrack" truly an "always start the
audio track" (like it was before)? Combining that, and the changes to
InitializeAudioSourceObject() proposed in other comments, the body of this loop
becomes:

  for (...) {
    blink::WebMediaStreamSource webkit_source;
    const bool is_already_capturing = InitializeAudioSourceObject(...);
    (*webkit_tracks)[i].initialize(webkit_source);
    if (is_already_capturing)
      request->StartAudioTrack((*webkit_tracks)[i])
  }

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:931: for (const auto& request :
user_media_requests_) {
Does this suffer from the same problem as above, where |user_media_requests_|
could be mutated, or requests destroyed, because we're calling
OnAudioSourceStarted()?

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1093:
CHECK(native_source->is_local_source());
Suggest moving the Android "readiness" state hack to
InitializeAudioSourceObject(), where we determine whether the source is already
capturing. (i.e., have that method return true for Android unconditionally)

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1099: }
So, basically, with all the other comments applied, StartAudioTrack() shouldn't
need any changes.

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.h:209: void
InitializeAudioSourceObject(
Please return a bool instead of make the caller provide a separate output
argument. (Discussed further in .cc file comments.) Also, suggest you use
WARN_UNUSED_RESULT to make sure code acts on this bool.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
File content/renderer/media/webrtc/processed_local_audio_source.cc (right):

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/webrtc/processed_local_audio_source.cc:42:
DCHECK(pc_factory_);
In SetSourceConstraints(), there was a DCHECK(!constraints.isNull()). Should we
add that here, or do we want to make |constraints| optional?

https://codereview.chromium.org/2622073003/diff/40001/media/base/audio_captur...
File media/base/audio_capturer_source.h (right):

https://codereview.chromium.org/2622073003/diff/40001/media/base/audio_captur...
media/base/audio_capturer_source.h:28: virtual void OnCaptureStarted() = 0;
Since not all implementations need this, consider making it an empty method
instead of pure virtual.

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

Address comments

[tommi (sloooow) - chröme, 2017-01-11 22:40:52]

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

[tommi (sloooow) - chröme, 2017-01-11 22:40:59]

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.cc:43: //
OnCaptureStarted() is expected to be called synchronously by this
On 2017/01/11 20:51:11, miu wrote:
> I don't think it's synchronous. The callback originates from
> CastReceiverSessionDelegate::StartAudio(), which is running on the IO thread.
> The code here is running on the UI thread.

OK, I traced through the code and may have missed something.  In order to
convince myself, I added the CHECK() below to see if I'd break any tests,
implemented OnCaptureStarted() below (note that when we get here, was_started_
will always be false).  So I guess this class isn't used in any test that runs
on the try or build bots?

> Taking a step back: Note that ExternalMediaStreamAudioSource is already
> constructed with a source. So, we don't have to worry about whether the source
> exists. Therefore, it seems this class needs no changes since it should never
> result in a device-not-found type of failure.
> 
> (Also, IMHO, you should define the
> AudioCapturerSource::CaptureCallback::OnCaptureStarted() to be an
> empty-by-default virtual method instead of a pure virtual method, to indicate
> that handling this event is optional.)

Done.  Added documentation for things to consider as well and removed the
implementation from this file.

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:505: if (source_extra_data ==
source) {
On 2017/01/11 20:51:11, miu wrote:
> style nit:
> 
>   if (source_extra_data != source)
>     continue;
>   ...rest of code one indent level less...

Done.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:509: // Since a request object
might get deleted and removed from the
On 2017/01/11 20:51:11, miu wrote:
> This comment leads me to believe the pointer may become invalid.

Indeed, the pointer to the request object will become invalid. The call sequence
is like this:

[this is the request object]
OnAudioSourceStarted()
  -> OnTrackStarted
    -> CheckAllTracksStarted
      -> (the request fires ready_callback_.Run())
        [|this| now becomes UserMediaClientImpl]
        -> UserMediaClientImpl::OnCreateNativeTracksCompleted
          -> UserMediaClientImpl::DeleteUserMediaRequestInfo(request)
            -> delete request
      ...now unwind stack with a deleted request object...

I didn't really like this so my first approach was to defer (Post) the deletion
of the request object.  However, that caused several content_unittests to fail
as they expected the request object to be deleted straight after
OnCreateNativeTracksCompleted.  So I decided to add documentation to make this
clear and change the new code to adapt to the existing design.  See difference
between patch set 2 and 3.

> How do we know
> all the pointers we're copying to the temp vector will be valid? Or, is it
that
> *only* the object being notified could be deleted (i.e., inside our
> OnAudioSourceStarted() call here)? If that's the case, let's clarify that in
the
> comment.

Correct, only the request that receives the notification can be deleted.  Added
clarification to comment.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:513:
std::vector<UserMediaRequestInfo*> requests;
On 2017/01/11 20:51:11, miu wrote:
> Suggest simplifying by just using the vector copy ctor here:
> 
>   const std::vector<UserMediaRequestInfo*> notifyees(user_media_requests_);
>   for (auto* request : notifyees)
>     request->OnAudioSourceStarted(...);

Unfortunately user_media_requests_ is a ScopedVector<UserMediaRequestInfo>.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:517: for (const auto& request :
requests)
On 2017/01/11 20:51:11, miu wrote:
> nit: The elements are pointers (plain old data types), so just "auto*" is all
> that's needed here.

Done.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:721:
request->StartAudioTrack((*webkit_tracks)[i], source_initialized);
On 2017/01/11 20:51:11, miu wrote:
> It's a bit confusing to pass in a bool turning "StartAudioTrack" into a "maybe
> start the audio track, if the bool is true, except for Android." So, could we
> move that decision here, and make "StartAudioTrack" truly an "always start the
> audio track" (like it was before)? Combining that, and the changes to
> InitializeAudioSourceObject() proposed in other comments, the body of this
loop
> becomes:
> 
>   for (...) {
>     blink::WebMediaStreamSource webkit_source;
>     const bool is_already_capturing = InitializeAudioSourceObject(...);
>     (*webkit_tracks)[i].initialize(webkit_source);
>     if (is_already_capturing)
>       request->StartAudioTrack((*webkit_tracks)[i])
>   }

The variable name I chose caused the confusion.  I changed the name of the bool
to now be |is_pending|.  The semantics of starting the audio track are unchanged
as far as Start() goes, since it always starts the audio track.

The difference is that for local audio tracks, Start() may not complete the
process.  If |is_pending| is true, then we need to wait for the
OnAudioSourceStarted event (or a failure to be reported).  Aside from that, the
behavior is the same.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:931: for (const auto& request :
user_media_requests_) {
On 2017/01/11 20:51:11, miu wrote:
> Does this suffer from the same problem as above, where |user_media_requests_|
> could be mutated, or requests destroyed, because we're calling
> OnAudioSourceStarted()?

Good catch - it must. I factored out the code that iterates through the requests
into a separate function that I call from here and the other place.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1093:
CHECK(native_source->is_local_source());
On 2017/01/11 20:51:11, miu wrote:
> Suggest moving the Android "readiness" state hack to
> InitializeAudioSourceObject(), where we determine whether the source is
already
> capturing. (i.e., have that method return true for Android unconditionally)

I think I've addressed this in the other comments.

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1099: }
On 2017/01/11 20:51:11, miu wrote:
> So, basically, with all the other comments applied, StartAudioTrack()
shouldn't
> need any changes.

Same here and I think StartAudioTrack still needs to be aware of whether the
Start() operation will be complete synchronously or not.

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.h:209: void
InitializeAudioSourceObject(
On 2017/01/11 20:51:11, miu wrote:
> Please return a bool instead of make the caller provide a separate output
> argument. (Discussed further in .cc file comments.) Also, suggest you use
> WARN_UNUSED_RESULT to make sure code acts on this bool.

Done.  Also changed InitializeVideoSourceObject to follow the same convention.

However, I can't return bool since that would indicate whether or not the
function failed to do its job. I realize this is a misunderstanding because I
chose a poor variable name for |source_initialized|.  Instead I've renamed that
variable to |is_pending| and reversed how it's used.

https://codereview.chromium.org/2622073003/diff/40001/media/base/audio_captur...
File media/base/audio_capturer_source.h (right):

https://codereview.chromium.org/2622073003/diff/40001/media/base/audio_captur...
media/base/audio_capturer_source.h:28: virtual void OnCaptureStarted() = 0;
On 2017/01/11 20:51:11, miu wrote:
> Since not all implementations need this, consider making it an empty method
> instead of pure virtual.

Done.

[commit-bot: I haz the power, 2017-01-11 22:42:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommi (sloooow) - chröme, 2017-01-11 23:46:12]

Revert cast and layout_test changes

[tommi (sloooow) - chröme, 2017-01-11 23:47:25]

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

[commit-bot: I haz the power, 2017-01-11 23:48:18]

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-12 01:16:48]

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

[commit-bot: I haz the power, 2017-01-12 01:16:49]

Dry run: This issue passed the CQ dry run.

[miu, 2017-01-12 21:25:37]

lgtm % two things I think you should try to change:

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/external_media_stream_audio_source.cc:43: //
OnCaptureStarted() is expected to be called synchronously by this
On 2017/01/11 22:40:58, tommi (chrømium) wrote:
> So I guess this class isn't used in any test that runs
> on the try or build bots?

IIRC, the original implementor added browser tests for Chrome's builtin Cast
streaming receiver (+ unit tests for the network+decode part of the impl). I
think they are built into the browser_tests binary.

> Done.  Added documentation for things to consider as well and removed the
> implementation from this file.

SGTM.

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:513:
std::vector<UserMediaRequestInfo*> requests;
On 2017/01/11 22:40:58, tommi (chrømium) wrote:
> On 2017/01/11 20:51:11, miu wrote:
> > Suggest simplifying by just using the vector copy ctor here:
> > 
> >   const std::vector<UserMediaRequestInfo*> notifyees(user_media_requests_);
> >   for (auto* request : notifyees)
> >     request->OnAudioSourceStarted(...);
> 
> Unfortunately user_media_requests_ is a ScopedVector<UserMediaRequestInfo>.

Ah! This might be a good time to change the type (to stop using the deprecated
ScopedVector class). The new way is:

  std::vector<std::unique_ptr<Foo>> user_media_requests_;

Then, implicit casting between std::unique_ptr<Foo> and Foo* might allow you to
use a copy ctor (either vector(const vector<U>&) or maybe the vector(iter begin,
iter end) form?). Simple to try, and if it doesn't work out, at least you've
stopped using the ScopedVector. ;)

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

https://codereview.chromium.org/2622073003/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1097:
CHECK(native_source->is_local_source());
In my last round of comments, I realized this Android workaround felt misplaced,
but I didn't realize why. Basically, this is a layering violation: It's not the
responsibility of UserMediaClientImpl to know how certain source impls will
behave on certain platforms. (Notice that this change would introduce the first
platform-specific switching code "#if defined(OS...)" in this otherwise 100%
platform-agnostic file.)

Suggestion: How about we move this into ProcessedLocalAudioSource and
LocalMediaStreamAudioSource? They could just call their OnCaptureStarted()
methods at the end of EnsureSourceIsStarted() for Android. It would still be a
small layering violation, but only because it is a workaround for broken
functionality just "one hop" away.

I'll leave it up to you: If you're going to address this TODO in the next week
or two, it's not such a big deal. But, if you think it'll take longer than that,
then we really shouldn't leave this here.

[Guido Urdaneta, 2017-01-13 15:20:46]

Description was changed from

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

This is a reland:
* Original CL https://codereview.chromium.org/2623443002/
* The originally reviewed CL is in the first patch.
* A fix for the issue discovered on the "Win7 Tests (dbg)(1)" bot is in the
second patch set.

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 is a reland:
* Original CL https://codereview.chromium.org/2623443002/
* The originally reviewed CL is in the first patch.
* A fix for the issue discovered on the "Win7 Tests (dbg)(1)" bot is in the
second patch set.

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-13 18:28:11]

Switch to std::vector and std::unique_ptr

[tommi (sloooow) - chröme, 2017-01-13 18:28:28]

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

[commit-bot: I haz the power, 2017-01-13 18:29:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[tommi (sloooow) - chröme, 2017-01-13 18:43:53]

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

https://codereview.chromium.org/2622073003/diff/40001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:513:
std::vector<UserMediaRequestInfo*> requests;
On 2017/01/12 21:25:37, miu wrote:
> On 2017/01/11 22:40:58, tommi (chrømium) wrote:
> > On 2017/01/11 20:51:11, miu wrote:
> > > Suggest simplifying by just using the vector copy ctor here:
> > > 
> > >   const std::vector<UserMediaRequestInfo*>
notifyees(user_media_requests_);
> > >   for (auto* request : notifyees)
> > >     request->OnAudioSourceStarted(...);
> > 
> > Unfortunately user_media_requests_ is a ScopedVector<UserMediaRequestInfo>.
> 
> Ah! This might be a good time to change the type (to stop using the deprecated
> ScopedVector class). The new way is:
> 
>   std::vector<std::unique_ptr<Foo>> user_media_requests_;
> 
> Then, implicit casting between std::unique_ptr<Foo> and Foo* might allow you
to
> use a copy ctor (either vector(const vector<U>&) or maybe the vector(iter
begin,
> iter end) form?). Simple to try, and if it doesn't work out, at least you've
> stopped using the ScopedVector. ;)

Type changed.

I gave the conversion a try and the only way I got it to work was by passing the
size to the ctor and then use std::transform() to copy the values from one array
to the other.  Due to the verbosity of that approach, with git cl format, it
takes 5 lines of code instead of the current 4 which include the 'reserve' step.
 So I figured I'd just keep it as is to not cause confusion for the next reader.

Still, at least we stopped using ScopedVector.

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

https://codereview.chromium.org/2622073003/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:1097:
CHECK(native_source->is_local_source());
On 2017/01/12 21:25:37, miu wrote:
> In my last round of comments, I realized this Android workaround felt
misplaced,
> but I didn't realize why. Basically, this is a layering violation: It's not
the
> responsibility of UserMediaClientImpl to know how certain source impls will
> behave on certain platforms. (Notice that this change would introduce the
first
> platform-specific switching code "#if defined(OS...)" in this otherwise 100%
> platform-agnostic file.)
> 
> Suggestion: How about we move this into ProcessedLocalAudioSource and
> LocalMediaStreamAudioSource? They could just call their OnCaptureStarted()
> methods at the end of EnsureSourceIsStarted() for Android. It would still be a
> small layering violation, but only because it is a workaround for broken
> functionality just "one hop" away.
> 
> I'll leave it up to you: If you're going to address this TODO in the next week
> or two, it's not such a big deal. But, if you think it'll take longer than
that,
> then we really shouldn't leave this here.

That's a good idea and I gave it a try.  I ran into some issues though that make
the code more complex.  I can't fire the callback from within
EnsureSourceIsStarted() since it would cause OnTrackStarted to trigger before
ConnectToTrack completes.  I don't want to use PostTask, since then I'd have to
incorporate something like a weak pointer factory in each of the audio sources
and that seems not worth it.  The third option is adding a 'bool callback_made_'
variable to each of the sources, only on android, check it and set it in the
audio callback in order to fire the 'source started' callback the first time we
get audio data.  That's 4 separate files instead of one place here, in addition
to being at a different semantic time than on other platforms.  So I figured it
would be less messy to live with this #if check for the time being instead of
the other hack.

[miu, 2017-01-13 20:02:14]

All sgtm, and patch 6 still lgtm. Thanks for trying out the alternative ideas.

[commit-bot: I haz the power, 2017-01-13 20:41:26]

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

[commit-bot: I haz the power, 2017-01-13 20:41:27]

Dry run: This issue passed the CQ dry run.

[tommi (sloooow) - chröme, 2017-01-13 20:43:29]

The CQ bit was checked by tommi@chromium.org

[tommi (sloooow) - chröme, 2017-01-13 20:43:29]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2622073003/#ps100001
(title: "Switch to std::vector and std::unique_ptr")

[commit-bot: I haz the power, 2017-01-13 20:44:31]

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-13 20:50:13]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1484340209033320,
"parent_rev": "bc1f9f18f5fc73921c06d29a54c48856493b7021", "commit_rev":
"5d0910b9e09533bf6dfe128adcfb882bc2d5e845"}

[commit-bot: I haz the power, 2017-01-13 20:50:45]

Description was changed from

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

This is a reland:
* Original CL https://codereview.chromium.org/2623443002/
* The originally reviewed CL is in the first patch.
* A fix for the issue discovered on the "Win7 Tests (dbg)(1)" bot is in the
second patch set.

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 is a reland:
* Original CL https://codereview.chromium.org/2623443002/
* The originally reviewed CL is in the first patch.
* A fix for the issue discovered on the "Win7 Tests (dbg)(1)" bot is in the
second patch set.

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/2622073003
Cr-Commit-Position: refs/heads/master@{#443660}
Committed:
https://chromium.googlesource.com/chromium/src/+/5d0910b9e09533bf6dfe128adcfb...
==========

[commit-bot: I haz the power, 2017-01-13 20:50:47]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/5d0910b9e09533bf6dfe128adcfb...