search

Temasys / SkylinkJS

SkylinkJS Javascript WebRTC SDK
http://skylink.io/web
Other
275 stars 57 forks source link

Can't mute audio when screensharing #245

Closed JanNorden closed 8 years ago

JanNorden commented 8 years ago

calling skylink.mutesteram(audioMuted: true})

while screensharing results in a log message:

'No audio available to mute / unmute'

The issue can be resolved by changing line 1969 in stream-media.js from

if (self._streamSettings.audio === false) {

to

if (self._streamSettings.audio === false && self._screenSharingStreamSettings.audio === false) {

It may make sense to make the same change for video, however I have not tried that.

oooookk7 commented 8 years ago

Hi @JanNorden, it seems like you invoke muteStream() before shareScreen() has retrieved the screensharing stream as shareScreen() is an asynchronous method, hence muteStream() outputs that there isn't any stream available to mute / unmute.

I'm assuming that you are attempting to mute the audio the moment when screensharing stream is retrieved.

Here's a suggestion on how it can be implemented without modifications to the SDK code:

skylink.once('mediaAccessSuccess', function (stream, isScreensharing) {
  if (isScreensharing) {
    skylink.muteStream({ audioMuted: true });
  }
});

skylink.shareScreen();

Due to #198, we are not using the shareScreen(callback) solution, but we can subscribe to the events that shareScreen() callback does to wait for request to be completed. In this manner, muteStream() is invoked only when stream is successfully retrieved.

For callback(.., success) state, we subscribe to the mediaAccessSuccess event as it triggers the moment stream is retrieved successfully. For callback(error, ..) state, there's the mediaAccessError event as it triggers the moment stream retrieval fails.

JanNorden commented 8 years ago

Thank you for the suggestion, however I am already calling muteStream in the mediaAccessSuccess handler.

As far as I can tell, this._streamSettings contains the settings for the latest regular (non-screenshare) stream, and not the properties of the screenshare stream, which are only in this._screenSharingStreamSettings, hence my proposed fix.

oooookk7 commented 8 years ago

@JanNorden Yes it does. Because it's easier to switch to the with non-screensharing stream settings when user invoke stopScreen() instead of sharing them.

I would not recommend updating the SDK codebase though, because if we have new SDK releases it might be troublesome for you.

oooookk7 commented 8 years ago

@JanNorden if there isn't any more issues on this I would be closing this ticket.

JanNorden commented 8 years ago

The original issue still remains, if a call mutestream in the mediaAccessSuccess called after shareScreent, I get the error, and when looking at this._streamSettings in the debugger it has .audio set to false, while _screenSharingStreamSettings has it set to true. I will write a small test case to demonstrate.

Are you by any chance using looking at a dev version rather than 0.6.13?

oooookk7 commented 8 years ago

I'm using the 0.6.x-development branch, but there wasn't updates to the stream settings (except like docs).

The _streamingSettings and the _screenSharingStreamSettings is the status of the stream object if there is audio and video tracks in it. I think what you want to look into is perhaps _mediaStreamsStatus? So regardless of setting the muteStream() function, it manipulates the _mediaStreamsStatus not the _streamingSettings nor _screenSharingStreamSettings since they are different.

JanNorden commented 8 years ago

Ok, I just remembered that we do have other local changes, so it may be something we did. I will create a small example using the version on your CDN.

JanNorden commented 8 years ago

The page below shows the problem. One possible reason that you did not see it is that you called joinRoom without parameters rather than with audio: false. I think _streamingSettings.audio will be set to true by default, and remain so when screen sharing is started.

<html>
<title>Skylink Screnshare mute problem.</title>
<nead>

</nead>
<body>
    <script src="//cdn.temasys.com.sg/skylink/skylinkjs/0.6.13/skylink.complete.js"></script>
    <script>
    var skylink;

    function joinRoom(event) {
        console.log('joinRoom');
        skylink = new Skylink();
        console.log(skylink);
        skylink.init({appKey: '33c2efb4-ab97-4fd8-99c8-a892ad0cc4d8'}, (error) => {
            skylink.joinRoom('defaultroom', { audio: false, video: false });
    });
    }

    function shareScreen(event) {
        skylink.once('mediaAccessSuccess', function (stream, isScreensharing) {
            if (isScreensharing) {
                console.log('muting');
                skylink.setLogLevel(4);
                skylink.muteStream({ audioMuted: true });
            }
        });
        console.log('shareScreen');
        skylink.shareScreen(true);
    }
    </script>
    <button onclick="joinRoom(event)">join room</button>
    <button onclick="shareScreen(event)">share screen</button>
</body>
</html>
oooookk7 commented 8 years ago

Hi @JanNorden, thanks for providing the demo code. I could reproduce the error you were facing. I looked at the code and yes it was this line that disallowed the muting.

While I do not recommend modifications to the SDK code, if it's an urgent issue, I would suggest the following modifications:

For audio muted fixes, modify at this line:

  // set the muted status
  if (typeof options.audioMuted === 'boolean') {
    if (self._streamSettings.audio === false && self._screenSharingStreamSettings.audio === false) {
      log.error('No audio available to mute / unmute');
      hasAudioError = true;
    } else {
      if (options.audioMuted) {
        self._mediaStreamsStatus.audioMuted = true;
      } else {
        self._mediaStreamsStatus.audioMuted = false;
      }
    }
  }

For video muted fixes, modify at this line:

  if (typeof options.videoMuted === 'boolean') {
    if (self._streamSettings.video === false && self._screenSharingStreamSettings.video === false) {
      log.error('No video available to mute / unmute');
      hasVideoError = true;
    } else {
      if (options.videoMuted) {
        self._mediaStreamsStatus.videoMuted = true;
      } else {
        self._mediaStreamsStatus.videoMuted = false;
      }
    }
  }

I can't give an ETA due to the priorities at hand but this ticket will be updated when a proper fix has been released in the SDK.

oooookk7 commented 8 years ago

Hi @JanNorden, please check out 0.6.15 release which should resolve your issue.

oooookk7 commented 8 years ago

Closing this issue since it's been resolved in the 0.6.15 release. If this issue persists, feel free to re-open it again.