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-10669): add ability to pass options to loadMedia request #374

Merged
merged 10 commits into from
Nov 25, 2020

Conversation

dan-ziv
Copy link
Contributor

@dan-ziv dan-ziv commented Nov 22, 2020

Description of the Changes

We have too many times where we want to override the provider config with data for sources object and then we need to explain to people how to do:

1.loadMedia()
2.configure()
3.play()

Instead, let's include the source config for the loadMedia request and add startTime to it (and of course not able to do preload or autoplay in this case).

New loadMedia signature will be:

loadMedia(mediaInfo: ProviderMediaInfoObject, mediaOptions?: KPLoadMediaOptions)

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 requested a review from a team November 22, 2020 11:18
@dan-ziv dan-ziv self-assigned this Nov 22, 2020
mediaConfig.playback = mediaConfig.playback || {};
mediaConfig.sources = mediaConfig.sources || {};
const {startTime} = mediaOptions;
if (typeof startTime === 'number') {
Copy link
Contributor

@Yuvalke Yuvalke Nov 22, 2020

Choose a reason for hiding this comment

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

@dan-ziv I think it would make a problem cause it'll be saved for the next media as we have today with startTime.
I think the better option is to set startTime on sources instead of playback config and it'll be possible to pass it on setMedia as well for specific media.

Copy link
Contributor

Choose a reason for hiding this comment

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

agree, this will require changes in playkit-js where startTime can come from source.
I would actually remove playkit-js startTime config from playback all together and move it to sources level and align it here in kaltura-player to merge it if it was given on playback config with deprecated warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

after discussing with @dan-ziv we should handle this in separate PR.
This PR solves issue with being able to pass mediaOptions in run time without requiring app to do reset, configure, loadMedia and just do loadMedia.
the startTime is indeed an issue to take care of, but as mentioned it will be done separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, we need to add to this PR to remove the playback config from setMedia config as well.

OrenMe
OrenMe previously approved these changes Nov 23, 2020
@OrenMe OrenMe requested a review from Yuvalke November 23, 2020 12:22
@dan-ziv dan-ziv merged commit 1db395a into master Nov 25, 2020
@dan-ziv dan-ziv deleted the FEC-10669 branch November 25, 2020 09:27
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.

3 participants