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(FEC-10656): set forceRedirectExternalStreams as true by default on IE11 and Chromecast #370

Merged
merged 10 commits into from
Nov 25, 2020

Conversation

yairans
Copy link
Contributor

@yairans yairans commented Nov 12, 2020

Description of the Changes

implement getDefaultRedirectOptions in ovp and ott player (void impl in ott)

Solves FEC-10656, KMS-22502

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 changed the title feat(FEC-10656): set forceRedirectExternalStreams as true by default on IE11 feat(FEC-10656): set forceRedirectExternalStreams as true by default on IE11 and Chromecast Nov 12, 2020
*/
export function getDefaultRedirectOptions(options: KPOptionsObject): Object {
const configObj = {};
if (Env.browser.name === 'IE' || Env.device.model === 'Chromecast') {
Copy link
Contributor

Choose a reason for hiding this comment

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

limit also to just live

if (typeof forceRedirectExternalStreams !== 'boolean') {
configObj.sources = {
options: {
forceRedirectExternalStreams: true
Copy link
Contributor

Choose a reason for hiding this comment

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

if sources handler wasn't passed set the default one

Copy link
Contributor

@OrenMe OrenMe left a comment

Choose a reason for hiding this comment

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

setup helper for RedirectExternalStreams can be moved and used here only

Comment on lines 20 to 55

/**
* JSONP handler function, returns the direct manifest uri.
* @private
* @param {Object} data - The json object that returns from the server.
* @param {string} uri - Original request uri.
* @returns {string} - The direct uri.
*/
function getDirectManifestUri(data: Object, uri: string): string {
const getHostName = uri => {
const parser = document.createElement('a');
parser.href = uri;
return parser.hostname;
};
// if the json contains one url, it means it is a redirect url. if it contains few urls, it means its the flavours
// so we should use the original url.
const uriHost = getHostName(uri);
let hasOneFlavor = false;
let redirectedUriHost = '';
let redirectedUri = '';
if (data) {
if (data.flavors && Array.isArray(data.flavors)) {
hasOneFlavor = data.flavors.length === 1;
redirectedUriHost = hasOneFlavor && getHostName(data.flavors[0].url);
redirectedUri = data.flavors[0].url;
} else if (data.result) {
hasOneFlavor = true;
redirectedUriHost = getHostName(data.result.url);
redirectedUri = data.result.url;
}
}
if (hasOneFlavor && uriHost !== redirectedUriHost) {
return redirectedUri;
}
return uri;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest we keep this in original file if only for sake of git history.

@yairans yairans merged commit 51d8a4a into master Nov 25, 2020
@yairans yairans deleted the FEC-10656 branch November 25, 2020 17:56
yairans added a commit that referenced this pull request Dec 6, 2020
Issue: #370 [moved](https://github.com/kaltura/kaltura-player-js/pull/370/files#diff-b20fda8cac0288af75e0f3c4304df5420584de585d923934f9606e3634d2dbf5L302) the redirect default settings from `setup` to `loadMedia`
so `sources` could be `undefined` in kaltura-player constructor  
Solution: pass empty object to `sources` to the local player if it's `undefined`

Solves FEC-10749
yairans added a commit that referenced this pull request Dec 23, 2020
* Update the sources after `forceRedirectExternalStreams` is set by `loadMedia`
* Save the `player.config.sources.options` for all item unless configured for an item specifically (needed for when the app wants to config the `sources.options` for all playlist items)

Solves FEC-10729

Cont. of #370
yairans added a commit that referenced this pull request Feb 18, 2021
…per (#416)

getting the default `sources.options.redirectExternalStreamsHandler` from common (as before #370)

Solves FEC-10968
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.

2 participants