Description was changed from ========== Add PeerConnectionObserver#onRemoveTrack to android sdk Implement track modification observer with ...
3 years, 6 months ago
(2017-08-24 21:32:16 UTC)
#1
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
==========
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 ...
3 years, 6 months ago
(2017-08-29 19:48:10 UTC)
#4
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
onAddTrack is implemented with PeerConnectionObserver onRemoveTrack is implemented with MediaStreamObserver PeerConnectionObserver#onAddTrack/onRemoveTrack now accepts RtpReceiver as ...
3 years, 6 months ago
(2017-08-30 21:47:47 UTC)
#5
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
Should I do something else or is there something wrong with the CL? Btw, are ...
3 years, 5 months ago
(2017-09-08 07:43:41 UTC)
#6
On 2017/09/08 07:43:41, korniltsev wrote: > Should I do something else or is there something ...
3 years, 5 months ago
(2017-09-08 20:47:35 UTC)
#7
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
On 2017/09/08 20:47:35, Taylor Brandstetter wrote: > On 2017/09/08 07:43:41, korniltsev wrote: > > Should ...
3 years, 5 months ago
(2017-09-18 17:28:33 UTC)
#8
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
On 2017/09/18 17:28:33, Taylor Brandstetter wrote: > On 2017/09/08 20:47:35, Taylor Brandstetter wrote: > > ...
3 years, 5 months ago
(2017-09-28 04:46:31 UTC)
#9
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/
Issue 3007543002: Add PeerConnectionObserver#onRemoveTrack to android sdk
Created 3 years, 6 months ago by korniltsev
Modified 3 years, 5 months ago
Reviewers: Taylor Brandstetter, sakal
Base URL:
Comments: 6