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

[Feature Review] Added mute stream event #1244

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

[Feature Review] Added mute stream event #1244

wants to merge 5 commits into from

Conversation

kekkokk
Copy link
Contributor

@kekkokk kekkokk commented Jun 11, 2018

[Mute stream propagation] - Feature Review

This is a proposal of a new feature, that extends the current one that is already present in the code but it is not consistent in Erizo and P2P mode.

This aims to provide a client side event to inform the "frontend" of a state of the published/subscribed video.
This can be useful for the frontend developers to better handle the frontend layout, such as display an image or avatar when the stream is muted (currently displayed as a black video)

I'll try to explain my problematic with current implementation an examples:

  • client A publish video when he is alone in the room
  • client A mute his video (this is very common in business meetings)
  • client B joins, subscribe A video but he sees only a spinning wheel or a black screen
  • if client B inspect remote video, videoMuted: false

My first thought was to extend updateStream, but It is sent to the controller only if a peerConnection is established (works for Erizo but could not work in p2p mode).

So I implemented a whole new message to send in socket.

Now, every time the stream is muted by publisher, the controller will also know, and if there are subscribers, it propagates the event so they can do something in frontend.

Also, if new clients subscribe this stream, they can inspect the Stream.muted object and can see that the stream is remotely muted (muted by publisher).
Also, everytime the publisher mute/unmute they will receive a Stream event: stream-mute-event with this arg:

{
    video: {
        local: false,
        remote: true,
    },
    audio: {
        local: false,
        remote: false,
    },
}

meaning that the remote video, is not being sent by the publisher, and we maybe should display something informing the client.

Architecture

Stream.js

When muteAudio or muteVideo is called, all tracks of that kind are muted in local peerConnection, setting them enabled: false;
If is not P2P, the event updatestream is sent to the controller (as before).
Meanwhile another event ('internal-mute-event') is emitted to the room.

The room take care of listening this internal event and managing it with the updateMuteConfigFromStreamEvent function.
This will send an update to the Erizo Controller with the event updateMuteConfig, needed to send a message to all the clients that subscribes that video and to update the internal state of this stream in the controller structure, so that new subscribers will know at the connection to the room that particular stream is muted.

API Changes

There's no change in the APIs

How does it affect Erizo Client?

There is a new Stream event which can be binded by clients to know in realtime when the muting change in that stream.
It was added in the basicexample:
https://github.com/lynckia/licode/pull/1244/files#diff-8f3407c4abcebd856a0057760ef1fcffR110

How does it affect Erizo Controller / Erizo Agent / Erizo JS?

The controller has a new socket message to listen.

@jcague
Copy link
Contributor

jcague commented Jun 11, 2018

This looks to me like a new feature, so it would be better if you fill a feature review doc explaining the changes. You can find an example we used here: https://github.com/lynckia/licode/tree/master/feature-review

We encourage the use of this methodology to keep the focus first on discussing the feature itself, and then to save time reviewing code when we all finally agree on the feature. You can propose the feature with a PR containing just the document itself, and not adding code.

Thanks a lot!

@kekkokk kekkokk changed the title Added mute stream event [Feature Review] Added mute stream event Jun 11, 2018
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.

2 participants