-
-
Notifications
You must be signed in to change notification settings - Fork 148
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 webp image polyfill #650
Comments
We need to be carefuly that this does not conflicts with the polyfill available in the ZIM file itself (which is the plan so far). |
OK, noted. In our jQuery mode (which we should probably rename "DOM traversal mode"), we would need to attach the polyfill manually, but it can be extracted from the ZIM. In SW mode, it will attach automatically. So this PR then would only need to cover the former mode (and there would be no conflict). |
The PR to add New version of Youtube based ZIM files have images using Webp. Both (will) have a polyfill. |
@mossroy See discussion in openzim/mwoffliner#1265 . As this is another potential moment of "deprecation" for old browsers running Kiwix JS, and as adding a test in jQuery mode for the existence of the webp polyfill in the ZIM, and attaching it if it is found, is not at all hard, I propose to add and test this here soon. The reason is that I don't believe we have any browsers capable of running SW mode that do not already support webp, so it is only needed for browsers that are stuck in jQuery mode. |
Great, I agree |
Here is WikiMed ZIM file for custom apps, with webp: http://mirror.download.kiwix.org/zim/.hidden/custom_apps/wikipedia_en_medicine-app_maxi_2020-10.zim. |
Wow, that file saves 800MB over the standard one, a substantial size reduction. |
@Jaifroid Indeed, but this file does not have the fulltext search index either. |
@kelson42 Are you sure the polyfills are included in the ZIM? I can see they are referenced in each article's HTML, but our Service Worker cannot find them at the referenced ZIM URL: |
@Jaifroid Yes, there is a bug which makes the ZIM created within Docker to have this problem. Fixed in openzim/mwoffliner#1274. I have also a super small PR to introduce lazy loading for images at openzim/mwoffliner#1275. As soon as merged, I will redo the WikiMed ZIM file and let you know. |
@Jaifroid I’m not at the office, but I believe this new version of the ZIM to fix the problem http://mirror.download.kiwix.org/zim/.hidden/custom_apps/wikipedia_en_medicine-app_maxi_2020-10.zim |
@kelson42 That ZIM has the polyfills, thank you! I have tested the generic solution -- JavaScript Support branch for jQuery mode -- and it has the maybe obvious issue that because images are loaded through a custom process in jQuery mode, the webp polyfill doesn't see them at load time, so does not attempt to convert them. So, we would either need to add a custom onload event to each image to run |
The polyfill should decode images only when the browser does not support webp. Maybe a simpler and more generic approach would be to run |
Indeed we should only convert images if the browser doesn't decode webp natively (the polyfill has a test). According to the documentation, polyfilling the document only works on elements that have the
|
You're right. If we use the polyfill library inside the ZIM file, there is a risk to make use of internal functions of the polyfill, that might change in future versions of the polyfill. We should make as few assumptions as we can on the content of ZIM files. Maybe another approach would be to override |
@kelson42 I've been looking at the issues for that polyfill, and the dev has posted this:
I know you only add the polyfills on articles which have images, but it looks like even that approach is quite wasteful on the majority of browsers. Effectively, these polyfills add a 1MB load just to support IE11 and Safari. Well, I realize iOS support is crucial, but as the Dev of the polyfill says, it is alarmingly wasteful on other browsers engines. I wonder if a better approach would be conditional loading of these polyfills? The detection part of the script is really very short in TypeScript (a bit longer when converted to universal JS). Maybe in fact, the best approach would be to make |
Interesting, yes. Do you mean we should write a custom function override for parts of the polyfill, sort of polyfill the polyfill? |
No, there seems to be a |
OK, that looks like a possibility. I guess we'll need to avoid killing the image blobs on first load as we currently do, and keep them residing in memory so that the polyfill can retrieve their data. Depending on how fast the polyfill works, the user is quite likely to see broken image icons till the data have been converted and the image reloaded. We could compare both approaches (run polyfill after all images are loaded vs convert each webm Uint8Array as it is retrieved if the browser doesn't support webm natively). |
Sorry for my dummy question, but does this kind of code (like in the youtube2zim scraper) would be better? https://github.com/openzim/youtube/blob/master/youtube2zim/templates/assets/webp-trigger.js. Maybe there is even a simplier approach like
I'm not against considering the idea, but this is in any cased blocked by this pending PR openzim/mwoffliner#1237 |
@kelson42 It looks very much like that code still loads the bundle so that it can use its built-in test. If we want to avoid loading the bundle at all if the browser supports webp, then we need to have a simple test script that then loads the other two scripts only if needed. A very short ES5 script to do this is suggested here, and copied below:
And this is how you'd use it. -- if it's of interest, I could complete this to attach the required scripts.
|
@Jaifroid ok, will try yo make a PR like this to mwoffliner. Or just make one, if you have one. |
Experimental branch pushed (commit 8232168) which works in Internet Explorer. Strictly for testing, as this branch is built on my JavaScript support brach which generically loads all JS that it can. It's just a proof-of-concept. This version does not test if the browser supports webp, just converts all image Prodcution code would run faster, because we wouldn't be attaching all JS in an article, only the webp JS. However, it's not too bad on IE11, surprisingly (especially after the memory cache is populated). I'll also try the other approach, but probably not today, as running out of time. |
@kelson42 Sorry, were you saying you would do it? (I can't test mwoffliner on my machine). |
OK @kelson42 an implementation all inside one external custom script could look something like this (caution: untested!!):
|
PS That attaches the polyfills to the body of the doc, not the head -- just in case there is a document without a head (unlikely?).
|
Maybe this complete version would work (needs careful testing!!!). It creates and attaches an inline script, which could be a problem for some strict frameworks. If so, the inline script could be replaced with a third external script, whcih should be the last one. That script should attach
|
EDIT: added missing end closures! |
@Jaifroid Thank you, the work has been done on MWoffliner and is part od latest release 1.11. For the moment only a feww ZIMS, like wikimed for app, have the option, but I plan to move all of them in the next months. |
OK, thanks @kelson42 , I see there is a test ZIM here: https://github.com/openzim/mwoffliner/files/5451044/zimfile_with_webp.zip which we can use to test/enable support in KJS and KJSW. |
I would like to introduce webp optimisation for most (if not all) of our MWoffliner generated ZIM files around end of the month (November). But I would like to secure the our Kiwix JS (based) reader support them properly. Does that sounds a reasonable timeline? If not, what would be a better one? |
@kelson42 I'll tell you after I've tried to incorporate the polyfill into the WikiMed release I'm doing now. If that goes smoothly, then the timeline you're thinking of should be fine. More soon. |
Hmm. I can get the polyfill to run in Internet Explorer, but so far not in Edge Legacy, surprisingly (as the latter is a more modern browser). |
Now working in Edge Legacy / Windows Mobile. 😊 |
@kelson42 I've tested the polyfill on a Windows Mobile physical device and on various VMs. It works fine on any device with 1GB or greater RAM. For the lowest-powered device, with 500MB RAM, the app launches and then crashes as soon as it tries to load this polyfill. While I don't believe there are many/any people with such a low-powered device attempting to run Kiwix, it is indicative that this polyfill is pretty heavy and wasteful of memory. Nevertheless, on the lowest-spec 1GB device, it runs smoothly. So I don't think this should be a blocker. The 500MB device struggled with XZ-encoded ZIMs in any case, so this would not be a major deprecation, and I do recommend 1GB minimum to run KJSW. @mossroy In jQuery mode, the most efficient approach is to decode the images inside the For SW mode, the included polyfill should just run. However that is currently untested, as I don't have any browser that can run Service Worker but cannot decode WebP natively. If there are none, then maybe we don't need to worry about SW mode, other than to ensure that we don't load the polyfill into memory if the user launches the app in SW mode. |
@Jaifroid Good, so I need we would need new releases to get this ready for users? |
@kelson42 Yes. I'll do a WikiMed release on KJSW as a pilot in the next few days. That can be followed with a PR here if my suspicions are correct that we do not need to do anything for the Service Worker mode of this app. If we do need to support SW mode explicitly, then the PR could be more complicated... |
My concern is, as usually, to do as few assumptions as we can on the ZIM content. Else we take a risk that it will not work on future versions of this ZIM file, or on other unknown ZIM files. The more we use high-level APIs of the polyfill, the safer we are. BUT we could have another approach : in jQuery mode, instead of using the polyfill found inside the ZIM file, we could provide our own polyfill with kiwix-js. This way, we don't make any assumptions on the ZIM content (it could even not contain the polyfill). And we can optimize the algorithm as we wish (because we know for sure that the version of our polyfill will not change depending on the ZIM file), including by using lower-level APIs of the polyfill. Regarding ServiceWorker mode, I agree that we certainly have nothing to do. The polyfill (included in the ZIM file) should work, as any other javascript. |
@mossroy This is how I've added (so far partial) WebP support in KJSW, i.e. I've included my own copy of the polyfill rather than extracting it from the ZIM, for speed and compatibility, as you say. I say the support is partial because it is currently implemented for Windows Mobile, i.e. I did not include the polyfills needed by IE11 (thought I have tested them and they work). I agree we'll need to test for those browsers with SW support and lacking WebP, or at least one of them. Thanks for researching that. |
Cool! |
I spent some time this morning trying to enable conditional loading of the WebP Polyfills. I just wanted to document my findings here:
|
@kelson42 Just to say that WebP is now enabled in both Kiwix JS and Kiwix JS Windows. In Kiwix JS it is in master, but not yet released, and there are small issues with the polyfill that need further investigation in #673, but I don't think the issue is a blocker for a release (it affects only a very small number of old browsers that mostly will have autoupdated to the latest, and even those browsers can fully access the Wikimedia ZIMs in jQuery mode, which is still the default). Let us know when / if you start to make ZSTD+WebP formats for all Wikimedia ZIMs (other than in the |
So, I guess we can move on with ZIM generation with webp images? I will ask to move all our Mediawiki based recipes. |
Yes, we're all ready. We can do a new release as soon as the new ZIMs start to appear. |
@Jaifroid From now, all new ZIM files using mwoffliner will be done with webp pictures |
OK, thanks, we'll schedule a new release in the next couple of weeks. |
See openzim/mwoffliner#1254 . This is medium to longer term. Currently planned for Android custom apps to reduce distribution size, but it would be useful to be able to support the same ZIMs. It's effectively another compression format, and the polyfill provides an asm/wasm layer that converts webp to png if the browser does not support webp. The polyfill is claimed to work on IE11.
Polyfill is here: https://github.com/chase-moskal/webp-hero
The text was updated successfully, but these errors were encountered: