Notify the track before source provider goes away.

Notify the track before source provider goes away.

And also I am forced to do some clean up on the existing code to avoid adding the source provider inside the WebRtcLocalAudioTrack.

NOTRY=true

BUG=358960
TEST=content_unittests

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

[no longer working on chromium, 2014-04-02 14:08:21]

Per, could you please review this CL? This patch fixes a crash where the source
provider goes away before the track::Stop.

And most of the change is to remove legacy code.

Thanks,
SX

[perkj_chrome, 2014-04-03 07:45:43]

nice cleanup. 
I am just wondering why the track clean up the sinks? Should the track always
outlive the sinks and the sink can always deregister from the tracks?

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider.cc:85: track_stopped_
= true;
Can' you instead always remove sinks from teh tracks when the sink is dying? Why
do the track cleanup the sinks when its stopped? Why not just stop providing
data to the sinks?

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider.h:34: //
WebRtcLocalAudioSourceProvider provides a bridge between classes:
Is this only used by WebAudio ? Can you add a sentence about that and the name
of WebAudio node that uses this?

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:121:
WebRtcLocalAudioTrack* native_track = static_cast<WebRtcLocalAudioTrack*>(
Can you make sure this is always the case?

[no longer working on chromium, 2014-04-03 09:18:56]

Per, PTAL.

Thanks,
SX

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider.cc (right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider.cc:85: track_stopped_
= true;
On 2014/04/03 07:45:43, perkj wrote:
> Can' you instead always remove sinks from teh tracks when the sink is dying?
Why
> do the track cleanup the sinks when its stopped? Why not just stop providing
> data to the sinks?

As discussed, lets do this in a follow-up to change all the sink to hold a
reference of the blink track object.

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider.h:34: //
WebRtcLocalAudioSourceProvider provides a bridge between classes:
On 2014/04/03 07:45:43, perkj wrote:
> Is this only used by WebAudio ? Can you add a sentence about that and the name
> of WebAudio node that uses this?

Done.

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:121:
WebRtcLocalAudioTrack* native_track = static_cast<WebRtcLocalAudioTrack*>(
On 2014/04/03 07:45:43, perkj wrote:
> Can you make sure this is always the case?

Yes, we set this up in SetUp() method, otherwise it will crash.

[perkj_chrome, 2014-04-03 09:42:05]

lgtm

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:121:
WebRtcLocalAudioTrack* native_track = static_cast<WebRtcLocalAudioTrack*>(
On 2014/04/03 09:18:57, xians1 wrote:
> On 2014/04/03 07:45:43, perkj wrote:
> > Can you make sure this is always the case?
> 
> Yes, we set this up in SetUp() method, otherwise it will crash.
 I think what I meant was, The sourceProvider should be destroyed before the
track. But as discussed, don't care at the moment.

https://codereview.chromium.org/221863003/diff/10001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/221863003/diff/10001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_source_provider.h:40: //
MediaStreamAudioSourceNode will periodically call provideInput() to get the
Nothing to do now but isn't provideInput() a weird name for something that fetch
audio data from a FIFO?

[no longer working on chromium, 2014-04-03 09:48:19]

thanks.

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
File content/renderer/media/webrtc_local_audio_source_provider_unittest.cc
(right):

https://codereview.chromium.org/221863003/diff/1/content/renderer/media/webrt...
content/renderer/media/webrtc_local_audio_source_provider_unittest.cc:121:
WebRtcLocalAudioTrack* native_track = static_cast<WebRtcLocalAudioTrack*>(
On 2014/04/03 09:42:06, perkj wrote:
> On 2014/04/03 09:18:57, xians1 wrote:
> > On 2014/04/03 07:45:43, perkj wrote:
> > > Can you make sure this is always the case?
> > 
> > Yes, we set this up in SetUp() method, otherwise it will crash.
>  I think what I meant was, The sourceProvider should be destroyed before the
> track. But as discussed, don't care at the moment.

Yes, it has been the case that track will always outlive the source provider.
This two test cases verify that we can stop the track before and after the
deletion of the source provider.

https://codereview.chromium.org/221863003/diff/10001/content/renderer/media/w...
File content/renderer/media/webrtc_local_audio_source_provider.h (right):

https://codereview.chromium.org/221863003/diff/10001/content/renderer/media/w...
content/renderer/media/webrtc_local_audio_source_provider.h:40: //
MediaStreamAudioSourceNode will periodically call provideInput() to get the
On 2014/04/03 09:42:06, perkj wrote:
> Nothing to do now but isn't provideInput() a weird name for something that
fetch
> audio data from a FIFO? 

It is an API designed by WebAudio to allow WebAudio to get data from audio
element. And I reuse it for media stream.

I am afraid we can't change it.

[no longer working on chromium, 2014-04-03 09:48:28]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-04-03 09:48:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/221863003/10001

[commit-bot: I haz the power, 2014-04-03 14:59:13]

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

[commit-bot: I haz the power, 2014-04-03 14:59:13]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[no longer working on chromium, 2014-04-03 15:04:57]

The CQ bit was checked by xians@chromium.org

[commit-bot: I haz the power, 2014-04-03 15:05:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/xians@chromium.org/221863003/10001

[commit-bot: I haz the power, 2014-04-03 15:08:21]

Change committed as 261401