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

Setting to turn on subtitles by default #4450

Merged

Conversation

kommunarr
Copy link
Collaborator

@kommunarr kommunarr commented Dec 13, 2023

Setting to turn on subtitles by default (re-added)

Pull Request Type

  • Feature Implementation

Related issue

closes #62

Description

When enabled, this feature turns on closed captions for a video whenever any are available, grabbing the first one in the sorted list. The YT behavioral analogue is "Always show captions" enabled and "Include auto-generated captions (when available)" enabled. Just as in YT, this will show the top caption result from the list even when it's not in your language. The setting is disabled by default.

Testing

  • Enable setting in Player Settings. Test this video with 1) Spanish (español (MX)) as locale and 2) German (Deutsch) as locale with a) Local API as primary and b) Invidious API as primary. See captions showing in:
    1a. Spanish
    1b. Spanish
    2a. German (auto-translated)
    2b. English

These are the same top locales as with the feature disabled.

Desktop

  • OS: OpenSUSE Tumbleweed
  • OS Version: 2023xxxx
  • FreeTube version: 0.19.1

Additional context

Things I noticed along the way:

  • We don't seem to have a visual indicator on the video player icon for captions when it's enabled, which is not good
  • We have a pre-existing bug with the auto-generated captions in the user's locale not being presented as an option for certain videos like this one.
    • More information: Seems to be what YT is sending back as the default options is the problem. In YT Web, you have to click "Auto-translate" and select your preferred output language to see your expected auto-generated captions for your locale.
  • Minor visual bug on ft-subscribe-button in specific views fixed in one line in PR Minor ft-subscribe-button visual bugfix #4451 (can be seen on this video).
  • A contributor accidentally set the Hungarian locale name to "English (US)".
  • I've already mentioned this before, but due to us not translating video player labels, an auto-generated caption in language X appears locale name followed by a localized (auto-generated from English) label.

@FreeTubeBot FreeTubeBot enabled auto-merge (squash) December 13, 2023 15:50
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Dec 13, 2023
@kommunarr kommunarr force-pushed the feat/add-subtitles-by-default branch from a95fe24 to 84ccce6 Compare December 13, 2023 15:51
@kommunarr kommunarr force-pushed the feat/add-subtitles-by-default branch from 84ccce6 to 6ee911c Compare December 13, 2023 15:52
@absidue
Copy link
Member

absidue commented Dec 13, 2023

We don't seem to have a visual indicator on the video player icon for captions when it's enabled, which is not good

Fixed in the player migration, the new player adds a diagonal line through the icon.

We have a pre-existing bug with the auto-generated captions in the user's locale not being presented as an option for certain videos like this one.

FreeTube currently only adds a translated track if the video is in English. I fixed this in the player migration, it could be done separately but as it requires removing a translation string and adding a new one it makes sense to do it at the same time as the rest of the string changes for the player.

I've already mentioned this before, but due to us not translating video player labels, an auto-generated caption in language X appears locale name followed by a localized (auto-generated from English) label.

This will likely continue to stay the same in the future (just without English hard-coded, see paragraph above), as the caption track labels are customisable by the uploader. Invidious tries to translate them, but when the uploader has customised the label you end up with multiple tracks with the same label and because they use the label in their API, you can't even request some of the tracks.

@kommunarr
Copy link
Collaborator Author

kommunarr commented Dec 13, 2023

We have a pre-existing bug with the auto-generated captions in the user's locale not being presented as an option for certain videos like this one.

FreeTube currently only adds a translated track if the video is in English. I fixed this in the player migration, [...]

Interesting, thanks & good to know! I do think there's a separate YouTube Web bug with that that you can see with this video in your browser where auto-translated German is the only default option, when I believe it should have auto-translated [currently selected locale] as the top default option. Although I'm not too knowledgeable about the intended behaviors of their system, so maybe this is intended and/or some odd edge case.

This will likely continue to stay the same in the future (just without English hard-coded, see paragraph above), as the caption track labels are customisable by the uploader. Invidious tries to translate them, but when the uploader has customised the label you end up with multiple tracks with the same label and because they use the label in their API, you can't even request some of the tracks.

Interesting, could we do it for the Local API or if it matches the label of "locale name"? For reference, I'm referring to this "locale name" string (along with the language selections, which we can try to translate after we sort them):
Screenshot_20231213_133009

@absidue
Copy link
Member

absidue commented Dec 13, 2023

Them only showing the German (auto-generated) track is not a bug, YouTube only shows you the actual caption tracks in the track list. Automatically adding a translated one is a FreeTube thing, because we don't have an equivalent of YouTube's extra menu's that let you translate any of the tracks, so we just pick one, translate it and add it to the list.

Okay that string is definitely weird, not sure why we would even have a string that just outputs "locale name" in FreeTube.

@absidue
Copy link
Member

absidue commented Dec 13, 2023

The auto-generated track is the result of YouTube running a sound recognition tool over the audio (I'm guessing that's what asr means, as it doesn't just transcribe text) on the audio, so it will always be in the main language of the video/the language that the uploader specified that the video is in.

On YouTube there are 3 types of captions, the auto-generated ones, auto-generated ones that were edited by the uploader (have the caps URL parameter set to asr like the auto-generated ones but don't have the kind parameter set to that) and user uploaded ones.

@efb4f5ff-1298-471a-8973-3d47447115dc

Should this be reviewed after shaka?

@absidue
Copy link
Member

absidue commented Dec 13, 2023

Nope, we discussed it and because I already have logic to restore the selected caption track when you change formats, this should be about a 3 line change to the shaka stuff, so it's fine to merge before.

absidue
absidue previously approved these changes Dec 13, 2023
@PikachuEXE
Copy link
Collaborator

Can't see any subtitle with Invidious API (though there is a selection menu, tried all options)

image

@@ -179,12 +179,12 @@ const state = {
defaultProfile: MAIN_PROFILE_ID,
defaultQuality: '720',
defaultSkipInterval: 5,
defaultSubtitles: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

defaultSubtitles makes me feel like default something (e.g. locales) for subtitles, but never linked to "enabled"

Suggested change
defaultSubtitles: false,
enableSubtitlesByDefault: false,

@kommunarr
Copy link
Collaborator Author

Can't see any subtitle with Invidious API (though there is a selection menu, tried all options)

image

Can you share the Invidious instance? I'd be surprised if this change broke subtitles somehow.

@PikachuEXE
Copy link
Collaborator

@kommunarr
Copy link
Collaborator Author

kommunarr commented Dec 14, 2023

I can consistently replicate the issue on this branch, but I can also do so on the development branch. EDIT: was testing with legacy format. Seems to be working on all instances with DASH & Audio formats.

PikachuEXE
PikachuEXE previously approved these changes Dec 14, 2023
Copy link
Collaborator

@PikachuEXE PikachuEXE left a comment

Choose a reason for hiding this comment

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

If not branch issue then not related

@kommunarr kommunarr requested a review from absidue December 14, 2023 00:36
@absidue
Copy link
Member

absidue commented Dec 14, 2023

Public Invidious instances get ratelimited on YouTube's captions endpoint, it's been an issue for years iv-org/invidious#2567. They have a workaround that uses the transcripts API and then generates a captions file from that, however instances have to turn on the relevant config option for that iv-org/invidious#2567 (comment). Because the transcripts API only returns the raw text and timings, you miss out on all the styling information that the captions might have, so instances that don't get ratelimited, eg. private ones, probably prefer to stick with the captions endpoints.

@efb4f5ff-1298-471a-8973-3d47447115dc

User gets to see that 2 captions are selected when locale is set to US

VirtualBoxVM_JbMmPwtThC.mp4

@kommunarr
Copy link
Collaborator Author

@efb4f5ff-1298-471a-8973-3d47447115dc Interesting - seems to be a form of an older bug, where when you have a non-primary track active, but press "C" on your keyboard, the old one still (incorrectly) shows as visually active. Looking into it.

… & fix similar pre-existing bug

Also fixes pre-existing bug with pressing 'c' having the effect of multiple tracks (inaccurately) showing as selected.
@kommunarr kommunarr force-pushed the feat/add-subtitles-by-default branch from 6b058f8 to b540e4c Compare December 16, 2023 01:35
@kommunarr
Copy link
Collaborator Author

I was able to fix the old bug & the behavior @efb4f5ff-1298-471a-8973-3d47447115dc mentioned, but the latter is achieved through setting a default property on the first text track, so this might be a tinge more Shaka work @absidue.

@absidue
Copy link
Member

absidue commented Dec 16, 2023

It won't require any extra changes, that bug doesn't happen for the audio and DASH formats with shaka. For the legacy ones, it only happens when enabling a track from the FreeTube code, not when you use the caption track selector, even though as far I can tell the code is the same.

TL;DR I already had some code to fix that bug in the shaka migration, but if it happens with video.js too, maybe it's a chromium bug instead.

@efb4f5ff-1298-471a-8973-3d47447115dc
Copy link
Member

efb4f5ff-1298-471a-8973-3d47447115dc commented Jan 2, 2024

Case 1a selects Spanish auto generated captions instead if the Spanish captions is this expected?

Edit 1: it doesnt do this when i set my locale to Spanish (Espana)

Edit 2: i tried a Mrbeast vid with local API, locale set to Deutsch and it also picked the auto-generated instead of the normal german captions

@absidue
Copy link
Member

absidue commented Jan 2, 2024

@efb4f5ff-1298-471a-8973-3d47447115dc When you say "auto-generated", do you mean "auto-translated" (just asking because those mean very different things and I'm a bit confused by your message)?

@kommunarr
Copy link
Collaborator Author

Case 1a selects Spanish auto generated captions instead if the Spanish captions is this expected?
Edit 2: i tried a Mrbeast vid with local API, locale set to Deutsch and it also picked the auto-generated instead of the normal german captions

This is a pre-existing problem with how we're sorting captions. The logic for this PR is relying on our sorting to deliver the most appropriate subtitles (the 1st in the list). I can look into it, but it's technically out of scope for this PR.

@absidue
Copy link
Member

absidue commented Jan 2, 2024

Personally I think that is out of scope of this pull request. In it's current state this pull request does what it says on the tin, which is select the first caption track, which is the same as the c key does.

@kommunarr kommunarr mentioned this pull request Jan 2, 2024
1 task
@FreeTubeBot FreeTubeBot merged commit bed4af1 into FreeTubeApp:development Jan 3, 2024
5 checks passed
@github-actions github-actions bot removed the PR: waiting for review For PRs that are complete, tested, and ready for review label Jan 3, 2024
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.

Enable subtitles by default
5 participants