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

Unclear how the getStats track selector works in the context of RtpSenders. #572

Closed
taylor-b opened this issue Apr 8, 2016 · 16 comments
Closed
Assignees

Comments

@taylor-b
Copy link
Contributor

taylor-b commented Apr 8, 2016

Suppose you have an RtpSender sending track A. It send 100 packets with this track.

Then you call sender.replaceTrack(B), and the sender sends 200 packets from the new track.

Now, if you call pc.getStats(A), what happens? The spec says:

For a track to be a valid selector, it must be a MediaStreamTrack that is sent or received by the RTCPeerConnection object on which the stats request was issued.

So is the promise rejected with an error, since track A is no longer being sent?

Conversely, if you call pc.getStats(B), do you see stats from only the period when the sender was sending track B (packetsSent is 200), or from the entire history of the sender (packetsSent is 300)?

@jan-ivar
Copy link
Member

jan-ivar commented Apr 8, 2016

Spec says for getStats: "If selectorArg is neither null nor a valid selector, return a promise rejected with a TypeError."

@jan-ivar
Copy link
Member

jan-ivar commented Apr 8, 2016

Since track B is the selector (not sender B), I'd expect seeing packets only for track B (not having checked yet what implementations do).

@jan-ivar
Copy link
Member

jan-ivar commented Apr 8, 2016

Maybe we should consider senders (and receivers) as selectors?

@taylor-b
Copy link
Contributor Author

taylor-b commented Apr 8, 2016

That's probably a good idea. But assuming we continue to allow tracks as selectors for backwards compatibility (and in case you do want the per-track stats), I think we'd still want to clarify this behavior.

@vr000m
Copy link

vr000m commented Apr 9, 2016

Typically, if the track is closed, the getstats returns the last stats with the corresponding timestamp.

I'd imagine using the selector as track A, it would return 100 and if the selector is set to track B should return 200.
Although I'm curious if the selector is an rtpsender then it could return both tracks A and B. With different timestamps.

@alvestrand
Copy link
Contributor

I agree with Varun on what's reasonable to return (100 for A, 200 for B). And agree that considering an rtpsender as selector makes sense (and should return 300). We always thought the selector should be multi-type eventually, we chose "track" rather than "any" to keep track of what we thought it should work with.

@taylor-b
Copy link
Contributor Author

So it sounds like people are in agreement that:

  1. If an RtpSender has sent multiple tracks, the browser should only return stats from the period during which it was sending the selector track.
  2. You should be able to obtain stats for previously sent tracks?

This does add some complexity though, as it means the selector doesn't only filter which RTCStats objects are returned; it could also cause some entirely different RTCStats objects to be returned. If I call getStats with no selector, I get an RTCRTPStreamStats for SSRC X. If I call getStats with a track selector, I get an RTCRTPStreamStats for SSRC X while it was sending the relevant track. The two may not be equivalent, so I assume they would have different IDs.

If this is what we want, I think there should be some text explicitly calling this out.

@jan-ivar
Copy link
Member

I agree with 1 (or rather, the RtpSender is irrelevant atm). I don't think anyone needs the complexity of 2.

I find no requirement in the spec that history needs to be kept, or that selector is anything but a filter.

I know there was talk about adding language about getStats being allowed after peer connection has been closed, and Firefox does implement a snapshot at the time of close, because we found it useful as a poor man's "overall call stats", and it didn't add much complexity. Keeping track of old tracks seems much more complex, and I don't think we should do that.

@alvestrand alvestrand self-assigned this Apr 26, 2016
@alvestrand
Copy link
Contributor

@vr000m what do you think? This is a stats issue....

@vr000m
Copy link

vr000m commented Apr 27, 2016

I would like to understand what is the complexity of keeping the last snapshot for all the tracks an RtpSender may have seen.

Consider the following case, where the getStats are polled regularly by a third-party library, which is not the app. The third party does not know that a track was going to be removed. Removing a track may fire an event and the third party library may catch it and poll for the stats for the track. I would love to be able to do that.

@vr000m
Copy link

vr000m commented Apr 27, 2016

re-reading the above. I could live with following:

  1. getStats on a specific RtpSender returns the currently active tracks
  2. getStats on a particular track that was previously sending can return the last dictionary of stats with the timestamp of when it was collected.

This way, the getStats returns active SSRCs, and if needed the third-party library detects some tracks missing, it could fetch the last stats for those missing track.

@vr000m
Copy link

vr000m commented Apr 27, 2016

@alvestrand This is definitely a stats issue, should it be tackled in webrtc-pc or webrtc-stats, is a matter of opinion. On what the selector can be and where should it be documented.

@jan-ivar
Copy link
Member

@vr000m how would (2) work if you remove a track from the connection and add it back later? This seems like a confusing handle.

Maybe it was a mistake to add track as a selector in the first place? We didn't have senders and receivers back then. Tracks are used for all kinds of (even non-peer connection) things, and aren't really specific to transmission.

I'm inclined to switch focus to senders and receivers, and look to deprecating tracks as selectors.

@alvestrand
Copy link
Contributor

Hm.... are we getting to a point where there's a suggestion to drop the selector parameter to getStats()?

@jan-ivar
Copy link
Member

Not from me. I was merely wondering if senders and receivers made for better selectors.

This is moving a little fast for me. In my mind (and implementation), any selector is merely a filter applied to the getStats snowball. This changed with w3c/webrtc-stats#8 (which has already merged) and marks a departure where we're suddenly talking about storing stats for objects that are no longer attached to the peer connection. Does this mean the selector is no longer a filter, or am I to infer that getStats(null) must now include these obsolete snapshots of tracks no longer associated with this peer connection?

This feels odd to implement, since there's no caching of stats in our implementation, all data is live and fresh. The only snapshoting that happens in Firefox is at close, and that's just a grab of the whole snowball at a high level.

It's also odd to see us cementing the snapshot idea without much discussion. It always felt like a hack to me, where these at-close snapshots feel like poor substitutes for stats representing an "overall" call, and I worry people will mistake them for that.

For stats that accumulate, like packets received, the last stat is also the overall stat, but for something like, say, jitter, the last jitter reported may be very different and inconsequential compared to the overall average jitter jitter for the call, if that makes any sense.

@alvestrand
Copy link
Contributor

Closing this issue for now, it seems that we don't want to add those things as selectors at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants