webrtc
webrtc copied to clipboard
webrtc-rs
Ability to retrieve stats from a peer connection
I see that the stats module exists, but does not have any content. I was hoping to contribute a pull request adding RTP stats, and wanted to make sure that 1) this is not stepping on work that someone else is doing, and 2) I implement it in a way that is likely to get merged.
I was planning to follow the model of Pion, using stats.go and GetStats as the basis for the API.
This would work out as an async fn get_stats(&self) -> StatsReport, with I'm sure some intervening modules along the way. Does this seem correct?
/cc @mickel8
Take a look at https://github.com/webrtc-rs/webrtc/pull/180. The are still some incomplete parts to that, and it would be helpful to have a review of it for consistency and quality.
This is really neat!
Should we have this behind a feature flag since it sounds like it'll potentially introduce more locking in potentially performance critical code paths?
It is opt-in via the requirement to call get_stats on the peer connection, but that does bring up the question of whether more of this code should be in threads. That said, my guess is that this is pretty quick since it's just collecting up existing data.
Great, I think as long as there's no overhead in the performance critical code paths it's fine for stats to not be feature flagged.
I don't expect the collecting code itself needs to be in a thread(or more appropriately tokio task) if the entry point is async users can always spawn a task for it themselves.
Hey @sax are you blocked on releases to continue this work at the moment? I seem to recall seeing something like that somewhere. If so I should be able to help with that
I am currently blocked on the ice crate being released, to move forward on serialization.
What's the best way to discuss possible implementations? Is this issue a good place, or is there a slack or discord? There are a few keys where I'm unclear where to get the data, and I'm sure a few minutes chat would clear things up.
There's a discord but I also think it's worthwhile to capture at least the final details in the issue too. Feel free to join the Discord and we can hammer out the details there.
Just wanted to add a comment here to reflect some conversation on the discord server.
@k0nserv suggested that bytes sent/received can be gotten from AgentConn through the Agent. I've done this in https://github.com/webrtc-rs/ice/pull/30, and it's reflected in new commits to https://github.com/webrtc-rs/webrtc/pull/189.
I've also switched reports to be a HashMap<String, StatsReportType>, and to serialize the StatsReport to its reports... that seems more in line with Pion while retaining the intermediate type.
That might be enough to allow serialization to be merged (once a new version of the ice crate is released)... one thing is that there are a bunch of extra allocations for cloning the HashMap keys. My understanding is that if I change all the String usages to &str it would reduce allocations at the cost of requiring lifetime indicators everywhere. Maybe that's worth the added code complexity?
That might be enough to allow serialization to be merged (once a new version of the ice crate is released)... one thing is that there are a bunch of extra allocations for cloning the HashMap keys. My understanding is that if I change all the String usages to &str it would reduce allocations at the cost of requiring lifetime indicators everywhere. Maybe that's worth the added code complexity?
&'static str is definitely the better choice. I'm not sure you'd need &'static str in that many places, just when declaring the reports type and in function arguments. "Hello" is implicitly 'static.
&'static str is definitely the better choice. I'm not sure you'd need &'static str in that many places, just when declaring the reports type and in function arguments. "Hello" is implicitly 'static.
Hmm, looking at the code it seems some value are dynamic i.e. they have to be String. For example, RTCRtpCodecParameters::stats_id. The only option then is Cow<str> but that seems overkill, let's keep String.