Add a 'remote' property to MediaSourceInterface. Also adding an implementation to the relevant sour…

Add a 'remote' property to MediaSourceInterface. Also adding an implementation to the relevant sources we have (audio/video) and an extra check where we're casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video sources).

While doing this I found that some of our tests are broken :) and fixed them.  They were broken because AudioTrack didn't previously do much such as updating its state.

BUG=chromium:569526
Committed: https://crrev.com/6eca7e3c371383020095ba346e1ac70f38a8c0fd
Cr-Commit-Position: refs/heads/master@{#11026}

[tommi, 2015-12-14 13:43:21]

tommi@webrtc.org changed reviewers:
+ perkj@webrtc.org

[perkj_webrtc, 2015-12-14 15:55:03]

Looks like you have a unit test to fix.

https://codereview.webrtc.org/1522903002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1522903002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectionfactory.cc:333: RTC_DCHECK(!source ||
!source->remote());
I think this currently can be a remote source. It would be like cloning a track.
The remote source implementation is not very nice but I think it works.  (This
"feature" is not used in Chrome or anywhere else that I am aware of though)

[tommi, 2015-12-14 16:15:06]

Fix tests

[tommi, 2015-12-14 16:38:25]

https://codereview.webrtc.org/1522903002/diff/1/talk/app/webrtc/peerconnectio...
File talk/app/webrtc/peerconnectionfactory.cc (right):

https://codereview.webrtc.org/1522903002/diff/1/talk/app/webrtc/peerconnectio...
talk/app/webrtc/peerconnectionfactory.cc:333: RTC_DCHECK(!source ||
!source->remote());
On 2015/12/14 15:55:03, perkj1 wrote:
> I think this currently can be a remote source. It would be like cloning a
track.
> The remote source implementation is not very nice but I think it works.  (This
> "feature" is not used in Chrome or anywhere else that I am aware of though) 

I see.  The same is not true for CreateAudioTrack at the moment.  The reason is
that remote audio sources do not have AddSink/RemoveSink functionality in the
AudioSourceInterface.  I guess I could add it and then make the audio part
agnostic to whether the source is remote or local too?  If I do that, we don't
really need a RemoteAudioTrack and I can just move the functionality from there
into AudioTrack.

[tommi, 2015-12-14 16:39:53]

Remove DCHECK and add a check+comment for CreateAudioTrack

[tommi, 2015-12-14 16:57:48]

Relax source check and add AddSink/RemoveSink to audio sources

[tommi, 2015-12-14 23:01:01]

Copy all the implementation from RemoteAudioTrack to AudioTrack and fix tests
for AudioTrack

[tommi, 2015-12-14 23:05:54]

Remove code from RemoteAudioTrack and switch usage over to AudioTrack

[tommi, 2015-12-14 23:11:54]

Format

[tommi, 2015-12-14 23:15:16]

OK, so I decided to do away with RemoteAudioTrack and fully implement AudioTrack
instead.  AddSink/RemoveSink are now on all audio sources like they are for
video sources, although the implementation might do nothing.
While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

[tommi, 2015-12-14 23:17:38]

Description was changed from

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

BUG=chromium:569526
==========

to

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove
RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video
sources).

While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

BUG=chromium:569526
==========

[perkj_webrtc, 2015-12-15 10:15:32]

