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

Add and use generic playlist attribute list parser #107

Merged
merged 4 commits into from Dec 22, 2015
Merged

Add and use generic playlist attribute list parser #107

merged 4 commits into from Dec 22, 2015

Conversation

kanongil
Copy link
Contributor

@kanongil kanongil commented Dec 2, 2015

This fixes order dependent parsing of EXT-X-STREAM-INF attributes.

This also exposes any stream info attribute for external processing.

Tested to work on a normal and encrypted stream in Chrome.

also exposes any stream info attribute for external processing
@mangui
Copy link
Member

mangui commented Dec 2, 2015

I would be keen to merge this, but it would be good to add some unit tests to cover the new parsing code, so that in case it breaks anything or if we modify it later on, we can easily spot any regression.
you can add your test playlist for example, I will complement it with mine ...

@mangui
Copy link
Member

mangui commented Dec 2, 2015

@kanongil
Copy link
Contributor Author

kanongil commented Dec 3, 2015

Updated with tests, which are failing due to old node version on travis.

@kanongil
Copy link
Contributor Author

kanongil commented Dec 4, 2015

Anything you are missing here?

@mangui
Copy link
Member

mangui commented Dec 4, 2015

@kanongil
thanks for the attr-list unit tests.
I need to check that the whole parsing still works with a bunch of playlists, so it will require a little bit more time before I can merge.
best would be to add some unit test on the playlist parsing as well. I know we don't have any at the moment ..

@@ -38,6 +38,8 @@
"webworkify": "^1.0.2"
},
"devDependencies": {
"arraybuffer-equal": "^1.0.4",
"babel": "^5.8.34",
Copy link
Member

Choose a reason for hiding this comment

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

Hi @kanongil i am not clear about this new dependency to babel ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mocha does not understand es6 code and needs a compiler, which I register using --compilers js:babel/register. As such it needs to be declared a dev dependency as well.

Copy link
Member

Choose a reason for hiding this comment

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

fine, I will give it a hand soon

@mangui mangui mentioned this pull request Dec 17, 2015
@mangui mangui merged commit e3c471b into video-dev:master Dec 22, 2015
@mangui
Copy link
Member

mangui commented Dec 22, 2015

merged, I also added a couple of unit tests to cover manifest parsing code ...

johnBartos pushed a commit that referenced this pull request Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants