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

odedhutzler · 2018-01-31T19:15:10Z

Description of the Changes

Added jsonp configuration and a helper module to configure it in the player.
the configuration has 2 fields:

the jsonp handler function.
should we use the jsonp 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

itaykinnrot · 2018-02-04T07:25:07Z

src/common/utils/jsonp-helper.js

+
+  const uriHost = getParsedUri(uri).hostname;
+  const flavorUriHost = getParsedUri(data.flavors[0].url).hostname
+  if (data.flavors.length === 1 && uriHost !== flavorUriHost) {


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

itaykinnrot · 2018-02-04T07:25:38Z

src/common/utils/jsonp-helper.js

+ * @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")){


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

can we do better then browser sniffing?

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

itaykinnrot · 2018-02-04T07:26:40Z

src/common/utils/jsonp-helper.js

+function configureJsonp(config: Object): void{
+  config.playback = config.playback || {};
+  config.playback.options = config.playback.options || {};
+  config.playback.options.html5 = config.playback.options.html5 || {};


@odedhutzler we have a function for that please use it

OrenMe · 2018-02-04T19:00:37Z

src/common/utils/jsonp-helper.js

+ * @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")){


can we do better then browser sniffing?

OrenMe · 2018-02-04T19:03:06Z

src/common/utils/jsonp-helper.js

+ * @returns {void}
+ */
+function configureJsonp(config: Object): void{
+  config.playback = config.playback || {};


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

OrenMe · 2018-02-04T19:05:32Z

src/kaltura-player.js

@@ -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);


move to default options getDefaultOptions?

dan-ziv · 2018-02-04T21:12:48Z

src/kaltura-player.js

@@ -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);


You are accessing a private member - you can't do it.

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.

dan-ziv · 2018-02-04T21:15:45Z

src/common/utils/jsonp-helper.js

+ * @param {Object} config - configuration relevant to jsonp
+ * @returns {void}
+ */
+function configureJsonp(config: Object): void{


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).

dan-ziv · 2018-02-04T21:18:02Z

src/common/utils/jsonp-helper.js

+  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;


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

dan-ziv · 2018-02-04T21:19:20Z

src/common/utils/jsonp-helper.js

+    return {
+      'hostname': parser.hostname,
+      'uri': uri
+    }


missing semicolon

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

dan-ziv · 2018-02-04T21:19:44Z

src/common/utils/jsonp-helper.js

+ */
+function getDirectManfiestUri(data: Object, uri: string): string {
+  const getParsedUri = uri => {
+    const parser = document.createElement('a');


parser? looks like an element

it's purpose is to parse the url

dan-ziv · 2018-02-04T21:25:24Z

src/common/utils/jsonp-helper.js

+  };
+
+  const uriHost = getParsedUri(uri).hostname;
+  const flavorUriHost = getParsedUri(data.flavors[0].url).hostname


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

dan-ziv · 2018-02-04T21:32:48Z

src/common/utils/jsonp-helper.js

+    config.playback.options.html5.dash.useJsonp = true;
+    config.playback.options.html5.dash.callback = getDirectManfiestUri;
+  }
+  if (shouldUseJsonp(config.playback.options.html5.hls)){


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.

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

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

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

dan-ziv · 2018-02-04T21:42:42Z

src/common/utils/jsonp-helper.js

+ * @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")){


Moving the login into setup.js

…into FEC-7815

dan-ziv · 2018-02-28T07:59:27Z

src/common/utils/jsonp-helper.js

+function configureExternalStreamRedirect(config: Object): void {
+  config.playback.options = config.playback.options || {};
+  let adapters = config.playback.options.adapters = config.playback.options.adapters || {};
+  if (typeof adapters.forceRedirectExternalStreams !== "boolean") {


@OrenMe
We should discuss those configuration names and placement.
I think it should be forceRedirectToExternalStreams and under sources config (it related directly to sources).
Adapters is internal and technical name that we've given to the MSEs but not should be exposed as external configuration.

… into FEC-7815

…into FEC-7815

configuring jsonp

3e79b0c

odedhutzler requested review from OrenMe, yairans and dan-ziv January 31, 2018 19:15

odedhutzler self-assigned this Jan 31, 2018

add hls configuration support

c4b4b9d

itaykinnrot reviewed Feb 4, 2018

View reviewed changes

OrenMe suggested changes Feb 4, 2018

View reviewed changes

dan-ziv suggested changes Feb 4, 2018

View reviewed changes

odedhutzler and others added 13 commits February 6, 2018 11:57

Unifing the adapters redirect config.

65e6956

Moving the login into setup.js

change config key name

a177bb4

Merge branch 'master' of https://github.com/kaltura/kaltura-player-js …

0282ff7

…into FEC-7815

merge

0c4f4ea

removed unneccery files

be3e78f

changing the browser sniffing

47b75b7

move to setup defaults

1e5e163

code cleanup

3eb1204

protect against access to null/undefined

1e32fa0

code cleanup

51e3054

Merge branch 'master' into FEC-7815

1a1cdfe

remove test page

9121f62

more concise config name

484767d

dan-ziv reviewed Feb 28, 2018

View reviewed changes

OrenMe added 4 commits February 28, 2018 10:00

renmae file

7facc56

fix condition check

29f8db7

move config location

6794d7a

Merge branch 'master' into FEC-7815

3fcdd5f

OrenMe previously approved these changes Mar 1, 2018

View reviewed changes

dan-ziv dismissed OrenMe’s stale review via 4d5cf0f March 1, 2018 09:08

Update external-stream-redirect-helper.js

4d5cf0f

dan-ziv previously approved these changes Mar 1, 2018

View reviewed changes

Dan Ziv added 2 commits March 4, 2018 16:36

Code cleanup

f45592f

Merge branch 'FEC-7815' of https://github.com/kaltura/kaltura-player-js…

cd9bd83

… into FEC-7815

dan-ziv dismissed their stale review via cd9bd83 March 4, 2018 14:37

Dan Ziv added 5 commits March 4, 2018 16:50

Update external-stream-redirect-helper.js

50f9cf8

Merge branch 'master' into FEC-7815

0a8cf79

Merge branch 'master' of https://github.com/kaltura/kaltura-player-js …

c7ea59e

…into FEC-7815

Fix flow

0f82a4f

Update config names

8556f06

dan-ziv changed the title ~~fix(FEC-7815): adding jsonp plugin~~ fix(FEC-7815): Support playManifest redirects for external streams Mar 6, 2018

Dan Ziv added 2 commits March 6, 2018 13:29

Update setup-helpers.js

878ad3c

Update external-stream-redirect-helper.js

49654b2

dan-ziv merged commit cac2bb6 into master Mar 6, 2018

dan-ziv deleted the FEC-7815 branch March 6, 2018 11:34

OrenMe mentioned this pull request Jun 4, 2018

feat(FEC-8281): support jsonP redirect for OTT #128

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

odedhutzler commented Jan 31, 2018 •

edited by dan-ziv

Loading

itaykinnrot Feb 4, 2018

odedhutzler Feb 5, 2018

itaykinnrot Feb 4, 2018

OrenMe Feb 4, 2018

dan-ziv Feb 4, 2018

odedhutzler Feb 5, 2018 •

edited

Loading

itaykinnrot Feb 4, 2018

odedhutzler Feb 5, 2018

OrenMe Feb 4, 2018

OrenMe Feb 4, 2018

OrenMe Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

odedhutzler Feb 5, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 4, 2018

odedhutzler Feb 5, 2018

dan-ziv Feb 5, 2018 •

edited

Loading

odedhutzler Feb 5, 2018

dan-ziv Feb 4, 2018

dan-ziv Feb 28, 2018 •

edited

Loading

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

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

Conversation

odedhutzler commented Jan 31, 2018 • edited by dan-ziv Loading

Description of the Changes

CheckLists

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

odedhutzler Feb 5, 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

dan-ziv Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dan-ziv Feb 28, 2018 • edited Loading

Choose a reason for hiding this comment

odedhutzler commented Jan 31, 2018 •

edited by dan-ziv

Loading

odedhutzler Feb 5, 2018 •

edited

Loading

dan-ziv Feb 5, 2018 •

edited

Loading

dan-ziv Feb 28, 2018 •

edited

Loading