feat: extend stats object #10

forbesjo · 2017-12-05T22:07:36Z

Description

This adds a few more properties to the hls.stats object as well as uses the new debug logging level.

TODO

Add more stats
Add and use debug logging from video.js

Requirements Checklist

Feature implemented / Bug fixed
Reviewed by Two Core Contributors

mjneil · 2017-12-11T19:49:16Z

Do we want this as a plugin on the player? or should it be apart of the player.vhs api?

mjneil · 2017-12-11T19:55:40Z

src/videojs-http-streaming.js

+    currentSource: player.currentSource(),
+    master: player.tech_.hls.masterPlaylistController_.masterPlaylistLoader_.master,
+    currentTech: player.tech_.name_,
+    // TODO: filter by VHS logging only


How do we want to go about this? Currently videojs doesn't do any type of filtering or bucketing of the history, just a single array that all logging args get dumped into.

Some options I can think of:

prefix all log messages we add with a VHS specific string so we can filter on that

send only objects into the logs e.g. { vhs: true, message: 'log message' } Downside to this is if the debug log is enabled, videojs will console log objects for all of our messages instead of just the message

extend videojs.log to seperate history by type or allow a way of requesting logs in history of a specific type. Downside to this would be not being available on previous versions of videojs

forbesjo · 2017-12-12T15:55:52Z

Perhaps this should be on player.vhs since this API only applies to this project.

forbesjo · 2017-12-12T16:10:09Z

Then again we have not yet exposed player.vhs yet. Those changes are in #8

mjneil · 2017-12-12T16:13:43Z

src/videojs-http-streaming.js

+    currentTime: player.currentTime(),
+    timestamp: Date.now(),
+    currentSource: player.currentSource(),
+    master: player.tech_.hls.masterPlaylistController_.masterPlaylistLoader_.master,


could use the shortcut player.tech_.hls.playlists.master

mjneil · 2017-12-12T16:14:07Z

src/videojs-http-streaming.js

+  return {
+    videoPlaybackQuality: player.getVideoPlaybackQuality(),
+    playerDimensions: player.currentDimensions(),
+    hlsStats: player.tech_.hls.stats,


do we want to call this hlsStats since the stats apply to dash sources as well?

This should probably be renamed vhsStats since it would be the vhs.stats object

mjneil · 2017-12-12T17:17:54Z

src/util/logger.js

+    return videojs.log.debug.bind(videojs, '[VHS]', `${source} >`);
+  }
+
+  return function() {};


Do we want to fall back to videojs.log when videojs.log.debug does not exist?

That would then print out the logging by default

ah ya we don't want that

gesinger

Would be good to have tests for logger and getPlaybackStats

gesinger · 2017-12-13T15:53:13Z

src/segment-loader.js

@@ -672,27 +671,17 @@ export default class SegmentLoader extends videojs.EventTarget {
      return null;
    }

-    this.logger_('checkBuffer_',


Any specific reason for removing this and other logs that currently exist?

@mjneil was removing some excessive logging

gesinger · 2017-12-13T15:55:06Z

src/videojs-http-streaming.js

+    return {
+      videoPlaybackQuality: this.tech_.getVideoPlaybackQuality(),
+      playerDimensions: this.tech_.currentDimensions(),
+      vhsStats: this.stats,


This seems a bit ambiguous in naming considering we have vhsStats as a sub property of playbackStats

Do we want to merge with stats or perhaps just extend stats?

mjneil · 2017-12-13T22:24:24Z

src/util/logger.js

@@ -0,0 +1,11 @@
+import videojs from 'video.js';
+
+const logger = (source) => {


This will not allow us the ability to turn debug logging on mid session via videojs.log.level('debug') . If we want to support that, we will need something like this

const logger = (source) => (...args) => { if (videojs.log.level() === 'debug') { videojs.log.debug('VHS:', `${source} >`, ...args); } };

you don't even need the .call(videojs, in your way @mjneil.

oh right, good call!

oops this is checking if debug api is available, not if the mode is debug

gesinger · 2017-12-15T17:39:15Z

src/util/logger.js

+import videojs from 'video.js';
+
+const logger = (source) => {
+  if (videojs.log.debug) {


I know we discussed how the history object might get crowded by the debug logging once it's available. Do we know about how many logs per minute of video we'll be generating?

Looks like around ~35 items per minute with what's being logged at the moment

If that's pretty standard (and we can't get into a case where the log messages explode), that seems pretty good (to not have to hide behind another config flag). We should be careful to look at some of the cases in case any could spew messages though, and test some scenarios to make sure we don't kill a browser tab from memory consumptions.

gesinger · 2017-12-15T17:40:29Z

src/videojs-http-streaming.js

@@ -377,6 +377,20 @@ class HlsHandler extends Component {
      }
    });

+    const getTimeRanges = (player, type) => {


Since player and type are only used once to get the time ranges, it might be simpler and clearer to only pass in the time ranges themselves. The function may also be better served as being called timeRangesToArray and if we put it outside of the function we can add some tests for it.

ChromeCanary Support

forbesjo mentioned this pull request Dec 5, 2017

feat: include a stats plugin #9

Closed

6 tasks

mjneil reviewed Dec 11, 2017

View reviewed changes

mjneil reviewed Dec 12, 2017

View reviewed changes

forbesjo force-pushed the feat/stats branch from aa75649 to 78ca849 Compare December 13, 2017 15:38

feat: include a stats plugin

ae09d85

forbesjo force-pushed the feat/stats branch from 70be202 to ae09d85 Compare December 13, 2017 15:46

gesinger reviewed Dec 13, 2017

View reviewed changes

Use existing stats object

26d0971

forbesjo force-pushed the feat/stats branch from 11bb138 to 26d0971 Compare December 13, 2017 16:39

forbesjo changed the title ~~feat: include a stats plugin~~ feat: extend stats object Dec 13, 2017

mjneil reviewed Dec 13, 2017

View reviewed changes

gesinger reviewed Dec 15, 2017

View reviewed changes

Moved ranges formatting function and added tests

4bb5486

gesinger approved these changes Dec 15, 2017

View reviewed changes

forbesjo merged commit ae6a53a into master Dec 15, 2017

forbesjo deleted the feat/stats branch December 15, 2017 20:36

forbesjo pushed a commit to forbesjo/http-streaming that referenced this pull request Jan 30, 2018

Merge pull request videojs#10 from ishita12/test

77fee22

ChromeCanary Support

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: extend stats object #10

feat: extend stats object #10

forbesjo commented Dec 5, 2017 •

edited

Loading

mjneil commented Dec 11, 2017

mjneil Dec 11, 2017

forbesjo commented Dec 12, 2017

forbesjo commented Dec 12, 2017

mjneil Dec 12, 2017

mjneil Dec 12, 2017

forbesjo Dec 12, 2017

mjneil Dec 12, 2017

forbesjo Dec 12, 2017

mjneil Dec 12, 2017

gesinger left a comment

gesinger Dec 13, 2017

forbesjo Dec 13, 2017

gesinger Dec 13, 2017

forbesjo Dec 13, 2017 •

edited

Loading

mjneil Dec 13, 2017 •

edited

Loading

gkatsev Dec 13, 2017

mjneil Dec 13, 2017

mjneil Dec 13, 2017

gesinger Dec 15, 2017

forbesjo Dec 15, 2017

gesinger Dec 15, 2017

gesinger Dec 15, 2017

		@@ -0,0 +1,11 @@
		import videojs from 'video.js';

		const logger = (source) => {

feat: extend stats object #10

feat: extend stats object #10

Conversation

forbesjo commented Dec 5, 2017 • edited Loading

Description

TODO

Requirements Checklist

mjneil commented Dec 11, 2017

Choose a reason for hiding this comment

forbesjo commented Dec 12, 2017

forbesjo commented Dec 12, 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

gesinger left a 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

forbesjo Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

mjneil Dec 13, 2017 • edited Loading

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

forbesjo commented Dec 5, 2017 •

edited

Loading

forbesjo Dec 13, 2017 •

edited

Loading

mjneil Dec 13, 2017 •

edited

Loading