Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: engine decorator #324

Merged
merged 13 commits into from
Apr 15, 2019
Merged

feat: engine decorator #324

merged 13 commits into from
Apr 15, 2019

Conversation

dan-ziv
Copy link
Contributor

@dan-ziv dan-ziv commented Dec 10, 2018

Description of the Changes

Engine decorator framework which will serve DAI use-cases.
The engine decorator will catch any API call to the engine and will decide what do to depends on its logic.
Plugin who wants to implement an engine decorator will need to have the signature:

getEngineDecorator(engine) { 
	...
}

The BaseEngineDecorator redirects each API call to the engine itself, so if plugin won't implement certain API call, by default it will bubble to the engine.
When an engine is chosen, the player will check if certain plugin implemented getEngineDecorator. If such exists, it will pass it the engine and will get the decorator instead.

CheckLists

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • test are passing in local environment
  • Travis tests are passing (or test results are not worse than on master branch :))
  • Docs have been updated

@dan-ziv dan-ziv self-assigned this Dec 10, 2018
@dan-ziv dan-ziv requested a review from a team December 10, 2018 09:17
@dan-ziv dan-ziv changed the title Engine decorator fear: engine decorator Dec 10, 2018
@dan-ziv dan-ziv changed the title fear: engine decorator feat: engine decorator Dec 10, 2018
import {EventType} from './event/event-type';
import EventManager from './event/event-manager';

class BaseEngineDecorator extends FakeEventTarget {
Copy link
Contributor Author

@dan-ziv dan-ziv Jan 22, 2019

Choose a reason for hiding this comment

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

Add implements IEngine

@@ -1609,6 +1628,14 @@ export default class Player extends FakeEventTarget {
this._onTextTrackChanged(event)
);
this._eventManager.listen(this._externalCaptionsHandler, Html5EventType.ERROR, (event: FakeEvent) => this.dispatchEvent(event));
if (this._adsController) {
this._eventManager.listen(this._adsController, AdEventType.AD_BREAK_START, () => {
Copy link
Contributor Author

@dan-ziv dan-ziv Jan 22, 2019

Choose a reason for hiding this comment

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

There's an issue with poster and ima-dai prerolls when autoplay=false (the poster is flashing before the preroll).
This logic comes to fix it.

@kaltura-hooks
Copy link

Hi @dan-ziv,
Thank you for contributing this pull request!
Please sign the Kaltura CLA so we can review and merge your contribution.
Learn more at http://bit.ly/KalturaContrib

@@ -273,6 +273,9 @@ export default class NativeAdapter extends BaseMediaSourceAdapter {
this._eventManager.listen(this._videoElement, Html5EventType.ENDED, () => this._clearHeartbeatTimeout());
this._eventManager.listen(this._videoElement, Html5EventType.ABORT, () => this._clearHeartbeatTimeout());
this._eventManager.listen(this._videoElement, Html5EventType.SEEKED, () => this._onSeeked());
// Sometimes when playing live in safari and switching between tabs the currentTime goes back with no seek events
this._eventManager.listen(window, 'focus', () => setTimeout(() => this._onSeeked(), 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

need to think if all of this check is relevant for live, so check if we even detect stalling/waiting when doing throttling test.
If we don't then just cancel the onTimeUpdate check when we are in live.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. onTimeUpdate check is relevant for live as well.
We have to detect internet disconnection for live to get an error like in chrome

@@ -367,6 +372,31 @@ export default class NativeAdapter extends BaseMediaSourceAdapter {
}
}

_handleMetadataTrackEvents(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to move this to html5 layer so it will be generic timed_metadata event.

} else {
Array.from(this._videoElement.textTracks).forEach((track: TextTrack) => {
if (track.kind === 'metadata') {
setTimeout(() => (track.mode = 'hidden'), 100);
Copy link
Contributor

Choose a reason for hiding this comment

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

100 to const

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment

@@ -0,0 +1,282 @@
// @flow
Copy link
Contributor

Choose a reason for hiding this comment

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

check to minimise the code

yairans added a commit to kaltura/playkit-js-ima-dai that referenced this pull request Apr 15, 2019
@yairans yairans merged commit aa67b09 into master Apr 15, 2019
@yairans yairans deleted the engine-decorator branch April 15, 2019 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants