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]Missing tracks #507

Merged
merged 3 commits into from
Sep 9, 2024

Conversation

ipavlidakis
Copy link
Collaborator

🔗 Issue Links

Provide all JIRA tickets and/or GitHub issues related to this PR, if applicable.

🎯 Goal

Describe why we are making this change.

📝 Summary

Provide bullet points with the most important changes in the codebase.

🛠 Implementation

Provide a detailed description of the implementation and explain your decisions if you find them relevant.

🎨 Showcase

Add relevant screenshots and/or videos/gifs to easily see what this PR changes, if applicable.

Before After
img img

🧪 Manual Testing Notes

Explain how this change can be tested manually, if applicable.

☑️ Contributor Checklist

  • I have signed the Stream CLA (required)
  • This change follows zero ⚠️ policy (required)
  • This change should receive manual QA
  • Changelog is updated with client-facing changes
  • New code is covered by unit tests
  • Comparison screenshots added for visual changes
  • Affected documentation updated (Docusaurus, tutorial, CMS)

🎁 Meme

Provide a funny gif or image that relates to your work on this pull request. (Optional)

@ipavlidakis ipavlidakis added the bug Something isn't working label Sep 3, 2024
@ipavlidakis ipavlidakis added this to the Reconnection v2 milestone Sep 3, 2024
@ipavlidakis ipavlidakis self-assigned this Sep 3, 2024
@ipavlidakis ipavlidakis requested a review from a team as a code owner September 3, 2024 18:04
Copy link

github-actions bot commented Sep 3, 2024

1 Warning
⚠️ Please be sure to complete the Contributor Checklist in the Pull Request description

Generated by 🚫 Danger

@ipavlidakis ipavlidakis marked this pull request as draft September 4, 2024 08:24
Copy link
Collaborator

@martinmitrevski martinmitrevski 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 one question about handling local users. Will also test this branch tomorrow.

var updatedParticipants = self.participants

guard
participant.sessionId != sessionID,
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, but if we exclude the current participant, how are we going to update their track visibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's an interesting one. Let me discuss the 2 different things we are doing here:

  • Update track.isEnabled
    By doing that on the local user ... it disables the track which means that we are not sending any more data to remote participants. I have no idea yet why that was working before.

  • Update participant.showTrack
    I have that disabled for now as it's causing some flickering that i need to investigate (probably due to something causing multiple redraws)

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Update track.isEnabled: but isn't it expected that we don't send any more data if the flag is false?

@ipavlidakis ipavlidakis force-pushed the feature/reconnection-v2 branch from 1e4964d to 6224a22 Compare September 4, 2024 15:10
@ipavlidakis ipavlidakis force-pushed the reconnection-v2/missing-tracks branch from 6ab22af to a69c4f9 Compare September 5, 2024 10:47
@Stream-SDK-Bot
Copy link
Collaborator

Stream-SDK-Bot commented Sep 5, 2024

SDK Size

title develop branch diff status
StreamVideo 7.4 MB 7.85 MB +454 KB 🟡
StreamVideoSwiftUI 2.18 MB 2.18 MB -2 KB 🚀
StreamVideoUIKit 2.32 MB 2.31 MB -2 KB 🚀
StreamWebRTC 8.69 MB 8.69 MB 0 KB 🟢

@ipavlidakis ipavlidakis force-pushed the feature/reconnection-v2 branch from 210e99b to feda9bc Compare September 6, 2024 08:58
@ipavlidakis ipavlidakis force-pushed the reconnection-v2/missing-tracks branch 2 times, most recently from 570fba3 to a346d07 Compare September 6, 2024 09:52
@ipavlidakis ipavlidakis force-pushed the reconnection-v2/missing-tracks branch from a346d07 to 4184a43 Compare September 6, 2024 13:17
@ipavlidakis ipavlidakis marked this pull request as ready for review September 6, 2024 18:13
Copy link

sonarqubecloud bot commented Sep 9, 2024

@ipavlidakis ipavlidakis merged commit 81bdc54 into feature/reconnection-v2 Sep 9, 2024
14 checks passed
@ipavlidakis ipavlidakis deleted the reconnection-v2/missing-tracks branch September 9, 2024 11:31
ipavlidakis added a commit that referenced this pull request Sep 9, 2024
ipavlidakis added a commit that referenced this pull request Sep 18, 2024
ipavlidakis added a commit that referenced this pull request Sep 19, 2024
ipavlidakis added a commit that referenced this pull request Sep 24, 2024
ipavlidakis added a commit that referenced this pull request Sep 24, 2024
ipavlidakis added a commit that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants