Mark optional for new constructors and interface in clientMetricReport (

#1304)

* Mark optional for new constructors and interface in clientMetricReport

* Add error log when attendeeId is null and return empty video metrics

CHANGELOG.md

@@ -17,12 +17,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Add support for `WKWebView` on iOS.
 - Output a warning message when the volume adapter cleans up the self-attendee after reconnection.
 - Add FAQ for more information on `AudioJoinFromAnotherDevice` meeting session status code.
-- Add downstream audio webrtc metrics in `observableMetricSpec`
-- Add `getObservableVideoMetrics` and in ClientMetricReport to expose video stream metrics in webrtc
-- Update `SignalingProtocol` with optional video metric fields
+- Add downstream audio webrtc metrics in `observableMetricSpec`.
+- Add `getObservableVideoMetrics` and in `ClientMetricReport` to expose video stream metrics in webrtc.
+- Update `SignalingProtocol` with optional video metric fields.
 
 ### Changed
-- Bump version for lodash, y18n, and ssri dependencies 
+- Bump version for lodash, y18n, and ssri dependencies.
+- Mark `getObservableVideoMetrics` optional in ClientMetricReprt and `videoStreamIndex` and `selfAttendeeId` optional in `DefaultClientMetricReport`.
 
 ### Removed
 

docs/interfaces/clientmetricreport.html

@@ -133,7 +133,7 @@ <h5><span class="tsd-signature-symbol">[</span>id: <span class="tsd-signature-ty
 				</section>
 				<section class="tsd-panel tsd-member tsd-kind-method tsd-parent-kind-interface">
 					<a name="getobservablevideometrics" class="tsd-anchor"></a>
-					<h3>get<wbr>Observable<wbr>Video<wbr>Metrics</h3>
+					<h3><span class="tsd-flag ts-flagOptional">Optional</span> get<wbr>Observable<wbr>Video<wbr>Metrics</h3>
 					<ul class="tsd-signatures tsd-kind-method tsd-parent-kind-interface">
 						<li class="tsd-signature tsd-kind-icon">get<wbr>Observable<wbr>Video<wbr>Metrics<span class="tsd-signature-symbol">(</span><span class="tsd-signature-symbol">)</span><span class="tsd-signature-symbol">: </span><span class="tsd-signature-symbol">{}</span></li>
 					</ul>

src/clientmetricreport/ClientMetricReport.ts

@@ -14,5 +14,5 @@ export default interface ClientMetricReport {
   /**
    * Gets video client media metrics
    */
-  getObservableVideoMetrics(): { [id: string]: { [id: string]: {} } };
+  getObservableVideoMetrics?(): { [id: string]: { [id: string]: {} } };
 }

src/clientmetricreport/DefaultClientMetricReport.ts

@@ -1,7 +1,6 @@
 // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
-import AudioVideoController from '../audiovideocontroller/AudioVideoController';
 import Logger from '../logger/Logger';
 import { SdkMetric } from '../signalingprotocol/SignalingProtocol.js';
 import VideoStreamIndex from '../videostreamindex/VideoStreamIndex';
@@ -20,8 +19,8 @@ export default class DefaultClientMetricReport implements ClientMetricReport {
 
   constructor(
     private logger: Logger,
-    private videoStreamIndex: VideoStreamIndex,
-    private audioVideoController: AudioVideoController
+    private videoStreamIndex?: VideoStreamIndex,
+    private selfAttendeeId?: string
   ) {}
 
   /**
@@ -558,6 +557,12 @@ export default class DefaultClientMetricReport implements ClientMetricReport {
 
   getObservableVideoMetrics(): { [id: string]: { [id: string]: {} } } {
     const videoStreamMetrics: { [id: string]: { [id: string]: {} } } = {};
+    if (!this.videoStreamIndex || !this.selfAttendeeId) {
+      this.logger.error(
+        'Need to define VideoStreamIndex and selfAttendeeId if using getObservableVideoMetrics API'
+      );
+      return videoStreamMetrics;
+    }
     for (const ssrc in this.streamMetricReports) {
       if (this.streamMetricReports[ssrc].mediaType === MediaType.VIDEO) {
         const metric: { [id: string]: number } = {};
@@ -575,7 +580,7 @@ export default class DefaultClientMetricReport implements ClientMetricReport {
         const streamId = this.streamMetricReports[ssrc].streamId;
         const attendeeId = streamId
           ? this.videoStreamIndex.attendeeIdForStreamId(streamId)
-          : this.audioVideoController.configuration.credentials.attendeeId;
+          : this.selfAttendeeId;
         videoStreamMetrics[attendeeId] = videoStreamMetrics[attendeeId]
           ? videoStreamMetrics[attendeeId]
           : {};
@@ -593,7 +598,7 @@ export default class DefaultClientMetricReport implements ClientMetricReport {
     const cloned = new DefaultClientMetricReport(
       this.logger,
       this.videoStreamIndex,
-      this.audioVideoController
+      this.selfAttendeeId
     );
     cloned.globalMetricReport = this.globalMetricReport;
     cloned.streamMetricReports = this.streamMetricReports;

src/statscollector/DefaultStatsCollector.ts

@@ -189,7 +189,7 @@ export default class DefaultStatsCollector implements StatsCollector {
       this.clientMetricReport = new DefaultClientMetricReport(
         this.logger,
         this.videoStreamIndex,
-        this.audioVideoController
+        this.audioVideoController.configuration.credentials.attendeeId
       );
     }
 

test/clientmetricreport/DefaultClientMetricReport.test.ts

@@ -3,7 +3,6 @@
 
 import * as chai from 'chai';
 
-import NoOpAudioVideoController from '../../src/audiovideocontroller/NoOpAudioVideoController';
 import Direction from '../../src/clientmetricreport/ClientMetricReportDirection';
 import MediaType from '../../src/clientmetricreport/ClientMetricReportMediaType';
 import DefaultClientMetricReport from '../../src/clientmetricreport/DefaultClientMetricReport';
@@ -15,12 +14,13 @@ import DefaultVideoStreamIndex from '../../src/videostreamindex/DefaultVideoStre
 describe('DefaultClientMetricReport', () => {
   const expect: Chai.ExpectStatic = chai.expect;
   let clientMetricReport: DefaultClientMetricReport;
+  const selfAttendeeId = 'attendee-1';
 
   beforeEach(() => {
     clientMetricReport = new DefaultClientMetricReport(
       new NoOpDebugLogger(),
       new DefaultVideoStreamIndex(new NoOpDebugLogger()),
-      new NoOpAudioVideoController()
+      selfAttendeeId
     );
   });
 
@@ -400,8 +400,8 @@ describe('DefaultClientMetricReport', () => {
       audioUpstreamReport.currentMetrics['packetsSent'] = 100;
       clientMetricReport.streamMetricReports[audioUpstreamSsrc] = audioUpstreamReport;
       const videoStreamMetrics = clientMetricReport.getObservableVideoMetrics();
-      expect(Object.keys(videoStreamMetrics[''][downstreamSsrc]).length).to.equal(0);
-      expect(Object.keys(videoStreamMetrics[''][upstreamSsrc]).length).to.equal(1);
+      expect(Object.keys(videoStreamMetrics[selfAttendeeId][downstreamSsrc]).length).to.equal(0);
+      expect(Object.keys(videoStreamMetrics[selfAttendeeId][upstreamSsrc]).length).to.equal(1);
     });
 
     it('returns the observable video metrics for streams with streamId', () => {
@@ -410,14 +410,12 @@ describe('DefaultClientMetricReport', () => {
       upstreamReport_1.mediaType = MediaType.VIDEO;
       upstreamReport_1.direction = Direction.UPSTREAM;
       upstreamReport_1.currentMetrics['framesEncoded'] = 100;
-      upstreamReport_1.streamId = 11;
       clientMetricReport.streamMetricReports[upstreamSsrc_1] = upstreamReport_1;
       const upstreamSsrc_2 = 12;
       const upstreamReport_2 = new StreamMetricReport();
       upstreamReport_2.mediaType = MediaType.VIDEO;
       upstreamReport_2.direction = Direction.UPSTREAM;
       upstreamReport_2.currentMetrics['framesEncoded'] = 100;
-      upstreamReport_2.streamId = 12;
       clientMetricReport.streamMetricReports[upstreamSsrc_2] = upstreamReport_2;
       const downstreamSsrc = 22;
       const downstreamReport = new StreamMetricReport();
@@ -429,8 +427,8 @@ describe('DefaultClientMetricReport', () => {
       clientMetricReport.previousTimestampMs = 0;
       const videoStreamMetrics = clientMetricReport.getObservableVideoMetrics();
       expect(Object.keys(videoStreamMetrics[''][downstreamSsrc]).length).to.equal(0);
-      expect(Object.keys(videoStreamMetrics[''][upstreamSsrc_1]).length).to.equal(1);
-      expect(Object.keys(videoStreamMetrics[''][upstreamSsrc_2]).length).to.equal(1);
+      expect(Object.keys(videoStreamMetrics[selfAttendeeId][upstreamSsrc_1]).length).to.equal(1);
+      expect(Object.keys(videoStreamMetrics[selfAttendeeId][upstreamSsrc_2]).length).to.equal(1);
     });
 
     it('returns 0 observable video metrics if no desired report in stream metric reports', () => {
@@ -480,4 +478,18 @@ describe('DefaultClientMetricReport', () => {
       expect(clientMetricReport.streamMetricReports[ssrc1]).to.equal(undefined);
     });
   });
+
+  describe('error logging', () => {
+    it('returns undefined observable video metrics if no VideoStreamIndex and selfAttendeeId defined stream metric reports', () => {
+      clientMetricReport = new DefaultClientMetricReport(new NoOpDebugLogger());
+      const ssrc = 1;
+      const report = new StreamMetricReport();
+      report.mediaType = MediaType.VIDEO;
+      report.direction = Direction.UPSTREAM;
+      report.currentMetrics['framesEncoded'] = 10;
+      clientMetricReport.streamMetricReports[ssrc] = report;
+      const videoStreamMetrics = clientMetricReport.getObservableVideoMetrics();
+      expect(Object.keys(videoStreamMetrics).length).to.equal(0);
+    });
+  });
 });

test/statscollector/DefaultStatsCollector.test.ts

@@ -76,7 +76,7 @@ describe('DefaultStatsCollector', () => {
     clientMetricReport = new DefaultClientMetricReport(
       logger,
       new DefaultVideoStreamIndex(logger),
-      audioVideoController
+      audioVideoController.configuration.credentials.attendeeId
     );
   });
 

test/task/MonitorTask.test.ts

@@ -383,11 +383,7 @@ describe('MonitorTask', () => {
       const clientMetricReport = new TestClientMetricReport();
       task.metricsDidReceive(clientMetricReport);
 
-      const defaultClientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const defaultClientMetricReport = new DefaultClientMetricReport(logger);
       const ssrc = 6789;
       defaultClientMetricReport.streamMetricReports[ssrc] = new StreamMetricReport();
       task.metricsDidReceive(defaultClientMetricReport);
@@ -402,11 +398,7 @@ describe('MonitorTask', () => {
       streamMetricReport.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReport.direction = ClientMetricReportDirection.DOWNSTREAM;
 
-      const clientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new DefaultClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       task.metricsDidReceive(clientMetricReport);
     });
@@ -434,11 +426,7 @@ describe('MonitorTask', () => {
       streamMetricReportAudio.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReportAudio.direction = ClientMetricReportDirection.UPSTREAM;
 
-      const clientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new DefaultClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       clientMetricReport.streamMetricReports[56789] = streamMetricReportAudio;
       task.metricsDidReceive(clientMetricReport);
@@ -477,11 +465,7 @@ describe('MonitorTask', () => {
       streamMetricReportAudio.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReportAudio.direction = ClientMetricReportDirection.UPSTREAM;
 
-      const clientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new DefaultClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       clientMetricReport.streamMetricReports[56789] = streamMetricReportAudio;
       task.metricsDidReceive(clientMetricReport);
@@ -503,11 +487,7 @@ describe('MonitorTask', () => {
       streamMetricReport.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReport.direction = ClientMetricReportDirection.DOWNSTREAM;
 
-      const clientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new DefaultClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       task.metricsDidReceive(clientMetricReport);
     });
@@ -527,11 +507,7 @@ describe('MonitorTask', () => {
       streamMetricReport.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReport.direction = ClientMetricReportDirection.DOWNSTREAM;
 
-      const clientMetricReport = new DefaultClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new DefaultClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       task.metricsDidReceive(clientMetricReport);
     });
@@ -607,11 +583,7 @@ describe('MonitorTask', () => {
       streamMetricReportAudio.mediaType = ClientMetricReportMediaType.VIDEO;
       streamMetricReportAudio.direction = ClientMetricReportDirection.UPSTREAM;
 
-      const clientMetricReport = new TestClientMetricReport(
-        logger,
-        context.videoStreamIndex,
-        context.audioVideoController
-      );
+      const clientMetricReport = new TestClientMetricReport(logger);
       clientMetricReport.streamMetricReports[1] = streamMetricReport;
       clientMetricReport.streamMetricReports[56789] = streamMetricReportAudio;
       task.metricsDidReceive(clientMetricReport);

test/videodownlinkbandwidthpolicy/AllHighestVideoBandwidthPolicy.test.ts

@@ -3,7 +3,6 @@
 
 import * as chai from 'chai';
 
-import NoOpAudioVideoController from '../../src/audiovideocontroller/NoOpAudioVideoController';
 import DefaultClientMetricReport from '../../src/clientmetricreport/DefaultClientMetricReport';
 import LogLevel from '../../src/logger/LogLevel';
 import NoOpLogger from '../../src/logger/NoOpLogger';
@@ -221,11 +220,7 @@ describe('AllHighestVideoBandwidthPolicy', () => {
         })
       );
 
-      const metricReport = new DefaultClientMetricReport(
-        logger,
-        index,
-        new NoOpAudioVideoController()
-      );
+      const metricReport = new DefaultClientMetricReport(logger);
       metricReport.globalMetricReport.currentMetrics['googAvailableReceiveBandwidth'] = 1000;
 
       policy.updateIndex(index);

test/videodownlinkbandwidthpolicy/NoVideoDownlinkBandwidthPolicy.test.ts

@@ -3,7 +3,6 @@
 
 import * as chai from 'chai';
 
-import NoOpAudioVideoController from '../../src/audiovideocontroller/NoOpAudioVideoController';
 import DefaultClientMetricReport from '../../src/clientmetricreport/DefaultClientMetricReport';
 import LogLevel from '../../src/logger/LogLevel';
 import NoOpLogger from '../../src/logger/NoOpLogger';
@@ -32,11 +31,7 @@ describe('NoVideoDownlinkBandwidthPolicy', () => {
       expect(policy.wantsResubscribe()).to.be.false;
       policy.updateIndex(emptyVideoStreamIndex);
       expect(policy.wantsResubscribe()).to.be.false;
-      const metricReport = new DefaultClientMetricReport(
-        logger,
-        emptyVideoStreamIndex,
-        new NoOpAudioVideoController()
-      );
+      const metricReport = new DefaultClientMetricReport(logger);
       metricReport.globalMetricReport.currentMetrics['googAvailableReceiveBandwidth'] = 500;
       policy.updateMetrics(metricReport);
       expect(policy.wantsResubscribe()).to.be.false;