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

Map out implementation status of getStats and make a plan #595

Closed
henbos opened this issue Feb 9, 2021 · 24 comments
Closed

Map out implementation status of getStats and make a plan #595

henbos opened this issue Feb 9, 2021 · 24 comments
Assignees

Comments

@henbos
Copy link
Collaborator

henbos commented Feb 9, 2021

Good source of reference: https://webrtc-stats.callstats.io/verify/

@vr000m @alvestrand @dontcallmedom

dontcallmedom added a commit to w3c/webrtc-interop-reports that referenced this issue Feb 10, 2021
Recast data in https://webrtc-stats.callstats.io/verify/ to determine how it informs webrtc-stats implementation status
see also w3c/webrtc-stats#595
@dontcallmedom
Copy link
Member

dontcallmedom commented Feb 10, 2021

I've built a basic analysis of the callstats.io report to see how it informs us about what's in the spec and what's actually implemented: https://w3c.github.io/webrtc-interop-reports/webrtc-stats-report.html

Before diving into analysis of what's implemented or not, the first thing to note is that the callstats.io report does not seem aligned with the hierarchy in the spec - the first list in the document above shows which fields of which types from the spec aren't listed in the report. In particular, it's missing data on the high level types data-channel, transceiver, sctp-transport and ice-server.

I'm not sure if we should look into fixing the callstats.io report (if that's even a possibility) or if we should look at taking the approach that was taken for MTI stats in WPT and extend it to the full stats hierarchy (and then based our further analysis on the data collected in WPT.fyi).

Thoughts?

@dontcallmedom
Copy link
Member

I have submitted a test for WPT that would achieve what I describe above web-platform-tests/wpt#27571 (running it locally, it shows 133 stats not implemented in Chrome, 174 implemented, although I recall there are at least a few where the cursory check of presence of the member won't take into account cases in WebRTC stats where presence is dependent on certain conditions being met)

@dontcallmedom
Copy link
Member

dontcallmedom commented Feb 10, 2021

Results from the said test in WPT.fyi: Chromes passes 177/308, FF 88/308, Safari 158/308

@dontcallmedom
Copy link
Member

I've updated the report mentioned above with data collected from the WPT test run.

In the end, the following stats seems to have no implementation (or at least no implementation detected by the test):

  • unimplemented stats types:
    • csrc
    • transceiver
    • sender
    • receiver
    • sctp-transport
    • ice-server
  • unimplemented stats fields:
    • codec:
      • codecType
    • inbound-rtp:
      • packetsDiscarded
      • packetsRepaired
      • burstPacketsLost
      • burstPacketsDiscarded
      • burstLossCount
      • burstDiscardCount
      • burstLossRate
      • burstDiscardRate
      • gapLossRate
      • gapDiscardRate
      • partialFramesLost
      • fullFramesLost
      • frameBitDepth
      • voiceActivityFlag
      • averageRtcpInterval
      • packetsFailedDecryption
      • packetsDuplicated
      • perDscpPacketsReceived
      • sliCount
      • totalProcessingDelay
      • estimatedPlayoutTimestamp
      • totalSamplesDecoded
      • samplesDecodedWithSilk
      • samplesDecodedWithCelt
    • outbound-rtp:
      • senderId
      • rid
      • lastPacketSentTimestamp
      • packetsDiscardedOnSend
      • bytesDiscardedOnSend
      • fecPacketsSent
      • targetBitrate
      • frameBitDepth
      • framesDiscardedOnSend
      • totalSamplesSent
      • samplesEncodedWithSilk
      • samplesEncodedWithCelt
      • voiceActivityFlag
      • averageRtcpInterval
      • qualityLimitationDurations
      • perDscpPacketsSent
      • sliCount
    • remote-inbound-rtp:
      • packetsDiscarded
      • packetsRepaired
      • burstPacketsLost
      • burstPacketsDiscarded
      • burstLossCount
      • burstDiscardCount
      • burstLossRate
      • burstDiscardRate
      • gapLossRate
      • gapDiscardRate
      • framesDropped
      • partialFramesLost
      • fullFramesLost
      • totalRoundTripTime
      • fractionLost
      • reportsReceived
      • roundTripTimeMeasurements
    • remote-outbound-rtp:
      • transportId
      • codecId
      • reportsSent
    • media-source:
      • relayedSource
      • echoReturnLoss
      • echoReturnLossEnhancement
      • bitDepth
      • frames
    • peer-connection:
      • dataChannelsRequested
      • dataChannelsAccepted
    • transport:
      • rtcpTransportStatsId
      • iceRole
      • iceLocalUsernameFragment
      • iceState
      • tlsGroup
    • candidate-pair:
      • packetsSent
      • packetsReceived
      • firstRequestTimestamp
      • lastRequestTimestamp
      • lastResponseTimestamp
      • availableIncomingBitrate
      • circuitBreakerTriggerCount
      • retransmissionsReceived
      • retransmissionsSent
      • consentExpiredTimestamp
      • packetsDiscardedOnSend
      • bytesDiscardedOnSend
      • requestBytesSent
      • consentRequestBytesSent
      • responseBytesSent
    • local-candidate:
      • url
      • relayProtocol
    • remote-candidate:
      • url
      • relayProtocol
    • certificate:
      • issuerCertificateId

A good next steps would be to map these gaps with implementations plans (e.g. related bugs in browser trackers); I also suspect some additional semantic grouping of the fields above might help move forward some of the conversations.

@vr000m
Copy link
Contributor

vr000m commented Feb 15, 2021

