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

Displays the region participants are connected to #3451

Merged
merged 5 commits into from
Sep 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions lang/main.json
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,9 @@
"connectionindicator":
{
"header": "Connection data",
"connectedTo": "Connected to:",
"bitrate": "Bitrate:",
"bridgeCount": "Server count: ",
"packetloss": "Packet loss:",
"resolution": "Resolution:",
"framerate": "Frame rate:",
Expand Down
22 changes: 14 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"jsc-android": "224109.1.0",
"jsrsasign": "8.0.12",
"jwt-decode": "2.2.0",
"lib-jitsi-meet": "github:jitsi/lib-jitsi-meet#3f46c64f4f373d3b573fcc55b59568dbe9b9d51f",
"lib-jitsi-meet": "github:jitsi/lib-jitsi-meet#1ac6df97e3aa5ff880129a95754d491d89ea8c25",
"libflacjs": "github:mmig/libflac.js#93d37e7f811f01cf7d8b6a603e38bd3c3810907d",
"lodash": "4.17.4",
"moment": "2.19.4",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,18 +410,21 @@ class ConnectionIndicator extends Component {
const {
bandwidth,
bitrate,
bridgeCount,
e2eRtt,
framerate,
packetLoss,
region,
resolution,
serverRegion,
transport
} = this.state.stats;

return (
<ConnectionStatsTable
bandwidth = { bandwidth }
bitrate = { bitrate }
bridgeCount = { bridgeCount }
connectionSummary = { this._getConnectionStatusTip() }
e2eRtt = { e2eRtt }
framerate = { framerate }
Expand All @@ -430,6 +433,7 @@ class ConnectionIndicator extends Component {
packetLoss = { packetLoss }
region = { region }
resolution = { resolution }
serverRegion = { serverRegion }
shouldShowMore = { this.state.showMoreStats }
Copy link
Contributor

Choose a reason for hiding this comment

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

Dang, maybe at this point, not saying for this PR, ConnectionStatsTable should just take in the stats object itself or we use a spread operator to pass in all stat values ({ ...this.state.stats })or have ConnectionStatsTable listen instead of just ConnectionIndicator.

transport = { transport } />
);
Expand Down
18 changes: 9 additions & 9 deletions react/features/connection-indicator/statsEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,33 +116,33 @@ const statsEmitter = {
* also update listeners of remote user stats of changes related to their
* stats.
*
* @param {string} currentUserId - The user id for the local user.
* @param {string} localUserId - The user id for the local user.
* @param {Object} stats - Connection stats for the local user as provided
* by the library.
* @returns {void}
*/
_onStatsUpdated(currentUserId: string, stats: Object) {
_onStatsUpdated(localUserId: string, stats: Object) {
const allUserFramerates = stats.framerate || {};
const allUserResolutions = stats.resolution || {};

// FIXME resolution and framerate are hashes keyed off of user ids with
// FIXME resolution and framerate are maps keyed off of user ids with
// stat values. Receivers of stats expect resolution and framerate to
// be primatives, not hashes, so overwrites the 'lib-jitsi-meet' stats
// objects.
// be primitives, not maps, so here we override the 'lib-jitsi-meet'
// stats objects.
const modifiedLocalStats = Object.assign({}, stats, {
framerate: allUserFramerates[currentUserId],
resolution: allUserResolutions[currentUserId]
framerate: allUserFramerates[localUserId],
resolution: allUserResolutions[localUserId]
});

this._emitStatsUpdate(currentUserId, modifiedLocalStats);
this._emitStatsUpdate(localUserId, modifiedLocalStats);

// Get all the unique user ids from the framerate and resolution stats
// and update remote user stats as needed.
const framerateUserIds = Object.keys(allUserFramerates);
const resolutionUserIds = Object.keys(allUserResolutions);

_.union(framerateUserIds, resolutionUserIds)
.filter(id => id !== currentUserId)
.filter(id => id !== localUserId)
.forEach(id => {
const remoteUserStats = {};

Expand Down
79 changes: 72 additions & 7 deletions react/features/connection-stats/components/ConnectionStatsTable.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,12 @@ class ConnectionStatsTable extends Component {
*/
bitrate: PropTypes.object,

/**
* The number of bridges (aka media servers) currently used in the
* conference.
*/
bridgeCount: PropTypes.number,

/**
* A message describing the connection quality.
*/
Expand Down Expand Up @@ -71,7 +77,7 @@ class ConnectionStatsTable extends Component {
packetLoss: PropTypes.object,

/**
* The region.
* The region that we think the client is in.
*/
region: PropTypes.string,

Expand All @@ -86,6 +92,11 @@ class ConnectionStatsTable extends Component {
*/
resolution: PropTypes.object,

/**
* The region of the media server that we are connected to.
*/
serverRegion: PropTypes.string,

/**
* Whether or not additional stats about bandwidth and transport should
* be displayed. Will not display even if true for remote participants.
Expand Down Expand Up @@ -124,7 +135,7 @@ class ConnectionStatsTable extends Component {

/**
* Creates a table as ReactElement that will display additional statistics
* related to bandwidth and transport.
* related to bandwidth and transport for the local user.
*
* @private
* @returns {ReactElement}
Expand All @@ -135,6 +146,7 @@ class ConnectionStatsTable extends Component {
<tbody>
{ this._renderBandwidth() }
{ this._renderTransport() }
{ this._renderRegion() }
</tbody>
</table>
);
Expand Down Expand Up @@ -226,23 +238,74 @@ class ConnectionStatsTable extends Component {
* @private
*/
_renderE2eRtt() {
const { e2eRtt, region, t } = this.props;
let str = e2eRtt ? `${e2eRtt.toFixed(0)}ms` : 'N/A';
const { e2eRtt, t } = this.props;
const str = e2eRtt ? `${e2eRtt.toFixed(0)}ms` : 'N/A';

if (region) {
str += ` (${region})`;
return (
<tr>
<td>
<span>{ t('connectionindicator.e2e_rtt') }</span>
</td>
<td>{ str }</td>
</tr>
);
}

/**
* Creates a table row as a ReactElement for displaying the "connected to"
* information.
*
* @returns {ReactElement}
* @private
*/
_renderRegion() {
const { region, serverRegion, t } = this.props;
let str = serverRegion;

if (!serverRegion) {
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you prefer showing N/A over not showing the row?

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to hide it because of deployments where regions are not configured. There it would show 'N/A' permanently (while for the rest of the stats it is reasonable to expect that they will appear once we get the data).

}


if (region && serverRegion && region !== serverRegion) {
str += ` from ${region}`;
}

return (
<tr>
<td>
<span>{ t('connectionindicator.e2e_rtt') }</span>
<span>{ t('connectionindicator.connectedTo') }</span>
</td>
<td>{ str }</td>
</tr>
);
}

/**
* Creates a table row as a ReactElement for displaying the "bridge count"
* information.
*
* @returns {*}
* @private
*/
_renderBridgeCount() {
const { bridgeCount, t } = this.props;

// 0 is valid, but undefined/null/NaN aren't.
if (!bridgeCount && bridgeCount !== 0) {
return;
}

return (
<tr>
<td>
<span>{ t('connectionindicator.bridgeCount') }</span>
</td>
<td>{ bridgeCount }</td>
</tr>
);
}

/**
* Creates a table row as a ReactElement for displaying frame rate related
* statistics.
Expand Down Expand Up @@ -373,8 +436,10 @@ class ConnectionStatsTable extends Component {
{ this._renderBitrate() }
{ this._renderPacketLoss() }
{ isRemoteVideo ? this._renderE2eRtt() : null }
{ isRemoteVideo ? this._renderRegion() : null }
Copy link
Contributor

Choose a reason for hiding this comment

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

I recall you asking me about this. I think you can also do isRemoteVideo && this._renderRegion() and get the same result, but either words for me.

{ this._renderResolution() }
{ this._renderFrameRate() }
{ isRemoteVideo ? null : this._renderBridgeCount() }
</tbody>
</table>
);
Expand Down