Skip to content
This repository has been archived by the owner on Mar 1, 2024. It is now read-only.

Add TCP+Relay detection event #485

Merged
merged 22 commits into from
Feb 19, 2024

Conversation

david-macpherson
Copy link
Collaborator

@david-macpherson david-macpherson commented Feb 14, 2024

Relevant components:

  • Signalling server
  • Frontend library
  • Frontend UI library
  • Matchmaker
  • Platform scripts
  • SFU

Problem statement:

There is currently no event emitted if the local candidate is relayed over TCP
this can happen if the local candidate is connected to a TURN server and the transport is TCP
This will degrade the stream quality severely

Solution

  • Log to the console a Stream quality severely degraded, local connection is relayed over TCP due to the local network environment. message
  • Once the stream has started if the local candidate type is relay and the relay protocol is TCP then emit a stream warning event for a user to consume to set their own message

Documentation

To subscribe to the event

	stream.addEventListener('webRtcTCPRelayDetected', (event: WebRtcTCPReldayDectectedEvent) => {
		console.log("TCP + TURN detected: " + JSON.stringify(event.data))
	});
	

@david-macpherson david-macpherson changed the title F stream warning event Add stream warning event Feb 14, 2024
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left some comments for changes around the specificity of the event, binding to it, and the general detection of the tcp/relay connection.

@lukehb lukehb changed the title Add stream warning event Add TCP+Relay detection event Feb 14, 2024
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requested some changes around how the stats are intercepted so that it works on each new connection, not just the first one.

@lukehb
Copy link
Contributor

lukehb commented Feb 15, 2024

Additionally, it appears the unit tests are now failing against your PR, please look into that too.

@david-macpherson david-macpherson marked this pull request as ready for review February 15, 2024 08:05
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor change requested refactoring lambda into a named function

Frontend/library/src/PixelStreaming/PixelStreaming.ts Outdated Show resolved Hide resolved
@lukehb lukehb self-requested a review February 16, 2024 00:51
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request for ambiguous this context change.

Frontend/library/src/PixelStreaming/PixelStreaming.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot about the remove listener case, you should revert it to using bind() as the arrow function syntax won't work for the remove case.

Frontend/library/src/PixelStreaming/PixelStreaming.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@lukehb lukehb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@lukehb lukehb merged commit 4969548 into EpicGames:master Feb 19, 2024
1 check passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants