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

Add ice transport api to get selected pair stats #2923

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

boks1971
Copy link
Contributor

@boks1971 boks1971 commented Oct 7, 2024

In use cases like SFU, it is useful to get just the selected candidate pair stats to have access to current RTT on the peer connection. The standard has a way to do GetSelectedCandidatePair on ICETransport, but does not have a way to get stats of that pair.

Although not in standard, adding a method to ICETransport to get selected candidate pair along similar lines of above method.

Copy link

codecov bot commented Oct 7, 2024

Codecov Report

Attention: Patch coverage is 84.31373% with 8 lines in your changes missing coverage. Please review.

Project coverage is 78.97%. Comparing base (64f32d2) to head (70504e4).

Files with missing lines Patch % Lines
icegatherer.go 57.14% 4 Missing and 2 partials ⚠️
stats.go 94.28% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2923      +/-   ##
==========================================
+ Coverage   78.92%   78.97%   +0.05%     
==========================================
  Files          89       89              
  Lines        8568     8586      +18     
==========================================
+ Hits         6762     6781      +19     
  Misses       1315     1315              
+ Partials      491      490       -1     
Flag Coverage Δ
go 80.51% <84.31%> (+0.05%) ⬆️
wasm 63.79% <0.00%> (-0.68%) ⬇️

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.

@boks1971 boks1971 force-pushed the raja_selected_candidate_pair_stats branch from 3851bc2 to b5efa6c Compare October 7, 2024 08:03
In use cases like SFU, it is useful to get just the selected candidate
pair stats to have access to current RTT on the peer connection. The
standard has a way to do `GetSelectedCandidatePair` on `ICETransport`,
but does not have a way to get stats of that pair.

Although not in standard, adding a method to `ICETransport` to get
selected candidate pair along similar lines of above method.
@boks1971 boks1971 force-pushed the raja_selected_candidate_pair_stats branch from b5efa6c to 5a44fc7 Compare October 7, 2024 08:18
if err != nil {
g.log.Error(err.Error())
continue
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A slight change in behaviour here. Consolidate the conversion from ice.CandidatePairStats -> webrtc.ICECandidatePairStats in stats.go so that it can be use both here and in getSelectedCandidatePairStats() 👇 . In fact, linter flagged it for repeated code 🎉 .

Previously, code continued to add stats after just logging the error. Now, it does not. The consolidation code in stats.go returns an empty stats if there is an error here.

I think the state error should never happen. There is also a comment in code like

    default:                                                                                                                                                    
        // NOTE: this should never happen[tm]                                                                                                                   
        err := fmt.Errorf("%w: %s", errStatsICECandidateStateInvalid, state.String())                                                                           
        return StatsICECandidatePairState("Unknown"), err

So, just skipping adding the stat on error.

@Sean-Der
Copy link
Member

Sean-Der commented Oct 7, 2024

Love how you did this @boks1971 :)

Zero API break, re-used existing structures and 'feels ORTC-like'!

@boks1971
Copy link
Contributor Author

boks1971 commented Oct 7, 2024

Thank you @Sean-Der !!!

@boks1971 boks1971 merged commit dc1f8ff into master Oct 7, 2024
18 checks passed
@boks1971 boks1971 deleted the raja_selected_candidate_pair_stats branch October 7, 2024 15:30
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.

2 participants