Stop media stream source when audio capture error occurs.

Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
Committed: https://crrev.com/9d67562879f4cbddb2c1ddd08225310cea891490
Cr-Commit-Position: refs/heads/master@{#426400}

[xjz, 2016-10-14 00:15:15]

Description was changed from

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
==========

to

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
==========

[xjz, 2016-10-14 00:15:15]

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

[xjz, 2016-10-14 00:16:47]

PTAL

[miu, 2016-10-14 05:38:27]

https://codereview.chromium.org/2416123002/diff/1/content/renderer/media/loca...
File content/renderer/media/local_media_stream_audio_source.cc (right):

https://codereview.chromium.org/2416123002/diff/1/content/renderer/media/loca...
content/renderer/media/local_media_stream_audio_source.cc:109: FROM_HERE,
base::Bind(&MediaStreamSource::StopSource, AsWeakPtr()));
Why do we need the task runner? Isn't OnCaptureError() always called on the main
thread?

Also, I'm not sure it's a good idea to close the source here. Instead, the
onEnded event should be sent via the tracks.

[miu, 2016-10-14 05:45:23]

Just occurred to me: This problem is probably present in the other subclasses of
MediaStreamAudioSource.

[xjz, 2016-10-14 17:32:53]

https://codereview.chromium.org/2416123002/diff/1/content/renderer/media/loca...
File content/renderer/media/local_media_stream_audio_source.cc (right):

https://codereview.chromium.org/2416123002/diff/1/content/renderer/media/loca...
content/renderer/media/local_media_stream_audio_source.cc:109: FROM_HERE,
base::Bind(&MediaStreamSource::StopSource, AsWeakPtr()));
On 2016/10/14 05:38:26, miu_OOO_until_17_Oct wrote:
> Why do we need the task runner? Isn't OnCaptureError() always called on the
main
> thread?
> 
> Also, I'm not sure it's a good idea to close the source here. Instead, the
> onEnded event should be sent via the tracks. 

When tab is closed, this is called on io thread, not main thread. 
When calling MediaStreamSource::StopSource, it will call setReadyState() and the
track will send onEnded event. I did this in the similar way as that is done for
video capture source. Not sure if this is the best way either.

[xjz, 2016-10-14 17:34:44]

On 2016/10/14 05:45:23, miu_OOO_until_17_Oct wrote:
> Just occurred to me: This problem is probably present in the other subclasses
of
> MediaStreamAudioSource.

Yes. The error case is not handled in other subclasses of MediaStreamAudioSource
either.
For tab mirroring, I think we need to stop the source to end the session.
Should it be handled same way for other subclasses?

[miu, 2016-10-18 21:23:34]

On 2016/10/14 17:32:53, xjz wrote:
> When tab is closed, this is called on io thread, not main thread.

I took a closer look, and it seems that *any* thread could call this method. I
see code that runs on all of these calling OnCaptureError(): main thread, IO
thread, audio device thread.

> When calling MediaStreamSource::StopSource, it will call setReadyState() and
the
> track will send onEnded event. I did this in the similar way as that is done
for
> video capture source. Not sure if this is the best way either.

OIC now. SGTM.

On 2016/10/14 17:34:44, xjz wrote:
> On 2016/10/14 05:45:23, miu_OOO_until_17_Oct wrote:
> > Just occurred to me: This problem is probably present in the other
subclasses
> of
> > MediaStreamAudioSource.
> 
> Yes. The error case is not handled in other subclasses of
MediaStreamAudioSource
> either.
> Should it be handled same way for other subclasses?

Yes. This is a hole in the implementation. Therefore, I'd suggest fixing this in
the base class (MediaStreamAudioSource) by implementing OnCaptureError() in the
base class (final virtual override) and remove the subclass overrides.

> For tab mirroring, I think we need to stop the source to end the session.

We should have the same, consistent behavior for all media stream sources.
(BTW--LocalMediaStreamAudioSource is also used by things other than tab
mirroring.)

[xjz, 2016-10-19 19:03:37]

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

[commit-bot: I haz the power, 2016-10-19 19:04:07]

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

[xjz, 2016-10-19 19:04:25]

On 2016/10/18 21:23:34, miu wrote:
> On 2016/10/14 17:32:53, xjz wrote:
> > When tab is closed, this is called on io thread, not main thread.
> 
> I took a closer look, and it seems that *any* thread could call this method. I
> see code that runs on all of these calling OnCaptureError(): main thread, IO
> thread, audio device thread.
> 
> > When calling MediaStreamSource::StopSource, it will call setReadyState() and
> the
> > track will send onEnded event. I did this in the similar way as that is done
> for
> > video capture source. Not sure if this is the best way either.
> 
> OIC now. SGTM.
> 
> On 2016/10/14 17:34:44, xjz wrote:
> > On 2016/10/14 05:45:23, miu_OOO_until_17_Oct wrote:
> > > Just occurred to me: This problem is probably present in the other
> subclasses
> > of
> > > MediaStreamAudioSource.
> > 
> > Yes. The error case is not handled in other subclasses of
> MediaStreamAudioSource
> > either.
> > Should it be handled same way for other subclasses?
> 
> Yes. This is a hole in the implementation. Therefore, I'd suggest fixing this
in
> the base class (MediaStreamAudioSource) by implementing OnCaptureError() in
the
> base class (final virtual override) and remove the subclass overrides.
> 
> > For tab mirroring, I think we need to stop the source to end the session.
> 
> We should have the same, consistent behavior for all media stream sources.
> (BTW--LocalMediaStreamAudioSource is also used by things other than tab
> mirroring.)

Done. Now handle capture error in MediaStreamAudioSource. PTAL.

[commit-bot: I haz the power, 2016-10-19 19:52:15]

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

[commit-bot: I haz the power, 2016-10-19 19:52:16]

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

[xjz, 2016-10-19 21:10:12]

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

[commit-bot: I haz the power, 2016-10-19 21:10:51]

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, 2016-10-19 22:25:26]

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

[commit-bot: I haz the power, 2016-10-19 22:25:27]

Dry run: This issue passed the CQ dry run.

[miu, 2016-10-19 22:43:22]

[ +cc olka@, FYI ]

lgtm % nits:

https://codereview.chromium.org/2416123002/diff/20002/content/renderer/media/...
File content/renderer/media/media_stream_audio_source.h (right):

https://codereview.chromium.org/2416123002/diff/20002/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:130: // Called by subclasses
when capture error occurs.
Please add:

 // Note: This can be called on any thread, and will post a task to the main
thread to stop the source soon.

https://codereview.chromium.org/2416123002/diff/20002/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:131: void
OnAudioCaptureError(const std::string& why);
naming nit (says what it does more accurately): StopSourceOnError()

https://codereview.chromium.org/2416123002/diff/20002/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:148: base::ThreadChecker
thread_checker_;
|thread_checker_| isn't needed anymore. The DCHECKs in the .cc file can become:

  DCHECK(task_runner_->RunsTasksOnCurrentThread());

https://codereview.chromium.org/2416123002/diff/20002/content/renderer/media/...
content/renderer/media/media_stream_audio_source.h:156: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
Please add comment (mostly the same as the one for the |thread_checker_|):

  // Task Runner for main thread. Also used to check that all methods that could
cause object graph or data flow changes are being called on the main thread.

[miu, 2016-10-19 22:56:10]

Oh, FYI: I have a change in the CQ right now
(https://codereview.chromium.org/2425353002/) that will require you to revert
your changes to processed_local_audio_source_unittest.cc: I added a MessageLoop
in my change too. :)

[xjz, 2016-10-19 23:59:36]

Addressed comments. Thanks. :)

https://chromiumcodereview.appspot.com/2416123002/diff/20002/content/renderer...
File content/renderer/media/media_stream_audio_source.h (right):

https://chromiumcodereview.appspot.com/2416123002/diff/20002/content/renderer...
content/renderer/media/media_stream_audio_source.h:130: // Called by subclasses
when capture error occurs.
On 2016/10/19 22:43:22, miu wrote:
> Please add:
> 
>  // Note: This can be called on any thread, and will post a task to the main
> thread to stop the source soon.

Done.

https://chromiumcodereview.appspot.com/2416123002/diff/20002/content/renderer...
content/renderer/media/media_stream_audio_source.h:131: void
OnAudioCaptureError(const std::string& why);
On 2016/10/19 22:43:22, miu wrote:
> naming nit (says what it does more accurately): StopSourceOnError()

Done.

https://chromiumcodereview.appspot.com/2416123002/diff/20002/content/renderer...
content/renderer/media/media_stream_audio_source.h:148: base::ThreadChecker
thread_checker_;
On 2016/10/19 22:43:22, miu wrote:
> |thread_checker_| isn't needed anymore. The DCHECKs in the .cc file can
become:
> 
>   DCHECK(task_runner_->RunsTasksOnCurrentThread());

Done.

https://chromiumcodereview.appspot.com/2416123002/diff/20002/content/renderer...
content/renderer/media/media_stream_audio_source.h:156: const
scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
On 2016/10/19 22:43:22, miu wrote:
> Please add comment (mostly the same as the one for the |thread_checker_|):
> 
>   // Task Runner for main thread. Also used to check that all methods that
could
> cause object graph or data flow changes are being called on the main thread.

Done.

[xjz, 2016-10-19 23:59:50]

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

[commit-bot: I haz the power, 2016-10-20 00:00:30]

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, 2016-10-20 02:04:38]

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

[commit-bot: I haz the power, 2016-10-20 02:04:39]

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

[xjz, 2016-10-20 02:37:46]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-10-20 02:37:46]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/2416123002/#ps70001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-10-20 02:38:12]

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, 2016-10-20 04:17:37]

Description was changed from

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
==========

to

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
==========

[commit-bot: I haz the power, 2016-10-20 04:17:38]

Committed patchset #5 (id:70001)

[commit-bot: I haz the power, 2016-10-21 13:15:23]

Description was changed from

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841
==========

to

==========
Stop media stream source when audio capture error occurs.
This is to ensure that session ends when closing the tab while audio
is mirroring.

BUG=655841

Committed: https://crrev.com/9d67562879f4cbddb2c1ddd08225310cea891490
Cr-Commit-Position: refs/heads/master@{#426400}
==========

[commit-bot: I haz the power, 2016-10-21 13:15:24]

Patchset 5 (id:??) landed as
https://crrev.com/9d67562879f4cbddb2c1ddd08225310cea891490
Cr-Commit-Position: refs/heads/master@{#426400}