We should definitely talk about the unimplemented statsTypes before we go down to the key values of the other statsTypes.

  • csrc
  • transceiver
  • sender
  • receiver
  • sctp-transport
  • ice-server

@henbos @jan-ivar any thoughts on these objects? I am assuming the senders and receivers are not implemented because we still have tracks implemented?

@henbos
Copy link
Collaborator Author

henbos commented Feb 16, 2021

I think we can "prune the tree" by removing sender, receiver and transceiver. Their purpose is to describe which object the RTP stream belongs to, but the *-rtp stats object could simply have a mid and trackIdentifier attribute and then sender/receiver/transceiver does not provide any additional information and doing the additional lookup is just cumbersome.

The remaining metrics fall into the category of "they have a definition, someone might want to implement them sometimes, but nobody is committed to it at the moment". This is true both for the missing dictionaries and the missing metrics inside of the dictionaries.

@vr000m
Copy link
Contributor

vr000m commented Feb 16, 2021

csrc we can get the data from getContributingSources, @karthikbr82 can you confirm this?

@henbos
Copy link
Collaborator Author

henbos commented Mar 3, 2021

FYI remote-inbound-rtp.totalRoundTripTime, roundTripTimeMeasurements and fractionLost just got implemented by an external contributor :)

@henbos
Copy link
Collaborator Author

henbos commented Mar 3, 2021

csrc we can get the data from getContributingSources, @karthikbr82 can you confirm this?

Yes so I think we should delete this one?

@alvestrand
Copy link
Contributor

There are mulitple RTP streams per sender and receiver. I get nervous about trying to represent senders and receivers as implicit objects that only exist as "these RTP streams are all pointing to the same track". If a track is replaced, and the SDP is with a legacy endpoint that doesn't support MID, how do we tell that the RTP stream is still belonging to the same sender as before?

(Receivers are less problematic since the track<->receiver attachment is permanent)

@alvestrand
Copy link
Contributor

And in the no-mid case, we can't tell which RTP streams are connected to the same media section unless we have transceivers.

@henbos
Copy link
Collaborator Author

henbos commented Mar 4, 2021

Are you saying it's possible to have negotiated transceivers that don't have a mid attribute?

@alvestrand
Copy link
Contributor

Yes, if the other end doesn't offer mids.
We may be synthesizing mids for this case; I'll have to check the specs (and implemenation!) for that.

@vr000m
Copy link
Contributor

vr000m commented Mar 18, 2021

Any updates on the mid issue?

@vr000m vr000m self-assigned this Apr 27, 2021
@henbos
Copy link
Collaborator Author

henbos commented Jun 22, 2021

Step 1: Implement "sender" and "receiver" in order to expose trackIdentifier for inbound-rtp.
Step 2: Deprecate "track" stats in implementations.
Step 3: Revisit remaining metrics and either make obsolete or implement.

@vr000m
Copy link
Contributor

vr000m commented Jun 26, 2021

In Step 3, My recommendation would be to mark as "yet to be implemented" instead of obsolete for the metrics that are not implemented. Obsolete to me means that they should not be implemented, which is not the case for these yet to be implemented metrics.

@alvestrand
Copy link
Contributor

If nobody's going to be implementing them, they should be marked obsolete ("maybe a good idea, but nobody cared enough to do it"). Otherwise, we should implement them.
Step 3 may take a while.

@henbos
Copy link
Collaborator Author

henbos commented Jun 27, 2021

”Feature at risk” is another name for it if you want to avoid the term ”obsolete”

@alvestrand
Copy link
Contributor

alvestrand commented Jun 28, 2021 via email

@henbos
Copy link
Collaborator Author

henbos commented Jun 28, 2021

Implementation step 1: Implement "sender" and "receiver" in order to expose trackIdentifier for inbound-rtp.
Implementation step 2: Deprecate "track" stats in implementations.
Spec step 1: Move unimplemented metrics into a Feature at risk section.
Spec step 2: Possibly revisit metrics in the future based on implementation and document status.

@henbos
Copy link
Collaborator Author

henbos commented Apr 7, 2022

Aaaand we have a plan: #621 / #622

Let's close this issue in favor of that one and iterate?

@dontcallmedom
Copy link
Member

Following the good work done in #622, I have updated my implementation report to match the latest data from WPT, taking into account some of the additional input gathered in #622.

I still have the following stats for which I'm not clear whether they're implemented in Chromium: remote-inbound-rtp.framesDropped, remote-outbound-rtp.roundTripTime. @henbos / @fippo?

There are a bunch that seems to be only implemented in Chromium based on the WPT supported stats test (although some of these may be false positives due to timing / environment issues, similar to some of the situations documented for Chromium in my manual annotations). @jan-ivar @youennf any chance you could help review the list of "single implementation" stats and share a sense of whether they are in fact implemented, under development, or not on the roadmap?

@henbos
Copy link
Collaborator Author

henbos commented Aug 30, 2022

I think the spec is in a pretty good state now, though we should sanity check by going over them again. Also I filed #660 as a follow-up since some metrics are kind-specific in the implementations.

Regarding the implementation report (nice work!), the status on "without implementation" metrics AFAIK:

To me, the big issue is "when to delete RTP metrics?" (spec says never, implementation deletes as soon as a stream goes away). I ramble about it in #643 but we may want to split that up into separate creation vs destruction issues to avoid wall of text.

@henbos
Copy link
Collaborator Author

henbos commented May 30, 2023

Closing this one in favor of more actionable smaller issues, the stats spec is in a pretty good state now

@henbos henbos closed this as completed May 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants