-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add support for video captions #910
Conversation
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.
Wow, you really reworked this, well done 👍.
Did you try this out on your instance, does it work well?
Converting a string to HTML like this patch suggests at the moment could be dangerous (XSS-attacks). Instead, I suggest you use v-for
(see this link for more info)
If you make changes you can just do them here, I will rewrite the git history to tidy things up afterwards 🙂
Note to self: Things to check before merging when this is reworked:
|
There is still a problem with the test Node / node12.x |
/compile fixup / |
That test means that you generated the JS in the wrong way (not with |
Its weird. This needs to be investigated... |
Should be better now. |
Hey! Thanks for diving into this! EDIT: I mislooked the PR, I saw a glimpse at some code, but seemed to behave properly, ignore previous comment 😉 |
ko: 'Korean', | ||
kr: 'Kanuri', | ||
ks: 'Kashmiri', | ||
ku: 'Kurdish', | ||
kv: 'Komi', | ||
kw: 'Cornish', | ||
ky: 'Kirghiz', | ||
la: 'Latin', | ||
lb: 'Luxembourgish', | ||
lg: 'Ganda', | ||
li: 'Limburgan', | ||
ln: 'Lingala', | ||
lo: 'Lao', | ||
lt: 'Lithuanian', | ||
lu: 'Luba-Katanga', | ||
lv: 'Latvian', | ||
mg: 'Malagasy', | ||
mh: 'Marshallese', | ||
mi: 'Maori', | ||
mk: 'Macedonian', | ||
ml: 'Malayalam', | ||
mn: 'Mongolian', | ||
mr: 'Marathi', | ||
ms: 'Malay', | ||
mt: 'Maltese', | ||
my: 'Burmese', | ||
na: 'Nauru', | ||
nb: 'Norwegian Bokmål', | ||
nd: 'Ndebele, North', | ||
ne: 'Nepali', | ||
ng: 'Ndonga', | ||
nl: 'Dutch', | ||
nn: 'Norwegian Nynorsk', | ||
no: 'Norwegian', | ||
nr: 'Ndebele, South', | ||
nv: 'Navajo', | ||
ny: 'Nyanja', | ||
oc: 'Occitan', | ||
oj: 'Ojibwa', | ||
om: 'Oromo', | ||
or: 'Oriya', | ||
os: 'Ossetian', | ||
pa: 'Panjabi', | ||
pi: 'Pali', | ||
pl: 'Polish', | ||
ps: 'Pushto', | ||
pt: 'Portuguese', | ||
qu: 'Quechua', | ||
rm: 'Romansh', | ||
rn: 'Rundi', | ||
ro: 'Romanian', | ||
ru: 'Russian', | ||
rw: 'Kinyarwanda', | ||
sa: 'Sanskrit', | ||
sc: 'Sardinian', | ||
sd: 'Sindhi', | ||
se: 'Northern Sami', | ||
sg: 'Sango', | ||
si: 'Sinhala', | ||
sk: 'Slovak', | ||
sl: 'Slovenian', | ||
sm: 'Samoan', | ||
sn: 'Shona', | ||
so: 'Somali', | ||
sq: 'Albanian', | ||
sr: 'Serbian', | ||
ss: 'Swati', | ||
st: 'Sotho', | ||
su: 'Sundanese', | ||
sv: 'Swedish', | ||
sw: 'Swahili', | ||
ta: 'Tamil', | ||
te: 'Telugu', | ||
tg: 'Tajik', | ||
th: 'Thai', | ||
ti: 'Tigrinya', | ||
tk: 'Turkmen', | ||
tl: 'Tagalog', | ||
tn: 'Tswana', | ||
to: 'Tonga', | ||
tr: 'Turkish', | ||
ts: 'Tsonga', | ||
tt: 'Tatar', | ||
tw: 'Twi', | ||
ty: 'Tahitian', | ||
ug: 'Uighur', | ||
uk: 'Ukrainian', | ||
ur: 'Urdu', | ||
uz: 'Uzbek', | ||
ve: 'Venda', | ||
vi: 'Vietnamese', | ||
vo: 'Volapük', | ||
wa: 'Walloon', | ||
wo: 'Wolof', | ||
xh: 'Xhosa', | ||
yi: 'Yiddish', | ||
yo: 'Yoruba', | ||
za: 'Zhuang', | ||
zh: 'Chinese', | ||
zu: 'Zulu', |
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.
This most likely already exists, but I can see where maintaining this list could be an issue.
We already maintain such list here: https://github.com/nextcloud/server/blob/9de329a4c2327767d86bd7f594b232eb56af0d01/resources/locales.json
We have a dedicated php method to get those ressources.
I'll suggest we use that instead (you can either have this injected in the page from a InitialState or maybe it's already available
https://github.com/nextcloud/server/blob/9de329a4c2327767d86bd7f594b232eb56af0d01/lib/public/L10N/IFactory.php#L88
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.
Thanks @skjnldsv... so I would like to invoke L10N's php function 'findLanguageFromLocale()' from 'fetchTracks()'.
Please would you provide guidance as to how to import this function into Videos.vue?
public function findLanguageFromLocale(string $app = 'core', string $locale = null)
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.
Why do you need the findLanguageFromLocale ?
All of the locale computation should be done on the php side
const davDir = this.davPath.replace(/[^/]*$/, '') | ||
const videoRoot = this.basename.replace(/[.][^.]+$/, '') | ||
// Create caption tracks for the HTML5 player | ||
// E.g.: <file>.mkv: look for <file>.xx.vtt or .<file>.xx.vtt | ||
const capTracks = [] | ||
for (const file of folder) { | ||
const basename = file.basename | ||
const index = basename.indexOf(videoRoot) | ||
// Consider only file... or .file... | ||
if (!(index === 0 || (index === 1 && basename[0] === '.'))) { | ||
continue | ||
} | ||
const suffix = basename.slice(videoRoot.length + index) | ||
// Consider only ...xx.vtt | ||
if (suffix.search(/^[.]..[.]vtt$/) !== 0) { | ||
continue | ||
} | ||
const lang = suffix.slice(1, 3) | ||
const language = languages[lang] || lang | ||
// an array of objects with src, label and lang keys | ||
capTracks.push({ | ||
src: davDir + basename, | ||
label: language, | ||
srclang: lang, | ||
}) | ||
} | ||
this.tracks = capTracks |
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 wondering if that should not be better to have all of this in a dedicated php Controller that, given a specific fileName, that would return an array of LANG => filename
.
I think the code would benefit from it, especially because the Locale list is managed by a php method.
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.
Not sure how to do that (writing a dedicated php Controller).
Please would you point to a similar Controller in NextCloud source, which I could use as a template?
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.
Yes, look our documentation about ocsControllers :)
https://docs.nextcloud.com/server/latest/developer_manual/basics/controllers.html
https://github.com/nextcloud/forms/blob/master/lib/Controller/ApiController.php
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 think the controller method should:
- Receive the filename as a parameter
- Search for existing files with the same filename+langcode using the list of known locale retrieved via the
findAvailableLocales
of theL10N/IFactory
using Dependency Injection - Return an array of
langcode => filename
so the viewer can use it and build the list accordingly. Should be much faster than our dav endpoint.
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.
OK. I take the challenge :)
By the way, the current proposal does not use the dav endpoint to look for candidate vtt files -- I reckon getFileList() works with the local filesystem?
So we may not see a performance improvement with your proposal.
But in any case the code will be cleaner and easier to maintain.
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.
reckon getFileList() works with the local filesystem?
getFileList is using the dav endpoint.
viewer/src/services/FileList.js
Line 37 in a5eec09
const response = await client.getDirectoryContents(fixedPath, Object.assign({ |
Btw, please change your target branch to master, and fix following potential conflicts 😬 |
Generate HTML5 subtitle tracks dynamically based on the contents of the folder where the video is located. For example: given a movie 'video.mkv', the player will look for VTT subtitle files named 'video.XX.vtt' and '.video.XX.vtt', where XX is the two-letter code for the language. Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Instead fetchTracks() can be called from options(). Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: Frederic Ruget <fred@atlant.is>
Signed-off-by: npmbuildbot-nextcloud[bot] <npmbuildbot-nextcloud[bot]@users.noreply.github.com>
Property track was not properly declared, and sometimes was not reactive. E.g. closing and re-opening the player would make subtitles disappear. Fixing entails declaring tracks in the data() function associated with the Videos vue. Signed-off-by: Frederic Ruget <fred@atlant.is>
(Templates are not made to call methods.) Signed-off-by: Frederic Ruget <fred@atlant.is>
OK, I just tested with master. The viewer builds OK. I don't know how easy it would be for me to setup an additional instance of NextCloud for testing master... and I don't want to break my production instance :) |
@douzebis I rebased your changes on master (you might need to re-clone your repository, or |
@douzebis you should just be able to take the built files of master and use them with stable21. Thats no problem (stable21 is practically the same as master). Does that work for you? |
One other thing you can do @douzebis is run the program |
Well I could not find time to work on the requested changes but I still managed to do a big mess by triggering github's 'fetch upstream' button... and updating my forked version of 'stable21' to 'master' :( :( The last big 'push' is my attempt at reverting to previous stable21 state. Please advise, I can re-fork from scratch and redo the modifications to src/components/Videos.vue, so that you no longer have to deal with this mess... |
Nope, unfortunately it does not work. This may have to do with the components version numbers in 'master' not being compatible with those in 'stable21'? |
@douzebis don't worry about it, just continue your work here, I'll rebase your changes when everything is done |
Alternatively, @douzebis , I can put this branch back on stable21 for you, just let me know. |
Moved and rebased to new branch here, will be much easier. |
fix #239
(add subtitles for video feature #239)
Generate HTML5 subtitle tracks dynamically based on the contents of the folder
where the video is located.
For example: given a movie 'video.mkv', the player will look for VTT subtitle
files named 'video.XX.vtt' and '.video.XX.vtt', where XX is the two-letter
code for the language.
Signed-off-by: Frederic Ruget fred@atlant.is