Do not preload subtitles in video.JS

rgaudin commented 4 years ago

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.

kelson42 commented 4 years ago

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

rgaudin commented 4 years ago

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[bot] commented 4 years ago

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.

satyamtg commented 4 years ago

@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 commented 4 years ago

Agrees.

stale[bot] commented 4 years ago

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.

kelson42 commented 1 month ago

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 commented 1 month ago

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

kelson42 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

benoit74 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

dan-niles commented 1 month ago

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 commented 1 month ago

This is even better then, thank you!

openzim / youtube

Do not preload subtitles in video.JS #38