Expose roundTripTime, jitter metric for audio and video stream #2173
Conversation
LeviSklaroff
force-pushed
the
addRTT
branch
4 times, most recently
from
6b99d2a
to
7599476
Compare
LeviSklaroff
changed the title
CHANGELOG.md
Outdated
| @@ -9,6 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
|
|
|||
| ### Added | |||
|
|
|||
| - Add `roundTripTime`, `audioUplinkJitter`, and `audioDownlinkJitter` to observable metrics | |||
There was a problem hiding this comment.
Does this require any browser specific documentation or existing doc updates?
There was a problem hiding this comment.
We don't have any doc for metricsDidReceive event for WebRTC metrics. I think it's fine.
| @@ -37,7 +37,7 @@ export default class ClientMetricReport { | |||
| }; | |||
|
|
|||
| decoderLossPercent = (metricName?: string, ssrc?: number): number => { | |||
| const metricReport = ssrc ? this.streamMetricReports[ssrc] : this.globalMetricReport; | |||
| const metricReport = this.streamMetricReports[ssrc]; | |||
There was a problem hiding this comment.
This is only for streamMetricReport, and the metric does not exist in globalMetricReport, so removed the code to reduce confusion.
There was a problem hiding this comment.
We have a snippet in our Quality, Bandwidth, and Connectivity guide that we can update. I think it would be good to add some more details on how it works, but I will be reworking this guide in my upcoming change if we wanna wait till then.
| @@ -55,7 +55,7 @@ export default class ClientMetricReport { | |||
| }; | |||
|
|
|||
| packetLossPercent = (sourceMetricName?: string, ssrc?: number): number => { | |||
| const metricReport = ssrc ? this.streamMetricReports[ssrc] : this.globalMetricReport; | |||
| const metricReport = this.streamMetricReports[ssrc]; | |||
There was a problem hiding this comment.
This is only for streamMetricReport, and the metric does not exist in globalMetricReport, so removed the code to reduce confusion.
| @@ -162,7 +162,10 @@ export default class ClientMetricReport { | |||
| transform: this.identityValue, | |||
| type: SdkMetric.Type.VIDEO_AVAILABLE_SEND_BANDWIDTH, | |||
| }, | |||
| currentRoundTripTime: { transform: this.identityValue, type: SdkMetric.Type.STUN_RTT_MS }, | |||
| currentRoundTripTime: { | |||
| transform: this.secondsToMilliseconds, | |||
There was a problem hiding this comment.
The type is named as STUN_RTT_MS, which means the metrics should be converted to millisecond based.
| @@ -175,7 +178,7 @@ export default class ClientMetricReport { | |||
| jitter: { transform: this.secondsToMilliseconds, type: SdkMetric.Type.RTC_MIC_JITTER_MS }, | |||
| packetsSent: { transform: this.countPerSecond, type: SdkMetric.Type.RTC_MIC_PPS }, | |||
| bytesSent: { transform: this.bitsPerSecond, type: SdkMetric.Type.RTC_MIC_BITRATE }, | |||
| roundTripTime: { transform: this.identityValue, type: SdkMetric.Type.RTC_MIC_RTT_MS }, | |||
| roundTripTime: { transform: this.secondsToMilliseconds, type: SdkMetric.Type.RTC_MIC_RTT_MS }, | |||
There was a problem hiding this comment.
The type is named as RTC_MIC_RTT_MS, which means the metrics should be converted to millisecond based.
| @@ -227,7 +230,10 @@ export default class ClientMetricReport { | |||
| source?: string; | |||
| }; | |||
| } = { | |||
| roundTripTime: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_SENT_RTT_MS }, | |||
| roundTripTime: { | |||
| transform: this.secondsToMilliseconds, | |||
There was a problem hiding this comment.
The type is named as VIDEO_SENT_RTT_MS, which means the metrics should be converted to millisecond based.
| @@ -243,6 +249,9 @@ export default class ClientMetricReport { | |||
| qpSum: { transform: this.countPerSecond, type: SdkMetric.Type.VIDEO_SENT_QP_SUM }, | |||
| frameHeight: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_ENCODE_HEIGHT }, | |||
| frameWidth: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_ENCODE_WIDTH }, | |||
| jitter: { | |||
There was a problem hiding this comment.
we already have jitter for audio upstream/downstream and video downstream. So add the same for video upstream.
There is no type for this metrics so leave it empty and do not send to Tincan.
| return source | ||
| ? transform(source, ssrcNum) | ||
| : transform(observableVideoMetricSpec.source, ssrcNum); |
There was a problem hiding this comment.
the video stream related metrics only exists in streamMetricReport and it requires ssrc to locate the corresponding streamMetricReport.
Remove the code path to get metric from globalMetricReport to reduce confusion.
xuesichao
changed the title
Issue #:
RTT and uplink audio jitter were not observable metrics
Description of changes:
Testing:
Can these tested using a demo application? Please provide reproducible step-by-step instructions. No
Checklist:
Have you successfully run
npm run build:releaselocally? YesDo you add, modify, or delete public API definitions? If yes, has that been reviewed and approved? Yes
Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved? No
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.