republishing Track fail using Twillio (Was Error: removeTrack() must be called with a RTCRtpSender instance as argument)

henritoivar commented 4 years ago

[x] I have used Google with the error message or bug in association with the library and Cordova words to make sure the issue I'm reporting is only related to iOSRTC.
[x] I have provided steps to reproduce (e.g. sample code or updated extra/renderer-and-libwebrtc-tests.js file).
[x] I have provided third party library name and version, ios, Xcode and plugin version and adapter.js version if used.

Versions affected

cordova-plugin-iosrtc version 6.0.13
iOS 13.5.1
Twilio 2.6.0

Description

Trying to switch between cameras by unpublishing the previous track and then publishing a new track.

Unpublishing track returns error: removeTrack() must be called with a RTCRtpSender instance as argument

Steps to reproduce

I have adjusted the Twilio sample to implement switching between cameras:

<!DOCTYPE HTML>
<html>
<head>
    <title>
        Twilio Video Room
    </title>
    <meta name="viewport" content="viewport-fit=cover, width=device-width, initial-scale=1.0, minimum-scale=1.0, maximum-scale=1.0, user-scalable=no">
    <meta http-equiv="Content-Security-Policy"
          content="default-src 'self' data: gap: wss://* https://* 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; media-src *">
    <script type="text/javascript" src="cordova.js"></script>
</head>
<body>
<br><br><br>
<div id="local-media" style="border: red 1px solid;"></div>
<div id="remote-media"></div>
<button onclick="toggleVideo()">Toggle video</button>
<button onclick="leaveMeeting()">Leave meeting</button>
<button onclick="startMeeting()">Start meeting</button>
<button onclick="switchCamera()">Switch camera</button>
<script type="text/javascript">

const token = '';
const roomName = 'test';
const scriptUrls = [];
let videoTrack = null;
let audioTrack = null;
let room = null;
let facing = true;

function loadScript(scriptUrl) {

  if (scriptUrls[scriptUrl]) {
    return Promise.resolve(scriptUrl);
  }

  return new Promise(function (resolve, reject) {
    // load adapter.js
    var script = document.createElement("script");
    script.type = "text/javascript";
    script.src = scriptUrl;
    script.async = false;
    document.getElementsByTagName("head")[0].appendChild(script);
    script.onload = function () {
      scriptUrls[scriptUrl] = true;
      console.debug('loadScript.loaded', script.src);
      resolve(scriptUrl);
    };
  });
}

async function startMeeting() {

  videoTrack = await getVideoTrack();
  audioTrack = await Twilio.Video.createLocalAudioTrack();
  const localMediaContainer = document.getElementById('local-media');
  localMediaContainer.appendChild(videoTrack.attach());
  localMediaContainer.appendChild(audioTrack.attach());
  console.log(videoTrack, audioTrack);

  Twilio.Video.connect(token, {
    tracks: [videoTrack, audioTrack],
    name: roomName,
    sdpSemantics: 'plan-b',
    bundlePolicy: 'max-compat'
  }).then(_room => {
    room = _room;
    console.log(`Successfully joined a Room: ${room}`);

    // Attach the Tracks of the Room's Participants.
    room.participants.forEach(function (participant) {
      console.log("Already in Room: '" + participant.identity + "'");
      participantConnected(participant);
    });

    room.on('participantConnected', participant => {
      console.log(`A remote Participant connected: ${participant}`);
      participantConnected(participant);
    });

    room.on('participantDisconnected', participant => {
      console.log(`A remote Participant connected: ${participant}`);
      participantDisconnected(participant);
    });

  }, error => {
    console.error(`Unable to connect to Room: ${error.message}`);
  });

  function participantConnected(participant) {
    console.log('Participant "%s" connected', participant.identity);
    const div = document.createElement('div');
    div.id = participant.sid;
    participant.tracks.forEach((publication) => {
      console.log('subbing to existing publication', publication);
      trackSubscribed(div, publication);
    });

    participant.on('trackPublished', (publication) => {
      trackSubscribed(div, publication)
    });
    participant.on('trackUnpublished', trackUnsubscribed);

    document.getElementById('remote-media').appendChild(div);
  }

  function participantDisconnected(participant) {
    console.log('Participant "%s" disconnected', participant.identity);

    var div = document.getElementById(participant.sid);
    if (div) {
      div.remove();
    }
  }

  function trackSubscribed(div, publication) {
    console.log('sub publication', publication);
    if(publication.track){
      attachTrack(div, publication.track);
    }
    publication.on('subscribed', track => attachTrack(div, track));
    publication.on('unsubscribed', track => detachTrack(track));
  }

  function attachTrack(div, track){
    console.log('attachTrack', track);
    div.appendChild(track.attach());
  }

  function trackUnsubscribed(publication) {
    console.log('unsub publication', publication);
    if(publication.track){
      detachTrack(publication.track);
    }
  }
}

function detachTrack(track) {
  console.log('detachTrack', track);
  track.detach().forEach(element => element.remove());
}

function toggleVideo() {
  console.log(videoTrack, room);
  if (videoTrack.isEnabled) {
    videoTrack.disable();
    // room.localParticipant.unpublishTrack(videoTrack);
    console.log('disable');
  } else {
    videoTrack.enable();
    // room.localParticipant.publishTrack(videoTrack);
    console.log('enable');
  }
}

async function switchCamera(){
  facing = !facing;
  // Get new track
  const newTrack = await getVideoTrack();

  // Stop old track
  videoTrack.stop();
  detachTrack(videoTrack);

  // Unpublish previous track
  room.localParticipant.unpublishTrack(videoTrack);

  // Publish new track
  room.localParticipant.publishTrack(newTrack);
}

function getVideoTrack(){
  return Twilio.Video.createLocalVideoTrack({facingMode: facing ? 'user' : {exact: 'environment'}});
}

function leaveMeeting() {
  room.disconnect();
  videoTrack.stop();
  audioTrack.stop();
  detachTrack(videoTrack);
  detachTrack(audioTrack);
}

async function ready() {
  // Note: This allow this sample to run on any Browser
  var cordova = window.cordova;
  if (cordova && cordova.plugins && cordova.plugins.iosrtc) {

    // Expose WebRTC and GetUserMedia SHIM as Globals (Optional)
    // Alternatively WebRTC API will be inside cordova.plugins.iosrtc namespace
    cordova.plugins.iosrtc.registerGlobals();

    // Enable iosrtc debug (Optional)
    cordova.plugins.iosrtc.debug.enable('*', true);
  }
  console.log('loading Twilio');

  await loadScript('https://media.twiliocdn.com/sdk/js/video/releases/2.6.0/twilio-video.js');
  console.log('loaded');
}

if(!window.cordova){
  ready();
}

document.addEventListener('deviceready', ready, false);
</script>
</body>
</html>

Expected results

Should switch to other camera.

Actual results

Error: removeTrack() must be called with a RTCRtpSender instance as argument

hthetiot commented 4 years ago

@henritoivar thank you, will look into it.

hthetiot commented 4 years ago

@henritoivar, this is expected error if removeTrack is not getting an iosRTC RTCRtpSender instance.

It's probably an issue witht the way twilio-video.js manage sender here:

https://github.com/twilio/twilio-video.js/blob/9d9d7a63434f28cb4090861d96250e1ef7103a0a/lib/signaling/localparticipant.js#L59

Thank to your full example, I should be able to reproduce and handle the issue.

hthetiot commented 4 years ago

Note: updated example with missing <script type="text/javascript" src="cordova.js"></script>

hthetiot commented 4 years ago

I think this is related https://github.com/cordova-rtc/cordova-plugin-iosrtc/issues/571 @henritoivar You may test this PR (see instruction in description) - https://github.com/cordova-rtc/cordova-plugin-iosrtc/pull/572

henritoivar commented 4 years ago

@hthetiot Thanks a lot. This seems to be a part of the solution.

The issue i have now is that unpublishing works, but trying to publish again does not.

I have it set up so that i have the example running on the browser with one token and on iOS with another token. When i unpublish my video track from iOS then it also dissappears on the browser - this is good. When i republish my video track from iOS then it does not reappear on the browser - but it should.

When i have the example running both on browsers, then it works.

I'll try to dig a bit more to get more details on this.

henritoivar commented 4 years ago

The following issue with black remote video is also present: https://github.com/cordova-rtc/cordova-plugin-iosrtc/issues/541

hthetiot commented 4 years ago

The following issue with black remote video is also present: #541

arf, can you confirm @sboudouk ?

hthetiot commented 4 years ago

The issue i have now is that unpublishing works, but trying to publish again does not.

replaceTrack require renegotiate, RTCRtpSender is a SHAM for now not a shim see https://github.com/cordova-rtc/cordova-plugin-iosrtc/pull/544

you may try https://github.com/cordova-rtc/cordova-plugin-iosrtc/pull/544 that now include #572

sboudouk commented 4 years ago

The following issue with black remote video is also present: #541

arf, can you confirm @sboudouk ?

I did not get into this issue, probably because I am using twilio 2.4.

What version of cordova ios are you running @henritoivar ? Can you try downgrading your twilio version to 2.4.X to see if it fix the issues ?

I have no access to my workstation for now, will test it asap.

henritoivar commented 4 years ago

@hthetiot I'm still having the same issue with unpublishing and republishing ( video not reappearing on the other side / or black)

But it seems i can now work around and avoid using unpublish/publish as i can now use replaceTrack (available in twilio using track.restart) to switch between front/rear camera. Also it seems i can implement muting the video/audio track by using enable/disable track without unpublishing/publishing.

@sboudouk I did try using twilio 2.4 and had the same issues. Actually i'm using Capacitor. I will also test this on cordova and get back with the results.

hthetiot commented 4 years ago

But it seems i can now work around and avoid using unpublish/publish as i can now use replaceTrack (available in twilio using track.restart) to switch between front/rear camera. Also it seems i can implement muting the video/audio track by using enable/disable track without unpublishing/publishing.

good to know @henritoivar

I have no access to my workstation for now, will test it asap.

if you test its all on master @sboudouk

henritoivar commented 4 years ago

@hthetiot @sboudouk

I also tested using cordova 10.0.0, cordova-ios 6.1.1. The following issues persist:

Video stream not visible on browser after unpublishing and then republishing video on iOS device (using toggleVideo)
Video stream hangs on browser after switching camera on iOS using track.restart

You can test it out aswell by cloning this repository: https://github.com/henritoivar/test-iosrtc

It's possible to work around restarting the track, but it seems there is some kind of memory leak happening. Probably a result of how the streams are terminated. Something i noticed is that the cordova.plugins.iosrtc.mediaStreams object will contain quite a few stream objects after the call has been closed.

I managed to get the video stream showing again on the browser after restarting the track on iOS by using the following hack:

async function switchCameraWorking() {
  facing = !facing;
  videoTrack.restart({ facingMode: facing ? 'user' : { exact: 'environment' } });
  setTimeout(() => {
    videoTrack.disable();
    room.localParticipant.unpublishTrack(videoTrack);
    setTimeout(() => {
      videoTrack.enable();
      room.localParticipant.publishTrack(videoTrack);
    }, 1000);
  }, 1000);
}

The problem with this hack is that if you switch the camera multiple times, then the app seems to get slower and will finally hangs. Probably a result of the beforementioned memory leak from unpublishing track.

hthetiot commented 4 years ago

@henritoivar I wonder what unpublishTrack and publishTrack does, we may need to shim better MediaTrack to fix this issue and prevent your workarround.

Notice that the issue with track destruction is fixed on master via https://github.com/cordova-rtc/cordova-plugin-iosrtc/commit/cc5bafc0136886dc4e2cb4056290a6bb61221c1e and will be released as part of 6.0.14.

To test master:

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/cordova-rtc/cordova-plugin-iosrtc#master --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

hthetiot commented 4 years ago

henritoivar commented 4 years ago

@hthetiot Thanks, i'll give it a try.

Also something i came upon. Might be unrelated, but maybe something worth taking note of:

I was trying to track down why unified plan doesn't work with Twilio. Twilio sets a isUnifiedPlan flag to implement unified-plan specific track management: https://github.com/twilio/twilio-video.js/blob/c86e0001d3706e2c13b1914cffb2f08faae2285c/lib/signaling/v2/peerconnection.js

const sdpFormat = getSdpFormat(configuration.sdpSemantics);
const isUnifiedPlan = sdpFormat === 'unified';

The problem here is that getSdpFormat depends on browser detection and in WKWebView it will always return null, thus isUnifiedPlan will always be false. In this case RTCPeerConnection still initializes with unified plan (default), but Twilio will use plan-b specific track management. Thus we get errors like: setRemoteDescription() failed: Failed to set remote answer sdp: The order of m-lines in answer doesn't match order in offer

I tried hardcoding Twilio to use unified plan, but it seems in that case Twilio is using RTCPeerConnection.addTransceiver which is not yet supported in iosrtc. See the following lines: It sets localMediaStream in case of plan-b.

const localMediaStream = isUnifiedPlan ? null : new options.MediaStream();

Which is later referenced to whether to use addTransceiver (in case of unified-plan) or addTrack (in case of plan-b).

 addMediaTrackSender(mediaTrackSender) {
    if (this._peerConnection.signalingState === 'closed' || this._rtpSenders.has(mediaTrackSender)) {
      return;
    }
    let sender;
    if (this._localMediaStream) {
      this._localMediaStream.addTrack(mediaTrackSender.track);
      sender = this._peerConnection.addTrack(mediaTrackSender.track, this._localMediaStream);
    } else {
      const transceiver = this._addOrUpdateTransceiver(mediaTrackSender.track);
      sender = transceiver.sender;
    }
    mediaTrackSender.addSender(sender);
    this._rtpSenders.set(mediaTrackSender, sender);
  }

So it seems to get unified-plan working with Twilio we would need Twilio to implement a way of setting isUnifiedPlan from configuration and in iosrtc we would need addTransceiver support.

henritoivar commented 4 years ago

@hthetiot after disconnecting there is still a large amount of MediaStream objects in cordova.plugins.iosrtc.mediaStreams.

samgabriel commented 4 years ago

We are experiencing the very same items on this thread. We tried with Twilio 2.7.2 and 2.4.0 iosrtc 6.0.13 and master cordova-ios 5

In having more than two participants each with 2 tracks sometimes the video of one of them does not show. Upon stopping and re-adding the track always gets a black screen. One huge problem is that the app after adding or removing tracks become completely unusable as the https://github.com/cordova-rtc/cordova-plugin-iosrtc/issues/569#issuecomment-697342729 mentioned the MediaStreams keeps getting added with no deactivation even if the participant completely disconnects.

The other problem is the black screen or completely missing video tracks we are seeing a lot of these errors https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/0fab7e8975614c8711db7161c8141144d386de1d/src/iosrtcPlugin.swift#L805 even when the we see the track was just added. Here is an example for track e62cf499-a407-4210-ae37-25ab9546a154. I am not sure if this is related but i don't think it should happen


2020-09-23 11:54:29.768551-0400 Brainfuse[3118:327852] PluginRTCPeerConnection | onsignalingstatechange [signalingState:Optional("have-remote-offer")] 
2020-09-23 11:54:29.771410-0400 Brainfuse[3118:327731] iosrtc:RTCPeerConnection onEvent() | [type:signalingstatechange, data:{"type":"signalingstatechange","signalingState":"have-remote-offer"}] +6ms 
2020-09-23 11:54:29.776383-0400 Brainfuse[3118:327852] PluginRTCPeerConnection | onaddtrack 
2020-09-23 11:54:29.776699-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#init() 
2020-09-23 11:54:29.777092-0400 Brainfuse[3118:327852] PluginMediaStream#init() 
2020-09-23 11:54:29.777397-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#init() 
2020-09-23 11:54:29.777727-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#run() [kind:video, id:e62cf499-a407-4210-ae37-25ab9546a154] 
2020-09-23 11:54:29.777997-0400 Brainfuse[3118:327852] PluginMediaStream#run() 
2020-09-23 11:54:29.778652-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#deinit() 
2020-09-23 11:54:29.778910-0400 Brainfuse[3118:327852] PluginRTCPeerConnection | onaddstream 
2020-09-23 11:54:29.779040-0400 Brainfuse[3118:327852] PluginMediaStream#init() 
2020-09-23 11:54:29.779213-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#init() 
2020-09-23 11:54:29.779458-0400 Brainfuse[3118:327852] PluginMediaStreamTrack#run() [kind:video, id:e62cf499-a407-4210-ae37-25ab9546a154] 
2020-09-23 11:54:29.780324-0400 Brainfuse[3118:327852] PluginMediaStream#run() 
2020-09-23 11:54:29.780677-0400 Brainfuse[3118:327731] iosrtc:RTCPeerConnection onEvent() | [type:track, data:{"streamId":"default_85E1DC00-C42F-41BA-A86F-FC60EF0B8689","type":"track","track":{"readyState":"live","id":"e62cf499-a407-4210-ae37-25ab9546a154","kind":"video","enabled":true,"trackId":"e62cf499-a407-4210-ae37-25ab9546a154"},"stream":{"id":"default_85E1DC00-C42F-41BA-A86F-FC60EF0B8689","audioTracks":{},"videoTracks":{"e62cf499-a407-4210-ae37-25ab9546a154":{"readyState":"live","id":"e62cf499-a407-4210-ae37-25ab9546a154","kind":"video","enabled":true,"trackId":"e62cf499-a407-4210-ae37-25ab9546a154"}}}}] +9ms 
2020-09-23 11:54:29.781378-0400 Brainfuse[3118:327731] iosrtc:MediaStreamTrack new() | [dataFromEvent:{"readyState":"live","id":"e62cf499-a407-4210-ae37-25ab9546a154","kind":"video","enabled":true,"trackId":"e62cf499-a407-4210-ae37-25ab9546a154"}] +0ms 
2020-09-23 11:54:29.782484-0400 Brainfuse[3118:327731] iosrtcPlugin#MediaStreamTrack_setListener() 
2020-09-23 11:54:29.782982-0400 Brainfuse[3118:327731] iosrtcPlugin#MediaStreamTrack_setListener() | ERROR: pluginMediaStreamTrack with id=e62cf499-a407-4210-ae37-25ab9546a154 does not exist 
2020-09-23 11:54:29.783961-0400 Brainfuse[3118:327731] iosrtc:MediaStream create() | [dataFromEvent:{"id":"default_85E1DC00-C42F-41BA-A86F-FC60EF0B8689","audioTracks":{},"videoTracks":{"e62cf499-a407-4210-ae37-25ab9546a154":{"readyState":"live","id":"e62cf499-a407-4210-ae37-25ab9546a154","kind":"video","enabled":true,"trackId":"e62cf499-a407-4210-ae37-25ab9546a154"}}}] +510ms 
2020-09-23 11:54:29.784476-0400 Brainfuse[3118:327731] iosrtc:MediaStream new MediaStream(arg) | [arg:[]] +0ms 
2020-09-23 11:54:29.785209-0400 Brainfuse[3118:327731] iosrtcPlugin#MediaStream_init() 
2020-09-23 11:54:29.786705-0400 Brainfuse[3118:327852] PluginRTCPeerConnection | onicegatheringstatechange [iceGatheringState:Optional("gathering")]

hthetiot commented 4 years ago

@samgabriel

We are experiencing the very same items on this thread.

THIS is not a "thread" on a forum this is an "issue", please DO NOT start reporting unrelated issue about stream not been removed from memory and black screen, create separate issues.

Look at the title of the issue "Error: removeTrack() must be called with a RTCRtpSender instance as argument"

If people keep adding separate unrelated report to this issue, instead of looking for already opened issue or create new this prevent the reporter to get his issue fixed and simply add noise.

Keep in mind I'm not paid to make this plugin to work with Twilio or in general.

hthetiot commented 4 years ago

@henritoivar

after disconnecting there is still a large amount of MediaStream objects in cordova.plugins.iosrtc.mediaStreams.

Please report separate issue @neeact already provided a fix we may have more to do to handle stream/track end properly.

henritoivar commented 4 years ago

Regarding unpublishing and then republishing track:

I can see a WebSocket message going out that the track is published: When doing this flow on Chrome: When doing the same flow on iOS:

In Chrome after this message I also see a message coming in that the track should be published:

But in iOS in the incoming message this track has disappeared for some reason:

Something to note of also is that the track id(this is the TrackSender/MediaStreamTrack id) and name on Chrome are different, but on iOS name and id of the track are equal. Also on Chrome when unpublishing and then republishing the track id (TrackSender/MediaStreamTrack id) changes, but the name stays the same . On iOS both the name and id stay the same.

I'm doing some more digging to see why this is happening.

@hthetiot Technically the initial error message in this issue is fixed, but there are still issues with republishing a track. Should i create a new issue for the republishing problem, or keep it here?

hthetiot commented 4 years ago

Technically the initial error message in this issue is fixed, but there are still issues with republishing a track. Should i create a new issue for the republishing problem, or keep it here?

That fine Let keep it about republishing a track, still i will release original fix on 6.0.14 in soon.

hthetiot commented 4 years ago

@hthetiot after disconnecting there is still a large amount of MediaStream objects in cordova.plugins.iosrtc.mediaStreams.

see new separate issue https://github.com/cordova-rtc/cordova-plugin-iosrtc/issues/575 cc @henritoivar @samgabriel

henritoivar commented 4 years ago

@hthetiot The issue might be because of the following missing implementation: https://github.com/cordova-rtc/cordova-plugin-iosrtc/blob/62648fd9688818289ee94a343436431458c2554d/dist/cordova-plugin-iosrtc.js#L1182-L1186 Twilio is utilizing clone in quite a few places. By definition it should return a MediaStreamTrack with a new id. This also explains why on Chrome the track id changes, but in iOS it stays the same. It seems Twilio is not picking up the republishing of the track as the id does not change.

If i apply the same code in Chrome, then i get the same issue that the track is not republishing.

I tried something like this as a fix:

MediaStreamTrack.prototype.clone = function () {
    console.log('MediaStreamTrack.prototype.clone ');
    //throw new Error('Not implemented.');
    // SHAM
    return new MediaStreamTrack({
        id: window.crypto.getRandomValues(new Uint32Array(4)).join('-'),
            kind: this.kind,
            label: this.label,
            readyState: this.readyState,
        enabled: this._enabled,
    });
};

But i'm not sure that's the right way to implement that and if there is something else internally depending on the mediastreamtrack id.

henritoivar commented 4 years ago

hthetiot commented 4 years ago

@henritoivar I'm looking at the IDs issues on media and track I Will handle clone also keep you posted.

henritoivar commented 4 years ago

@hthetiot Thanks a lot. Let me know if i can help anyhow.

henritoivar commented 4 years ago

@hthetiot Have you managed to look into this?

I'm considering having a go at fixing this. Any pointers are welcome.

cah-dgreif commented 4 years ago

We are testing cordova with both mediasoup and the new Azure Communication Services (ACS) and both clients run into issues because clone and addTransceiver aren't implemented. With the number of new streaming platforms being released lately (ACS, Zoom, NVIDIA, Twilio, etc.) it's going to be critical that these modern interfaces are supported.

hthetiot commented 4 years ago

@cah-dgreif

We are testing cordova with both mediasoup and the new Azure Communication Services (ACS) and both clients run into issues because clone and addTransceiver aren't implemented. With the number of new streaming platforms being released lately (ACS, Zoom, NVIDIA, Twilio, etc.) it's going to be critical that these modern interfaces are supported.

Cool story, that help NOT

it's going to be critical that these modern interfaces are supported.

Great, how much do you pay for iosRTC remind me already, may be a more contructive way to say that would have been "I really need this but i dont know how to code swift so i complain about it"

hthetiot commented 4 years ago

@hthetiot Have you managed to look into this?

Not yet, we focusing on #576 for 6.0.14 for now, however, i did technicaly fixed the "removeTrack() must be called with a RTCRtpSender instance as argument" as part of 6.0.14 and master.

hthetiot commented 4 years ago

Another off topic comment for @cah-dgreif, your comment is reflective of brogrammer that think they are owned by open source contributors their wish and get credit for copy paste in their source code that other produces, unless this person apologize with one repository without even one commit from him (https://github.com/cah-dgreif/ng2-daterangepicker/commits/master) I will ban him. I noticed also someone that knows him, I give you 24 hours to apologize publicly or be ban for life.

If you are not happy you can fork and go away.

hthetiot commented 4 years ago

@hthetiot Have you managed to look into this?

I'm considering having a go at fixing this. Any pointers are welcome.

Feel free to give it a shoot, it's good to know or understand the source code you depend or use anyway. Notice that if it was that simple I would have fix it already (this issue is going in so many directions it's not even clear for me anymore).

hthetiot commented 4 years ago

Please create separate issue @henritoivar there is one for clone already.

dgreif commented 4 years ago

@hthetiot As a heavy contributor to open-source, I can understand why my comment frustrated you. My intent was to emphasize that @henritoivar is likely not the only user of this plugin that will be looking for these features. It is definitely frustrating when "brogrammers" post entitled comments, and I apologize for giving you the impression that's what I was doing. My coworkers, @calebboyd and @cah-dunn, have both made contributions to this plugin and we are in the process of evaluating whether we should contribute more to this plugin or fork/make our own. I encouraged @calebboyd to reach out to you earlier today about contract work, but it looks like that may be off the table.

Regarding ng2-daterangepicker, I did add commits in a branch and opened a PR back to the original repo (https://github.com/evansmwendwa/ng2-daterangepicker/pull/122). That's not brogramming, that's open-source contribution. @cah-dgreif is my work account, which made sense to post with since the plugin is used at work.

While we sincerely appreciate your contributions to this plugin, your rash response to a simple comment shows that you may be getting burned out on this project. You may want to consider taking a break. We'll likely move forward with our own cordova plugin and will let you know if we have an updated plugin that supports the modern features that most users of this plugin will need going forward.

hthetiot commented 4 years ago

@dgreif

we are in the process of evaluating whether we should contribute more to this plugin or fork/make our own.

The fact that you thinking making your own or fork instead of contribute via PR and meaning full comment is you own choice and right, but making that choice based on my comment is purely nonsense.

I encouraged @calebboyd to reach out to you earlier today about contract work, but it looks like that may be off the table.

I will make this clear, I do not take paid work for iosRTC, this will prioritize issue for people that have money instead of helping people that can use this plugin for free to provide public or free service, for example, if you can actually pay for Twillio for example or any paid provider may be you should use their iOS solution instead of iosRTC or ask them to contribute to it instead of complain that the fix is not available.

While we sincerely appreciate your contributions to this plugin, your rash response to a simple comment shows that you may be getting burned out on this project.

This only thing that burn me is stupid comment like yours, this is the cancer of OpenSource you simply demotivate maintainers by your pretentious attitude and by acting like we own you something.

You may want to consider taking a break. We'll likely move forward with our own cordova plugin and will let you know if we have an updated plugin that supports the modern features that most users of this plugin will need going forward.

Please go fork, you are not the first and nor will be the last. If that can prevent you for making useless or pretentious comments that waste everyone time would be a good thing.

dgreif commented 4 years ago

@hthetiot

Great, how much do you pay for iosRTC remind me already I do not take paid work for iosRTC

I'm getting conflicting impressions on whether or not you want to be paid 😅.

this is the cancer of OpenSource you simply demotivate maintainers by your pretentious attitude and by acting like we own you something.

Again, I am sorry my comment came across as pretentious or entitled. I was really just trying to add to the conversation by pointing out that Twilio isn't the only client that will need these features. It is very likely that @calebboyd or I will add these features in a fork and open a PR in the near future.

I give you 24 hours to apologize publicly or be ban for life. If you are not happy you can fork and go away. The fact that you thinking making your own or fork instead of contribute via PR and meaning full comment is you own choice and right, but making that choice based on my comment is purely nonsense.

Threatening to ban someone sheds serious doubt that any PRs will be accepted. We will try to contribute back to this plugin, and I hope you won't ignore my contributions because of this initial conversation. I'm sorry we got off on the wrong foot and I hope we can work together to bring the features I mentioned to life.

hthetiot commented 4 years ago

Threatening to ban someone sheds serious doubt that any PRs will be accepted.

You don't do PR for now anyway, only comments that distrup.

We will try to contribute back to this plugin, and I hope you won't ignore my contributions because of this initial conversation.

I judge people by there actions not by there words, contributions are always welcome.

I'm sorry we got off on the wrong foot and I hope we can work together to bring the features I mentioned to life.

Sure, only stupid people don't change their mind, I love to be proven wrong anyway.

hthetiot commented 4 years ago

Threatening to ban someone

I only ban bad reporter and commenters, I always give them a chance to explain themselves. This days this is the only way I found to regular bad actors on open source project.

I will never ban a real contributor that do PR, contributors that make code are the most valuable actors.

hthetiot commented 4 years ago

@dgreif I did check the PR from your "team", I had to myself fix the indentation to complete the review in order to have a clean diff, I had to make a new PR to make things move faster, unfortunately too late for 6.0.14, can be part of 6.0.15

Once @calebboyd applied the review change (notably removing unused fonction he added) their is no reason for it not to be merged.

https://github.com/cordova-rtc/cordova-plugin-iosrtc/pull/584#pullrequestreview-510979672

hthetiot commented 4 years ago

@henritoivar Please try this PR see testing instructions: #587

hthetiot commented 4 years ago

@henritoivar can you check #592

hthetiot commented 4 years ago

The last know Twillio issue should have been fixed on #605 that has landed on master target is 6.0.16

Can you test @sboudouk @henritoivar @dgreif we kept Janus fix in place also.

cordova-rtc / cordova-plugin-iosrtc