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

fix(FEC-7815): Support playManifest redirects for external streams #85

Merged
merged 29 commits into from
Mar 6, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
3e79b0c
configuring jsonp
odedhutzler Jan 31, 2018
c4b4b9d
add hls configuration support
odedhutzler Feb 1, 2018
65e6956
Unifing the adapters redirect config.
odedhutzler Feb 6, 2018
a177bb4
change config key name
odedhutzler Feb 6, 2018
0282ff7
Merge branch 'master' of https://github.com/kaltura/kaltura-player-js…
odedhutzler Feb 8, 2018
0c4f4ea
merge
odedhutzler Feb 8, 2018
be3e78f
removed unneccery files
odedhutzler Feb 8, 2018
47b75b7
changing the browser sniffing
odedhutzler Feb 8, 2018
1e5e163
move to setup defaults
OrenMe Feb 28, 2018
3eb1204
code cleanup
OrenMe Feb 28, 2018
1e32fa0
protect against access to null/undefined
OrenMe Feb 28, 2018
51e3054
code cleanup
OrenMe Feb 28, 2018
1a1cdfe
Merge branch 'master' into FEC-7815
OrenMe Feb 28, 2018
9121f62
remove test page
OrenMe Feb 28, 2018
484767d
more concise config name
OrenMe Feb 28, 2018
7facc56
renmae file
OrenMe Feb 28, 2018
29f8db7
fix condition check
OrenMe Feb 28, 2018
6794d7a
move config location
OrenMe Feb 28, 2018
3fcdd5f
Merge branch 'master' into FEC-7815
OrenMe Feb 28, 2018
4d5cf0f
Update external-stream-redirect-helper.js
Mar 1, 2018
f45592f
Code cleanup
Mar 4, 2018
cd9bd83
Merge branch 'FEC-7815' of https://github.com/kaltura/kaltura-player-…
Mar 4, 2018
50f9cf8
Update external-stream-redirect-helper.js
Mar 4, 2018
0a8cf79
Merge branch 'master' into FEC-7815
Mar 5, 2018
c7ea59e
Merge branch 'master' of https://github.com/kaltura/kaltura-player-js…
Mar 6, 2018
0f82a4f
Fix flow
Mar 6, 2018
8556f06
Update config names
Mar 6, 2018
878ad3c
Update setup-helpers.js
Mar 6, 2018
49654b2
Update external-stream-redirect-helper.js
Mar 6, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 62 additions & 0 deletions src/common/utils/jsonp-helper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// @flow
import {Env} from 'playkit-js'

/**
* jsonp callback function, returns the direct manifest uri
* @param {Object} data - the json object that returns from the server
* @param {string} uri - original request uri
* @returns {string} returns the direct uri
*/
function getDirectManfiestUri(data: Object, uri: string): string {
const getParsedUri = uri => {
const parser = document.createElement('a');
Copy link
Contributor

Choose a reason for hiding this comment

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

parser? looks like an element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's purpose is to parse the url

parser.href = uri;
return {
'hostname': parser.hostname,
'uri': uri
}
Copy link
Contributor

Choose a reason for hiding this comment

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

missing semicolon

Copy link
Contributor

Choose a reason for hiding this comment

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

Why to return an object if u use only the hostname field outside?

};

const uriHost = getParsedUri(uri).hostname;
const flavorUriHost = getParsedUri(data.flavors[0].url).hostname
Copy link
Contributor

Choose a reason for hiding this comment

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

unsafe access to object data.flavors[0].url. check for existence of fields.

if (data.flavors.length === 1 && uriHost !== flavorUriHost) {

Choose a reason for hiding this comment

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

@odedhutzler please document this part inline - since this logic is not very clear at first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

return data.flavors[0].url;
} else {
return uri;
}
}

/**
* returns if we should use our jsonp http plugin
* @param {Object} config - configuration relevant to jsonp
* @returns {boolean} should or not use jsonp requests on manifests
*/
function shouldUseJsonp(config: Object): boolean{
if ((config && config.useJsonp) || Env.browser.name.includes("IE") || Env.browser.name.includes("Edge")){

Choose a reason for hiding this comment

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

@odedhutzler we already know that Panasonic browser need it too - please add

Copy link
Contributor

Choose a reason for hiding this comment

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

can we do better then browser sniffing?

Copy link
Contributor

Choose a reason for hiding this comment

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

beautify

Copy link
Contributor Author

@odedhutzler odedhutzler Feb 5, 2018

Choose a reason for hiding this comment

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

@OrenMe I can change it to more configured... like
config.playback.options.adapters.forceRedriectBrowsers.includes(Env.browser.name) // "IE,Edge".includes(Env.browser.name)

return true;
}
return false;
}

/**
* add jsonp configuration to the general config
* @param {Object} config - configuration relevant to jsonp
* @returns {void}
*/
function configureJsonp(config: Object): void{
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think configureJsonp is a good name, this is like to call a method configureXHR. JSONP is a generic method for sending JSON data. Try to think what is the real purpose here (redirect for external streams, for example).

Copy link
Contributor

Choose a reason for hiding this comment

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

beautify

config.playback = config.playback || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

We should align this as general adapter config and not via direct and specific HLS or DASH settings.
try to generalise the setting.

config.playback.options = config.playback.options || {};
config.playback.options.html5 = config.playback.options.html5 || {};

Choose a reason for hiding this comment

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

@odedhutzler we have a function for that please use it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

if (shouldUseJsonp(config.playback.options.html5.dash)){
config.playback.options.html5.dash = config.playback.options.html5.dash || {};
config.playback.options.html5.dash.useJsonp = true;
config.playback.options.html5.dash.callback = getDirectManfiestUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

Too generic names for configurations (see my suggestion bellow).

}
if (shouldUseJsonp(config.playback.options.html5.hls)){
Copy link
Contributor

Choose a reason for hiding this comment

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

why separate config for hls and dash?
I don't even think its an engine dependent.

{
playback: {
  options: {
    tryRedirectForExternalStreams: true,
    redirectForExternalStreamsHandler: () => {....}
    html5: {
    }
   }
 }
}

Not even sure it should be under options, need to discuss the placement of this anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree, though i must add the handler to the dash/hls config

Copy link
Contributor

@dan-ziv dan-ziv Feb 5, 2018

Choose a reason for hiding this comment

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

why?
in the createAdapter method of each adapter you get the whole player config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought it gets the specific config, but you are right, i will change it.

config.playback.options.html5.hls = config.playback.options.html5.hls || {};
config.playback.options.html5.hls.useJsonp = true;
config.playback.options.html5.hls.callback = getDirectManfiestUri;
}
}

export {configureJsonp}
2 changes: 2 additions & 0 deletions src/kaltura-player.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
} from './common/utils/setup-helpers'
import {evaluatePluginsConfig} from './common/plugins/plugins-config'
import './assets/style.css'
import {configureJsonp} from './common/utils/jsonp-helper'

export default class KalturaPlayer {
_player: Player;
Expand All @@ -22,6 +23,7 @@ export default class KalturaPlayer {
constructor(options: KalturaPlayerOptionsObject) {
this._player = loadPlayer(options.player);
this._logger = getLogger('KalturaPlayer' + Utils.Generator.uniqueId(5));
configureJsonp(this._player._config);
Copy link
Contributor

Choose a reason for hiding this comment

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

move to default options getDefaultOptions?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. You are accessing a private member - you can't do it.
  2. If you want to manipulate the configuration of the player you need to move the configureJsonp to the setup workflow, before the kaltura player has been created.

this._uiManager = new UIManager(this._player, options.ui);
setUITouchConfig(options.ui.forceTouchUI, this._uiManager);
this._provider = new Provider(options.provider, __VERSION__);
Expand Down