Skip to content

Conversation

@xuesichao
Copy link
Contributor

Issue #:

Description of changes:

  • Add audioSpeakerDelayMs, audioUpstreamRoundTripTimeMs, audioUpstreamJitterMs, audioDownstreamJitterMs and currentRoundTripTimeMs metrics to useMediaStreamMetrics hook. Also add rtcStatsReport to expose the original RTCStatsReport from RTCPeerConnection.getStats().

Testing

  1. Have you successfully run npm run build:release locally?
    Yes

  2. How did you test these changes?
    Change the test demo and use the hook. Check the console log and confirm the metrics are correctly returned.

  3. If you made changes to the component library, have you provided corresponding documentation changes?
    Yes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xuesichao xuesichao requested a review from a team as a code owner April 27, 2022 23:22
availableOutgoingBandwidth,
availableIncomingBandwidth,
videoStreamMetrics,
audioPacketsSentFractionLossPercent: audioPacketLossPercent | 0,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No Math.trunc anymore here? Earlier see that we use Math.trunc.

Copy link
Contributor Author

@xuesichao xuesichao May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I knew this could be confusing so I left comment above to explain this:

// Return 0 if the metric value is NaN, otherwise return its integer part.

The bitwise OR operator truncates the value.
for example:

NaN | 0 = 0
undefined | 0 = 0
1 | 0 = 1
1.1 | 0 = 1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this is hard to understand and readers or contributors may get confused, to keep this simple can we simply use the Math.trunc?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already left comment for this, don't think it's necessary to add extra Math.trunc. I would like to change this if you have strong opinion, but personally I prefer not. Don't think this redundancy benefits too much.

setMediaStreamMetrics({
  audioPacketsSentFractionLossPercent:
    Math.trunc(audioPacketLossPercent) | 0,
  audioPacketsReceivedFractionLossPercent:
    Math.trunc(audioPacketsReceivedFractionLoss) | 0,
  audioSpeakerDelayMs: Math.trunc(audioSpeakerDelayMs) | 0,
  audioUpstreamRoundTripTimeMs:
    Math.trunc(audioUpstreamRoundTripTimeMs) | 0,
  audioUpstreamJitterMs: Math.trunc(audioUpstreamJitterMs) | 0,
  audioDownstreamJitterMs: Math.trunc(audioDownstreamJitterMs) | 0,
  currentRoundTripTimeMs: Math.trunc(currentRoundTripTimeMs) | 0,
  availableOutgoingBandwidth:
    Math.trunc(availableOutgoingBitrate / 1000) | 0,
  availableIncomingBandwidth:
    Math.trunc(availableIncomingBitrate / 1000) | 0,
  rtcStatsReport: clientMetricReport.getRTCStatsReport(),
  videoStreamMetrics: clientMetricReport.getObservableVideoMetrics(),
});
setMediaStreamMetrics({
  audioPacketsSentFractionLossPercent: audioPacketLossPercent | 0,
  audioPacketsReceivedFractionLossPercent:
    audioPacketsReceivedFractionLoss | 0,
  audioSpeakerDelayMs: audioSpeakerDelayMs | 0,
  audioUpstreamRoundTripTimeMs: audioUpstreamRoundTripTimeMs | 0,
  audioUpstreamJitterMs: audioUpstreamJitterMs | 0,
  audioDownstreamJitterMs: audioDownstreamJitterMs | 0,
  currentRoundTripTimeMs: currentRoundTripTimeMs | 0,
  availableOutgoingBandwidth: (availableOutgoingBitrate / 1000) | 0,
  availableIncomingBandwidth: (availableIncomingBitrate / 1000) | 0,
  rtcStatsReport: clientMetricReport.getRTCStatsReport(),
  videoStreamMetrics: clientMetricReport.getObservableVideoMetrics(),
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have to trunc for units in Ms or truncing only applies to the percent metrics?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways fine to keep.

Copy link
Contributor Author

@xuesichao xuesichao May 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have to trunc for units in Ms or truncing only applies to the percent metrics?

In the previous version, all metrics are truncated.

We did something like this:

const { audioPacketLossPercent } = clientMetricReport.getObservableMetrics();

// 1. Set the default value 0
let audioPacketsSentFractionLossPercent = 0;

// 2. Validate metric value
if (isValidMetric(audioPacketLossPercent)) {
  // 3. Truncate the value
  audioPacketsSentFractionLossPercent = Math.trunc(
    audioPacketLossPercent
  );
}

// 4. Return
setMediaStreamMetrics({
  audioPacketsSentFractionLossPercent
)}

It's quite redundant if there are more and more metrics, cause we have to use a local variable to store the default value for every metrics and validate all of them.

The main goal here to use | is to merge the previous step 1, 2 and 3 to simplify the logic.

@xuesichao xuesichao merged commit 20cfe39 into main May 3, 2022
@xuesichao xuesichao deleted the add-metrics branch May 3, 2022 18:13
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

Successfully merging this pull request may close these issues.

2 participants