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

Incompatibility with Janus WebRTC gateway when using WebRTC-adapter >= 7.6.0 #505

Closed
DaveLomber opened this issue May 8, 2020 · 38 comments

Comments

@DaveLomber
Copy link

DaveLomber commented May 8, 2020

Expected behavior

Using Janus Video Room plugin, it should be possible to have a call between Web browser and iOS Cordova app.

Observerd behavior

  • Web browser can hear and see a stream from iOS Cordova app
  • But iOS Cordova app does not receive a remote stream, hence can't see Web app

According to janus.js implementation, it implements a 'ontrack' callback of RTCPeerConnection where it obtains a remote stream and passes it to app via 'onremotestream' callback.

https://github.com/meetecho/janus-gateway/blob/master/html/janus.js#L1772

But, cordova-plugin-iosrtc does not implement the 'ontrack' callback - instead it provides a legacy one - 'onaddstream'

https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/js/RTCPeerConnection.js#L97

hence the incompatibility issue arises

As for a solution - the cordova-plugin-iosrtc plugin should support both 'ontrack' property https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/ontrack and 'track' event https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/track_event

Steps to reproduce the problem

To run VideoRoom demo app on Web and Cordova https://github.com/meetecho/janus-gateway/blob/master/html/videoroomtest.html

Platform information

  • Cordova version:
    9, 8.1.2

  • Plugin version:
    6.0.11

  • iOS version:
    13.3.1

  • Xcode version:
    11.2.1

@ekzotech
Copy link

ekzotech commented May 8, 2020

I had an issue, but remote audio works fine from the start. The only event that I get is 'addtrack' and it doesn't have stream.

@hthetiot hthetiot added this to the 6.0.x milestone May 8, 2020
@hthetiot hthetiot self-assigned this May 8, 2020
@hthetiot
Copy link
Contributor

hthetiot commented May 8, 2020

Notice that it worked before.
It seems that the videocall is working properly on both sides using latest janus.js, adapter 6.4.0 and task/libwebrtc-update-m69-unified-plan branch. Well done @hthetiot ! <3
See #408 (comment)

Just saying...

@hthetiot
Copy link
Contributor

hthetiot commented May 8, 2020

Regression on Janus or iosrtc or non proper usage or webrtc-adater:

@DaveLomber can you confirm you do include webrtc-adapter ?
Also do you use plan-b or unified-plan ?

@hthetiot
Copy link
Contributor

hthetiot commented May 8, 2020

@oscarvadillog @akilude Can you confirm if it works with Janus for you guys?

Every 2 release we get a new issue with Janus, I'm starting to think that this one is not an "incompatibility" but more a issue with last Janus or Adapter not used.

@hthetiot
Copy link
Contributor

hthetiot commented May 8, 2020

@DaveLomber please avoid "Plugin version:
latest" that not good reporting, if we ask version there is a reason....

@DaveLomber
Copy link
Author

@hthetiot sorry, I have updated the plugin version. We tried with 6.0.11

Re plan-b or unified-plan - I'm not sure we pick anything specific. I would say it works in a default mode, which is 'unified-plan' for Chrome and something default for iOS according to janus.js https://github.com/meetecho/janus-gateway/blob/master/html/janus.js#L1708

For the adapter - we use :

<script type="text/javascript" src="https://webrtc.github.io/adapter/adapter-latest.js"></script>
From what I understand - an adapter should proxy 'onaddstream' to 'ontrack' in environments where 'ontrack' is not supported (Cordova iOS plugin), but obviously it does not happen

I'm investigation for more.

@DaveLomber
Copy link
Author

DaveLomber commented May 9, 2020

I have just checked the adapter code https://webrtchacks.github.io/adapter/adapter-latest.js

  • adapter.browserDetails.browser reports it's Safari browser
  • there is no shimOnTrack defined for Safari cause Safari supports it from the beginning. The shim is only for Chrome and FF.

So I think it explains it - the adapter treats Safari as 'ontrack' compatible and does not add any shim here. But, in fact - iOS Cordova plugin does not have the 'ontrack' implemented.

@oscarvadillog
Copy link

Please provide the version of Janus you are using it @DaveLomber

@DaveLomber
Copy link
Author

We use 0.9.1 gateway version

@DaveLomber
Copy link
Author

Just to prove my above statement about a shim

I guess I made it working well now and the 'onremotestream' callback is firing now

What I did - https://webrtchacks.github.io/adapter/adapter-latest.js I have applied the shimOnTrack function body:

Object.defineProperty(window.RTCPeerConnection.prototype, 'ontrack', {
      get: function get() {
        return this._ontrack;
      },
      set: function set(f) {
        if (this._ontrack) {
          this.removeEventListener('track', this._ontrack);
        }
        this.addEventListener('track', this._ontrack = f);
      },

      enumerable: true,
      configurable: true
    });
    var origSetRemoteDescription = window.RTCPeerConnection.prototype.setRemoteDescription;
    window.RTCPeerConnection.prototype.setRemoteDescription = function setRemoteDescription() {
      var _this = this;

      if (!this._ontrackpoly) {
        this._ontrackpoly = function (e) {
          // onaddstream does not fire when a track is added to an existing
          // stream. But stream.onaddtrack is implemented so we use that.
          e.stream.addEventListener('addtrack', function (te) {
            var receiver = void 0;
            if (window.RTCPeerConnection.prototype.getReceivers) {
              receiver = _this.getReceivers().find(function (r) {
                return r.track && r.track.id === te.track.id;
              });
            } else {
              receiver = { track: te.track };
            }

            var event = new Event('track');
            event.track = te.track;
            event.receiver = receiver;
            event.transceiver = { receiver: receiver };
            event.streams = [e.stream];
            _this.dispatchEvent(event);
          });
          e.stream.getTracks().forEach(function (track) {
            var receiver = void 0;
            if (window.RTCPeerConnection.prototype.getReceivers) {
              receiver = _this.getReceivers().find(function (r) {
                return r.track && r.track.id === track.id;
              });
            } else {
              receiver = { track: track };
            }
            var event = new Event('track');
            event.track = track;
            event.receiver = receiver;
            event.transceiver = { receiver: receiver };
            event.streams = [e.stream];
            _this.dispatchEvent(event);
          });
        };
        this.addEventListener('addstream', this._ontrackpoly);
      }
      return origSetRemoteDescription.apply(this, arguments);
    };

@hthetiot
Copy link
Contributor

Thank you @DaveLomber

I will see what I can do to make it working out of the box, thank you for investigating that help a lot.

@hthetiot
Copy link
Contributor

@DaveLomber feel free to make a PR to add ConnectyCube to https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/master/WHO_USES_IT.md

@ekzotech
Copy link

@DaveLomber do you get working video? If so, could you provide more information, please? I'm stuck on video with Janus, and I didn't get how you fix this. Thank you!

@akilude
Copy link
Contributor

akilude commented May 13, 2020

@hthetiot sorry didn't see your message, with my old version of my app with Janus 7.4 and iosrtc 6.0.7 it works, will get back when i try the latest versions.

@akilude
Copy link
Contributor

akilude commented May 13, 2020

If my memory is correct, i also had a similar issue when using unified plan with this deprecated branch of janus meetecho/janus-gateway#1459 , it started working when i added the onaddstream to janus.js

@DaveLomber
Copy link
Author

DaveLomber commented May 13, 2020

@akilude exactly. Adding onaddstream also fixes an issue. But what I want is to omit any changes in janus.js and to use an origin version.

@DaveLomber
Copy link
Author

DaveLomber commented May 13, 2020

@ekzotech Here is what I ended up with as a working solution (see applyShimOnTrack):

const loadScript = path => {
  let $script = document.createElement("script");

  $script.type = "text/javascript";
  $script.async = false;
  $script.src = path;
  document.body.appendChild($script);
};

const onDeviceReady = () => {
  if (window.device.platform === "Android") {
    const { permissions } = cordova.plugins;

    permissions.requestPermissions([permissions.CAMERA, permissions.RECORD_AUDIO, permissions.MODIFY_AUDIO_SETTINGS]);
  }

  if (window.device.platform === "iOS") {
    const { iosrtc } = cordova.plugins;

    // Connect 'iosrtc' plugin, only for iOS devices
    iosrtc.registerGlobals();
    // Use speaker audio output
    iosrtc.selectAudioOutput("speaker");
    // Request audio and/or camera permission if not requested yet
    iosrtc.requestPermission(true, true, function(permissionApproved) {
      console.log("requestPermission status: ", permissionApproved ? "Approved" : "Rejected");
    });
    // Refresh video element
    window.addEventListener("orientationchange", () => iosrtc.refreshVideos());
    window.addEventListener("scroll", () => iosrtc.refreshVideos());

    applyShimOnTrack();
  }

  loadScript("main.js");
};

// more info re why we need this
// https://github.com/cordova-rtc/cordova-plugin-iosrtc/issues/505
const applyShimOnTrack = () => {
  console.log("applyShimOnTrack");

  Object.defineProperty(window.RTCPeerConnection.prototype, 'ontrack', {
    get: function get() {
      return this._ontrack;
    },
    set: function set(f) {
      if (this._ontrack) {
        this.removeEventListener('track', this._ontrack);
      }
      this.addEventListener('track', this._ontrack = f);
    },

    enumerable: true,
    configurable: true
  });
  var origSetRemoteDescription = window.RTCPeerConnection.prototype.setRemoteDescription;
  window.RTCPeerConnection.prototype.setRemoteDescription = function setRemoteDescription() {
    var _this = this;

    if (!this._ontrackpoly) {
      this._ontrackpoly = function (e) {
        // onaddstream does not fire when a track is added to an existing
        // stream. But stream.onaddtrack is implemented so we use that.
        e.stream.addEventListener('addtrack', function (te) {
          var receiver = void 0;
          if (window.RTCPeerConnection.prototype.getReceivers) {
            receiver = _this.getReceivers().find(function (r) {
              return r.track && r.track.id === te.track.id;
            });
          } else {
            receiver = { track: te.track };
          }

          var event = new Event('track');
          event.track = te.track;
          event.receiver = receiver;
          event.transceiver = { receiver: receiver };
          event.streams = [e.stream];
          _this.dispatchEvent(event);
        });
        e.stream.getTracks().forEach(function (track) {
          var receiver = void 0;
          if (window.RTCPeerConnection.prototype.getReceivers) {
            receiver = _this.getReceivers().find(function (r) {
              return r.track && r.track.id === track.id;
            });
          } else {
            receiver = { track: track };
          }
          var event = new Event('track');
          event.track = track;
          event.receiver = receiver;
          event.transceiver = { receiver: receiver };
          event.streams = [e.stream];
          _this.dispatchEvent(event);
        });
      };
      this.addEventListener('addstream', this._ontrackpoly);
    }
    return origSetRemoteDescription.apply(this, arguments);
  };
}

document.addEventListener("deviceready", onDeviceReady, false);

taken from here https://github.com/ConnectyCube/connectycube-cordova-samples/blob/master/sample-videochat-conf-cordova/www/index.js#L32

@hthetiot
Copy link
Contributor

I will try to update iosRTC to shim ontrack properly without the need to import the shim manualy.
Keep you posted. Thank all for your collaboration much apreciated.

@hthetiot hthetiot modified the milestones: 6.0.x, 6.0.12 May 13, 2020
@hthetiot
Copy link
Contributor

iOSRTC WebRTC swift bridge already implement addtrack in fact, see here:

What is missing is the event support for addtrack setter on iosRTC PeerConnection.

I somewhat beleive that peerConnection.addEventListener('addtrack', function() {}); should actually works, but people may still use peerConnection.onaddtrack. I will confirm and address the issue.

@DaveLomber
Copy link
Author

@hthetiot this WebRTC ontrack stuff is a bit confusing..

In fact it should be RTCPeerConnection.ontrack and not a RTCPeerConnection.onaddtrack

and also it should be a "track" event and not a "addtrack"

Just to let you know about it

@hthetiot
Copy link
Contributor

@DaveLomber Yes, I'm on it :)
WIP: #508

@hthetiot
Copy link
Contributor

To be clear the issue I think, was that I Implemented addtrack instead of track event on RTCPeerConnection and did not implement the setter for the event RTCPeerConnection.

@ekzotech
Copy link

@DaveLomber thank you very much! With your 'applyShimOnTrack' I'm able to get remote video.

@hthetiot
Copy link
Contributor

@DaveLomber I think it might be ready, give it a shot and let me know.

@DaveLomber
Copy link
Author

@hthetiot cool thanks! I will give it a try and let you know

@hthetiot
Copy link
Contributor

hthetiot commented May 19, 2020

The fix from #508 has been tested using the new cordova-plugin-iosrtc-sample (see Announcement #515).

The test was made using only Local-Peer not Janus, because I dot have access to a Janus instance. Some tuning over this index-janus.js file (https://github.com/cordova-rtc/cordova-plugin-iosrtc-sample/blob/master/www/js/index-janus.js) and one change on index.html (here https://github.com/cordova-rtc/cordova-plugin-iosrtc-sample/blob/master/www/index.html#L79) to use index-janus.js instead of index-local.js should allow to test Janus easely.

@hthetiot
Copy link
Contributor

hthetiot commented May 19, 2020

#508 got confirmation for Twilio #497 if we can have someone confirm it solve Janus I may release in the next 12-24 hours.

@DaveLomber
Copy link
Author

@hthetiot I was able to test it against Janus

seems the fix works and I can get a janus 'onremotestream' callback now which is good

The only one thing which is now I need to fix my app code is that in 'onremotestream' I'm getting a stream with audio track only at first place, but what I expect is to have both audio and video tracks.

Screen Shot 2020-05-19 at 11 20 29 PM

@DaveLomber
Copy link
Author

And what worries me more is that same code works well under Safari browser,
But under cordova I'm getting a stream with audio only tracks and my app reports that a remote user joined as audio only

This is something to investigate at my side

@hthetiot
Copy link
Contributor

hthetiot commented May 20, 2020

@DaveLomber I think the issue is might be that in the #508 I create a new even.streams[stream] for each track instead of reusing the same stream to add track compare to the polyfill, and that Janus use event.streams instead of event.track, I will see what I can do.
8a06fa5

@hthetiot
Copy link
Contributor

@DaveLomber I made some ajustement to #508 to make event.streams more reliable.

Can you test again, please.

@hthetiot
Copy link
Contributor

Twilio tester will also re-test to double check see #497 (comment)

@DaveLomber
Copy link
Author

Just checked - seems it does not fire 'pc.ontrack' anymore,
seems something is broken

https://github.com/meetecho/janus-gateway/blob/master/html/janus.js#L1772

@hthetiot
Copy link
Contributor

Shit

@hthetiot
Copy link
Contributor

@DaveLomber Do you still have you shim in place? I'm not sure the Shim will play nice with #508

It's possible that was a regression on #508 that has been fixed by 44e5b8b

@hthetiot
Copy link
Contributor

Confirmed the regression of track event support come from webrtc-adapter 7.6.0 + see issue webrtcHacks/adapter#1041

@hthetiot hthetiot changed the title Incompatibility with Janus WebRTC gateway Incompatibility with Janus WebRTC gateway when using WebRTC-adapter >= 7.6.0 May 24, 2020
@DaveLomber
Copy link
Author

@hthetiot appologize for the delayed feedback

I just tried latest master
https://github.com/cordova-rtc/cordova-plugin-iosrtc#ca3eb4b720b6cf8442e19732fdc3e98bf220e4b9

and all is working fine

thanks for you effort on this matter!

@hthetiot
Copy link
Contributor

\0/

@cordova-rtc cordova-rtc locked as resolved and limited conversation to collaborators May 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants