meetecho / janus-gateway

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

n.setDirection is not a function. (In 'n.setDirection(\"sendonly\")', 'n.setDirection' is undefined) on Safari 12.1 #1583

Closed Tobiasz-Rumian closed 5 years ago

Tobiasz-Rumian commented 5 years ago

When connecting to Janus from Safari 12.1 the bellow errors appears:

[Error] WebRTC error: – {code: undefined, name: "TypeError", message: "n.setDirection is not a function. (In 'n.setDirection(\"sendonly\")', 'n.setDirection' is undefined)"} error error (videoroomtest.js:419) (anonymous function) (janus.js:2307) promiseReactionJob

[Error] WebRTC error: – "enumerateDevices error" error error (videoroomtest.js:419) (anonymous function) (janus.js:2313) promiseReactionJob

After that Janus is not receiving any streams from Safari (other clients can't see Safari) but Safari is able to see all other clients. This errors only appears on Safari (I didn't test it on older versions). I've tested it on Chrome 73 and Firefox 67 but the errors doesn't appear there.

lminiero commented 5 years ago

Yeah, it looks like it's related to the fix @alienpavlov made in #1567. You don't see it on Chrome and Firefox because it's specific to Safari. Have you checked if reversing that PR fixes it for you?

Tobiasz-Rumian commented 5 years ago

Unfortunately reversing that PR doesn't help. Now there is no setDirection in code but the error is still the same. It looks like some internal function is failing but it makes no sens to me.

lminiero commented 5 years ago

Have you checked if it isn't a caching issue?

Tobiasz-Rumian commented 5 years ago

Yep. Twice.

lminiero commented 5 years ago

Then no idea, I'm not a Mac OS or Safari guy :slightly_smiling_face: I'll try to have a look later with the old machine we have to see if can spot the reason. If you manage to get it working in the meanwhile, pull requests are always welcome of course!

Tobiasz-Rumian commented 5 years ago

I found that the error is in implementation of createOffer (line 2532 config.pc.createOffer()) on Safari. When you pass media constraints to it the implementation fails. With no constraints the connection is working fine but I think they were there for a reason so it's not a proper fix.

alienpavlov commented 5 years ago

If it relative to my PR https://github.com/meetecho/janus-gateway/pull/1567 I think I can improve it from if (audioTransceiver.setDirection) {... to if (typeof audioTransceiver.setDirection === "function") {... it will make sense only when setDirection property exist and it is not a function, but to be honest I think the problem is somewhere in another place - which allows us to get into if statement but the RTCRtpTransceiver is in different state (not initialized or something like that)

I'll try to reproduce and check it later with my Safari Version 12.1 (14607.1.40.1.4)

vahidov commented 5 years ago

@Tobiasz-Rumian whats ver of webrtc-adapter are you using? Guess - 6.4.8. In this version Safari 12.1 has an error n.setDirection is not a function

lminiero commented 5 years ago

@vahidov meaning we should just change the version of the adapter in our demos? I think we still have 6.0.3 there.

vahidov commented 5 years ago

@lminiero yep. But 7+ version of webrtc-adapter didn't support old browsers (Chrome <52, FF < 48 etc)

Tobiasz-Rumian commented 5 years ago

@vahidov 6.0.3 The one from the demo. I've checked with the new version and you were right. The problem is old webrtc-adapter.

lminiero commented 5 years ago

I'm not sure we can just move to 7.x, as I don't know how many people still depend on older browsers. @Tobiasz-Rumian can you let me know if commenting out the offerToReceiveAudio/offerToReceiveVideo lines fixes it for you?

Tobiasz-Rumian commented 5 years ago

@lminiero Yes. After commenting out mediaConstraints["offerToReceiveAudio"] = isAudioRecvEnabled(media); mediaConstraints["offerToReceiveVideo"] = isVideoRecvEnabled(media); it looks like the application is working fine.

lminiero commented 5 years ago

Ok, so then that was the issue. My guess is that we should extend the "Firefox only" branch we have now to any browser that does unified (so Chrome >=72 and Safari). This way, Safari will do the proper thing with transceivers, rather than use those broken constraints. I'll prepare a commit for that.

mtltechtemp commented 5 years ago

Ok, so then that was the issue. My guess is that we should extend the "Firefox only" branch we have now to any browser that does unified (so Chrome >=72 and Safari). This way, Safari will do the proper thing with transceivers, rather than use those broken constraints. I'll prepare a commit for that.

As i said getSupportedConstraints() seems like a better deal. Maybe we can do both ? unified + getSupportedConstraints() to get something robust.

lminiero commented 5 years ago

Integrating getSupportedConstraints() requires much more work than I'm willing to do to fix this, honestly...

mtltechtemp commented 5 years ago

Integrating getSupportedConstraints() requires much more work than I'm willing to do to fix this, honestly...

I could help.

But, you should at least use it to make a condition before entering the block.

Edit: Doc: https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getSupportedConstraints

DaGLiMiOuX commented 5 years ago

@lminiero Yes. After commenting out mediaConstraints["offerToReceiveAudio"] = isAudioRecvEnabled(media); mediaConstraints["offerToReceiveVideo"] = isVideoRecvEnabled(media); it looks like the application is working fine.

It worked. Need a fix in janus.js like this:

function createOffer(handleId, media, callbacks) {
    ...
            } else if (Janus.webRTCAdapter.browserDetails.browser !== "safari") {
        mediaConstraints["offerToReceiveAudio"] = isAudioRecvEnabled(media);
        mediaConstraints["offerToReceiveVideo"] = isVideoRecvEnabled(media);
    }
    ...
}

Or find a way to create a getConstraints function for every browser/device.

lminiero commented 5 years ago

I already fixed this in master a week ago :wink: