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.

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

HTML5 progress bar fixes (gzip+engine load) #5211

Merged
merged 14 commits into from Oct 15, 2020

Conversation

britzl
Copy link
Contributor

@britzl britzl commented Sep 15, 2020

Still missing progress when loading the .wasm file though

Fixes #5096

Still missing progress when loading the .wasm file though
@britzl britzl marked this pull request as draft September 15, 2020 20:45
@britzl
Copy link
Contributor Author

britzl commented Sep 16, 2020

Also fixes #5097

/* ********************************************************************* */

var Combine = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine was not a very accurate name. The purpose of Combine.* functions was mainly to download the game archive files (and combine them if a file consisted of more than one piece).

@britzl
Copy link
Contributor Author

britzl commented Sep 16, 2020

@aglitchman and @GreenArrow18: Draft PR for the two progressbar related issues. If you have some input please let us know!

@britzl britzl linked an issue Sep 16, 2020 that may be closed by this pull request
@britzl britzl changed the title Issue-5096 Load engine and start updating progress immediately HTML5 progress bar fixes (gzip+engine load) Sep 16, 2020
@GreenArrow18
Copy link

GreenArrow18 commented Sep 17, 2020

Did some tests, this works quite well (when gzip is disabled), which fixes #5096, once the wasm progress is also working (currently just a big jump from 10% to 50%).

For #5097 though, this is only a very slight improvement. Instead of going from 0 to 100% (with no progress in between), I now have 3 progress:
0 to 10% for the js
10 to 50% for the wasm
50 to 100% for the app files

Note that there is still no progress in between those ranges though (so 0% jumps to 10%, 50% jumps to 100% without progress in between). So instead of 1 big jump, I now have 3 big jumps.

Not sure if it's just because of my nginx server set-up. But in my case, I get the following headers:
Content-Encoding = gzip
Content-Length = not present, as per spec I think because Transfer-Encoding = chunked (when gzip is enabled)

So this goes straight to the else part (because total will be null):

var total = xhr.getResponseHeader('content-length')
var encoding = xhr.getResponseHeader('content-encoding')
if (total && encoding && (encoding.indexOf('gzip') > -1)) {
     total /= 0.4; // assuming 40% compression rate
     onprogress(xhr, e.loaded, total);
} else {
     onprogress(xhr, 1, 1);
}

One thing I noticed, when gzip is enabled, say we have the following:
Normal file: 10MB
Gzipped version: 5MB

The progress (e.loaded) from above will return progress from 0 to 10MB (not 5MB). So if we know the total (which we do, for the app files because it's in the archive json file), we can just pass the progress like:

var total = xhr.getResponseHeader('content-length')
var encoding = xhr.getResponseHeader('content-encoding')
if (total && encoding && (encoding.indexOf('gzip') > -1)) {
     total /= 0.4; // assuming 40% compression rate
     onprogress(xhr, e.loaded, total);
} else {
    onprogress(xhr, e.loaded, e.loaded); // nginx with gzip enabled goes here
}

For the app archive files, total is ignored anyway, because it's computed from the json file instead. I did some tests, and the 50-100% part (for the app files) will have the proper progression. We also won't need to approximate the compression, because e.loaded still returns values for the original file size, not the compressed size.

So here's an idea: for the js/wasm files, is it possible to include them in the archive json file? Then we will know their actual sizes too (uncompressed). Then those files just become a part of the pieces that need to be loaded. We won't even need to divide the progress into 3 parts anymore (js, wasm, app), because we would know the total size of everything.

Just need to make sure that we still only load either the asm/wasm js file and not both. Also, not sure if gzip enabled behaves the same for all server set-ups (I tested on nginx only).

@britzl
Copy link
Contributor Author

britzl commented Sep 18, 2020

Thanks @GreenArrow18 ! I tested with a rudimentary Python server and it didn't send gzipped content using chunked content encoding, which is why I didn't notice the problem with the progress.

Some improvements when the encoding is chunked:

  • Each chunk contains the length of the chunk. We could perhaps use this to update the progress.
  • We could do a HEAD request to get the content length and then a GET to actually download the content

for the js/wasm files, is it possible to include them in the archive json file

Maybe. I'd have to look into this. It is an interesting idea for sure!

@britzl
Copy link
Contributor Author

britzl commented Sep 18, 2020

@GreenArrow18 I've updated the code to now also download the .wasm file and update progress. When the total download size is unknown (gzipped with no content length) an unscientific guesstimate is made. On a slow connection it will result in a progressbar which progresses, but sometimes also regresses when the guesstimate isn't accurate.

To solve this we either need to first make a HEAD request to get the size or as you suggested include them in the archive.json.

I'd love to hear your opinion on the current behaviour.

if (typeof callback !== 'function') throw "Invalid callback registration";
list.push(callback);
},
notifyListeners: function(list, ...args) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spread syntax (...) is an ES6 thing. https://caniuse.com/?search=spread%20operator
If you use it, old browsers can't parse dmloader.js and users will get a blank page.

Possible replacement:

    notifyListeners: function(list) {
        for (i=0; i<list.length; ++i) {
            list[i].apply(list[i], Array.prototype.slice.call(arguments, 1));
        }
    },

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, damn. Thanks.

@GreenArrow18
Copy link

Did some tests on nginx.

Non-gzip: it worked great, fixes #5096 quite well.

Gzipped: simulating a 3G connection, the bar regressed quite often. Personally, I didn't really like how it looked.

If you can make a proper fix, that would be great. You mentioned the HEAD request option, I think that is actually preferrable than the archive.json idea, because your current loader scripts can be useful for other things as well (loading the FB sdk, ad scripts etc).

If that is not an easy fix (I understand you're very busy), here's another idea: instead of making a guesstimate on the size (which causes the bar to regress currently), why not just pass the actual sizes of the js/wasm files?

For example, I did this something like this:

    loadWasmAsync: function(src, fromProgress, toProgress, sizeEstimate, callback) {
           ...
           {
                retryCount: 4,
                retryInterval: 1000,
                responseType: 'arraybuffer',
                sizeEstimate: sizeEstimate
            });
    },

    loadScriptAsync: function(src, fromProgress, toProgress, sizeEstimate) {
            ...
            {
                retryCount: 4,
                retryInterval: 1000,
                sizeEstimate: sizeEstimate
            });
    },

    load: function(appCanvasId, exeName) {
        Progress.addProgress(Module.setupCanvas(appCanvasId));
        if (Module['isWASMSupported']) {
            EngineLoader.loadWasmAsync(exeName + ".wasm", 0, 45, 2097152, function(wasm) {
                EngineLoader.loadScriptAsync(exeName + '_wasm.js', 45, 50, 249856);
            });
        } else {
            EngineLoader.loadScriptAsync(exeName + '_asmjs.js', 0, 50, 6860800);
        }
    }

    ...
            if (total && encoding && (encoding.indexOf('gzip') > -1)) {
                total /= 0.4; // assuming 40% compression rate
                onprogress(e.loaded, total);
            } else {
                onprogress(e.loaded, options.sizeEstimate); // For archive files, total here is ignored anyway
            }
    ...

The sizes of the js/wasm files do not really change by a significant amount unless there are major Defold changes. These will still be estimates, but the estimates will be closer to the actual file sizes and will not cause bar regressions.

I understand the file sizes will differ between builds (debug or release, custom manifest etc). But there are only 3 values to change, and that's easy enough to edit manually as needed. Personally, I would prefer this over seeing a bar that regresses.

@britzl
Copy link
Contributor Author

britzl commented Sep 18, 2020

@GreenArrow18 you're right, the progressbar is not that great. I implemented HTTP HEAD requests to get the size of resources before loading them. Please check it out!

@GreenArrow18
Copy link

Tested and unfortunately, the HEAD request doesn't solve things for the gzipped files.

Similar to GET, the HEAD request isn't returning a content-length header. So we're back to getting 2 big jumps, one each for the js and wasm file. Then 50-100% will be proper progression (because we know the file sizes from archive.json).

I think it makes sense though? I was reading the specs, and it says a HEAD request should be identical to a GET request (minus the body). So if we are not getting content-length from GET, it will also not be available for HEAD, because the files are compressed on-the-fly and we won't really know the total compressed size beforehand?

@britzl
Copy link
Contributor Author

britzl commented Sep 19, 2020

Ah, that is unfortunate. I thought HEAD requests would include content length. Most annoying.... Ok, I think the best approach is to provide an average size for the wasm and js files like your suggested.

Use estimate when no size can be computed (gzipped content)
@britzl
Copy link
Contributor Author

britzl commented Sep 20, 2020

I pushed a new version where an estimated size is provided to the loader.

@GreenArrow18
Copy link

Tested with both normal and gzipped files, works great.

Thanks @britzl ! This significantly improves the overall user experience for html5 builds.

@britzl britzl requested a review from JCash September 21, 2020 11:43
@britzl britzl marked this pull request as ready for review September 21, 2020 11:43
@britzl britzl added this to In progress in 1.2.175 via automation Sep 22, 2020
1.2.175 automation moved this from In progress to Reviewer approved Oct 15, 2020
Copy link
Contributor

@JCash JCash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@britzl britzl merged commit 0dc6916 into dev Oct 15, 2020
1.2.175 automation moved this from Reviewer approved to Done Oct 15, 2020
@britzl britzl deleted the Issue-5096-html5-progressbar-excludes-engine branch October 15, 2020 09:58
pull bot pushed a commit to proteanblank/defold that referenced this pull request Jul 28, 2021
* Load engine and start updating progress immediately

Still missing progress when loading the .wasm file though

* Update dmloader.js

* Added unified file downloader

* Fixed indentation

* Cleanup

* Indentation fixes

* Update dmloader.js

* Improved file downloader. Added progress for wasm download.

* Reverted name change for progress update function

* Update dmloader.js

* Removed use of spread arg

* Use HTTP HEAD request to get resource size

* Add estimated size to file download requests

Use estimate when no size can be computed (gzipped content)

* Made wasm and asmjs size configurable for the progressbar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
1.2.175
  
Done
Development

Successfully merging this pull request may close these issues.

html5 progress bar stays at 0% when archive files are gzipped html5 progress bar excludes engine
4 participants