feat: add flash engine #2

itaykinnrot · 2018-06-25T08:40:43Z

No description provided.

itaykinnrot · 2018-06-25T08:41:48Z

@OrenMe @dan-ziv @yairans @odedhutzler - lets use this pull for review - i'll update it to master after it. thanks!

OrenMe

In general - I think many comments would have been resolved if the adapter would have implemented base-media-source-adapter.
I can see the exception that it expects HTMLVideoELement, but we could have easily handle this(I think) and this will enforce more alignment between our owned adapters and engines.
My main concern here is maintainability - all the small variations between this and HTML5 engine/adapters may lead to bug fixing or feature implementing to be more hard.

OrenMe · 2018-07-22T12:02:23Z

src/flash.js

+      this._eventManager.listen(this._api, EventType.TRACKS_CHANGED, (event: FakeEvent) =>{ this.dispatchEvent(event)});
+      this._eventManager.listen(this._api, EventType.ERROR, (event: FakeEvent) => this.dispatchEvent(event));
+      this._eventManager.listen(this._api, EventType.PLAYING, (event: FakeEvent) => this.dispatchEvent(event));
+      this._eventManager.listen(this._api, EventType.TIME_UPDATE, (event: FakeEvent) => {


I think this should be managed in adapter and exposed on API of adapter

OrenMe · 2018-07-22T12:15:49Z

src/flash.js

+        }
+        this._currentTime = event.payload.position;
+        this._duration = event.payload.duration;
+        this._buffer = event.payload.buffer;


buffer and watch which feed the data for seekable and played API - they should hold the actual data and create additional entries in the TimeRanges object returned from them.
For example, watching a video and then seeking while it is played will cause the seekable and played APIs to return an array with several timeRange entries.
The possible solution is on seek events to see if seek target is not in range already and if so add new timeRange entry for played, buffered and seekable
All of the above should be done in the adapter somehow.
I would also create a class for timeRange and timeRanges to manage this

OrenMe · 2018-07-22T12:17:40Z

src/flash.js

+        this._duration = event.payload.duration;
+        this._buffer = event.payload.buffer;
+        this._watched = event.payload.watched;
+        this._paused = false;


better just have the value of this be saved in the adapter and have the engine fo something like return this._api.paused' on the API call for paused`

get paused(): boolean { return this._api.paused; }

also, this way you don't have to manage the source of truth in many places

OrenMe · 2018-07-22T12:20:19Z

src/flash.js

+      });
+      this._eventManager.listen(this._api, EventType.SEEKED, (event: FakeEvent) =>{
+        this.dispatchEvent(event);
+        this._seeking = false;


same here - maybe better do it in adapter

OrenMe · 2018-07-22T12:20:23Z

src/flash.js

+      });
+      this._eventManager.listen(this._api, EventType.ENDED, (event: FakeEvent) =>{
+        this.dispatchEvent(event);
+        this._ended = true;


same here - maybe better do it in adapter

OrenMe · 2018-07-22T13:55:58Z

src/flash.js

+   * @returns {TimeRanges} - First buffered range (part) of the video in seconds.
+   * @public
+   */
+  get buffered(): any {


return TimeRanges

OrenMe · 2018-07-22T14:00:00Z

src/flashhls-adapter.js

+      this._config.flashvars= {};
+    }
+    this._config.flashvars.callback = 'flashlsCallback';
+    this._el.innerHTML = FlashHLSAdapter.getFlashCode(this._config.swfUrl || 'https://cdnapisec.kaltura.com/html5/static/flashhls/v0.4.4.24/flashlsChromeless.swf?inline=1',this._config.flashvars,this._config.params,this._config.attributes)


set the default in a default config object and merge in the constructor on init

OrenMe · 2018-07-22T14:01:41Z

src/flash.js

+
+  _init(source: PKMediaSourceObject, config: Object): void {
+    this._eventManager = new EventManager();
+    this._flashConfig = Utils.Object.getPropertyPath(config, "playback.options.flash");


playback.options.* is mainly meant for 3rd party usage - engines should opt to use the defined config object of the player - and AFAICS you pass it as is to adapter so preferably you relate to this object only in the adapter part.

OrenMe · 2018-07-22T14:12:28Z

src/flash.js

+   */
+  enableAdaptiveBitrate(): void {
+    if (this._api) {
+      this._api.selectVideoTrack({index: -1});


use only the defined object, you can't actually(although it's javascript, you can) pass this - the API should only accept typed object, in this case VideoTrack.
Do what hls and dash adapters do, expose a method for setting adaptive mode, e.g. enableAdaptiveBitrate and in it call the native FLASH implementation which dictates setting -1

OrenMe · 2018-07-22T14:15:48Z

src/flashhls-adapter.js

+
+  getVideoTrack(): ?number {
+    if (this._api){
+      return this._api.getAutoLevelCapping();


sounds like this is incorrect? getVideoTrack--> getAutoLevelCapping

yairans · 2018-07-26T07:20:36Z

src/flash.js

+   * @public
+   * @static
+   */
+  static canPlaySource(source: PKMediaSourceObject, preferNative: boolean): boolean {


preferNative unused

yairans · 2018-07-26T07:35:53Z

src/flash.js

+   * @public
+   */
+  get currentTime(): number {
+    return this._api.currentTime ? this._api.currentTime : 0;


add if (this._api)

yairans · 2018-07-26T07:39:23Z

src/flash.js

+    let duration: number = 0;
+    if (this._api.duration) {
+      duration = this._api.duration;
+    } else if (this._api) {


the sequence here doesn't make sense. if (this._api) should be before if (this._api.duration)

yairans · 2018-07-26T07:41:29Z

src/flash.js

+   * @public
+   */
+  get paused(): boolean {
+    return this._api.paused;


add if (this._api)

yairans · 2018-07-26T07:41:47Z

src/flash.js

+   * @public
+   */
+  get seeking(): boolean {
+    return this._api.seeking;


add if (this._api)

yairans · 2018-07-26T07:43:10Z

src/flash.js

+   * @public
+   */
+  get played(): any {
+    return {


add if (this._api)

yairans · 2018-07-26T07:43:40Z

src/flash.js

+    }
+    return {
+      length: 1, start: () => {
+        return this._api.currentTime - backBufferLength;


add if (this._api) (actually this is flow's job. so just run it....)

yairans · 2018-07-26T07:48:45Z

src/flash.js

+      this._volumeBeforeMute = this.volume;
+      this.volume = 0;
+    } else {
+      if (this._volumeBeforeMute) {


could be 0 so check typeof this._volumeBeforeMute === 'number'

yairans · 2018-07-26T08:04:20Z

src/capabilities/flash-is-supported.js

+   * @returns {void}
+   */
+  static runCapability(): void {
+    let version = '0,0,0';


use const (in the whole pull where applicable)

But i'm changing the version...

yairans · 2018-07-26T08:14:40Z

src/flashhls-adapter.js

+
+    let flashlsEvents = {
+      ready:() => {
+        this._api = new FlashAPI(this._el.firstElementChild);


where the event here?

yairans · 2018-07-26T09:01:31Z

src/index.js

+
+Flash.runCapabilities();
+Flash.getCapabilities().then(((capabilites) =>{
+  if (capabilites["flash"].isSupported) {


use Flash.id

yairans · 2018-07-26T09:15:57Z

src/flash.js

@@ -0,0 +1,575 @@
+// @flow
+import {EventManager, EventType, FakeEvent, FakeEventTarget, ICapability, IEngine} from 'playkit-js';
+import * as JsLogger from 'js-logger';


import from playkit-js in the line above

OrenMe · 2018-07-29T09:38:50Z

src/flashhls-adapter.js

+
+  isABR(): boolean {
+    if (this._api) {
+      return this._api.getAutoLevelCapping() == -1;


use hls.autolevel API for that: https://github.com/mangui/flashls/blob/dev/API.md#hlsautolevel

OrenMe · 2018-07-29T09:40:09Z

src/flashhls-adapter.js

+
+  setABR(): void {
+    if (this._api) {
+      this._api.playerSetAutoLevelCapping(-1);


use hls.nextlevel = -1 API for that: https://github.com/mangui/flashls/blob/dev/API.md#hlsnextlevel

OrenMe · 2018-07-29T09:47:31Z

src/flashhls-adapter.js

+
+  selectVideoTrack(videoTrack: VideoTrack): void {
+    if (this._api) {
+      this._api.playerSetAutoLevelCapping(videoTrack.index);


See hlsjs adapter, which has similar API:

https://github.com/kaltura/playkit-js-hls/blob/44905ee6bbad904aaa8c6a3ba42659d7e67b4681/src/hls-adapter.js#L504-L511

selectVideoTrack(videoTrack: VideoTrack): void { if (this._api) { if (this.isABR()) { this._trigger(EventType.ABR_MODE_CHANGED, {mode: 'manual'}); } hls.currentlevel = videoTrack.index; } }

see more here: https://github.com/mangui/flashls/blob/dev/API.md#hlscurrentlevel

I think in current implementation you throw the ABR_MODE_CHANGED event every time.

OrenMe · 2018-07-29T12:02:22Z

src/flash.js

+   * @returns {Promise<Object>} - The loaded data
+   */
+  load(startTime: ?number): Promise<Object> {
+    if (!this._api) {


@itaykinnrot , how can this._api not be set?
You have a protection in playkit player.js to not enable accessing engine if it's not selected, and the attach method is simple DOM operation which most likely won't fail.
The load should be a promise over the ready event that is available in https://github.com/kaltura/playkit-js-flash/pull/2/files#diff-934f9a3b93099594e3708f315667203fR103

OrenMe · 2018-07-29T12:02:51Z

src/flash.js

+   * @returns {void}
+   */
+  play(): void {
+    if (!this._loadPromise) {


@itaykinnrot did you check this?

…o support external captions

OrenMe · 2018-07-31T14:03:02Z

src/flash.js

@@ -127,6 +129,9 @@ class Flash extends FakeEventTarget implements IEngine {
    this._init(source, config);
  }

+  hideTextTrack(): void {


this is in the flow typings - didn't it alert you on missing interface?

fixed issues

itay.kinnrot added 2 commits June 25, 2018 11:32

add code

56412bb

udpate

45d05ff

add playback rate

ad18811

OrenMe suggested changes Jul 22, 2018

View reviewed changes

OrenMe assigned itaykinnrot Jul 22, 2018

OrenMe added the enhancement label Jul 22, 2018

itay.kinnrot and others added 2 commits July 25, 2018 15:34

review fixes

5ca4d28

chore: prettier configuration

c05d0bc

yairans suggested changes Jul 26, 2018

View reviewed changes

chore: prettier fixes

52218e0

yairans previously requested changes Jul 26, 2018

View reviewed changes

fix review +flow + getLogger from playkit-js

356c8c2

OrenMe suggested changes Jul 29, 2018

View reviewed changes

itay.kinnrot added 5 commits July 29, 2018 15:52

remove capping and move to currentLevel param

545b308

use the adapter promise

0cc9d02

change the api load to promise

7eaab44

fix(FEC-8435,FEC-8437): add muted state and add empty hideTextTrack t…

355bd7e

…o support external captions

fix(FEC-8441): after end event send time update

d3799d5

OrenMe approved these changes Aug 2, 2018

View reviewed changes

OrenMe merged commit 3ec15c7 into review-branch Aug 2, 2018

OrenMe deleted the flash-code branch August 2, 2018 06:47

itaykinnrot pushed a commit that referenced this pull request Aug 2, 2018

feat: add flash engine (#2)

bf04ce0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flash engine #2

feat: add flash engine #2

itaykinnrot commented Jun 25, 2018

itaykinnrot commented Jun 25, 2018

OrenMe left a comment

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

OrenMe Jul 22, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

itaykinnrot Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

yairans Jul 26, 2018

OrenMe Jul 29, 2018

OrenMe Jul 29, 2018

OrenMe Jul 29, 2018

OrenMe Jul 29, 2018

OrenMe Jul 29, 2018

OrenMe Jul 31, 2018

feat: add flash engine #2

feat: add flash engine #2

Conversation

itaykinnrot commented Jun 25, 2018

itaykinnrot commented Jun 25, 2018

OrenMe 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

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