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

Report encode/decode time and implementation metric #2434

Merged
merged 2 commits into from
Sep 16, 2022
Merged

Report encode/decode time and implementation metric #2434

merged 2 commits into from
Sep 16, 2022

Conversation

shi-su
Copy link
Contributor

@shi-su shi-su commented Sep 13, 2022

add implementation name as a dimension.

Issue #:

Description of changes:
Report encode/decode time and if encoder/decoder is in hardware, and add encoder/decoder name as a metric dimension

Testing:
Added unit tests to cover the changes

Can these tested using a demo application? Please provide reproducible step-by-step instructions.
Join a meeting with Chrome and select log level as DEBUG, turn on video on one user. Use the following string to filter console logs [DEBUG] SDK - sending:. Verify the log contains the following fields on sender and receiver side, respectively.

Sender side:

  1. Message has VIDEO_ENCODE_MS metric field. Example:
{"type":"VIDEO_ENCODE_MS","value":49.850448654035056}
  1. Message has VIDEO_ENCODER_NAME dimension. Example:
"dimensions":[{"type":"VIDEO_ENCODER_NAME","value":{"stringValue":"OpenH264"}}]

Message example:

2022-09-20T23:25:14.763Z [DEBUG] SDK - sending: {"timestampMs":"1663716314763","type":"CLIENT_METRIC","clientMetric":{"globalMetrics":[{"type":"STUN_RTT_MS","value":83},{"type":"SOCKET_DISCARDED_PPS","value":0},{"type":"VIDEO_AVAILABLE_SEND_BANDWIDTH","value":2200176}],"streamMetricFrames":[{"metrics":[{"type":"RTC_SPK_JITTER_MS","value":5},{"type":"RTC_SPK_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"RTC_SPK_PPS","value":49},{"type":"RTC_SPK_BITRATE","value":25547}]},{"metrics":[{"type":"RTC_MIC_PPS","value":49},{"type":"RTC_MIC_BITRATE","value":24263},{"type":"RTC_MIC_JITTER_MS","value":20},{"type":"RTC_MIC_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"RTC_MIC_RTT_MS","value":62}]},{"metrics":[{"type":"VIDEO_SENT_PPS","value":161},{"type":"VIDEO_SENT_BITRATE","value":1420115},{"type":"VIDEO_ENCODE_FPS","value":14},{"type":"VIDEO_ENCODE_MS","value":49.850448654035056},{"type":"VIDEO_ENCODE_WIDTH","value":960},{"type":"VIDEO_ENCODE_HEIGHT","value":540},{"type":"VIDEO_INPUT_FPS","value":15},{"type":"VIDEO_FIRS_RECEIVED","value":0},{"type":"VIDEO_PLIS_RECEIVED","value":0},{"type":"VIDEO_NACKS_RECEIVED","value":0},{"type":"VIDEO_SENT_QP_SUM","value":231},{"type":"VIDEO_SENT_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"VIDEO_SENT_RTT_MS","value":92}],"dimensions":[{"type":"VIDEO_ENCODER_NAME","value":{"stringValue":"OpenH264"}}]}]}}

Receiver side:

  1. Message has VIDEO_DECODE_MS metric field. Example:
{"type":"VIDEO_DECODE_MS","value":40.120361083249755}
  1. Message has VIDEO_DECODER_NAME dimension. Example:
"dimensions":[{"type":"VIDEO_DECODER_NAME","value":{"stringValue":"ExternalDecoder (VDAVideoDecoder)"}}]

Message example:

2022-09-20T23:21:21.581Z [DEBUG] SDK - sending: {"timestampMs":"1663716081581","type":"CLIENT_METRIC","clientMetric":{"globalMetrics":[{"type":"STUN_RTT_MS","value":70},{"type":"SOCKET_DISCARDED_PPS","value":0},{"type":"VIDEO_AVAILABLE_SEND_BANDWIDTH","value":300000},{"type":"VIDEO_AVAILABLE_RECEIVE_BANDWIDTH","value":1335167}],"streamMetricFrames":[{"metrics":[{"type":"RTC_SPK_JITTER_MS","value":3},{"type":"RTC_SPK_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"RTC_SPK_PPS","value":49},{"type":"RTC_SPK_BITRATE","value":19875}]},{"streamId":1,"metrics":[{"type":"VIDEO_RECEIVED_JITTER_MS","value":41},{"type":"VIDEO_RECEIVED_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"VIDEO_RECEIVED_PPS","value":149},{"type":"VIDEO_RECEIVED_BITRATE","value":1315979},{"type":"VIDEO_RECEIVED_FPS","value":75},{"type":"VIDEO_DECODE_FPS","value":15},{"type":"VIDEO_DECODE_MS","value":40.120361083249755},{"type":"VIDEO_FIRS_SENT","value":0},{"type":"VIDEO_PLIS_SENT","value":0},{"type":"VIDEO_NACKS_SENT","value":0},{"type":"VIDEO_DECODE_WIDTH","value":960},{"type":"VIDEO_DECODE_HEIGHT","value":540}],"dimensions":[{"type":"VIDEO_DECODER_NAME","value":{"stringValue":"ExternalDecoder (VDAVideoDecoder)"}}]},{"metrics":[{"type":"RTC_MIC_PPS","value":50},{"type":"RTC_MIC_BITRATE","value":25853},{"type":"RTC_MIC_JITTER_MS","value":19.25},{"type":"RTC_MIC_FRACTION_PACKET_LOST_PERCENT","value":0},{"type":"RTC_MIC_RTT_MS","value":63}]}]}}

Checklist:

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

  2. Do you add, modify, or delete public API definitions? If yes, has that been reviewed and approved?
    n

  3. Do you change the wire protocol, e.g. the request method? If yes, has that been reviewed and approved?
    n

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

@shi-su shi-su force-pushed the report_hw branch 2 times, most recently from 2103581 to c731c87 Compare September 13, 2022 17:56
@shi-su shi-su marked this pull request as ready for review September 13, 2022 20:25
@shi-su shi-su requested a review from a team as a code owner September 13, 2022 20:25
Copy link
Contributor

@hensmi-amazon hensmi-amazon left a comment

Choose a reason for hiding this comment

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

lgtm just some naming nits

@@ -4,4 +4,5 @@
export default class GlobalMetricReport {
previousMetrics: { [id: string]: number } = {};
currentMetrics: { [id: string]: number } = {};
stringValues: { [id: string]: string } = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe currentDimensions or just dimensions instead of stringValues?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think they are just dimension, it can also be metric derived from string value. For example, whether decoder/encoder is in hardware is one of the derived metric.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to currentStringMetrics. Also added comments that this is a workaround in case metric values are strings.

@@ -10,4 +10,5 @@ export default class StreamMetricReport {
direction: Direction;
previousMetrics: { [id: string]: number } = {};
currentMetrics: { [id: string]: number } = {};
stringValues: { [id: string]: string } = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to currentStringMetrics. Also added comments that this is a workaround in case metric values are strings.

@@ -297,6 +333,14 @@ export default class ClientMetricReport {
},
frameHeight: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_DECODE_HEIGHT },
frameWidth: { transform: this.identityValue, type: SdkMetric.Type.VIDEO_DECODE_WIDTH },
totalDecodeTime: {
Copy link
Contributor

Choose a reason for hiding this comment

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

should this name be changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This key is just about the field name in webrtc report where the metric is derived from, we cannot change that naming as it comes from webrtc..

/**
* Dimensions derived from metric
*/
readonly streamMetricDimensionMap: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Don't think we need 'Metric' in any of this or related naming?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated naming as suggested.

Copy link
Contributor

@hensmi-amazon hensmi-amazon left a comment

Choose a reason for hiding this comment

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

lgtm

@shi-su shi-su merged commit 9d7d94e into main Sep 16, 2022
@shi-su shi-su deleted the report_hw branch September 16, 2022 19:39
@@ -126,6 +126,35 @@ export default class ClientMetricReport {
return Number(metricReport.currentMetrics[metricName] * 1000);
};

millisecondsPerSecond = (metricName?: string, ssrc?: number): number => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we name this to be clearer what it does? milliseconds per second is kinda confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. Created #2443 to update function naming

manasisurve pushed a commit that referenced this pull request Sep 19, 2022
Report encode/decode time and if encoder/decoder is in hardware, and add encoder/decoder name as a metric dimension

Co-authored-by: Shi Su <[email protected]>
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.

4 participants