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

Cleanup MediaPlayerFactory simplifying declarative setup #1138

Merged

Conversation

boushley
Copy link
Member

@boushley boushley commented Feb 6, 2016

This should resolve #1136

This also has new instructions that should replace #1135

@wilaw @AkamaiDASH any input on this would be great. I feel like this simplifies the overall usage of dash.js, especially for users who don't want / need any custom settings.

@wilaw
Copy link
Member

wilaw commented Feb 6, 2016

@boushley - this is looking good and I like it. My one concern is around the removal of the explicit call to initiate the dash.js players. We did discuss when this was first implemented a year ago, and backed away from that for the following reason: with this approach we cannot control when in the page load cycle our scripts are going to get executed. In allowing us to set window.addEventListener('load', loadHandler); we get in line with all the other scripts that are waiting to execute on that event and the order of execution is indeterminate. This could affect other scripts that wanted to have elements in place (like poster images, or analytics plugins etc) that needed to be called after dash.js was created. We provided the original method

onload="dashjs.MediaPlayerFactory.createAll()"

not because we wanted to require using onLoad , but just as a handy means of demonstrating that the page author can control when in the page build process that the creation occurs.

Perhaps the answer here is this approach is for the simple implementation when exact order is unimportant. It's certainly clean and lightweight. Anyone requiring an exact load order can revert to the default implementation of

player = MediaPlayer().create();
player.initialize(video, url, true);

which provides them full control. With that understanding, I would accept this PR.

Cheers

Will

@dsparacio
Copy link
Contributor

@boushley so this cleans up the code a bit and removes the need for adding selector and will find in DOM which is great. I have reservations about the rename. I think People that want to use this may not understand what it is. I personally prefer mediaplayerfactory for this one since it creates media players.

@boushley
Copy link
Member Author

boushley commented Feb 6, 2016

@AkamaiDASH I can change the name back, no big deal.

@wilaw you can still initiate this in the same way. You just have to add something like:

<script> window.dashjs = { skipAutoCreate: true }; </script>

before you include the dash.js library. If you do then the auto creation won't happen. And then you can call the create function the same as before when you want to. I figure this is going to be most useful for the simple case, so making it easier for the simple case and more complicated for the complex case seems like a good way to go.

Seem reasonable?

@wilaw
Copy link
Member

wilaw commented Feb 6, 2016

@boushley - yes. If someone is going to go to the trouble to flag their video attribute and then disable the auto-creation, they may as well use the two lines it takes to explicitly create the player in the first place. So let's roll with this as you have it as it makes it super-simple for unsophisticated users.

dsparacio pushed a commit that referenced this pull request Feb 8, 2016
Cleanup MediaPlayerFactory simplifying declarative setup
@dsparacio dsparacio merged commit 0b9edbc into Dash-Industry-Forum:development Feb 8, 2016
@boushley boushley deleted the declarative-setup branch February 8, 2016 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup MediaPlayerFactory API
3 participants