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.

Sign up for GitHub

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

Jump to bottom

Add support for forced subtitles #2938

Merged
merged 10 commits into from
Oct 27, 2020
Merged

Add support for forced subtitles #2938

merged 10 commits into from
Oct 27, 2020

Conversation

avelad
Copy link
Member

@avelad avelad commented Oct 26, 2020

Add support to forced subtitles in HLS, DASH and src=

Resolves: #2916 & #2122

@avelad
Copy link
Member Author

avelad commented Oct 26, 2020

Stream to test in src= https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8

@avelad
Copy link
Member Author

avelad commented Oct 26, 2020

@joeyparrish I have added the translations of languages ​​that I know. I don't know if it is enough or should I use google translate.

TheModMaker
TheModMaker suggested changes Oct 26, 2020
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

This appears to just parse the setting, not change any behavior. It will show the "forced" label in the track list, but the track won't actually be shown unless the user enables them. Are you going to send another PR to actually show the tracks?

lib/player.js
@@ -3846,6 +3848,9 @@ shaka.Player = class extends shaka.util.FakeEventTarget {
shaka.util.Error.Code.TEXT_ONLY_WEBVTT_SRC_EQUALS,
mimeType);
}
if (forced) {
kind = 'forced';
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this some Apple feature to force the cues to be displayed? Wouldn't it be better to just set the mode to showing? This seams platform-specific and doesn't appear in the HTML spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the way Safari works. I have seen it testing with https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8 and other private streams.

Copy link
Contributor

Choose a reason for hiding this comment

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

The I suggest at least a comment. I'd also be concerned with browser support. This is supposed to be an enum value, so some browsers may throw if we set to an invalid value. Maybe you should set the kind after we set the original value and do it inside a try-catch.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is only used in src = and since you have to create a track element a try-catch is not effective in this case.

Tried an unknown kind in Chrome and it works fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Safari: https://github.com/WebKit/webkit/blob/ae1c6da14bb6bf86cd92d9bfb3d5670043b3c00e/Source/WebCore/html/track/TextTrack.cpp#L134-L150

Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=271185

More info: https://github.com/whatwg/html/iss

Copy link
Member

Choose a reason for hiding this comment

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

Here's the correct html spec issue link: whatwg/html#4472

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest replacing the comment with a link to the HTML spec issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

lib/dash/dash_parser.js Show resolved Hide resolved
ui/locales/source.json Outdated Show resolved Hide resolved
lib/util/stream_utils.js
@@ -473,6 +475,9 @@ shaka.util.StreamUtils = class {
if (textTrack.kind) {
track.roles = [textTrack.kind];
}
if (textTrack.kind == 'forced') {
track.forced = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Like above, is this Apple-specific? I don't see anything in the HTML spec for this. Do you have any documentation you could link to, or is this just from trying it?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the way Safari works. I have seen it testing with https://devimages.apple.com.edgekey.net/streaming/examples/bipbop_16x9/bipbop_16x9_variant.m3u8 and other private streams.

@TheModMaker
Copy link
Contributor

And yes, the translations are fine. You just need to edit the source.json and en.json, we'll send it to the translation team to update the rest.

@avelad
Copy link
Member Author

avelad commented Oct 26, 2020

This appears to just parse the setting, not change any behavior. It will show the "forced" label in the track list, but the track won't actually be shown unless the user enables them. Are you going to send another PR to actually show the tracks?

Yes, my idea is to send another PR to handle this behavior. But I want to go little by little and without anything breaking.

@avelad
Copy link
Member Author

avelad commented Oct 27, 2020

@TheModMaker Are any more changes necessary to approve this RP?

TheModMaker
TheModMaker approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

Just change the comments and LGTM.

TheModMaker
TheModMaker reviewed Oct 27, 2020
View reviewed changes
ui/language_utils.js Outdated
@@ -95,6 +98,9 @@ shaka.ui.LanguageUtils = class {

span.textContent =
shaka.ui.LanguageUtils.getLanguageName(language, localization);
if (forced) {
span.textContent += ' (' + forcedString + ')';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is mostly overwritten below and the LANGUAGE_ROLE case gets this twice. So I think it would be better to put this after the switch so all cases get it exactly once.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the LABEL format it does not apply, but I have changed it to fit what you say.

TheModMaker
TheModMaker approved these changes Oct 27, 2020
View reviewed changes
Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for working on this.

@shaka-bot
Copy link
Collaborator

All tests passed!

@TheModMaker TheModMaker merged commit 01998f3 into shaka-project:master Oct 27, 2020
@avelad avelad deleted the forced-subtitles branch October 28, 2020 07:13
@avelad avelad mentioned this pull request Oct 28, 2020
Closed
@theRealRobG theRealRobG mentioned this pull request Nov 23, 2021
Closed
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@joeyparrish joeyparrish joeyparrish left review comments

@TheModMaker TheModMaker TheModMaker approved these changes

Assignees
No one assigned
Labels
status: archived Archived and locked; will not be updated
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support HLS FORCED attribute
4 participants
@avelad @TheModMaker @shaka-bot @joeyparrish