webrtc-stats icon indicating copy to clipboard operation
webrtc-stats copied to clipboard

Map out implementation status of getStats and make a plan

Open henbos opened this issue 4 years ago • 23 comments

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

@vr000m @alvestrand @dontcallmedom

henbos avatar Feb 09 '21 14:02 henbos

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 avatar Feb 10 '21 12:02 dontcallmedom

I have submitted a test for WPT that would achieve what I describe above https://github.com/web-platform-tests/wpt/pull/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 avatar Feb 10 '21 16:02 dontcallmedom

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

dontcallmedom avatar Feb 10 '21 17:02 dontcallmedom

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.

dontcallmedom avatar Feb 11 '21 15:02 dontcallmedom

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?

vr000m avatar Feb 15 '21 11:02 vr000m

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.

henbos avatar Feb 16 '21 14:02 henbos

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

vr000m avatar Feb 16 '21 14:02 vr000m

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

henbos avatar Mar 03 '21 15:03 henbos

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

Yes so I think we should delete this one?

henbos avatar Mar 03 '21 15:03 henbos

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 avatar Mar 04 '21 14:03 alvestrand

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

alvestrand avatar Mar 04 '21 14:03 alvestrand

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

henbos avatar Mar 04 '21 14:03 henbos

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.

alvestrand avatar Mar 04 '21 16:03 alvestrand

Any updates on the mid issue?

vr000m avatar Mar 18 '21 13:03 vr000m

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.

henbos avatar Jun 22 '21 13:06 henbos

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.

vr000m avatar Jun 26 '21 13:06 vr000m

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.

alvestrand avatar Jun 26 '21 19:06 alvestrand

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

henbos avatar Jun 27 '21 17:06 henbos

"Feature at risk" is a fine name for the state where we haven't decided between "implemented" and "obsolete".

On Sun, Jun 27, 2021 at 7:01 PM henbos @.***> wrote:

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

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/w3c/webrtc-stats/issues/595#issuecomment-869194568, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADVM7LAIW432R3RTOCCAC3TU5KN5ANCNFSM4XLEFG7Q .

alvestrand avatar Jun 28 '21 14:06 alvestrand

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 avatar Jun 28 '21 14:06 henbos

Aaaand we have a plan: #621 / #622

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

henbos avatar Apr 07 '22 15:04 henbos

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?

dontcallmedom avatar Aug 30 '22 11:08 dontcallmedom

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:

  • remote-inbound-rtp.framesDropped: While framesDropped is implemented for inbound-rtp, this does not exist in remote-inbound-rtp. I filed #661 to make spec match.
  • remote-outbound-rtp.roundTripTime: It looks like chromium sets it for audio according to code search but I was unable to repro in a WPT so not sure how to trigger it being set... @alvestrand any ideas?
  • certificate.issuerCertificateId: There is code for it, but I don't know how to trigger it in practise. @alvestrand know if its possible?

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 avatar Aug 30 '22 12:08 henbos

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

henbos avatar May 30 '23 13:05 henbos