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

Do not preload subtitles in video.JS #38

Closed
rgaudin opened this issue Sep 16, 2019 · 17 comments · Fixed by #283
Closed

Do not preload subtitles in video.JS #38

rgaudin opened this issue Sep 16, 2019 · 17 comments · Fixed by #283
Assignees
@dan-niles
Labels
enhancement
Milestone
3.0.1

Comments

@rgaudin
Copy link
Member

rgaudin commented Sep 16, 2019

When building ZIM with --all-subtitles, all videos gets more than a hundred subtitles. Subtitles are loaded asynchronously on page load but the video and controls only starts/become active when all of them have been called which can takes seconds.
@rgaudin rgaudin added the bug label Sep 16, 2019
@rgaudin rgaudin self-assigned this Sep 16, 2019
@kelson42
Copy link
Contributor

@rgaudin This sounds like an enhancement for video.js?! Or can we do something about that?

@rgaudin
Copy link
Member Author

rgaudin commented Sep 17, 2019

Maybe we could be smarter and offer the ability to select some subtitles to include instead of this enormous list which is not very usable anyway.

@stale
Copy link

stale bot commented Nov 27, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Nov 27, 2019
@kelson42 kelson42 added enhancement and removed bug labels Mar 4, 2020
@stale stale bot removed the stale label Mar 4, 2020
@rgaudin rgaudin removed their assignment Apr 28, 2020
@satyamtg
Copy link
Contributor

satyamtg commented May 8, 2020

@rgaudin I think that we should actually have some way to select subtitles. Maybe we introduce another argument which would take in a list of language codes. I've also opened a similar issue on ted and I think we should harmonize this behaviour on scrapers dealing with videos.

@rgaudin
Copy link
Member Author

rgaudin commented May 8, 2020

Agrees.

@rgaudin rgaudin mentioned this issue May 8, 2020
Closed
@stale
Copy link

stale bot commented Jul 8, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jul 8, 2020
@rgaudin rgaudin mentioned this issue Jul 8, 2020
Open
@Popolechien Popolechien mentioned this issue Aug 10, 2022
Closed
@kelson42 kelson42 pinned this issue Jul 8, 2024
@stale stale bot removed the stale label Jul 8, 2024
@kelson42
Copy link
Contributor

I can say that doing performance check on video loading on Android has shown that this pb can significantly slow down video playback (more than one second). To me it sounds like a pretty serious fix to implement.

@benoit74
Copy link
Collaborator

Do you have a sample ZIM/recipe to consider for the tests?

@benoit74 benoit74 added this to the 3.1.0 milestone Jul 23, 2024
@kelson42
Copy link
Contributor

kelson42 commented Jul 23, 2024 •
edited
Loading

Do you have a sample ZIM/recipe to consider for the tests?

No, but Mohit knows as he was the tester. It might be @rgaudin videi test zim.

@kelson42
Copy link
Contributor

There seems to be a clear architecture problem around this at video.js level. Solution does not scale properly/without serious perf impact. This is really unrelated to ZIM storage i'm really in favour of attempting to implement a fix at video.js level. Most obvious idea would be to load subtitle file only when needed/chose. First thing first: opening issue upstream (at video js repo).

@benoit74 benoit74 mentioned this issue Jul 23, 2024
Open
@benoit74
Copy link
Collaborator

If the issue is really that we pass too many subtitles, then I don't agree about opening a video.js issue ; video.js is simply presenting the subtitles we configure. Even if there was no perf issue, presenting 10s of subtitles languages in a dropdown is never going to be a good idea from a UX perspective. So I'm glad there is a perf issue in video.JS because it will force us to revisit the UI/UX.

I think this problem should be handled in the scraper with proper UI to select which subtitles the user is interested in, and JS code to configure video.JS with only these subtitles. It was probably difficult to anticipate and even more difficult to code when only plain JS was used. Now that we have a powerful framework like Vue.JS almost in place and a real problem, it seems pretty straigthforward to implement. At least a proposition in this direction has been made in #278

@kelson42
Copy link
Contributor

@benoit74 OK, but considering your proposal runs at runtime, it will end up like an issue for video.js probably.

@benoit74
Copy link
Collaborator

I don't get why it would end up like an issue for video.js, sorry, could you elaborate? if the problem is that we pass too many subtitles like it has been suggested so far in this issue, and if we adapt the UI to never pass too many subtitles, then there is no more video.js issue to handle.

@benoit74
Copy link
Collaborator

Problem to the issue that subtitles are too slow to load when there are many seems pretty straightforward: tell video.js to not preload tracks with a data-setup='{"html5": {"preloadTextTracks": false}}' attribute on <video> tag. Note: this is supposed to work only with the html5 tech ; to be tested a bit further with ogv tech (but then the tracks are handled by whom? the native browser player or ogv.js?)

Whole code used to test this:

<!DOCTYPE html>

<head>
    <link href="https://vjs.zencdn.net/8.16.1/video-js.css" rel="stylesheet" />
</head>

<body>
    <video class="video-js" controls preload="metadata" width="640" height="264" data-setup='{"html5": {"preloadTextTracks": false}}'>
        <source src="//vjs.zencdn.net/v/oceans.mp4" type="video/mp4">
        <source src="//vjs.zencdn.net/v/oceans.webm" type="video/webm">
        <track kind="captions" src="./captions_en.vtt" srclang="en" label="English">
        <track kind="captions" src="./captions_fr.vtt" srclang="fr" label="French">
        <track kind="captions" src="./captions_de.vtt" srclang="de" label="German">
        <track kind="captions" src="./captions_es.vtt" srclang="es" label="Spanish">
    </video>


    <script src="https://vjs.zencdn.net/8.16.1/video.min.js"></script>
</body>

I suggest that we never preload subtitles in Youtube scraper, I do not see any benefit of doing this.

@kelson42
Copy link
Contributor

kelson42 commented Jul 30, 2024 •
edited
Loading

@benoit74 Great, should be implemented and released quickly. Same for TED. Proper testing on all readers is mandatory here.

@benoit74 benoit74 changed the title --all-subtitles is slow Do not preload subtitles in video.JS Jul 30, 2024
@benoit74 benoit74 modified the milestones: 3.1.0, 3.0.1 Jul 30, 2024
@benoit74 benoit74 mentioned this issue Jul 30, 2024
Closed
@dan-niles dan-niles self-assigned this Aug 1, 2024
@dan-niles
Copy link
Collaborator

dan-niles commented Aug 2, 2024 •
edited
Loading

Adding the following line from tech.js in the video.js library, to the videojs-ogvjs.js plugin in zimui let's us disable preloading subtitles in ogv.js also.

this.preloadTextTracks = options.preloadTextTracks !== false;

@benoit74
Copy link
Collaborator

benoit74 commented Aug 2, 2024

This is even better then, thank you!

@dan-niles dan-niles mentioned this issue Aug 2, 2024
Merged
@kelson42 kelson42 unpinned this issue Aug 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dan-niles dan-niles

Labels
enhancement
Projects
None yet
Milestone
3.0.1
Development

Successfully merging a pull request may close this issue.

Disable preloading of subtitles in video.js openzim/youtube
5 participants
@rgaudin @kelson42 @benoit74 @satyamtg @dan-niles