feat(FEC-8046): 360 Support #1

yairans · 2018-06-11T20:28:37Z

dependes on:
kaltura/playkit-js-providers#55
kaltura/kaltura-player-js#130
kaltura/playkit-js#239
kaltura/playkit-js-ui#242

OrenMe · 2018-06-12T08:44:31Z

src/vr.js

+   */
+  _addBindings(): void {
+    this.eventManager.listen(this.player, this.player.Event.SOURCE_SELECTED, () => {
+      if (this.player.is360()) {


this should be source attribute, not player

If we do it as player we should abstract it more and get change name to VR as well

@OrenMe what do u mean 'abstract it more'?

OrenMe · 2018-06-12T10:25:48Z

src/vr.js

+        this.eventManager.listen(
+          this.player,
+          this.player.Event.FIRST_PLAY,
+          this._initComponents.bind(this)


you can use () => ... as the eventManager saves reference internally to callbacks and we call destroy on event manager which clears them

OrenMe · 2018-06-12T10:28:36Z

src/vr.js

+  _appendCanvasOverlayAction(): void {
+    this._canvasOverlayAction = document.createElement('div');
+    this._canvasOverlayAction.id = CANVAS_360_OVERLAY_ID;
+    this.player.getView().appendChild(this._canvasOverlayAction);


use playkit-js utils for DOM manipulation

for all of above actions

didn't we say this will be abstracted away form the plugin and managed as an API? with the controller interface?

OrenMe · 2018-06-12T10:30:14Z

src/vr.js

+    this._renderer = new THREE.WebGLRenderer();
+    this._canvas = this._renderer.domElement;
+    Utils.Dom.addClassName(this._canvas, CANVAS_360_CLASS);
+    this.player.getView().insertBefore(this._canvas, videoElement.nextSibling);


use playkit-js utils for DOM manipulation, add method if missing

OrenMe · 2018-06-12T10:30:38Z

src/vr.js

+    this._scene = new THREE.Scene();
+    this._scene.add(sphere);
+
+    // this.effect = new THREE.StereoEffect(this._renderer);


remove comment?

OrenMe · 2018-06-12T10:33:54Z

src/vr.js

+   * @returns {void}
+   */
+  destroy(): void {
+    this.reset();


a bit strange that on destroy we call reset which in turn calls _initMembers and _addBindings

@yairans please comment to this.
on destroy you don't want to re-register the source selected event

OrenMe · 2018-06-12T10:35:05Z

src/vr.js

+   * @returns {void}
+   */
+  reset(): void {
+    this.player.getView().removeChild(this._canvas);


use playkit-js utils removeChild

OrenMe · 2018-06-12T10:35:18Z

src/vr.js

+   */
+  reset(): void {
+    this.player.getView().removeChild(this._canvas);
+    this._canvasOverlayAction.parentNode.removeChild(this._canvasOverlayAction);


same here on removeChild

OrenMe · 2018-06-12T10:53:20Z

src/vr.js

+    });
+    this.eventManager.listen(window, 'mouseup', this._onDocumentPointerUp.bind(this));
+    this.eventManager.listen(window, 'touchend', this._onDocumentPointerUp.bind(this));
+    this.eventManager.listen(window, 'devicemotion', this._onDeviceMotion.bind(this));


You can check for support before registering window.DeviceMotionEvent
Ref: https://developers.google.com/web/fundamentals/native-hardware/device-orientation/

OrenMe · 2018-06-12T10:53:22Z

src/vr.js

+    this.eventManager.listen(window, 'mouseup', this._onDocumentPointerUp.bind(this));
+    this.eventManager.listen(window, 'touchend', this._onDocumentPointerUp.bind(this));
+    this.eventManager.listen(window, 'devicemotion', this._onDeviceMotion.bind(this));
+    this.eventManager.listen(window, 'deviceorientation', this._onMobileOrientation.bind(this));


You can check for support before registering window.DeviceOrientationEvent
Ref: https://developers.google.com/web/fundamentals/native-hardware/device-orientation/

Does this method even exist?

dan-ziv · 2018-06-13T05:56:06Z

src/index.js

+declare var __VERSION__: string;
+declare var __NAME__: string;
+
+export default Vr;


Plz use our new conventions

No default exports

Export plugin class as Plugin
export {Vr as Plugin}

dan-ziv · 2018-06-13T05:57:04Z

src/style.css

+  height: 100%;
+}
+
+.indicator {


add playkit- prefix to all classes to avoid collisions

dan-ziv · 2018-06-13T05:58:01Z

src/vr.js

+ * Your class description.
+ * @classdesc
+ */
+export default class Vr extends BasePlugin {


Use new conventions without default export

class Vr { .. } export {Vr};

please follow @dan-ziv 's comment

dan-ziv · 2018-06-13T05:58:29Z

src/vr.js

+      if (this.player.is360()) {
+        this.logger.debug('360 entry has detected');
+        this._appendCanvasOverlayAction();
+        this.eventManager.listen(this.player, this.player.Event.FIRST_PLAY, this._initComponents.bind(this));


Use arrow function

dan-ziv · 2018-06-13T05:59:17Z

webpack.config.js

+  output: {
+    path: __dirname + '/dist',
+    filename: '[name].js',
+    library: ['playkit', 'vr'],


library: ['playkit', 'plugins', 'vr'],

OrenMe · 2018-06-20T05:12:02Z

src/vr.js

+    if (this.player.env.browser.name === 'IE') {
+      // a workaround for ie11 texture issue
+      // see https://github.com/mrdoob/three.js/issues/7560
+      const ctx2d = document.createElement('canvas').getContext('2d');


use playkit utils for DOM manipulation

also, you don't seem to destroy/remove it anywhere
and give it an id...

@OrenMe this element not attached to the dom but used by THREE.Texture

OrenMe · 2018-06-20T05:13:45Z

src/vr.js

+ * Your class description.
+ * @classdesc
+ */
+export default class Vr extends BasePlugin {


please follow @dan-ziv 's comment

OrenMe · 2018-06-20T05:15:35Z

src/vr.js

+      clearColor: 0xffffff,
+      antialias: true
+    });
+    const canvas = this._renderer.domElement;


don't see this gets removed from DOM on cleanup anywhere

actually, now I see you do....

OrenMe · 2018-06-20T05:16:16Z

src/vr.js

+   * @returns {boolean} - Whether the plugin is valid.
+   */
+  static isValid(): boolean {
+    return true;


is this always valid?

OrenMe · 2018-06-20T05:22:01Z

src/vr.js

+   * @returns {void}
+   */
+  destroy(): void {
+    this.reset();


@yairans please comment to this.
on destroy you don't want to re-register the source selected event

OrenMe · 2018-06-20T05:30:14Z

src/vr.js

+    }
+  }
+
+  _getCanvasDimensions(): Object {


maybe define the dimensions object?

type dimensions = { width: number, height: number }

OrenMe · 2018-06-20T05:31:53Z

src/vr.js

+        this._texture.render(videoElement);
+      }
+    }
+    this._requestId = requestAnimationFrame(this._render.bind(this));


better use some abbreviation - it is usually referred to as RAF so you can change to rafId

OrenMe · 2018-06-20T05:32:11Z

src/vr.js

+  }
+
+  _onPlay(): void {
+    if (!this._requestId) {


this condition is not clear

a comment has been added

OrenMe · 2018-06-20T05:44:39Z

src/vr.js

+
+  toggleStereoMode(): void {
+    this._stereoMode = !this._stereoMode;
+    this._updateCanvasSize();


if this will be externlized as API need to add protection if plugin is active and able to go to stereo

_updateCanvasSize is already protected

OrenMe · 2018-06-20T05:46:49Z

src/vr.js

+  }
+
+  _updateCanvasSize(): void {
+    if (this._renderer) {


is there any way the renderer can be not defined when calling this?
The only place I see this happening is when calling toggleStereoMode and there is where you need to add protection

flow requires this protection

OrenMe · 2018-06-20T05:58:30Z

src/vr.js

+            this.eventManager.listen(this.player, this.player.Event.ENDED, this._cancelAnimationFrame.bind(this));
+            this.eventManager.listen(this.player, this.player.Event.PLAY, this._onPlay.bind(this));
+            this.eventManager.listen(this.player, this.player.Event.PLAYING, this._onPlaying.bind(this));
+            this.eventManager.listen(window, 'resize', () => this._updateCanvasSize());


don't you need to add fullscreen event listener as well?

fullscreen fires resize

OrenMe · 2018-06-20T07:51:53Z

src/vr.js

+   * @returns {void}
+   */
+  _initComponents(): void {
+    this.logger.debug('Init 360 components');


init VR components :-)

OrenMe · 2018-06-20T07:52:20Z

src/vr.js

+ * @type {string}
+ * @const
+ */
+const CANVAS_360_CLASS: string = 'playkit-360-canvas';


CANVAS_VR_CLASS, playkit-vr-canvas

OrenMe · 2018-06-20T07:52:38Z

src/vr.js

+import './style.css';
+
+/**
+ * The 360 canvas class.


feat(FEC-8046): 360 Support

e376c71

yairans self-assigned this Jun 11, 2018

yairans requested review from itaykinnrot, OrenMe, dan-ziv and odedhutzler June 11, 2018 20:28

yairans added 2 commits June 12, 2018 10:37

remove host

08700a7

add prettier

be7d31b

OrenMe suggested changes Jun 12, 2018

View reviewed changes

yairans added 2 commits June 12, 2018 22:35

remove replay

83aee5c

prettier

1b284f6

dan-ziv suggested changes Jun 13, 2018

View reviewed changes

yairans added 5 commits June 13, 2018 16:38

support ie11

399e14a

remove overlay canvas and get is from UI

ad4dd68

stereo mode

6eb0b2e

listen to overlay action

8787de2

throw error for playsinline false and DRM content

4734125

OrenMe suggested changes Jun 20, 2018

View reviewed changes

yairans added 6 commits June 20, 2018 18:08

CR fixes

75d96d6

change to isInVrStereoMode

368e552

change to isInStereoMode

440b2d7

fix flow and eslint

ce6a335

fix webpack file

f53d86e

jsDoc

49be81e

yairans merged commit 65d7856 into master Jun 20, 2018

yairans deleted the FEC-8046 branch June 20, 2018 21:10

feat(FEC-8046): 360 Support #1

feat(FEC-8046): 360 Support #1

Conversation

yairans commented Jun 11, 2018 • 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

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

yairans Jun 20, 2018 • 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

yairans commented Jun 11, 2018 •

edited

Loading

yairans Jun 20, 2018 •

edited

Loading