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

Fix missed TrackPublished events. #275

Merged
merged 5 commits into from
Jun 22, 2022
Merged

Conversation

davidzhao
Copy link
Member

When initial participant info is sent down along with tracks, we would
miss emitting TrackPublished events for them. This is due an incorrect
assumption that participants would first join without tracks.

Instead of relying on if it's the first update, we now use Room connection
state to determine if those TrackPublished events should be emitted or not.

When initial participant info is sent down along with tracks, we would
miss emitting TrackPublished events for them. This is due an incorrect
assumption that participants would first join without tracks.

Instead of relying on if it's the first update, we now use Room connection
state to determine if those TrackPublished events should be emitted or not.
@changeset-bot
Copy link

changeset-bot bot commented Jun 22, 2022

🦋 Changeset detected

Latest commit: 3c65d06

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
livekit-client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@davidzhao
Copy link
Member Author

found a related bug that I'm fixing together. protocol 8 causes us to fire TrackSubscribed prior to Room connection state going to connected.

@davidliu
Copy link
Contributor

One small thing is that other participant events might fire from updating the info. When implementing for android, participant metadata event fired for me.

@davidzhao
Copy link
Member Author

One small thing is that other participant events might fire from updating the info. When implementing for android, participant metadata event fired for me.

Yeah, I've noticed that too. will fix that too before merging.

@davidzhao davidzhao merged commit 591c218 into main Jun 22, 2022
@davidzhao davidzhao deleted the handle-combined-participant-update branch June 22, 2022 23:54
@github-actions github-actions bot mentioned this pull request Jun 22, 2022
// technically subscribed.
// We'll defer these events until when the room is connected or eventually disconnected.
if (this.state === ConnectionState.Connecting || this.state === ConnectionState.Reconnecting) {
setTimeout(() => {
Copy link
Contributor

@cloudwebrtc cloudwebrtc Jun 27, 2022

Choose a reason for hiding this comment

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

@davidzhao If it is to ensure that the track can be rendered (can receive RTP) when onTrackAdded is triggered, would it be more accurate to listen iceConnectionState instead of delaying 10 ms?

Copy link
Member Author

Choose a reason for hiding this comment

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

right, more precisely, we can wait for ConnectionStateChanged event, and trigger the update when it flips to Connected.

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

Successfully merging this pull request may close these issues.

4 participants