Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
==========================================
- Coverage 81.54% 81.42% -0.13%
==========================================
Files 91 91
Lines 4688 4689 +1
==========================================
- Hits 3823 3818 -5
- Misses 691 694 +3
- Partials 174 177 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
I rebased and simplifed this PR. |
| ssrc: header.SSRC, | ||
| sequenceNumber: header.SequenceNumber, | ||
| ecn: 0, // ECN is not supported (yet). | ||
| ecn: uint8(ecn), |
There was a problem hiding this comment.
we could convert the ecn field to rtcp.ECN, too, because that is what it will be converted back to when we build the report.
There was a problem hiding this comment.
It's unfortunate because the public API uses uint8 Recorder.AddPacket(..., ecn uint8) I can store it as rtcp.ECN but we'll have a mismatch in types until we break the public API.
There was a problem hiding this comment.
I'll do that and move the cast to AddPacket but the other side. Until we can break Recorder.AddPacket
There was a problem hiding this comment.
Done we now cast 3 times instead of 1 before :) but we can get rid of them once we break the public API :)
There was a problem hiding this comment.
Oh, sorry, I didn't realize that API is public. I'm not sure if there are any downsides of having rtcp.ECN in the public API of this package. If not, I'm fine with breaking it at any time since we're still v0. (I'd also be fine with unexporting the recorder, but I guess that may be really annoying if anyone is using it already...).
There was a problem hiding this comment.
searched both package names "github.com/pion/interceptor/pkg/rfc8888" "NewRecorder(" language:Go -repo:pion/interceptor and it doesn't seem that anyone is using this API, I'll break it :)
There was a problem hiding this comment.
It might break the deprecated RFC8888 package that just redirects here, but that should be fine, too.
There was a problem hiding this comment.
yeah it broke both rfc8888 and ccfb, but removed the casts, from a quick github search nobody used this API from the old package or the new.
There was a problem hiding this comment.
We could just drop rfc8888 already, but we can also do it separately from this PR.
da4ffd3 to
8fd942e
Compare
3 participants
Just updated
ecn's type fromuint8tortcp.ECNplus actually reading the ECN value from the interceptor attributes.