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

Support toggling audio/video in subscribed streams. #239

Merged
merged 2 commits into from
May 10, 2022

Conversation

fancycode
Copy link
Member

Follow-up to #191

The command selectStream also supports optional boolean flags audio and video that can be used to enable/disable receiving the corresponding media from the stream. The parameters of selectStream can also be included in the payload of requestoffer for example to subscribe a stream with video initially disabled.

The new flags are similar to the existing substream / temporal.

@danxuliu please review, is this something you could work with?

@coveralls
Copy link

coveralls commented Apr 28, 2022

Pull Request Test Coverage Report for Build 2299528517

  • 0 of 84 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.2%) to 43.077%

Changes Missing Coverage Covered Lines Changed/Added Lines %
mcu_janus.go 0 84 0.0%
Files with Coverage Reduction New Missed Lines %
hub.go 2 63.39%
Totals Coverage Status
Change from base Build 2280077395: -0.2%
Covered Lines: 4371
Relevant Lines: 10147

💛 - Coveralls

@danxuliu
Copy link
Contributor

@danxuliu please review, is this something you could work with?

Totally :-) Thanks a lot!

Only one thing: the substream, temporal, audio and video parameters are handled only when requesting a new offer, but not when updating a previous one. I have checked* and if a previous subscriber connection is updated (without changing anything, just requesting an offer again with the same sid) the video is started again. Therefore those parameters should be handled too when updating an existing subscriber connection to be able to set the expected state.

However, after modifying the code... it seems that Janus (at least version 0.11.3) ignores the audio and video parameter in request: configure if update: true is also set (that is, audio and video will be active even if audio: false and video: false were provided). substream and temporal parameters, on the other hand, still work as expected with update: true, so maybe I did something wrong (it is late... :-P ).

In any case, if a renegotiation enables again audio and video this would not be a problem, as a new selectStream message can be sent once the renegotiation is done to adjust the subscriber again to the desired state; I was just trying to do it directly in the renegotiation for consistency with an initial subscription.

*In master Talk:

  • Start a call with audio and video
  • In a private window, join the call without audio nor video
  • Open the WebRTC stats in another tab; in Firefox about:webrtc, in Chromium chrome://webrtc-internals
  • Block the received video; the stats of the received connection show that the video is no longer received
  • In the browser console, execute OCA.Talk.SimpleWebRTC.connection.requestOffer(OCA.Talk.SimpleWebRTC.webrtc.peers[0].id, 'video', OCA.Talk.SimpleWebRTC.webrtc.peers[0].sid)
  • The stats show that the video is being received again

@fancycode
Copy link
Member Author

I updated the PR to also support audio / video in update requests.

From reading the code of Janus, they should also be evaluated with update: true. In your tests with Talk master: did you change something that the new flags are included in the requestoffer? At least I couldn't find them being included.

@danxuliu
Copy link
Contributor

From reading the code of Janus, they should also be evaluated with update: true. In your tests with Talk master: did you change something that the new flags are included in the requestoffer? At least I couldn't find them being included.

Yes, I added a payload object to data in requestOffer, although I did not push the code as it was just for testing.

Nevertheless, I also added configure_msg["video"] = false before configure_response, err := handle.Message(ctx, configure_msg, nil), which from what I understand should block the video when requesting an updated offer, yet the received video was still active after the renegotiation.

@fancycode
Copy link
Member Author

From reading the code of Janus, they should also be evaluated with update: true.

Checked the Janus code again and found the issue: meetecho/janus-gateway#2963

@danxuliu
Copy link
Contributor

From reading the code of Janus, they should also be evaluated with update: true.

Checked the Janus code again and found the issue: meetecho/janus-gateway#2963

Nice catch! :-D

If I understand it correctly your fix solves both issues, right? I mean:

  • send configure request with video: false, then send configure request with update: true -> video will be still disabled in the subscriber after the update
  • send configure request with update: true and video: false -> video will be now disabled in the subscriber after the update

@fancycode
Copy link
Member Author

I only tested the second part and this was fixed. Didn't test the first scenario yet but assume this will be fixed, too.

The command `selectStream` also supports optional boolean flags `audio`
and `video` that can be used to enable/disable receiving the corresponding
media from the stream.
@fancycode fancycode merged commit c0f56ff into master May 10, 2022
@fancycode fancycode deleted the toggle-audio-video branch May 10, 2022 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants