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

Implementation of WebVTT subtitle support #92

Merged
merged 1 commit into from
May 8, 2023

Conversation

Sgtfishtank
Copy link
Contributor

@Sgtfishtank Sgtfishtank commented Apr 3, 2023

Before the work was done in this PR, we did not have any support for subtitles in hls-vodtolive. This pr will solve that by implementing support for WebVTT subtitles.

How it works is that it takes a subtitle playlist from a VOD and splits it into smaller playlists, similar to how audio and video do it. The biggest difference in how it works is that it looks at each segment and checks if the duration is longer than the corresponding video segment and if it is longer it splits it into smaller segments, where it changes the URI to a predetermined URI where we you can handle the slicing of the actual WebVTT file. The other major difference is that if a VOD without subtitles is loaded after VOD with subtitles, we will create dummy segments with a predetermined URI where you can server empty WebVTT files to ensure that everything runs smoothly if you add a VOD with subtitles later.

In this PR there has also been done a bit of a refactoring to increase the readability of the code.

This PR aims to solve issue #50

index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
@Sgtfishtank Sgtfishtank marked this pull request as ready for review April 5, 2023 20:48
@Sgtfishtank Sgtfishtank requested a review from birme April 5, 2023 20:48
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@birme
Copy link
Contributor

birme commented Apr 15, 2023

Don't know if this is about to be completed soon and ready to be merged to v3-release-candidate branch. I'd recommend to clean up the PR a bit first (use rebase to squash some commits) and rebase this branch with the current head of the v3-release-candidate branch

@Sgtfishtank
Copy link
Contributor Author

Don't know if this is about to be completed soon and ready to be merged to v3-release-candidate branch. I'd recommend to clean up the PR a bit first (use rebase to squash some commits) and rebase this branch with the current head of the v3-release-candidate branch

It should be completed soon, just making sure it is working with eyevinn/channel-engin first, and I will need to fix some of the previous comments first. Then I will rebase.

@Sgtfishtank Sgtfishtank changed the title new implementation subtitle implementation Apr 24, 2023
@Sgtfishtank Sgtfishtank requested a review from birme April 25, 2023 07:08
@tobbee tobbee force-pushed the subtitle-new-implementation branch from 474b0ca to 6352376 Compare April 25, 2023 09:39
@Sgtfishtank Sgtfishtank changed the title subtitle implementation Implementation of subtitle Apr 25, 2023
@Sgtfishtank Sgtfishtank changed the title Implementation of subtitle Implementation of subtitle support Apr 25, 2023
@Sgtfishtank Sgtfishtank changed the title Implementation of subtitle support Implementation of WebVTT subtitle support Apr 25, 2023
@Sgtfishtank Sgtfishtank force-pushed the subtitle-new-implementation branch from bc6b0aa to 18ca43c Compare May 3, 2023 15:18
Copy link
Contributor

@birme birme left a comment

Choose a reason for hiding this comment

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

Just some small changes. Update the readme and have @Nfrederiksen have a glance at it and we are good to merge!

spec/hlsvod_audio_spec.js Outdated Show resolved Hide resolved
spec/hlsvod_audio_spec.js Outdated Show resolved Hide resolved
spec/hlsvod_audio_spec.js Outdated Show resolved Hide resolved
@Sgtfishtank Sgtfishtank force-pushed the subtitle-new-implementation branch from bbf676c to 5038953 Compare May 8, 2023 09:03
Support for VOD with subtitles. A dummy URL can be provided as a fallback when no subtitle segments are available

---------

Co-authored-by: Jonas Birmé <[email protected]>

removed unnecessary files from testvectors and added a new test

added support for starting without subs and added dummy fallback on all vods with subs enabled

added bugfix to aduio

solved issue regarind deltaTime/playhead position

solved bug regarding overwrighting and shifting of subtitltes

chore: removed mac OS file

chore: remove mac OS file

fixed code review comments

updated documentation

forgot to account for capital letters and added a shouldContainSubtitleCheck

removed unused varible
@Sgtfishtank Sgtfishtank force-pushed the subtitle-new-implementation branch from 5038953 to 7226cb9 Compare May 8, 2023 09:19
@birme birme merged commit 5b47807 into v3-release-candidate May 8, 2023
@birme birme deleted the subtitle-new-implementation branch May 8, 2023 09:21
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.

3 participants