lgtm
Nice

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/audiotra...
File talk/app/webrtc/audiotrack.cc (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/audiotra...
talk/app/webrtc/audiotrack.cc:100: if (audio_source_->state() ==
MediaSourceInterface::kEnded)
Should we allow the track to change to live again? 
I know the spec does not allow it but I think that we do it with video on
Android when we start the camera and later stop it if the user jumps to another
app. When you jump back to the application, the source is restarted instead of
recreated.

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/mediastr...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/mediastr...
talk/app/webrtc/mediastreaminterface.h:74: // TODO(perkj): Implement sources for
local and remote audio tracks and
Remove this todo please.

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/rtpsende...
File talk/app/webrtc/rtpsender.cc (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.cc:189: // TODO(xians): Remove this static_cast since
we should be able to connect
Should we rephrase this todo to say that we should look into handling
cricket::AudioOptions options from remote audio sources (whatever that means in
the end. With a little luck there will not be any constraints/AudioOptions
needed in a track/source that a PeerConnection must care about.

[tommi, 2015-12-15 11:00:46]

Address comments

[tommi, 2015-12-15 11:00:54]

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/audiotra...
File talk/app/webrtc/audiotrack.cc (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/audiotra...
talk/app/webrtc/audiotrack.cc:100: if (audio_source_->state() ==
MediaSourceInterface::kEnded)
On 2015/12/15 10:15:31, perkj1 wrote:
> Should we allow the track to change to live again? 
> I know the spec does not allow it but I think that we do it with video on
> Android when we start the camera and later stop it if the user jumps to
another
> app. When you jump back to the application, the source is restarted instead of
> recreated.

Interesting. OK, moved the switch() statement in the ctor to here and call
OnChanged from the constructor.  The only state that we can't "recover" a track
from will be kFailed.

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/mediastr...
File talk/app/webrtc/mediastreaminterface.h (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/mediastr...
talk/app/webrtc/mediastreaminterface.h:74: // TODO(perkj): Implement sources for
local and remote audio tracks and
On 2015/12/15 10:15:31, perkj1 wrote:
> Remove this todo please.

Done.

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/rtpsende...
File talk/app/webrtc/rtpsender.cc (right):

https://codereview.webrtc.org/1522903002/diff/120001/talk/app/webrtc/rtpsende...
talk/app/webrtc/rtpsender.cc:189: // TODO(xians): Remove this static_cast since
we should be able to connect
On 2015/12/15 10:15:31, perkj1 wrote:
> Should we rephrase this todo to say that we should look into handling
> cricket::AudioOptions options from remote audio sources (whatever that means
in
> the end. With a little luck there will not be any constraints/AudioOptions
> needed in a track/source that a PeerConnection must care about.

I'd like to keep the TODO untouched for now so that it's easy to get to the CL
that added it from the blamelist and see the context.  Right now I think that
this isn't needed at all and is from a time when the APM was inside
PeerConnection and not on a local track.  With a little luck I'll delete this in
the next few days.

[tommi, 2015-12-15 11:01:12]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2015-12-15 11:01:12]

The patchset sent to the CQ was uploaded after l-g-t-m from perkj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1522903002/#ps140001 (title:
"Address comments")

[commit-bot: I haz the power, 2015-12-15 11:01:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1522903002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1522903002/140001

[commit-bot: I haz the power, 2015-12-15 12:27:14]

Description was changed from

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove
RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video
sources).

While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

BUG=chromium:569526
==========

to

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove
RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video
sources).

While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

BUG=chromium:569526
==========

[commit-bot: I haz the power, 2015-12-15 12:27:15]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-15 12:27:25]

Description was changed from

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove
RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video
sources).

While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

BUG=chromium:569526
==========

to

==========
Add a 'remote' property to MediaSourceInterface. Also adding an implementation
to the relevant sources we have (audio/video) and an extra check where we're
casting a source into a local audio source :(

Additionally:
* Moving all implementation inside RemoteAudioTrack into AudioTrack and remove
RemoteAudioTrack.
* AddSink/RemoveSink are now on all audio sources (like they are for video
sources).

While doing this I found that some of our tests are broken :) and fixed them. 
They were broken because AudioTrack didn't previously do much such as updating
its state.

BUG=chromium:569526

Committed: https://crrev.com/6eca7e3c371383020095ba346e1ac70f38a8c0fd
Cr-Commit-Position: refs/heads/master@{#11026}
==========

[commit-bot: I haz the power, 2015-12-15 12:27:26]

Patchset 8 (id:??) landed as
https://crrev.com/6eca7e3c371383020095ba346e1ac70f38a8c0fd
Cr-Commit-Position: refs/heads/master@{#11026}