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

Hls live webvtt subtitle #2074

Closed
wants to merge 5 commits into from
Closed

Hls live webvtt subtitle #2074

wants to merge 5 commits into from

Conversation

vlee-harmonicinc
Copy link

This PR will...

  1. subtitle default map 0 to 0 if X-TIMESTAMP-MAP not found
  2. merge subtitle playlist so that subtitle can be found by PTS
  3. update range of fragments (+subtitle buffer) by cue range if the difference of range of fragment and its cues > average duration

Why is this Pull Request needed?

hls.js cannot display live webVTT subtitle

Resolves issues:

hls.js cannot play webVTT subtitle because it

  1. do not sync PTS when there is no X-TIMESTAMP-MAP, while Spec mention:

If a WebVTT segment does not have the X-TIMESTAMP-MAP, the client
MUST assume that the WebVTT cue time of 0 maps to an MPEG-2 timestamp
of 0.

  1. When reload subtitle playlist, it always start from 0 and subtitle-stream-controller unable to find fragment in the reloaded subtitle playlist
  2. fragment.start range maybe incorrect when 1. switching text track, 2. SN of new playlist out of range of previous playlist, 3. delay of loading subtitle playlist during initialization

Are there any points in the code the reviewer needs to double check?

alt. subtitle switching + fail sliding (e.g. can checked by hide subtitle, wait all fragments in the the previous playlist out of range, show subtitle)
When the fragment range is updated by cue range, the range of other fragments are shifted to front because the range of cue usually smaller than the actual fragment range. I increase the subtitle lookup tolerance by duration so that it can still find next subtitle fragment. Is there other design that update fragment range without increase tolerance?
subtitle with fmp4's initPTS
Other type of subtitle

Checklist

  • changes have been done against master branch, and PR does not conflict
  • new unit / functional tests have been added (whenever applicable)
  • API or design changes are documented in API.md

@michaelcunningham19
Copy link
Member

Hey @htleeab !

Thanks for the contribution, do you have a working livestream with WebVTT subtitles we can test your changes with?

@vlee-harmonicinc
Copy link
Author

Hey @htleeab !

Thanks for the contribution, do you have a working livestream with WebVTT subtitles we can test your changes with?

Sorry, I cannot provide my livestream with WebVTT to public due to copyright.

@mtoczko
Copy link
Collaborator

mtoczko commented Jan 29, 2019

I testing your PR and see one problem. After adjust subtitle fragment start time:

[warn] > Adjust subtitle fragment start time [12,undefined] to [22.100000000000023,29.100000000000023]
[warn] > subtitle playlist adjust start by cue PTS [22.100000000000023, 29.100000000000023] of fragment 83

Player skipping next fragment and loads only 'even' segments
2019-01-29 o 16 50 19

@vlee-harmonicinc
Copy link
Author

I testing your PR and see one problem. After adjust subtitle fragment start time:

[warn] > Adjust subtitle fragment start time [12,undefined] to [22.100000000000023,29.100000000000023]
[warn] > subtitle playlist adjust start by cue PTS [22.100000000000023, 29.100000000000023] of fragment 83

Player skipping next fragment and loads only 'even' segments
2019-01-29 o 16 50 19

Thanks for review!
I can repeated this error. I mistakenly thought increasing maxFragLookUpTolerance should always tolerate / accept more fragment, includes previous fragment.
In this commit, I estimate the subtitle fragment end by first cue.start + duration, so it chould match findFragmentByPTS

@stale
Copy link

stale bot commented Apr 9, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale label Apr 9, 2019
@johnBartos
Copy link
Collaborator

johnBartos commented Apr 9, 2019 via email

@stale stale bot removed the Stale label Apr 9, 2019
@johnBartos
Copy link
Collaborator

Closed in favor of #2148. @htleeab, please verify that these changes (as of latest, 0.12.4) are working for you. If not please open a new PR and I'll be happy to review.

@johnBartos johnBartos closed this May 7, 2019
@vlee-harmonicinc
Copy link
Author

Not really work for my stream, but it should be issue of encoder rather than the player. The stream I using do not have WebVTT header or EXT-X-MAP, so it do not match HLS Spec. My branch estimate timestamp from cue time. It is fine to just close this PR since this patch is not strictly follow HLS Spec.

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

4 participants