Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #861 +/- ##
==========================================
- Coverage 88.47% 88.39% -0.09%
==========================================
Files 43 44 +1
Lines 5512 5540 +28
==========================================
+ Hits 4877 4897 +20
- Misses 441 446 +5
- Partials 194 197 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // of DTLS packets (embedded in STUN or without embedding). | ||
| type DtlsInStunAckAttribute []uint32 | ||
|
|
||
| // Acks are 32 bit values, the attribute can carry up to four of these. |
There was a problem hiding this comment.
I don't think we have consensus on the maximum length to put in the spec but this matches what is currently in libwebrtc.
776156e to
e18f368
Compare
JoTurk
left a comment
There was a problem hiding this comment.
Hello, All of our stun attributes serialization/deserialization helpers live in stun, why this live in ice and not stun? i think if we move them to stun we can merge that right away, unless there is something i'm missing.
|
We have priority.go (priority), icecontrol.go (controlled/controlling) and usecandidate.go (use-candidate) in this repo though. |
JoTurk
left a comment
There was a problem hiding this comment.
We have priority.go (priority), icecontrol.go (controlled/controlling) and usecandidate.go (use-candidate) in this repo though.
All of these examples are from the library transition d895187#diff-034522ab4c2a896e9b3be8605c36afbb252cbc0648ff9f1378227a484ab34fda I think the reason they were added here was because of the STUN library transition, and this was to make it possible to swap from gortc/stun to pion/stun attributes without a breaking change.
But we also added nomination attribute few month ago which i kinda regret. This is the reason for my comment.
Lines 71 to 86 in 91a3151
The way i see it, is that that the user shouldn't not add another dependency to use attributes helpers, and we should have them in one place in case we wanted to change the interface (like we did to all of our rtp extensions few weeks ago pion/rtp#344)
but i guess we can change this later in the next ICE major release, I'll added it to the issue.
Go ahead with the merge.
defined in pion/stun#260