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

Parse Candidate Extensions (RFC5245) #754

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Conversation

JoeTurki
Copy link
Member

@JoeTurki JoeTurki commented Jan 14, 2025

Description

  • Rewrote UnmarshalCandidate to better align with RFC5245.
  • Added new methods for extracting candidate extensions.
  • Updated Equal and Marshal to accommodate these changes.
  • Added tests for everything.
  • Fixed a "bug" where tcptype would appear before raddr .
  • Made a minor change to gather.go; because we can't use the config struct as a map key anymore.
  • Added CandidateExtension.

I'm aware this is a breaking change but this change is required for many issues like pion/webrtc#2993

Edit: I'll make it non breaking change tomorrow.

Reference issue

#753

@JoeTurki JoeTurki requested review from edaniels and Sean-Der January 14, 2025 18:10
Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.01%. Comparing base (abdc0ca) to head (ab6e243).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #754      +/-   ##
==========================================
+ Coverage   78.63%   80.01%   +1.37%     
==========================================
  Files          41       41              
  Lines        4769     5023     +254     
==========================================
+ Hits         3750     4019     +269     
+ Misses        785      775      -10     
+ Partials      234      229       -5     
Flag Coverage Δ
go 80.01% <100.00%> (+1.37%) ⬆️
wasm 27.89% <100.00%> (+4.79%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JoeTurki JoeTurki marked this pull request as draft January 14, 2025 18:18
Copy link

@nils-ohlmeier nils-ohlmeier left a comment

Choose a reason for hiding this comment

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

Just a few minor suggestions. But looks good otherwise.

@Sean-Der
Copy link
Member

Nice, love these improvements :)

Glad we can do this without API break. It means people don’t update and new versions don’t get improved :(

@JoeTurki JoeTurki force-pushed the add/candidate-extensions branch 2 times, most recently from a2e7c06 to 6528f3e Compare January 15, 2025 03:21
@JoeTurki JoeTurki marked this pull request as ready for review January 15, 2025 03:30
@JoeTurki
Copy link
Member Author

I added CandidateExtensions and CandidateExtension, And fixed all the other issues.

@Sean-Der @nils-ohlmeier let me know if there are other problems :)

Thanks.

@JoeTurki JoeTurki linked an issue Jan 15, 2025 that may be closed by this pull request
Copy link

@nils-ohlmeier nils-ohlmeier left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Member

@Sean-Der Sean-Der left a comment

Choose a reason for hiding this comment

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

Fantastic! This is great.

Sorry about the stress of APIs. It’s just so hard to fix them. We can always fix/improve code though.

Its nice to expose as little as possible until someone requests it :)

@JoeTurki JoeTurki force-pushed the add/candidate-extensions branch from 2b64677 to dddadd5 Compare January 15, 2025 06:19
@JoeTurki
Copy link
Member Author

@Sean-Der When you have a time can you take a look.

Sorry about the stress of APIs. It’s just so hard to fix them. We can always fix/improve code though.

It's not stressing at all for me :)

Its nice to expose as little as possible until someone requests it :)

This makes sense, Noted :)

@JoeTurki JoeTurki requested a review from Sean-Der January 16, 2025 00:26
@Sean-Der
Copy link
Member

@JoeTurki sorry one last change to keep return types consistent. After that merge it! If we do have bugs/improvements to make we call roll forward :)

sorry for all the rounds on this.

@Sean-Der
Copy link
Member

no need to wait for another approval for me

- Rewrote `UnmarshalCandidate` to better align with RFC5245.
- Added Candidate `Extensions` and `GetExtension`.
- Updated `Equal` and `Marshal` to accommodate these changes.
- New Type `CandidateExtension` to handle.
@JoeTurki JoeTurki force-pushed the add/candidate-extensions branch from ac1224f to ab6e243 Compare January 16, 2025 03:38
@JoeTurki JoeTurki merged commit ab6e243 into master Jan 16, 2025
15 checks passed
@JoeTurki JoeTurki deleted the add/candidate-extensions branch January 16, 2025 03:44
@JoeTurki
Copy link
Member Author

@Sean-Der
Hopefully, this is my first API change of many :)

I now have a better understanding of how changes are made to the Pion API, I'll take all feedback into consideration to try to make future iterations shorter.

Thanks!

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

Successfully merging this pull request may close these issues.

Add Support for Parsing Extensions in Candidates
4 participants