search

meetecho / janus-gateway

Janus WebRTC Server
https://janus.conf.meetecho.com
GNU General Public License v3.0
8.11k stars 2.46k forks source link

JSEP not an instance of RTCSessionDescription #1422

Closed menelike closed 5 years ago

menelike commented 5 years ago

I think that there is regression on https://github.com/meetecho/janus-gateway/commit/ad61e5f1101934d45c8d30ac9cab2c5ac4f188a7#diff-151c1165a0606ba9f4d6e10c44f2b3e1

Maybe I'm missing something, but I don't understand the reason behind also get rid of some RTC* constructors.

https://github.com/meetecho/janus-gateway/blob/4f7b715dd1a1a2111f1a100aeb4a7833cbe1c69d/html/janus.js#L1675

and

https://github.com/meetecho/janus-gateway/blob/4f7b715dd1a1a2111f1a100aeb4a7833cbe1c69d/html/janus.js#L2232

shouldn't use the plain jsep and as it is not an instance of RTCSessionDescription (config.pc.setRemoteDescription(new RTCSessionDescription(jsep))) .

We discovered this issue in conjunction with cordova-plugin-iosrtc where jsep is explicitly type checked and throws in this case.

Also I'm not sure if

https://github.com/meetecho/janus-gateway/blob/4f7b715dd1a1a2111f1a100aeb4a7833cbe1c69d/html/janus.js#L566

https://github.com/meetecho/janus-gateway/blob/4f7b715dd1a1a2111f1a100aeb4a7833cbe1c69d/html/janus.js#L1689

https://github.com/meetecho/janus-gateway/blob/4f7b715dd1a1a2111f1a100aeb4a7833cbe1c69d/html/janus.js#L2246

should be reverted as well (new RTCIceCandidate(candidate)).

lminiero commented 5 years ago

This is probably a consequence of #1336 but it works as expected for me, pinging @fippo (the author of that PR) to see if he has an opinion on this.

fippo commented 5 years ago

That plugin is way more strict than the specification these days (which moved from RTCSessionDescription to RTCSessionDescriptionInit)... passing {type: ..., sdp: ...} is supposed to work. Does this work when 1) removing the check 2) converting to RTCSessionDescription before processing further ?

lminiero commented 5 years ago

Pinging @ibc and @saghul, who I believe are maintaining it these days, to see if those checks are indeed needed or not. If they are, I guess I can change janus.js to be stricter only if it detects it's used within that plugin (but I'd need a programmatic check I can use in case).

menelike commented 5 years ago

I don't think the in a real-life scenario jsep must not be an instance RTCSessionDescription to get things working (obviously).

Still, https://developer.mozilla.org/en-US/docs/Web/API/RTCSessionDescription#Example uses new RTCSessionDescription() which also fulfills the specs, which in addition makes it easier to use in conjunction with flow type.

lminiero commented 5 years ago

The documentation for the constructor, though, is explicitly marked as deprecated, which is what I think Philipp referred to. The example is there because it shows how the constructor works, but the main point is that it shouldn't be used.

Anyway, I have no problem partly restoring the previous behaviour, but I want to make sure it's really needed: and again, if it is, what I can use as a check in janus.js to force the old behaviour instead of the new, considering I won't force it for all instances.

ibc commented 5 years ago

Unfortunately we are not maintaining iosrtc plugin anynore :(

lminiero commented 5 years ago

Ah, sorry for dragging you into this then! Could you just let me know if you're aware of an easy JavaScript check I can use to detect if the iosrtc plugin is in use? So that I can force the constructor if I know it's there, and do it the new way otherwise.

lminiero commented 5 years ago

I guess I can just check if the window.cordova object exists...

lminiero commented 5 years ago

@menelike can you let me know if changing those setRemoteDescription occurrences to

config.pc.setRemoteDescription(!!window.cordova ? new RTCSessionDescription(jsep) : jsep)

fixes it for you? In case, I'll push. Not touching the candidates stuff, I'll wait for you to tell me if those are breaking changes as well.

menelike commented 5 years ago

@lminiero Thanks for your explanation, I totally missed the deprecation note. Also, I highly appreciate your open mindset on this issue, this is one of the many reasons which make Janus such a great project.

We should try to keep janus.js as clean as possible, edge cases like this will make maintaining more complex as it needs to be. I'm absolutely fine with just closing this issue (we're using janus.js with some additional patches in our case anyway). What do you think?

lminiero commented 5 years ago

If that simple patch works, it's not a problem for me adding it in: otherwise sure, we can close the issue and you can handle the strictness of the environment as part of your patches.

ibc commented 5 years ago

@lminiero you can check if window.cordova.plugins.iosrtc exists (of course, step by step).

menelike commented 5 years ago

I'm closing this issue since the current implementation follows the specs. For anyone who is interested, these are the patches to make Janus work with cordova-plugin-iosrtc

Note: not every feature works, especially switching camera inputs, for this a unpublish => publish cycle has to be done

add_janus_addStream_hooks_to_meet_ios_cordova.patch.txt fixes_https___github_com_meetecho_janus-gateway_issues_1422.patch.txt

I never submitted those patches as a PR because

  1. cordova-plugin-iosrtc is dead
  2. VP8 support will be supported on iOS in Safari (if the rumors are true)
  3. wkwebview hopefully will get support for getUserMedia at some point
hthetiot commented 5 years ago

cordova-plugin-iosrtc is not dead ;)

didlie commented 5 years ago

@hthetiot Maybe you can direct me to the best STUN implementation for a years long project thats working now a the url: didlie.com . This cordova plugin looks great for simple deployment on iphones, but I need the serverside STUN to be compliant with today's browser SDP standards.... forgive me asking, but I've boon working on this for years, and I have spent hundreds of hours playing with different code...and got a little github-code-testing burnout. Maybe you can help. I saw your easyrtc fork, also. Is this RFC 5389 compliant?

hthetiot commented 5 years ago

WTF @didlie you don't know what you talking about easyrtc or iosrtc has nothing to do with STUN it's a signaling and client API while iosrtc is a WebRTC cordova plugin. There is plenty of TURN server like coturn that do their part of the job. Stop writing comment that are non-sense.

didlie commented 5 years ago

"easyrtc or iosrtc has nothing to do with STUN it's a signaling and client API while iosrtc is a WebRTC cordova plugin" --- so sorry for my misunderstanding. I wrote a serverless browser2broswer p2p app that uses SDP from a STUN, and no desire to implement TURN. In my world WebRTC means STUN SDP and serverless P2P.

ibc commented 5 years ago

Two things:

  1. a TURN server (including coturn) also behaves as a regular STUN server.
  2. STUN stuff is completely offtopic here in this issue.
alexamirante commented 5 years ago

Locking this issue (which was already closed 10 months ago). @hthetiot please keep your tone polite in the future: I see you deleted your messages but email notifications still hit everybody's mailboxes.