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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix glare related regressions #1851

Merged
merged 7 commits into from Aug 16, 2021
Merged

Conversation

SimonBrandner
Copy link
Contributor

@SimonBrandner SimonBrandner commented Aug 15, 2021

Fixes element-hq/element-web#18538
Requires matrix-org/matrix-react-sdk#6614
Type: defect

(will make a PR to release after approval)


Here's what your changelog entry will look like:

馃悰 Bug Fixes

Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
If we can get localUsermediaStream gotUserMediaForAnswer() has alredy been called before

Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner SimonBrandner requested a review from a team as a code owner August 15, 2021 11:06
@github-actions github-actions bot added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Aug 15, 2021
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Also I get the general principle but unsure specifically why this would have caused glare to break - would have been good to explain this in the PR description. I spent a while staring at the changes trying to work out what was related to glare, before realising when I looked at the individual commits.

src/webrtc/callEventHandler.ts Show resolved Hide resolved
Signed-off-by: 艩imon Brandner <simon.bra.ag@gmail.com>
@SimonBrandner
Copy link
Contributor Author

SimonBrandner commented Aug 16, 2021

Also I get the general principle but unsure specifically why this would have caused glare to break - would have been good to explain this in the PR description. I spent a while staring at the changes trying to work out what was related to glare, before realising when I looked at the individual commits.

Sorry, about that, I was a bit tired of this when making the PR, so I somehow didn't do that... The problem on the js-sdk side was that it tried to add a track to an undefined peerConn. On the react-sdk side, the problem was that I didn't properly handle call changes which lead to VideoFeeds having an undefined feed prop

The other changes were the things that I discovered on the way which confused me

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Ah - makes sense, thanks

@dbkr dbkr merged commit ccf296e into matrix-org:develop Aug 16, 2021
@SimonBrandner SimonBrandner deleted the fix/glare/18538 branch August 16, 2021 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VoIP glare is broken.
2 participants