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.

Sign up for GitHub

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

Jump to bottom

feat(FEC-8046): 360 support #130

Merged
merged 11 commits into from
Jun 20, 2018
Merged

feat(FEC-8046): 360 support #130

merged 11 commits into from
Jun 20, 2018

Conversation

yairans
Copy link
Contributor

@yairans yairans commented Jun 14, 2018 •
edited
Loading

Description of the Changes

  • force inBrowserFullscreenForIOS when 'vr' plugin is set
  • set vrStereo ui component when 'vr' plugin is set
  • pass the target dom element to 'vr' plugin

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

@yairans yairans self-assigned this Jun 14, 2018
@yairans yairans mentioned this pull request Jun 14, 2018
Merged
yairans
yairans commented Jun 14, 2018
View reviewed changes
src/common/ui-wrapper.js Outdated
@@ -38,6 +38,14 @@ class UIWrapper {
this.setConfig(Utils.Object.mergeDeep({}, previewThumbnailConfig, seekbarConfig), 'seekbar');
}

setFullscreenConfig(config: ProviderMediaConfigObject): void {
if (this._disabled) return;
if (Utils.Object.getPropertyPath(config, 'plugins.vr')) {
Copy link
Contributor Author

@yairans yairans Jun 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

maybe better to check the plugin does exist via getPlugin('vr')

dan-ziv
dan-ziv suggested changes Jun 18, 2018
View reviewed changes
src/kaltura-player.js Outdated
@@ -77,4 +79,16 @@ export default class KalturaPlayer {
set: undefined
};
}

_addBindings(): void {
Copy link
Contributor

@dan-ziv dan-ziv Jun 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is massive and too specific referring between kaltura player and a plugin.
The plugin should handle this logic and not the kaltura player.
If the problem is that the plugin cannot listen to the UI events the solution is to move the plugin manager to the kaltura player level and not bind the kaltura player to a specific plugin.

Copy link
Contributor

@dan-ziv dan-ziv Jun 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

one more thing is - if we will run kaltura player in chromeless mode ({ui: {disable: true}}) the vr navigation won't work since the ui manager isn't created and the following events will not be triggered.

Copy link
Contributor

Choose a reason for hiding this comment

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

@dan-ziv, @yairans we will move this in to the player and then the UI will call player.toggleStereoMode() directly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

OrenMe
OrenMe suggested changes Jun 20, 2018
View reviewed changes
src/common/plugins/plugins-config.js Outdated
@@ -12,7 +12,8 @@ function evaluatePluginsConfig(options: KalturaPlayerOptionsObject): void {
if (options.plugins) {
const dataModel: Object = {
pVersion: __VERSION__,
pName: __NAME__
pName: __NAME__,
target: options.targetId
Copy link
Contributor

Choose a reason for hiding this comment

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

target is too generic term, suggest to change to domRootElementId

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/common/ui-wrapper.js Outdated
}

_setStereoConfig(config: KalturaPlayerOptionsObject): void {
if (config.plugins.vr.toggleStereo || (Env.device.type && config.plugins.vr.toggleStereo !== false)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure I understand this condition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OrenMe toggleStereo is false by default for desktop and true for mobile. is it clear?

src/kaltura-player.js Outdated
@@ -23,12 +23,14 @@ export default class KalturaPlayer {
this._logger = getLogger('KalturaPlayer' + Utils.Generator.uniqueId(5));
this._uiWrapper = new UIWrapper(this._player, options.ui);
this._provider = new Provider(options.provider, __VERSION__);
this._uiWrapper.handleVr(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

why not handle this in the uiWrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

src/kaltura-player.js Outdated
@@ -77,4 +79,16 @@ export default class KalturaPlayer {
set: undefined
};
}

_addBindings(): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

@dan-ziv, @yairans we will move this in to the player and then the UI will call player.toggleStereoMode() directly

yairans added a commit to kaltura/playkit-js-vr that referenced this pull request Jun 20, 2018
@yairans
feat(FEC-8046): 360 Support (#1)
65d7856 
dependes on:
kaltura/playkit-js-providers#55
kaltura/kaltura-player-js#130
kaltura/playkit-js#239
kaltura/playkit-js-ui#242
@yairans yairans merged commit cec9ea6 into master Jun 20, 2018
@yairans yairans deleted the FEC-8046 branch June 20, 2018 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-ziv dan-ziv dan-ziv requested changes

@OrenMe OrenMe OrenMe requested changes

@itaykinnrot itaykinnrot Awaiting requested review from itaykinnrot

@odedhutzler odedhutzler Awaiting requested review from odedhutzler

Assignees

@yairans yairans

Labels
Priority: High Type: Feature
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yairans @OrenMe @dan-ziv