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

[SIP] Video not displayed if initially blank and only received after some time #13565

Open
fancycode opened this issue Oct 16, 2024 · 5 comments

Comments

@fancycode
Copy link
Member

Noticed this while working with #13486 on the SIP bridge.

Currently the video unmute event is triggered if more than 2000 bytes are received within a second of the stream starting:

if (statsReport.bytesReceived > 0) {
if (mediaType === 'video' && statsReport.bytesReceived < 2000) {
// A video with less than 2000 bytes is an empty single frame of the MCU
// console.debug('Participant is registered with with video but didn\'t send a lot of data, so we assume the video is disabled for now.')
result = true
return
}
webrtc.emit('unmute', {
id: peer.id,
name: mediaType,
})

If less than 2000 bytes (but more than 0) are received, the video is assumed as "disabled" and further checks are disabled:

stopPeerCheckVideoMedia(peer)

While this doesn't happen all the time, when it happens, only a datachannel unmute event will make the video appear in the callview:

webrtc.emit('unmute', { id: peer.id, name: 'video' })

This is a problem for SIP video which might be empty / black at the beginning and only start after some time, and datachannels are not supported by the SIP bridge.

Adding here for tracking. Maybe @danxuliu has an idea if there is a simple way to support this case without having to implement datachannels in the SIP bridge.

@danxuliu
Copy link
Member

Actually that detection code was added back in the day to work around some problems sending the unmute events, but my idea was always to get rid of it :-)

Although in the beginning audio/video enable/disabled was notified through data channels now the clients do that also through signaling messages (well, right now the Android client does not, but it will do soon ;-) ). Would the SIP bridge be able to send a signaling message when audio/video is enabled/disabled? Then it should™ work independently of the data channels and that detection code.

Only thing to keep in mind is that the signaling messages are different than the data channel messages. Data channel messages are audioOn/audioOff/videoOn/videoOff, while signaling messages are type mute/unmute with payload { name: 'audio/video' }.

@fancycode
Copy link
Member Author

We could use the incall flags, they are already set correctly for the SIP users:

spreed/src/constants.js

Lines 132 to 138 in 0a0ced1

CALL_FLAG: {
DISCONNECTED: 0,
IN_CALL: 1,
WITH_AUDIO: 2,
WITH_VIDEO: 4,
WITH_PHONE: 8,
},

The internal client has IN_CALL and WITH_AUDIO, the (virtual) phone users have IN_CALL and WITH_PHONE. It would be easy to switch the internal client to also set WITH_VIDEO.

I didn't check if the flags are correctly set for web- / mobile clients already, but they should:

if ($flags === null) {
// Default flags: user is in room with audio/video.
$flags = Participant::FLAG_IN_CALL | Participant::FLAG_WITH_AUDIO | Participant::FLAG_WITH_VIDEO;
}

@fancycode
Copy link
Member Author

Although in the beginning audio/video enable/disabled was notified through data channels now the clients do that also through signaling messages [...]

So the clients have to use three different ways to notify about their state: the "incall" flags, the datachannel events and the signaling events 🙈

@danxuliu
Copy link
Member

We could use the incall flags

The WITH_AUDIO and WITH_VIDEO flags say whether the participant is (or is expected to be) publishing audio/video, but not whether the audio/video is enabled or disabled. But if needed I guess we could do a special handling in the clients to assume that video is always enabled for the internal client of the SIP bridge if the flag is set. It would be necessary to be able to differentiate the internal client of the SIP bridge from other internal clients, though, which I do not know if it is currently possible.

So the clients have to use three different ways to notify about their state: the "incall" flags, the datachannel events and the signaling events 🙈

I expect to finally remove sending the audio/video state through data channels for the next Talk major release (it requires the Android app to both send and receive the state with signaling messages, but that should be ready soon). Actually once the Android app supports that it would not be needed to send data channels either in previous versions (at least, until the WebUI started to send and receive both data channel and signaling messages for the state), because mobile apps are backwards compatible and they are expected to be used in their latest major version even against older servers. But well, we will not mess with that and data channels will be kept also in older versions.

So technically the WebUI and the mobile clients will only need to use signaling messages to notify about their state (if we talk about audio/video enabled/disabled and ignore the speaking state, which is sent only through data channels... 🙈).

Having said that... at some point the state may be set using transient states instead of signaling messages :-P But that should be a full replacement rather than adding yet another way of sending the state ;-)

@fancycode
Copy link
Member Author

Good news, the new version of the SIP bridge will support DataChannels 🎆

So now I just need a way to make sure the clients will actually enable them - which they currently don't:

// The SIP bridge publisher does not have data channels, so they
// need to be explicitly disabled in the subscriber. Otherwise it
// would try to open them, which would cause an endless loop of
// renegotiations, as after a negotiation the data channels will
// still not be opened, which will trigger a negotiation again.
if (callParticipantModel.get('internal')) {
peer.enableDataChannels = false
}

I'm pretty sure there is similar code in the mobile apps, too.

My idea is to expose the features list together with the internal flag, so the clients know what each internal client supports (e.g. feature datachannels).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants