-
Notifications
You must be signed in to change notification settings - Fork 55
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 #915
base: master
Are you sure you want to change the base?
Conversation
Thanks a lot @skjnldsv (and @beardhatcode)! I now have setup a dedicated test environment with a dedicated server separate from my production instance. --> I was able to check that the modifications to Videos.vue do not break master. I hope to make some progress this weekend. |
[ Intermediate status report ] Hi @skjnldsv and @beardhatcode. I believe I have gathered most of the pieces I will need to implement the requested changes and I am now confident that I will be able to get it to completion. I have installed xdebug so I can breakpoint/watch the server-side code from vscode, plus I use chrome's builtin javascript debugger for the vue code on client side. I use apps/forms's ApiController.php and SubmissionService.php for inspiration. Currently I have three new/modified files:
On client side, I use generateUrl() and axios() to call the server, and I package the filename as a query parameter to the url. On server side, I use OCP\Files\File to get access to the filesystem utility functions, and I use OCP\IL10N to retrieve factory.this.availableLanguages. So far I have successfully implemented roundtrip communication between client and server, passing parameters to the server and retrieving the response on client side. |
One question still: From OCP\IL10N I can retrieve a list of abbreviated language names (e.g.: en, fr). |
You can push them, so we can review and comment :) |
You have those in OCP\L10N\IFactory |
OK. I'll try and push later today. |
It's sunday, don't pressure yourself! 🤗 |
I have just pushed to feat/video-caption. A couple of questions as comments in 'VideoController.php' -- about @nnotations (I don't understand what they do). Looking forward to studying your review. |
lib/Controller/VideoController.php
Outdated
$rootName = pathinfo($video->getFileInfo()->getPath())['filename']; | ||
$rootLen = strlen($rootName); | ||
// eg: $rootName is 'movie', $rooLen is 5 | ||
$files = $video->getParent()->getDirectoryListing(); |
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.
What does this line return when only the video file is shared? (And not its folder)
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.
Good question.
I made a test with the following setup: Alice shares 'movie.mkv' with John, but does not share 'movie.en.vtt'.
When john open the video for viewing, 'getDirectoryListing()' returns a shorter list of files, including 'movie.mkv', but not 'movie.en.vtt'. As a result, John does not have access to captions.
Does this answer the question? or maybe you had a different setup in mind?
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 had the public share setup in mind. I don't know what getParent
returns if the video would be shared with a link
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.
Good call for the public side of this feature
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.
@beardhatcode I did a second test today.
Sharing 'movie.mkv' (and only this file) using federation sharing -- john@https://sandbox.nc.com (remote)
There was no problem for john viewing the movie (without captions of course).
Single stepping the php controller,getParent()
seems to return the "local" parent folder, not the "remote" parent folder where the file is actually hosted.
Btw, is there a specification of the expected behavior for getParent()
, including exception cases and/or error return values?
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.
See the code: https://github.com/nextcloud/server/blob/master/lib/public/Files/Node.php#L234 you can click on getParrent to find all the implementations of it for various types of files
The "normal" one is here: https://github.com/nextcloud/server/blob/f2101487a0a32cd5be5068329a2c7cd6d8a1583f/lib/private/Files/Node/Node.php#L282-L288 , it appears that the root folder is returned whenever the dirname cannot be obtained. I can't figure out what happens when te file is shared, in the share20 folder "setParent" is marked as deprecated ... (@skjnldsv do you know how this works?)
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 think it is OK then.
The path is always from the point of view of the local user file space.
Regarding the "normal" implementation, I understand it maps "" (empty path), ".", and "/" to the root directory of the local user. (It does not necessarily mean that the dirname could not be obtained.)
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.
@beardhatcode I believe I took most of the review comments into account.
Is there more to be done about the getParent
issue?
@beardhatcode @skjnldsv I suppose we want some non-regression test to be added to viewer's automated test suite? Maybe we can setup a mini video folder, have viewer "render" the video and parse the resulting "Elements" (as in chrome -> inspect). E.g.:
-> Parse the resulting HTML5 for correctness: checking video.en.vtt and .video.fr.vtt are present as tracks and nothing else.
|
@beardhatcode @skjnldsv what about user documentation? Should the feature be mentioned in the Github's README for the viewer? |
Yes
I think we should do this by querying the captions of the plyr object. I think that is the best way to test this.
It is different for every app, but since you added PHP code, we will also need to add some PHP test (something like this). We'll also need to add video.mkv.spec.js.
Yes, see the video.mkv.spec.js
We should have PHP and cypress tests |
Oh and you should also test video's and captions with odd names like:
|
I have been thinking, wouldn't it be better to use the fileid in the API (no escaping issues). And, with @CarlSchwan search suggestion we can try all the locales :). I was also wondering what:
gives on public shares. I might continue working on this over the next month or so (if not, and you are reading this in 2 months, feel free to continue the work 😅) |
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 <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Instead fetchTracks() can be called from options(). Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
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 <[email protected]>
(Templates are not made to call methods.) Signed-off-by: Frederic Ruget <[email protected]>
Move folder-analysis and mapping-of-locales-to-languages code to server-side. Big thanks to @skjnldsv and @beardhatcode for their help and guidance. Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
If there are 10k+ files in the folder, we prefer not to have to "manually" iterate through all or them. Prefiltering by locale allows to limit the search to approximately 400 files. Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Frederic Ruget <[email protected]>
Signed-off-by: Carl Schwan <[email protected]>
d6bfa74
to
305c63b
Compare
I think it would be awesome if you could add support for looking for these .vtt files in hidden directories of the same name as the video file. I.e. :
I believe that there is benefit to having auxillary files to the videos (captions, thumbnail and such) all be hidden and grouped, the first to not have extra things visible in the files view, the second to not create chaos when people choose to view hidden files to add or change the auxillary files. Imagine 6 translations per video + thumbnail etc. - unless every video is in its own directory, which I don't think many have, this will be hard to navigate. I believe supporting this subdirectory approach is the way to go, also for thumbnails (and different qualities if that gets implemented) but that's not relevant to this pull req. |
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 [email protected]