Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Janus Video Room - video streams get overridden. #432

Closed
akilude opened this issue Nov 1, 2019 · 22 comments
Closed

Janus Video Room - video streams get overridden. #432

akilude opened this issue Nov 1, 2019 · 22 comments

Comments

@akilude
Copy link
Contributor

akilude commented Nov 1, 2019

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

  • Cordova version: 9.0
  • Plugin version: 6.0.1
  • iOS version: 10.2
  • Xcode version: 10.2
@hthetiot
Copy link
Contributor

hthetiot commented Nov 1, 2019

@akilude I think it may be wontfix due spec you explained on meetecho/janus-gateway#1847

It look Janus have a fix already in the pipe to respect StreamID to be unique. Here meetecho/janus-gateway#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
Copy link
Contributor Author

akilude commented Nov 2, 2019

@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
Copy link
Contributor Author

akilude commented Nov 3, 2019

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
Copy link
Contributor Author

akilude commented Nov 5, 2019

@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
Copy link
Contributor

hthetiot commented Nov 5, 2019

@akilude Ok I will look into that.

@menelike

This comment was marked as off-topic.

menelike added a commit to risetechnologies/cordova-plugin-iosrtc that referenced this issue Nov 8, 2019
@hthetiot
Copy link
Contributor

hthetiot commented Nov 18, 2019

@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
Copy link
Contributor

@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
Copy link
Contributor

hthetiot commented Nov 18, 2019

@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
Copy link
Contributor Author

akilude commented Nov 20, 2019

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

@hthetiot
Copy link
Contributor

@akilude Thank you for update, keep me posted.

@hthetiot
Copy link
Contributor

hthetiot commented Nov 20, 2019

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
Copy link
Contributor Author

akilude commented Nov 21, 2019

@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
Copy link
Contributor

hthetiot commented Nov 21, 2019

Thank you @akilude your QA is really valuable.

@hthetiot
Copy link
Contributor

merged #447 on master will test master then release 6.0.4

@akilude
Copy link
Contributor Author

akilude commented Nov 22, 2019

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

@oscarvadillog
Copy link

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

@hthetiot
Copy link
Contributor

hthetiot commented Nov 27, 2019

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

@akilude
Copy link
Contributor Author

akilude commented Nov 29, 2019

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
Copy link

oscarvadillog commented Nov 29, 2019

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

  • Janus 0.7.4
  • cordova-plugin-iosrtc 6.0.5

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

@akilude
Copy link
Contributor Author

akilude commented Nov 29, 2019

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
Copy link

Great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants