-
Notifications
You must be signed in to change notification settings - Fork 54
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
fix: switch off in-manifest caption support #8
Conversation
ea16342
to
15ca988
Compare
src/toM3u8.js
Outdated
@@ -141,7 +141,8 @@ export const toM3u8 = dashPlaylists => { | |||
master.mediaGroups.AUDIO.audio = organizeAudioPlaylists(audioPlaylists); | |||
} | |||
|
|||
if (vttPlaylists.length) { | |||
// turn off vtt playlist support until issues with VHS are resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we should directly reference VHS here. If it's an issue with VHS, then we can make the change in that repo. mpd-parser should be "independent" when it can be. If it's an issue with how we're handling the parsing in mpd-parser then we can turn it off here and reference that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if it is an issue here, we should add a TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is an issue here and at the moment this parser produces an output that only really makes sense with VHS. I'll add the TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do agree that right now it is primarily geared towards VHS, however, it could be used for other reasons (just as the m3u8-parser can). Either way though, since the issue may be here, it may be best to simple say TODO: vtt playlists not yet supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the note
Right now selection a caption will produce an error, let's turn off this feature until the bugs are worked out.