Eyevinn / webrtc-player

WebRTC (recvonly) player
MIT License
87 stars 18 forks source link

feat: emit peer connection events for connection failure & no-media #30

Closed oshinongit closed 1 year ago

oshinongit commented 1 year ago

Basis for discussion on issue #29 'Player auto-reconnect on loss of WHIP Client video'

Also I realize my syntax formatting causes annoying changes. Perhaps we can sync on a standard.

bwallberg commented 1 year ago

Regarding syntax standards, yes, we should use prettier but we can do that in a separate PR. I'll create a ticket for you to do some improvements regarding that. ( We'll have to align some stuff internally first, but we'll certainly deal with it, for now I'd suggest disabling that in your editor ).

marcusspangenberg commented 1 year ago

The PR title suggests there were to log message added but I only see one?

I started sending WHPP and then closed the WHIP tab, the PC was still active but no media was obviously being sent so it seems we still need to do some work to detect media not being sent.

How would I go about testing this particular version of media ending?

WebRTCPlayer is reading the stats object with a certain interval in WebRTCPlayer.onConnectionStats(). That should be extended to read the received packet count or byte count per "inbound-rtp" element (https://www.w3.org/TR/webrtc-stats/#dom-rtcinboundrtpstreamstats) and checking if the streams have stopped. Note that audio is not sent during silence, but video should always be sent.

georgewoofbates commented 1 year ago

Could we also have the stream muted property exposed to the player object? There's also an onmute and onunmute that could emit an event out of the class as well. The mute property seems to be enabled when the video stops on the stream so is great for detecting when a video feed isn't being received.

oshinongit commented 1 year ago

The PR title suggests there were to log message added but I only see one?

I started sending WHPP and then closed the WHIP tab, the PC was still active but no media was obviously being sent so it seems we still need to do some work to detect media not being sent.

How would I go about testing this particular version of media ending?

The PR title can be revised to clarify that (state=='connected' AND 'media has stopped streaming')

birme commented 1 year ago

Could we also have the stream muted property exposed to the player object? There's also an onmute and onunmute that could emit an event out of the class as well. The mute property seems to be enabled when the video stops on the stream so is great for detecting when a video feed isn't being received.

Good idea, though perhaps we should keep that in a separate issue and PR

oshinongit commented 1 year ago

The PR title suggests there were to log message added but I only see one?

I think the PR can eventually be renamed into "feat: events emitted for RTCPeerConnection transitions to the failed state and RTCPeerConnection is still in the connected state, but no media is received" or similar.

oshinongit commented 1 year ago

One thought I had is regarding if the event should be limited to firing once or not? A problem is when having a very low timeoutThreshold. This might lead to the media being lost at times but regained during the next iteration of stat gathering. I have removed the limit of firing it once. Any ideas?

bwallberg commented 1 year ago

One thought I had is regarding if the event should be limited to firing once or not? A problem is when having a very low timeoutThreshold. This might lead to the media being lost at times but regained during the next iteration of stat gathering. I have removed the limit of firing it once. Any ideas?

I'd suggest that at the very least it shouldn't emit the "NO_MEDIA" event unless the media has recovered. So only one event per loss so to speak.

oshinongit commented 1 year ago

Changes: