Skip to content
This repository has been archived by the owner on Jan 12, 2019. It is now read-only.

HLS Tech #62

Merged
merged 14 commits into from
May 21, 2014
Merged

HLS Tech #62

merged 14 commits into from
May 21, 2014

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented May 20, 2014

This is pretty close to being done. The tests pass if run directly but currently karma is being flaky and stalling for some reason.

Moved initializations to init.
Rename and repurpose original init method to initSource which gets
called from #src.
dispose tech, playlist loader
don't trigger loadedmetadata if there was an error
no need for duration update workarounds, add duration to tech
update all the tests
remove irrelevant tests
@@ -7,7 +7,7 @@
<link href="node_modules/video.js/dist/video-js/video-js.css" rel="stylesheet">

<!-- video.js -->
<script src="node_modules/video.js/dist/video-js/video.js"></script>
<script src="node_modules/video.js/dist/video-js/video.dev.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

@gkatsev - Should this be pushed back to non-dev version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably.

return mpegurlRE.test(srcObj.type) || videojs.Flash.canPlaySource.call(this, srcObj);
};

videojs.Hls.supportsNativeHls = (function() {
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed now?

};

videojs.Hls.prototype.duration = function() {
var playlists = this.player().hls.playlists;
Copy link
Member

Choose a reason for hiding this comment

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

There's a number of places where the HLS code accesses itself through the player reference player.hls. I think that's a remnant of the plugin workflow and would be good to move away from that.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably better to internally reference the tech through this but there are some properties and configuration that can be handy to have access to at runtime. It's useful to understand what state the playlist loader is in, for instance.

@gkatsev gkatsev mentioned this pull request May 21, 2014
dmlap added 5 commits May 21, 2014 16:23
Make the withCredentials test synchronous so that karma doesn't hang on it. Pull out test player creation so that it can be re-used. Re-indented player.src invocations.
Get rid of grunt-contrib-qunit because it was inexplicably failing in phantomjs. It's still possible to test in phantom through karma and the tests all pass there. Add test cases for object disposal on the tech and the playlist loader.
The video.js tech selection now manages whether the HLS polyfill is used or not, so we don't need to detect HLS support in the project itself.
When in the scope of the tech, use `this` to reference the tech instead of going through the player.
Using the dev version can lead to accidental dependencies on non-public functionality.
dmlap added a commit that referenced this pull request May 21, 2014
@dmlap dmlap merged commit 50a7806 into master May 21, 2014
@dmlap dmlap deleted the tech2 branch May 21, 2014 21:55
@heff
Copy link
Member

heff commented May 21, 2014

Ran the project through code climate in case anyone's interested. Not sure yet if it's helpful or distracting to know this stuff.
https://codeclimate.com/github/videojs/videojs-contrib-hls

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants