Skip to content

Conversation

@philipch07
Copy link
Contributor

@philipch07 philipch07 commented Sep 11, 2025

Description

Attempts to add IceCandidatePairStats + updates the respective tests. All feedback is welcome!

Reference issue

Fixes an old-ish issue in webrtc

This is an attempt at completing #721

@codecov
Copy link

codecov bot commented Sep 12, 2025

Codecov Report

❌ Patch coverage is 86.36364% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.57%. Comparing base (1ce9ff1) to head (a2fe985).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
candidatepair.go 75.00% 6 Missing and 2 partials ⚠️
candidate_base.go 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #809   +/-   ##
=======================================
  Coverage   86.56%   86.57%           
=======================================
  Files          43       43           
  Lines        5636     5676   +40     
=======================================
+ Hits         4879     4914   +35     
- Misses        596      600    +4     
- Partials      161      162    +1     
Flag Coverage Δ
go 86.46% <86.36%> (+<0.01%) ⬆️
wasm 33.78% <0.00%> (-0.25%) ⬇️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

nice, thank you @philipch07 :)

@Sean-Der
Copy link
Member

Hopefully isn't too CPU intensive taking the lock every time to get the inbound data. Might have to find a way to make that cheaper, hopefully not :)

@philipch07
Copy link
Contributor Author

philipch07 commented Sep 15, 2025

Hopefully isn't too CPU intensive taking the lock every time to get the inbound data. Might have to find a way to make that cheaper, hopefully not :)

Thanks for the feedback! That's a good point about the locking mechanism: from a quick search, [edit: I was wrong]. I can look into improving this tomorrow when I get back from work!

update: I think the agent.loop.Run() wasn't necessary. The tests still pass when it's removed so I think it should be better now? Let me know if you had something else in mind!

@philipch07 philipch07 force-pushed the pch07/add-ice-candidate-pair-stats branch from bee1e31 to 666311d Compare September 15, 2025 20:07
@philipch07 philipch07 force-pushed the pch07/add-ice-candidate-pair-stats branch from 666311d to a2fe985 Compare September 15, 2025 20:41
Copy link
Member

@JoeTurki JoeTurki left a comment

Choose a reason for hiding this comment

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

cool

@philipch07 philipch07 merged commit 965031e into pion:master Sep 16, 2025
14 checks passed
@danjenkins
Copy link

If this is based off of the original PR it would have been nice to acknowledge that in this PR commit with a co-author. The original PR didn't move forward simply because of no feedback... it's not an issue for me... but imagine someone tries to make their first commit to the project, the PR stalls, then someone comes along and makes a new PR and merges that... not great from a encouragement perspective. @Sean-Der

@philipch07
Copy link
Contributor Author

If this is based off of the original PR it would have been nice to acknowledge that in this PR commit with a co-author. The original PR didn't move forward simply because of no feedback... it's not an issue for me... but imagine someone tries to make their first commit to the project, the PR stalls, then someone comes along and makes a new PR and merges that... not great from a encouragement perspective. @Sean-Der

You're correct, sorry about that. I forgot that it's possible to acknowledge co-authors in commits, so that's my fault. I did refer to this pr and the original issue where applicable, but I agree that someone coming along and being able to get their PR merged in first can be frustrating.

At the same time, the original PR was up for a long time: a little over a year ago. Regardless, I will leave a comment to ask if the author is still around in the future. Thank you for the feedback, I really appreciate it.

@danjenkins
Copy link

Super glad to have this in! Thanks for expanding on the previous PR to get this done!

@philipch07
Copy link
Contributor Author

No problem, and thank you for starting with your initial PR!

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.

Issue accessing ICECandidatePairStats

4 participants