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

Jump to bottom

Use pion/webrtc/v4 #370

Merged
merged 6 commits into from
Oct 23, 2024
Merged

Use pion/webrtc/v4 #370

merged 6 commits into from
Oct 23, 2024

Conversation

edaniels
Copy link
Contributor

@edaniels edaniels commented Oct 9, 2024 •
edited
Loading

I'd suggest moving to v4 since v3 is no longer supported.

@edaniels edaniels requested a review from dgottlieb October 9, 2024 14:58
@viambot viambot added the safe to test Mark as safe to test label Oct 9, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 9, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 9, 2024
@edaniels
Copy link
Contributor Author

edaniels commented Oct 9, 2024

Test failure unrelated

@dgottlieb
Copy link
Member

All of the changes you made in the v3 branch for us are in the v4 branch? Particularly the ones that weren't included in the old v3 branch because they were deemed backwards breaking?

I think the main one being the rtp passthrough related change where OnTrackReceived will be called once the SDP is renegotiated (as opposed to also requiring the first packet to be sent).

@edaniels
Copy link
Contributor Author

All of the changes you made in the v3 branch for us are in the v4 branch? Particularly the ones that weren't included in the old v3 branch because they were deemed backwards breaking?

I think the main one being the rtp passthrough related change where OnTrackReceived will be called once the SDP is renegotiated (as opposed to also requiring the first packet to be sent).

Yeah that RTP one would still need to be addressed here. Everything else in the fork is present in v4.

@edaniels
Copy link
Contributor Author

All of the changes you made in the v3 branch for us are in the v4 branch? Particularly the ones that weren't included in the old v3 branch because they were deemed backwards breaking?
I think the main one being the rtp passthrough related change where OnTrackReceived will be called once the SDP is renegotiated (as opposed to also requiring the first packet to be sent).

Yeah that RTP one would still need to be addressed here. Everything else in the fork is present in v4.

I'm offering up pion/webrtc#2933 to see if we can get it in without simulcast changes (that viam doesn't use).

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 15, 2024
@edaniels edaniels marked this pull request as ready for review October 15, 2024 16:14
@edaniels
Copy link
Contributor Author

@dgottlieb I got the OnTrack bit in as a setting

@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 15, 2024
@viambot viambot added safe to test Mark as safe to test and removed safe to test Mark as safe to test labels Oct 15, 2024
edaniels
edaniels commented Oct 15, 2024
View reviewed changes
rpc/wrtc_base_channel.go
}

func (ch *webrtcBaseChannel) onChannelError(err error) {
if errors.Is(err, sctp.ErrResetPacketInStateNotExist) ||
isUserInitiatedAbortChunkErr(err) {
ch.onChannelClose()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgottlieb this plus a few lines above ends up fixing a case where some tests don't see the channel close with an error "in time"

@edaniels
Copy link
Contributor Author

@dgottlieb do you think we should move forward with this or abandon?

@dgottlieb
Copy link
Member

I'm offering up pion/webrtc#2933 to see if we can get it in without simulcast changes (that viam doesn't use).

@dgottlieb I got the OnTrack bit in as a setting

Oops, I missed this update. That changes the calculus. Just to prepare for how painful this is to propagate/rollback: we really only have to change this repo + rdk? I think app just pulls this in as an indirect dependency?

@edaniels
Copy link
Contributor Author

Yeah it'll be easy to revert. And yes for app too, shouldn't matter.

@edaniels edaniels merged commit 3452a58 into main Oct 23, 2024
8 checks passed
@edaniels edaniels deleted the webrtcv4 branch October 23, 2024 18:49
cheukt added a commit to cheukt/goutils that referenced this pull request Oct 28, 2024
@cheukt
Revert "Use pion/webrtc/v4 (viamrobotics#370)"
9489153 
This reverts commit 3452a58.
cheukt added a commit that referenced this pull request Oct 28, 2024
@cheukt
Revert "Use pion/webrtc/v4 (#370)" (#376)
22fb1e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgottlieb dgottlieb dgottlieb approved these changes

Assignees
No one assigned
Labels
safe to test Mark as safe to test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edaniels @dgottlieb @viambot