search

Temasys / AdapterJS

AdapterJS Javascript Polyfill and Tools for WebRTC - Skylink WebRTC
http://skylink.io/web
Other
432 stars 100 forks source link

Complete support for multi-stream calls and renegotiation #63

Closed ekzobrain closed 1 year ago

ekzobrain commented 9 years ago

Hello.

Currently plugin supports renegotiation in general, but I cannot make use of it because of these problems:

  1. lack of support for PC.removeStream(), see #51
  2. When I add a new stream to PC on the other side - onaddstream event is fired and I succesfilly obtain a new stream object. But when I attach it to rendering (video) element (instead of the old one) - video track from the stream is not displayed (i.e. the first stream had only audio, and the new one - audio + video). Or if the first stream had both audio and video, and the new only video - rendeding element doesn't become black, it just displays whe last frame from previous stream. So general problem is when a rendering element already has an attached stream, and I try to attach another one instead - it does not honor new or removed stream tracks.
  3. When I add a new stream to the connection (also on the other side), renegotiate peers - closing peer connection causes Safari to freeze.
  4. When I remove stream from the connection on the other side - onremovestream event doesn't fire.
johache commented 9 years ago
  1. This is on the roadmap
  2. This should work fine. See this example for instance: https://plugin.temasys.com.sg/demo/samples/web/content/peerconnection/munge-sdp/index.html You attach a new mediaStream whether or not the previous one is still running. Would you have a code semple for us? (or is it the same application as we tried together last time?)
  3. I think this is a known issue.
  4. How do you remove a stream without PC::removeStream implemented? If you are just stopping the MS, it should fire an "onended" event
ekzobrain commented 9 years ago

Hi. This ticket is not abandoned, just the situation changed a little. As you may already know - Firefox implemented support for renegotiation in v.38, byt they did it according the latest spec editors draft, so they did not implement method pc.removeStream(), till it is not in the spec any more. Now pc.addTrack(), pc.removeTrack() and 'track' events are used to send media data via peerconnection, no more streams at all. So i think there migth be no need to implement pc.removeStream() method at all, because no one could use it to implement a cross-browser solution (Firefor will not support it: https://bugzilla.mozilla.org/show_bug.cgi?id=842455). I am currently implementing a new renegotiation solution:

  1. Try pc.addTrack()/removeTrack() methods
  2. If no support - try pc.addStream() and then stream.addTrack()/removeTrack() I will test this approach and then publish testing results here.
ekzobrain commented 9 years ago

Hi. So, I am using the following aproach with the new version of the plugin:

  1. When call is Webkit -> Webkit - use stream.addTrack()/stream.removeTrack() with Plan B SDP.
  2. When call is FF -> FF - use new API pc.addTrack()/pc.removeTrack() with Unified Plan SDP.
  3. When call is Webkit -> Plugin/Plugin -> Plugin - use pc.addStream()/pc.removeStream() with Plan B SDP. So I now have three questions:
  4. There is a bug in the new version of the Plugin (tested under Safari 7.1, OSX Maverics) - camera is not released when calling pc.removeStream() and stream.stop(). Steps to reproduce: a. Start new call with video enabled. b. Get new_stream without video. c. pc.removeStream(old_stream_with_video), pc.addStream(new_stream) d. old_stream_with_video.stop() - here camera should be released, because we removed stream from connection and stoped it, but camera signaling on Mac shows that it is still active. Even pc.close() doesn't release camera, only page refresh.
  5. Do you plan to implement Unified Plan instead of Plan B for multisteam calls?
  6. When do you plan to implement stream.removeTrack()? You now have stream.addTrack() now, but it is useless without removeTrack().
johache commented 9 years ago

Hi,

1 - @magmaerupts can you try reproducing this on Safari, IE and Chrome, and fill a JIRA if necessary ? 2 - Yes, eventually. We are based on libWebRTC and therefore are dependent on the implementation from Google. I think the ticket on their side is there: https://code.google.com/p/chromium/issues/detail?id=465349 Note that I think you should still be able to send two streams in one PC using plan B. (addStream on the PC and re-negociation) 3 - Yes, It is planned for the next iteration. (although I would not say addTrack is useless without removeTrack

ekzobrain commented 9 years ago

Hi, what about camera lock? Is it confirmed and would it be fixed in the next release?

johache commented 9 years ago

Hi, I just tested the scenario a. Start new call with video enabled. b. Get new_stream without video. c. pc.removeStream(old_stream_with_video), pc.addStream(new_stream) d. old_stream_with_video.stop()

The camera was properly released.

I am not sure of what is the purpose of getting a second stream if it have no track though. The scenario could have been simplfied to : a. Start new call with video enabled. c. pc.removeStream(old_stream_with_video), d. old_stream_with_video.stop()

Do you agree on this ?

ekzobrain commented 9 years ago

Streams with or without video of course contain audio, otherwise it really makes no sense. The goal is to disable/enable video in an audio call. Tested under Safari 7.1 and OSX 10.9?

agouaillard commented 9 years ago

actually Dmitry, streams from a screen/window (chrome) or tab (firefox) capturers only have one video track and no audio track, so you cannot assume that if there is a video track there also is an audio track.

On Tue, Aug 25, 2015 at 1:22 AM, Dmitry notifications@github.com wrote:

Streams with or without video of course contain audio, otherwise it really makes no sense. The goal is to disable/enable video in an audio call. Tested under Safari 7.1 on OSX?

— Reply to this email directly or view it on GitHub https://github.com/Temasys/AdapterJS/issues/63#issuecomment-134307006.

Alex. Gouaillard, PhD, PhD, MBA

CTO - Temasys Communications, S'pore / Mountain View

President - CoSMo Software, Cambridge, MA

sg.linkedin.com/agouaillard

-

ekzobrain commented 9 years ago

Yes, I generally agree that I did not provide complete information about that. But you missed that we are talking about camera, so screensharing is not aplicable here as a video stream....

agouaillard commented 9 years ago

fair enough ;)

On Tue, Aug 25, 2015 at 1:59 AM, Dmitry notifications@github.com wrote:

Yes, I generally agree that I did not provide complete information about that. But you missed that we are talking about camera, so screensharing is not aplicable here as a video stream....

— Reply to this email directly or view it on GitHub https://github.com/Temasys/AdapterJS/issues/63#issuecomment-134318345.

Alex. Gouaillard, PhD, PhD, MBA

CTO - Temasys Communications, S'pore / Mountain View

President - CoSMo Software, Cambridge, MA

sg.linkedin.com/agouaillard

-

johache commented 9 years ago

Streams without audio make a lot of sense. Think surveillance cameras for example. I think we also have people sending the audio and video different PCs so that in case of connection breaks or renegociations, the audio stream is kept alive.

Anyways, tested on Safari 8, OSX 10.10, but it will be the same result on Safari 7.1, OSX 10.9. I tested with AJS 0.12.0 and the plugin version 0.8.854.