feat: expose more of vrs methods #10

brandonocasey · 2017-08-04T19:23:03Z

Debugging was a pain without access to all the currently private variables
Some of the variables need to be accessed again when we change videos

fix: switching a source should not create two canvases
fix: loading via player.src and video element source should work
fix: switching between 360 and non-360 sources with AUTO should work Fixes #8
fix: allow Sphere/Cube as mediainfo projections

brandonocasey · 2017-08-04T19:23:37Z

index.html

@@ -18,7 +18,11 @@
  <script>
    (function(window, videojs) {
      var player = window.player = videojs('videojs-vr-player');
-      player.vr({projection: '360', debug: true});
+      player.mediainfo = player.mediainfo || {};


switched the demo to mediainfo as that is probably the best way to do it (unless you only have one video)

I don't love the way mediainfo has leaked out of the Brightcove Player and into a couple of our open source projects. Is there a different/better way?

We may be able to read spacial metadata from the video file. I am not sure that would work for HLS or if we even have access to it for mp4

Yeah. Ignore me, I don't think it's a big deal. It's not BC-specific.

It wasn't a leak, it was a conscious decision. At least for ads.

However, it seems to me like it might be possible to get rid of the mediainfo usage? Just do everything through the VR options and also expose it via player.vr.projection? Then update usage of current mediainfo to reflect that?

When we are dealing with switching media (I.E 360 video switches to non-360 or vice versa), it is not possible to tell wether a video is 360 or not. We could manually set it with player.vr.projection but it seems like it would be better to have a flag in the media itself that tells the player that the source is 360.

brandonocasey · 2017-08-04T19:24:16Z

src/plugin.js

+};
+
+const vr = function(options) {
+  this.ready(() => initPlugin(this, options));


Not waiting for player.ready caused issues on iOS devices when the source was set in the video element. Presumable because of async source setting in video.js 6

Most likely.

fix: switching a source should not create two canvases fix: loading via player.src and video element source should work fix: switching between 360 and non-360 sources with AUTO should work fix: allow Sphere/Cube as mediainfo projections

brandonocasey · 2017-08-04T19:27:07Z

src/plugin.js

-    }
-  });
+  // mobile devices
+  if (videojs.browser.IS_ANDROID || videojs.browser.IS_IOS) {


IS_IPAD and IS_IOS are the same thing

brandonocasey · 2017-08-04T19:27:41Z

src/plugin.js


-  player.ready(function() {


no need to wait for ready since the entire function does this now

brandonocasey · 2017-08-04T19:28:05Z

src/plugin.js

-
-  player.on('loadstart', function() {
-    initScene();
+  player.on('loadedmetadata', function() {


we want to re-init the scene every video, but only when we know how big the video will be.

brandonocasey · 2017-08-04T19:29:00Z

src/plugin.js

  }

-  function initScene() {
+  /* reset player.vr to a default un-initialized state */
+  player.vr.reset = function() {


reset the vr plugin in-between inits, so that if the source is not 360 we don't mess with it.

brandonocasey · 2017-08-04T19:29:29Z

src/plugin.js


  // don't initialize twice
-  if (this.usingPlugin('vr')) {
+  if (player.vr && player.vr.currentProjection) {


usingPlugin wasn't consistent for some reason.

Do you remember why/how? We probably should open a bug for this.

I think that I may just have had the if check in the wrong place. It should probably go below all the error checking return statements. I also think that having it here would probably work now because the plugin waits for player.ready

gkatsev

Do we want to expose all the functions and settings publicly? I guess this makes it easier to debug and test?

gkatsev · 2017-08-07T15:51:46Z

index.html

@@ -18,7 +18,11 @@
  <script>
    (function(window, videojs) {
      var player = window.player = videojs('videojs-vr-player');
-      player.vr({projection: '360', debug: true});
+      player.mediainfo = player.mediainfo || {};


It wasn't a leak, it was a conscious decision. At least for ads.

gkatsev · 2017-08-07T15:52:12Z

src/plugin.js


  // don't initialize twice
-  if (this.usingPlugin('vr')) {
+  if (player.vr && player.vr.currentProjection) {


Do you remember why/how? We probably should open a bug for this.

gkatsev · 2017-08-07T15:55:06Z

src/plugin.js

+};
+
+const vr = function(options) {
+  this.ready(() => initPlugin(this, options));


Most likely.

gkatsev · 2017-08-07T15:56:20Z

index.html

@@ -18,7 +18,11 @@
  <script>
    (function(window, videojs) {
      var player = window.player = videojs('videojs-vr-player');
-      player.vr({projection: '360', debug: true});
+      player.mediainfo = player.mediainfo || {};


However, it seems to me like it might be possible to get rid of the mediainfo usage? Just do everything through the VR options and also expose it via player.vr.projection? Then update usage of current mediainfo to reflect that?

brandonocasey · 2017-08-07T16:07:43Z

I think exposing it all makes sense, I think it could be useful in many ways

chore: Swap webvr for webxr polyfill packages

brandonocasey commented Aug 4, 2017

View reviewed changes

feat: expose more of vrs methods

59b4c43

fix: switching a source should not create two canvases fix: loading via player.src and video element source should work fix: switching between 360 and non-360 sources with AUTO should work fix: allow Sphere/Cube as mediainfo projections

brandonocasey force-pushed the feat/expose-more branch from 0ae2e48 to 59b4c43 Compare August 4, 2017 19:26

brandonocasey commented Aug 4, 2017

View reviewed changes

src/plugin.js

player.ready(function() {

Copy link

Contributor Author

brandonocasey Aug 4, 2017

Choose a reason for hiding this comment

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

no need to wait for ready since the entire function does this now

brandonocasey commented Aug 4, 2017

View reviewed changes

make default private

50d4185

brandonocasey mentioned this pull request Aug 4, 2017

Fix/separate big play button #11

Merged

gkatsev reviewed Aug 7, 2017

View reviewed changes

misteroneill approved these changes Aug 7, 2017

View reviewed changes

gkatsev approved these changes Aug 7, 2017

View reviewed changes

brandonocasey merged commit 3cc1092 into master Aug 7, 2017

brandonocasey deleted the feat/expose-more branch August 7, 2017 18:10

mister-ben pushed a commit to mister-ben/videojs-vr that referenced this pull request Dec 4, 2023

Merge pull request videojs#10 from kevleyski/main

de6db15

chore: Swap webvr for webxr polyfill packages

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: expose more of vrs methods #10

feat: expose more of vrs methods #10

brandonocasey commented Aug 4, 2017 •

edited

Loading

brandonocasey Aug 4, 2017

misteroneill Aug 4, 2017

brandonocasey Aug 4, 2017

misteroneill Aug 4, 2017

gkatsev Aug 7, 2017

gkatsev Aug 7, 2017

brandonocasey Aug 7, 2017

brandonocasey Aug 4, 2017

gkatsev Aug 7, 2017

brandonocasey Aug 4, 2017

brandonocasey Aug 4, 2017

brandonocasey Aug 4, 2017

brandonocasey Aug 4, 2017

brandonocasey Aug 4, 2017

gkatsev Aug 7, 2017

brandonocasey Aug 7, 2017

gkatsev left a comment

gkatsev Aug 7, 2017

gkatsev Aug 7, 2017

gkatsev Aug 7, 2017

gkatsev Aug 7, 2017

brandonocasey commented Aug 7, 2017

feat: expose more of vrs methods #10

feat: expose more of vrs methods #10

Conversation

brandonocasey commented Aug 4, 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

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

gkatsev 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

brandonocasey commented Aug 7, 2017

brandonocasey commented Aug 4, 2017 •

edited

Loading