cordova-rtc / cordova-plugin-iosrtc

Cordova iOS plugin exposing the WebRTC W3C API
MIT License
688 stars 340 forks source link

Janus Video Room - video streams get overridden. #432

Closed akilude closed 4 years ago

akilude commented 4 years ago

Expected behavior

When using Janus Video Room, when 2 or more remote users publish and join the room one after the other, the videos display the respective video streams.

Observed behavior

All remote videos render one remote user's stream, the scr of the videos of the others get overridden.

Steps to reproduce the problem

  1. On an iOS device using this plugin, join a janus video room
  2. On another device join the same room
  3. on a third device join the same room.

You will see that both remote videos display the same stream.

Possible Cause

All janus remote streams have the same id "janus" , they are supposed to be unique https://developer.mozilla.org/en-US/docs/Web/API/MediaStream/id This plugin relies on stream id's to identify streams.

Platform information

hthetiot commented 4 years ago

@akilude I think it may be wontfix due spec you explained on https://github.com/meetecho/janus-gateway/issues/1847

It look Janus have a fix already in the pipe to respect StreamID to be unique. Here https://github.com/meetecho/janus-gateway/pull/1459

Currently for the video room plugin at least, remote streams all have the same id "janus". Media Stream ID's are supposed to be unique https://developer.mozilla.org/en-US/docs/Web/API/MediaStream/id

Tell me what you think @akilude , but we really on internal WebRTC StreamID on Unified-Plan a lot, I think it would be a mistake to create a wrapper for this case of Janus only.

Unified-Plan will land on iosRTC soon and is already functional on #407

akilude commented 4 years ago

@hthetiot i just tried the unified branch of janus with the unique ids for streams and am facing some issues as the onremotestream of janus.js is not being called which i think is because they changed their api to track based, will check if it i need to make some changes and get back to you.

Thank you.

akilude commented 4 years ago

Hi @hthetiot , testing just the janus videoroom demo on the janus unified plan branch, janus does indeed provide unique stream ids, but they do this by giving us tracks instead of streams, for example all video tracks for all peers are given id "janus1" , all audio tracks are given ids "janus0".

we create the media stream on the client side

var stream = new MediaStream(); stream.addTrack(audioTrack.clone()) stream.addTrack(videoTrack.clone());

This plugin (iosrtc) has not implemented cloning of tracks so i just add the track as is to the stream and i'm getting the same issue of streams overriding each other, this happens only when using the iosrtc plugin. i don't know enough of the internals of this plugin, but do the tracks of the stream also need to have unique ids in order to track them separately?

PS: Tracks, like streams are supposed to have unique ids https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/id

akilude commented 4 years ago

@hthetiot according to the creators at Janus, the id's of streams, tracks need to be unique only within the same peer connection.

https://groups.google.com/forum/?fromgroups#!topic/meetecho-janus/MgVcVa6KyBs

hthetiot commented 4 years ago

@akilude Ok I will look into that.

menelike commented 4 years ago

@akilude

Can you test if changing

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/4dc28f651efcfb6223e77dd0e6f541c1e4316bc2/src/PluginMediaStream.swift#L20

to

        self.id = rtcMediaStream.streamId + "-" + UUID().uuidString

solves your issue? Though I have absolutely no idea if this breaks other parts of the code.

hthetiot commented 4 years ago

@menelike this is not a good idea, because doing so will append UUID on existing valid UUID, this is a bad workarround not a fix. as it will break WebRTC internal usage of PluginMediaStream.id inside PluginRTCPeerConnection (you wasting my time again and leading people in the wrong directions).

hthetiot commented 4 years ago

@menelike, please stay out of the issues and PR you are not the reporter, you are not helping desptite what you think. Instead this should be the real fix #447

hthetiot commented 4 years ago

@akilude @oscarvadillog can you test this PR and let me know if that fix your Janus stream and track id collision issue (See testing instructions).

akilude commented 4 years ago

thanks @hthetiot , i currently don't have an iphone to test, will test and report back as soon as i get one.

hthetiot commented 4 years ago

@akilude Thank you for update, keep me posted.

hthetiot commented 4 years ago

Note for @akilude should work on emulator with audio send only (video code is ignored on emulator no client side change needed) but will receive videos from janus.

akilude commented 4 years ago

@hthetiot thank you, it's working properly on the emulator, i will get an iphone tomorrow and report back when i test on an actual device.

hthetiot commented 4 years ago

Thank you @akilude your QA is really valuable.

hthetiot commented 4 years ago

merged #447 on master will test master then release 6.0.4

akilude commented 4 years ago

@hthetiot , just an update, It works on the physical iphone as well. Thank you.

oscarvadillog commented 4 years ago

Sorry for the delay. Tested on iPhone 7 and it works properly. Thanks @hthetiot 😄

hthetiot commented 4 years ago

Thank you @oscarvadillog and @akilude for testing it's really helpful so iosRTC is stable.

akilude commented 4 years ago

I'm having issues with streams not going out in janus video room, i have made lots of changes to the code, will revert the changes and test and report back whether the error is in my code or introduced in the new version.

oscarvadillog commented 4 years ago

@akilude it's working properly for me with Janus videoroom (tested with 3 peers)

What exactly is wrong with your code? Let us know when you review it please 👍

akilude commented 4 years ago

It was caused due to me using a canvas as the source for the stream using captureStream, the plugin is working fine, sorry for the confusion.

oscarvadillog commented 4 years ago

Great!