JSDK-1510 Implementing the Bandwidth Constraints example app. #25

manjeshbhargav · 2017-09-19T00:32:01Z

This PR contains the implementation of the Bandwidth Constraints Example.

Screenshot:

markandrus · 2017-09-19T15:49:42Z

README.md

@@ -45,8 +45,10 @@ video in both the tabs!

 ## Examples

-The project contains some common use-case examples for the Twilio Video JS SDK.
+The project contains some common use-case examples for the Twilio Video JS SDK. After running the application
+by following the instructions above, click on these links to run the examples.


I think this indicates we ought to find a way to include these links in the app, e.g. a navigation bar or something?

@markandrus I'll add an examples/index.html page linking to all the examples.

markandrus · 2017-09-19T15:50:48Z

examples/bandwidthconstraints/src/index.js

+  if (!canvas) {
+    waveformContainer.appendChild(waveform.element);
+  }
+}


We need to start factoring this stuff out, otherwise it will get difficult to maintain, IMO.

@markandrus I've already moved Waveform to the utils folder.

How about our "attach"/"detach" helpers? Can these be made common?

@markandrus They're not entirely common. In this case, we're also attaching the Track to the Waveform module, whereas in other examples we are not. And I also don't want too much of the app logic to be modularized as it affects the flow of reading the code.

markandrus · 2017-09-19T15:51:28Z

examples/bandwidthconstraints/src/index.js

+  // display the media of the second Participant that will enter
+  // the Room with bandwidth constraints.
+  return Video.connect(creds.token, { tracks: [] });
+}).then(function(_room) {


I think we can use async/await now. This also lets us avoid binding _room.

@markandrus Do Firefox and Safari support it?

Yes, recent versions support it.

markandrus · 2017-09-19T15:52:33Z

examples/util/getroomcredentials.js

+        resolve(JSON.parse(xhr.responseText));
+      }
+    };
+    xhr.send(null);


Can we use fetch instead?

@markandrus Do Firefox and Safari support it?

markandrus · 2017-09-19T15:53:23Z

Looks good. What about indicating to the user that the constraints took effect? E.g., by reading getStats? Will it visually be clear enough that the video is at a lower bitrate? Audibly?

manjeshbhargav · 2017-09-19T16:05:04Z

@markandrus I'm working on adding the bitrate graphs. I'm thinking of leveraging the graph module used by the webrtc sample app demonstrating bandwidth constraints: https://github.com/webrtc/samples/blob/gh-pages/src/js/third_party/graph.js
WDYT?

manjeshbhargav · 2017-09-19T23:09:31Z

@markandrus I've updated the PR with the following changes:

Used async/await in the Bandwidth Constraints example app.
Used fetch() for getting the Room token.
Added examples/index.html that lists the example apps' links.
Added bitrate graphs in the Bandwidth Constraints example app.

Screenshot of examples/index.html:

Screenshot of the bitrate filters:

markandrus · 2017-09-21T13:19:57Z

examples/bandwidthconstraints/src/index.js

+var helpers = require('./helpers');
+var waveform = require('../../util/waveform');
+var connectWithBandwidthConstraints = helpers.connectWithBandwidthConstraints;
+var updateBandwidthConstraints = helpers.updateBandwidthConstraints;


Can you also use const and let instead of var? If async/await is supported, so is const and let.

markandrus · 2017-09-21T13:20:59Z

examples/bandwidthconstraints/src/index.js

+  // Connect to a random Room with no media. This Participant will
+  // display the media of the second Participant that will enter
+  // the Room with bandwidth constraints.
+  var _room = await Video.connect(creds.token, { tracks: [] });


Why are we binding _room instead of assigning to room?

Ah, why not randomRoom or someRoom?

markandrus · 2017-09-21T13:22:17Z

examples/bandwidthconstraints/src/index.js

+async function connectToOrDisconnectFromRoom(e) {
+  e.preventDefault();
+  if (room) {
+    e.target.value = 'Connect to Room';


You already have connectOrDisconnect, can you set connectOrDisconnect.value instead of e.target?

markandrus · 2017-09-21T13:22:30Z

examples/bandwidthconstraints/src/index.js

+/**
+ * Connect to or disconnect from a Room.
+ */
+async function connectToOrDisconnectFromRoom(e) {


I think event is better than e here.

markandrus · 2017-09-21T13:23:27Z

examples/bandwidthconstraints/src/index.js

+}
+
+/**
+ * Connect to or disconnect from a Room.


I think it is better to have two functions, connectToRoom, and disconnectFromRoom, then let connectToRoom reassign the "click" event handler of to disconnectFromRoom and vice-versa.

markandrus · 2017-09-21T13:23:58Z

examples/bandwidthconstraints/src/index.js

+        ? stats[0].remoteAudioTrackStats[0]
+        : stats[0].remoteVideoTrackStats[0]
+      var _bytesReceived = remoteTrackStats.bytesReceived;
+      var _timestamp = remoteTrackStats.timestamp;


Why are we binding _bytesReceiver and _timestamp?

@markandrus We are not binding these values. These are just the current values, which will be stored in their non-underscored counterpart variables to serve as previous values for the next bitrate calculation.

markandrus · 2017-09-21T13:55:09Z

examples/util/getroomcredentials.js

+function getRoomCredentials() {
+  return fetch('/token').then(function(response) {
+    return response.json();
+  });


Can use fat-arrow functions and async/await. Basically, I would like us to use all the modern syntax since it is widely available now, e.g.

async function getRoomCredentials() { const response = await fetch('/token') return response.json(); }

markandrus · 2017-09-22T09:11:32Z

examples/util/timelinegraph.js

+ *  Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source


We need to add/update our LICENSE.md file. It can take the same form as https://github.com/twilio/twilio-video.js/blob/master/LICENSE.md with a couple changes:

First section: Twilio license

Second section: the WebRTC license

markandrus · 2017-09-22T09:12:22Z

One comment, otherwise looks good 👍

manjeshbhargav force-pushed the bandwidth-constraints branch from dd0402e to add2bef Compare September 19, 2017 07:49

JSDK-1510 Implementing the Bandwidth Constraints example app.

add2bef

markandrus reviewed Sep 19, 2017

View reviewed changes

JSDK-1510 Adding bitrate graphs to Bandwidth Constraints example app.

cf19a0a

manjeshbhargav force-pushed the bandwidth-constraints branch 4 times, most recently from da00882 to 94792f0 Compare September 19, 2017 23:04

JSDK-1510 Adding examples page, using fetch() and using async/await.

94792f0

markandrus reviewed Sep 21, 2017

View reviewed changes

JSDK-1510 Using const/let and other refactorings.

8f0ebfb

markandrus reviewed Sep 22, 2017

View reviewed changes

JSDK-1510 Updating LICENSE.md.

1cf49cb

manjeshbhargav merged commit b77b816 into twilio:master Sep 22, 2017

manjeshbhargav deleted the bandwidth-constraints branch September 22, 2017 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSDK-1510 Implementing the Bandwidth Constraints example app. #25

JSDK-1510 Implementing the Bandwidth Constraints example app. #25

manjeshbhargav commented Sep 19, 2017

markandrus Sep 19, 2017

manjeshbhargav Sep 19, 2017

markandrus Sep 19, 2017

manjeshbhargav Sep 19, 2017

markandrus Sep 19, 2017

manjeshbhargav Sep 19, 2017

markandrus Sep 19, 2017

manjeshbhargav Sep 19, 2017

markandrus Sep 19, 2017

markandrus Sep 19, 2017

manjeshbhargav Sep 19, 2017

markandrus Sep 19, 2017

markandrus commented Sep 19, 2017

manjeshbhargav commented Sep 19, 2017

manjeshbhargav commented Sep 19, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 21, 2017

manjeshbhargav Sep 21, 2017

markandrus Sep 21, 2017

markandrus Sep 22, 2017

markandrus commented Sep 22, 2017

JSDK-1510 Implementing the Bandwidth Constraints example app. #25

JSDK-1510 Implementing the Bandwidth Constraints example app. #25

Conversation

manjeshbhargav commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markandrus commented Sep 19, 2017

manjeshbhargav commented Sep 19, 2017

manjeshbhargav commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markandrus commented Sep 22, 2017