Add PeerConnectionObserver#onRemoveTrack to android sdk

Add PeerConnectionObserver#onRemoveTrack to android sdk

Implement track modification observer with MediaStreamObserver

BUG=5677

[korniltsev, 2017-08-24 21:32:16]

Description was changed from

==========
Add PeerConnectionObserver#onRemoveTrack to android sdk

Implement track modification observer with MediaStreamObserver

BUG=5677
==========

to

==========
Add PeerConnectionObserver#onRemoveTrack to android sdk

Implement track modification observer with MediaStreamObserver

BUG=5677
==========

[korniltsev, 2017-08-24 21:32:16]

korniltsev.anatoly@gmail.com changed reviewers:
+ deadbeef@webrtc.org, sakal@webrtc.org

[korniltsev, 2017-08-24 21:33:12]

previous attempt: https://codereview.webrtc.org/2990683002/

[Taylor Brandstetter, 2017-08-29 19:48:10]

Note that this doesn't really fix
https://bugs.chromium.org/p/webrtc/issues/detail?id=5677; that bug is about the
Java MediaStream being updated when tracks are added to it/removed from it, but
this CL won't do that; it will just call PeerConnection Observer callbacks.

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (left):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/PeerConnection.java:94: public void
onAddTrack(RtpReceiver receiver, MediaStream[] mediaStreams);
It's not safe to change the signature of this; existing applications may be
depending on it.

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/PeerConnection.java:100: void
onRemoveTrack(MediaStreamTrack track, MediaStream[] mediaStreams);
Should be public. Also, can you give it an empty default implementation so that
this change doesn't break existing Observer subclasses? I believe this is
possible now, since the chromium build infrastructure recently started
supporting Java 8 features.

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/src/jni/pc...
File webrtc/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc (right):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/src/jni/pc...
webrtc/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc:351: // with
MediaStreamObserver - see PeerConnectionObserverJni::OnAddStream
Can you explain the issue more? Can this be fixed, rather than piggybacking off
of OnAddStream? Relying on OnAddStream isn't a future-proof solution, since it
will be possible later to have a track without a stream.

It will also be possible in the future to have a track that's a part of two
streams, which I don't think the code above would support. So that's another
reason to use OnAddTrack and keep the JNI code as minimal as possible.

[korniltsev, 2017-08-30 21:47:47]

onAddTrack is implemented with PeerConnectionObserver
onRemoveTrack is implemented with MediaStreamObserver
PeerConnectionObserver#onAddTrack/onRemoveTrack now accepts RtpReceiver as a
first argument
RtpSender now stores VideoTrack/AudioTrack, not MediaStreamTrack

Please note: there is a potential concurrent modification (MediaStream is
modified on signaling thread
and can be read on different thread from java-land). Should I make
MediaStream#audioTracks/videoTracks
CopyOnWriteArrayList?

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (left):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/PeerConnection.java:94: public void
onAddTrack(RtpReceiver receiver, MediaStream[] mediaStreams);
On 2017/08/29 19:48:09, Taylor Brandstetter wrote:
> It's not safe to change the signature of this; existing applications may be
> depending on it.
reverted it back

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
File webrtc/sdk/android/api/org/webrtc/PeerConnection.java (right):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/api/org/we...
webrtc/sdk/android/api/org/webrtc/PeerConnection.java:100: void
onRemoveTrack(MediaStreamTrack track, MediaStream[] mediaStreams);
On 2017/08/29 19:48:09, Taylor Brandstetter wrote:
> Should be public. Also, can you give it an empty default implementation so
that
> this change doesn't break existing Observer subclasses? I believe this is
> possible now, since the chromium build infrastructure recently started
> supporting Java 8 features.

Done.

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/src/jni/pc...
File webrtc/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc (right):

https://codereview.webrtc.org/3007543002/diff/1/webrtc/sdk/android/src/jni/pc...
webrtc/sdk/android/src/jni/pc/peerconnectionobserver_jni.cc:351: // with
MediaStreamObserver - see PeerConnectionObserverJni::OnAddStream
On 2017/08/29 19:48:09, Taylor Brandstetter wrote:
> Can you explain the issue more? 

It was not actually broken, I was wrong when I wrote it. The problem with this
method is - it did not modify org.webrtc.MediaStream#audioTracks/videoTracks
fields and onAddStream did create org.webrtc.VideoTrack/AudioTrack only
once(when the stream is added).

On 2017/08/29 19:48:09, Taylor Brandstetter wrote:
> Can this be fixed, rather than piggybacking off
> of OnAddStream?
Yes. Rewritten.
onAddTrack is implemented with PeerConnectionObserver
onRemoveTrack is implemented with MediaStreamObserver

[korniltsev, 2017-09-08 07:43:41]

Should I do something else or is there something wrong with the CL?

Btw, are you sure about using default method in interfaces?
https://bugs.chromium.org/p/webrtc/issues/detail?id=8217

[Taylor Brandstetter, 2017-09-08 20:47:35]

On 2017/09/08 07:43:41, korniltsev wrote:
> Should I do something else or is there something wrong with the CL?

Sorry, I've just been on vacation + sick so I'm behind on code reviews, but I
still plan to review this.

[Taylor Brandstetter, 2017-09-18 17:28:33]

On 2017/09/08 20:47:35, Taylor Brandstetter wrote:
> On 2017/09/08 07:43:41, korniltsev wrote:
> > Should I do something else or is there something wrong with the CL?
> 
> Sorry, I've just been on vacation + sick so I'm behind on code reviews, but I
> still plan to review this.

This week I'm at an offsite in Stockholm, so I probably won't be able to respond
until next week. Again, my apologies.

[guptaromi2529, 2017-09-28 04:46:31]

On 2017/09/18 17:28:33, Taylor Brandstetter wrote:
> On 2017/09/08 20:47:35, Taylor Brandstetter wrote:
> > On 2017/09/08 07:43:41, korniltsev wrote:
> > > Should I do something else or is there something wrong with the CL?
> > 
> > Sorry, I've just been on vacation + sick so I'm behind on code reviews, but
I
> > still plan to review this.
> 
> This week I'm at an offsite in Stockholm, so I probably won't be able to
respond
> until next week. Again, my apologies.

http://kodidownloadz.com/kodi-17-4-apk-download/
http://kodidownloadz.com/install-genesis-on-kodi/