This repository has been archived by the owner on Mar 22, 2022. It is now read-only.
Update the Unity library to handle split sources #404
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Convert WebcamSource and MicrophoneSource to source objects shareable among multiple peer connections.
djee-ms
added
enhancement
fibann
reviewed
Jun 8, 2020
fibann
reviewed
Jun 8, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
reviewed
Jun 9, 2020
fibann
suggested changes
Jun 9, 2020
- Split audio and video hierarchies - Use base class VideoRendererSource for polymorphic serialization of the source of a VideoRenderer. - Use interfaces for media types (audio or video), and serialize as MonoBehaviour. In the Unity UI, the concrete type used is always known, so we can override the UI and ensure proper filtering and selection mechanisms work as intended.
fibann
reviewed
Jun 10, 2020
Move OnEnable/OnDisable implementations into the concrete leaf classes to flatten the track creation and destruction.
Merge the MediaSender and its derived classes AudioSender and VideoSender into the MediaLine itself, since those classes contain very few fields and the logic is entirely internal and managed by the MediaLine object they are attached to.
fibann
reviewed
Jun 16, 2020
| @@ -61,17 +61,25 @@ protected override async void OnEnable() | |||
| } | |||
| #endif | |||
|
|
|||
| protected override async Task CreateAudioTrackSourceAsyncImpl() | |||
| protected virtual async Task OnEnable() | |||
There was a problem hiding this comment.
Why indeed ¯_(ツ)_/¯
I think that might have been an automated refactor removing the implementation in the base class, because I don't see why I'd have added that in a leaf class.
Also I just noted there are 2 implementations of OnEnable() now on UWP, will merge.
fibann
reviewed
Jun 16, 2020
Comment on lines
103
to
113
| /// This value must comply with the 'msid' attribute rules as defined in | ||
| /// https://tools.ietf.org/html/draft-ietf-mmusic-msid-05#section-2, which in | ||
| /// particular constraints the set of allowed characters to those allowed for a | ||
| /// 'token' element as specified in https://tools.ietf.org/html/rfc4566#page-43: | ||
| /// - Symbols [!#$%'*+-.^_`{|}~] and ampersand & | ||
| /// - Alphanumerical characters [A-Za-z0-9] | ||
| /// | ||
| /// Users can manually test if a string is a valid SDP token with the utility | ||
| /// method <see cref="SdpTokenAttribute.Validate(string, bool)"/>. | ||
| /// </remarks> | ||
| /// <seealso cref="SdpTokenAttribute.Validate(string, bool)"/> |
There was a problem hiding this comment.
nit: can we test that is a valid SDP token on set?
There was a problem hiding this comment.
Hmm is this done when assigning _senderTrackName?
There was a problem hiding this comment.
_senderTrackName has the attribute, so this will make some error message appear when editing the field in the Unity editor if the SDP token is not valid. We can add some extra validation in the setter too for runtime.
fibann
approved these changes
Jun 16, 2020
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Convert
WebcamSourceandMicrophoneSourceto source objects shareableamong multiple peer connections.
Because the
SceneVideoSendercomponent uses the ARGB32 callback, also re-addthat callback, and add a TODO to remove all ARGB32 callbacks in favor of explicit
conversion utility functions, for clarity about the performance implication of the
conversion.
This change also splits the
VideoChatDemoandSceneCaptureDemofor clarity,leaving each demo with a single audio and video track per peer connection.
The
SceneCaptureDemonow has a placeholder showing the capture cameraposition. Multiple tracks per peer connection are demo-ed in the
StandaloneDemo.All demos are also updated to show essential components as Unity objects in the
scene, instead of adding multiple of them on the same
GameObject. This helps withdiscoverability.