VAST Player #14

SJAnderson · 2018-06-13T02:17:48Z

No description provided.

Signed-off-by: Steven Anderson <[email protected]>

jasongornall · 2018-06-13T03:07:25Z

package.json

@@ -83,6 +86,8 @@
    "karma-sourcemap-loader": "^0.3.7",
    "karma-webpack": "^2.0.9",
    "mocha": "^4.1.0",
+    "node-sass": "^4.9.0",


lots of new deps here... anyway we can async load this only for VAST?

would be good to bundle this all in the maestro-videojs-vast player and async import it maybe

why on earth do we need this? are we compiling SASS style files? O_o

v1 was, fixing this

jasongornall · 2018-06-13T03:07:39Z

package.json

@@ -103,6 +110,7 @@
  "dependencies": {
    "deepmerge": "^2.0.1",
    "load-script": "^1.0.0",
+    "maestro-videojs-vast": "0.0.6-alpha",


can I see this guy?

https://github.com/lessthan3/maestro-videojs-vast

jasongornall · 2018-06-13T03:08:33Z

src/players/index.js

@@ -24,4 +25,5 @@ export default [
  UstreamLive,
  FilePlayer,
  Iframe,
-]
+  VAST,


does this work? Iframe returns canPlay true...

not sure how this type is ever played?

because the regex in Iframe fails on vast URLs

wait but our iframe player just does return true i thought

^ this.. not sure how VAST is ever played

non issue anymore

jasongornall · 2018-06-13T03:08:53Z

webpack.config.babel.js

@@ -11,6 +11,7 @@ const PATH_DEMO = join(__dirname, 'demo')
 const PATH_SRC = join(__dirname, 'src')
 const PATH_INDEX = join(__dirname, 'index.html')
 const PATH_TESTS = join(__dirname, 'test', 'specs')
+const VIDEOJS_VAST = join(__dirname, 'node_modules', 'maestro-videojs-vast', 'dist')


what's this doing?

shouldn't all this just come in from the dependency?

narrows down the files to include in the parsers

jasongornall · 2018-06-13T03:09:40Z

webpack.demo.babel.js

@@ -12,7 +12,7 @@ export const minifyPlugins = [
    sourceMap: true,
    comments: false,
    mangle: {
-      except: [ 'ReactPlayer' ]
+      except: ['ReactPlayer', 'maestro-videojs-vast']


why not mangle?

when i mangled it broke

also its already uglified by the webpack in that plugin

jasongornall · 2018-06-13T03:10:08Z

src/players/VAST.js

+      width: '100%',
+    };
+    return (
+      <div data-vjs-player style={style}>


what is this tag for?

from the videojs docs here

wrap the player in a div with a data-vjs-player attribute so videojs won't create additional wrapper in the DOM see videojs/video.js#3856

jasongornall · 2018-06-13T03:12:44Z

src/players/VAST.js

+
+  render() {
+    const style = {
+      ...this.props.style,


why you overwriting styles?

i copied and pasted this from youtube or something

jasongornall · 2018-06-13T03:15:10Z

src/players/VAST.js

+      };
+
+      const removeListeners = () => {
+        this.player.off('adsready', this.props.onReady);


does this hit if it doesn't serve?

jasongornall · 2018-06-13T03:15:43Z

src/players/VAST.js

+      const removeListeners = () => {
+        this.player.off('adsready', this.props.onReady);
+        this.player.off('adstart', onAdStart);
+        this.player.off('play', this.props.onPlay);


all this stuff is for the empty mp4 right? we likely want to end if the mp4 starts right?

yeah exactly. naturally videojs and it's ad's plugin will start playing the blank video, but it won't start playing without a video to play (like you can't start videojs with a vast and not a video), however it will try its hardest to start the blank video after the VAST has ended, so this is the hack to get around that.

jasongornall · 2018-06-13T03:16:21Z

package.json

@@ -83,6 +86,8 @@
    "karma-sourcemap-loader": "^0.3.7",
    "karma-webpack": "^2.0.9",
    "mocha": "^4.1.0",
+    "node-sass": "^4.9.0",


would be good to bundle this all in the maestro-videojs-vast player and async import it maybe

jasongornall · 2018-06-13T03:16:37Z

.gitignore

@@ -1,3 +1,4 @@
+.idea


what's this?

ignore webstorm config files

jasongornall · 2018-06-13T03:19:45Z

src/players/VAST.js

+import { callPlayer } from '../utils';
+import createSinglePlayer from '../singlePlayer';
+
+const PLACEHOLDER_VIDEO = 'https://storage.googleapis.com/maestro_video/blank.mp4';


we got a way we have been handling binaries for the App... wonder if it makes sense to use that

i don't know how to do that so i'll need some guidance on adding this

andybarron · 2018-06-13T16:58:35Z

webpack.config.babel.js

+      {
+        test: /\.(eot|woff|woff2|svg|ttf)([\?]?.*)$/,
+        loader: 'file-loader',
+        include: [VIDEOJS_VAST]


why are we doing this? i don't see any eot/woff/svg/etc files in the maestro-videojs-vast repo

andybarron · 2018-06-13T17:02:52Z

package.json

    "html-webpack-plugin": "^2.30.1",
+    "imports-loader": "^0.8.0",


don't see this referenced anywhere

Signed-off-by: Steven Anderson <[email protected]>

kamronbatman · 2018-06-13T21:42:14Z

src/players/VAST.js

+import createSinglePlayer from '../singlePlayer'
+import { FilePlayer } from './FilePlayer'
+
+const MATCH_URL = /^(https?:\/\/)?((bs\.serving-sys\.com)|(pubads\.g\.doubleclick\.net))/


change this to be insensitive:
const MATCH_URL = /^(https?:\/\/)?((bs\.serving-sys\.com)|(pubads\.g\.doubleclick\.net))/i

kamronbatman · 2018-06-13T21:44:42Z

src/players/VAST.js

+  callPlayer = callPlayer;
+
+  createSourceFiles (mediaFiles = []) {
+    return mediaFiles.reduce((arr, {fileURL: src, mimeType: type}) => {


Should use a map instead since you are always returning an array with the same element structure:

createSourceFiles = (mediaFiles = []) => ( mediaFiles.map(({ fileUrl: src, mimeType: type }) => { src, type }) );

kamronbatman · 2018-06-13T21:50:09Z

src/players/VAST.js

+      width: width === 'auto' ? width : '100%',
+      height: height === 'auto' ? height : '100%'
+    }
+    if (!sources.length) return null


This can be incorporated into the return:

return source.length ? ( <jsx here> ) : null;

kamronbatman · 2018-06-13T21:50:44Z

src/players/VAST.js

+    return (
+      <div onClick={this.onAdClick} style={wrapperStyle}>
+        <FilePlayer
+          {...this.props}


Are we sure we want to do this? Are there props we do NOT want to pass through?

this is fine

kamronbatman · 2018-06-13T21:51:56Z

src/players/VAST.js

+          onProgress={this.onProgress}
+          onReady={this.onReady}
+          onVolumeChange={this.onVolumeChange}
+          ref={this.ref}


This should be changed to createRef:

... container = React.createRef(); ...

ref={this.container}

not changing this

kamronbatman · 2018-06-13T21:52:34Z

src/players/VAST.js

+    return this.container.getSecondsLoaded()
+  }
+
+  ref = (container) => {


This can be removed.

also not changing this

Signed-off-by: Steven Anderson <[email protected]>

kamronbatman

I am not sure exactly how to QA this, but the code looks good.

shahrodkh

looks good

shahrodkh · 2018-06-13T23:39:11Z

src/demo/App.js

@@ -74,7 +74,7 @@ class App extends Component {
    this.player.seekTo(parseFloat(e.target.value))
  }
  onProgress = state => {
-    console.log('onProgress', state)
+    // console.log('onProgress', state)


andybarron

small q's

andybarron · 2018-06-13T23:47:37Z

src/Player.js

+
+          // need to send progress to VAST player
+          if (this.props.activePlayer.displayName === 'VAST') {
+            this.player.onProgress(progress);


andybarron · 2018-06-13T23:49:09Z

src/players/VAST.js

+import createSinglePlayer from '../singlePlayer'
+import { FilePlayer } from './FilePlayer'
+
+const MATCH_URL = /^(https?:\/\/)?((bs\.serving-sys\.com)|(pubads\.g\.doubleclick\.net))/i


we're positive this will always work? they may be using different serving... aren't they xml files? could we use /\.xml$/i?

Signed-off-by: Steven Anderson <[email protected]>

jasongornall · 2018-06-14T00:38:28Z

src/players/VAST.js

+  }
+
+  load (url) {
+    vast.client.get(url.slice('VAST:'.length), { withCredentials: true }, (response, error) => {


this is strange shouldn't this just be 5?

@cyrdax said it's fine

jasongornall · 2018-06-14T00:46:55Z

src/players/VAST.js

+
+      // Inform ad server we can't find suitable media file for this ad
+      vast.util.track(ad.errorURLTemplates, { ERRORCODE: 403 })
+      onEnded()


this could trigger several times if a liner is not the first one?

Signed-off-by: Steven Anderson <[email protected]>

jasongornall · 2018-06-14T00:53:48Z

src/players/VAST.js

+  callPlayer = callPlayer;
+
+  createSourceFiles (mediaFiles = []) {
+    return mediaFiles.map(({fileURL: src, mimeType: type}) => ({src, type}))


is it possible this could contain empty entries? might de-structure a null right?

Signed-off-by: Steven Anderson <[email protected]>

jasongornall · 2018-06-14T01:21:17Z

src/players/VAST.js

+  // todo: add skip functionality
+  skip () {
+    const {props: {onEnded}, state: {tracker}} = this
+    tracker.skip()


this can crash if there is no "linear"

Signed-off-by: Steven Anderson <[email protected]>

cyrdax · 2018-06-14T01:28:26Z

src/players/VAST.js

+  // track play
+  onPlay = (event) => {
+    const {props: {onPlay}, state: {tracker}} = this
+    tracker.setPaused(false)


set paused? should this be play?

cyrdax · 2018-06-14T01:30:39Z

src/players/VAST.js

+  // track volume change
+  onVolumeChange = (event) => {
+    const {props: {onVolumeChange}, state: {tracker}} = this
+    tracker.setMuted(this.container.muted)


is this mute? or volume change?

SJAnderson added 3 commits June 6, 2018 17:46

update gitignore

4be2e07

Signed-off-by: Steven Anderson <[email protected]>

add vast player

9f659dc

Signed-off-by: Steven Anderson <[email protected]>

update demo

f3f9a36

Signed-off-by: Steven Anderson <[email protected]>

SJAnderson requested review from jasongornall, cyrdax and andybarron June 13, 2018 02:18

jasongornall reviewed Jun 13, 2018

View reviewed changes

jasongornall suggested changes Jun 13, 2018

View reviewed changes

jasongornall reviewed Jun 13, 2018

View reviewed changes

andybarron reviewed Jun 13, 2018

View reviewed changes

package.json Outdated

"html-webpack-plugin": "^2.30.1",

"imports-loader": "^0.8.0",

Copy link

andybarron Jun 13, 2018

Choose a reason for hiding this comment

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

don't see this referenced anywhere

SJAnderson added 10 commits June 13, 2018 10:02

updates

300bbb5

Signed-off-by: Steven Anderson <[email protected]>

Merge branch 'master' into vast

4a0a16c

moar updates

8673e0e

Signed-off-by: Steven Anderson <[email protected]>

update package lock

f6fe39a

Signed-off-by: Steven Anderson <[email protected]>

revert webpack configs

274884f

Signed-off-by: Steven Anderson <[email protected]>

update package.json

5670183

Signed-off-by: Steven Anderson <[email protected]>

revert maestro player changes

8b31295

Signed-off-by: Steven Anderson <[email protected]>

remove console logs

3a77b1f

Signed-off-by: Steven Anderson <[email protected]>

use react players one second timeout

4ae69ee

Signed-off-by: Steven Anderson <[email protected]>

need to call onProgress for vast

d758109

Signed-off-by: Steven Anderson <[email protected]>

kamronbatman suggested changes Jun 13, 2018

View reviewed changes

stphnlngdncoding and others added 3 commits June 13, 2018 15:22

Fixed multiple on progresss firing

e965fef

updates

b687341

Signed-off-by: Steven Anderson <[email protected]>

update version

60fddaa

Signed-off-by: Steven Anderson <[email protected]>

kamronbatman approved these changes Jun 13, 2018

View reviewed changes

shahrodkh suggested changes Jun 13, 2018

View reviewed changes

andybarron suggested changes Jun 13, 2018

View reviewed changes

shahrodkh approved these changes Jun 13, 2018

View reviewed changes

andy updates

d5b1a10

Signed-off-by: Steven Anderson <[email protected]>

SJAnderson added 2 commits June 13, 2018 17:06

new ddist

fbabed6

Signed-off-by: Steven Anderson <[email protected]>

remove semi

ce25d87

Signed-off-by: Steven Anderson <[email protected]>

jasongornall suggested changes Jun 14, 2018

View reviewed changes

return onended if fail

10713fc

Signed-off-by: Steven Anderson <[email protected]>

jasongornall reviewed Jun 14, 2018

View reviewed changes

SJAnderson added 3 commits June 13, 2018 17:54

dont destructure a null

300986f

Signed-off-by: Steven Anderson <[email protected]>

filter invalid types

383da7a

Signed-off-by: Steven Anderson <[email protected]>

add question marks

0551167

Signed-off-by: Steven Anderson <[email protected]>

jasongornall suggested changes Jun 14, 2018

View reviewed changes

updates

4bd0dc3

Signed-off-by: Steven Anderson <[email protected]>

cyrdax suggested changes Jun 14, 2018

View reviewed changes

SJAnderson merged commit 4a097b0 into master Jun 14, 2018

		"html-webpack-plugin": "^2.30.1",
		"imports-loader": "^0.8.0",

VAST Player #14

VAST Player #14

Conversation

SJAnderson commented Jun 13, 2018

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

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

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

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

kamronbatman left a comment

Choose a reason for hiding this comment

shahrodkh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andybarron 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

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