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.

Sign up for GitHub

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

Fix ICE reconnect flashings #3930

Merged
merged 7 commits into from
Mar 9, 2021
Merged

Fix ICE reconnect flashings #3930

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
590d32a
Fix ICE reconnect flashings
keianhzo Feb 18, 2021
ab1dd01
Move NAF adapter events to EventEmitter
keianhzo Feb 23, 2021
53a0462
Replace track when updating audio stream in audio source
keianhzo Feb 23, 2021
5edbbab
getPeers -> refreshConsumers
keianhzo Feb 24, 2021
da873fc
add/remove track doesn't work, using a node graph better
keianhzo Feb 25, 2021
6d45372
Do not handle disconnected state
keianhzo Feb 25, 2021
827f9e2
PR review fixes
keianhzo Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 41 additions & 8 deletions src/components/avatar-audio-source.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,24 @@ function createSilentAudioEl(stream) {
return audioEl;
}

async function getMediaStream(el) {
async function getOwnerId(el) {
const networkedEl = await NAF.utils.getNetworkedEntity(el).catch(e => {
console.error(INFO_INIT_FAILED, INFO_NO_NETWORKED_EL, e);
});
if (!networkedEl) {
return null;
}
const ownerId = networkedEl.components.networked.data.owner;
if (!ownerId) {
return networkedEl.components.networked.data.owner;
}

async function getMediaStream(el) {
const peerId = await getOwnerId(el);
if (!peerId) {
console.error(INFO_INIT_FAILED, INFO_NO_OWNER);
return null;
}
const stream = await NAF.connection.adapter.getMediaStream(ownerId).catch(e => {
console.error(INFO_INIT_FAILED, `Error getting media stream for ${ownerId}`, e);
const stream = await NAF.connection.adapter.getMediaStream(peerId).catch(e => {
console.error(INFO_INIT_FAILED, `Error getting media stream for ${peerId}`, e);
});
if (!stream) {
return null;
Expand Down Expand Up @@ -72,10 +76,13 @@ AFRAME.registerComponent("avatar-audio-source", {
createSilentAudioEl(stream); // TODO: Do the audio els need to get cleaned up?
}

const mediaStreamSource = audio.context.createMediaStreamSource(stream);
audio.setNodeSource(mediaStreamSource);
this.destination = audio.context.createMediaStreamDestination();
this.mediaStreamSource = audio.context.createMediaStreamSource(stream);
const destinationSource = audio.context.createMediaStreamSource(this.destination.stream);
this.mediaStreamSource.connect(this.destination);
audio.setNodeSource(destinationSource);
this.el.setObject3D(this.attrName, audio);
this.el.emit("sound-source-set", { soundSource: mediaStreamSource });
this.el.emit("sound-source-set", { soundSource: destinationSource });
},

destroyAudio() {
Expand All @@ -88,9 +95,34 @@ AFRAME.registerComponent("avatar-audio-source", {

init() {
this.el.sceneEl.systems["hubs-systems"].audioSettingsSystem.registerAvatarAudioSource(this);
// We subscribe to audio stream notifications for this peer to update the audio source
// This could happen in case there is an ICE failure that requires a transport recreation.
NAF.connection.adapter.on("stream_updated", this._onStreamUpdated, this);
this.createAudio();
},

async _onStreamUpdated(peerId, kind) {
const audio = this.el.getObject3D(this.attrName);
if (!audio) return;
const stream = audio.source.mediaStream;
if (!stream) return;

getOwnerId(this.el).then(async ownerId => {
if (ownerId === peerId && kind === "audio") {
// The audio stream for this peer has been updated
const newStream = await NAF.connection.adapter.getMediaStream(peerId, "audio").catch(e => {
console.error(INFO_INIT_FAILED, `Error getting media stream for ${peerId}`, e);
});

if (newStream) {
keianhzo marked this conversation as resolved.
Show resolved Hide resolved
this.mediaStreamSource.disconnect();
this.mediaStreamSource = audio.context.createMediaStreamSource(newStream);
this.mediaStreamSource.connect(this.destination);
}
}
});
},

update(oldData) {
if (this.isCreatingAudio) return;

Expand All @@ -108,6 +140,7 @@ AFRAME.registerComponent("avatar-audio-source", {

remove: function() {
this.el.sceneEl.systems["hubs-systems"].audioSettingsSystem.unregisterAvatarAudioSource(this);
NAF.connection.adapter.off("stream_updated", this._onStreamUpdated);
this.destroyAudio();
}
});
18 changes: 18 additions & 0 deletions src/components/media-views.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,23 @@ AFRAME.registerComponent("media-video", {
if (url.startsWith("hubs://")) {
const streamClientId = url.substring(7).split("/")[1]; // /clients/<client id>/video is only URL for now
const stream = await NAF.connection.adapter.getMediaStream(streamClientId, "video");
// We subscribe to video stream notifications for this peer to update the video element
// This could happen in case there is an ICE failure that requires a transport recreation.
if (this._onStreamUpdated) {
NAF.connection.adapter.off("stream_updated", this._onStreamUpdated);
}
this._onStreamUpdated = async (peerId, kind) => {
keianhzo marked this conversation as resolved.
Show resolved Hide resolved
if (peerId === streamClientId && kind === "video") {
// The video stream for this peer has been updated
const stream = await NAF.connection.adapter.getMediaStream(peerId, "video").catch(e => {
console.error(`Error getting video stream for ${peerId}`, e);
});
if (stream) {
videoEl.srcObject = new MediaStream(stream);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi! I discovered this doesn't work unless you also call videoEl.play(), because if the track dies the video is stopped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this happen in all browsers? Can you provide STRs? I can can't really reproduce it with current master in FF/Chrome.

The stream_update event in media-view.js and avatar-audio-source.js should only be as a consequence of a receive transport recreation, otherwise both components are not yet created so the stream_update event is not triggered on them. Maybe we are are missing some case where it's being triggered? I haven't found any but having STRs would really help.

A receive transport recreation should only happen in case where the transport has been closed but signaling is still opened. The transport disconnected/failed states are handled by the WebRTC stack and we should recover without any consumer recreation or transport updates whatsoever (so no stream_update events).

I couldn't also reproduce the issue mentioned above. How are you triggering it?

}
}
};
NAF.connection.adapter.on("stream_updated", this._onStreamUpdated, this);
videoEl.srcObject = new MediaStream(stream.getVideoTracks());
// If hls.js is supported we always use it as it gives us better events
} else if (contentType.startsWith("application/dash")) {
Expand Down Expand Up @@ -944,6 +961,7 @@ AFRAME.registerComponent("media-video", {
if (this.video) {
this.video.removeEventListener("pause", this.onPauseStateChange);
this.video.removeEventListener("play", this.onPauseStateChange);
NAF.connection.adapter.off("stream_updated", this._onStreamUpdated);
}

if (this.hoverMenu) {
Expand Down
Loading