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

ImageBitmapLoader: Support Firefox's partial createImageBitmap implementation. #12456

Merged
merged 1 commit into from
Nov 1, 2017

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Oct 21, 2017

Update for ImageBitmapLoader:

  1. Add Firefox support. The current implementation only works in Chrome, as Firefox does not accept an options argument to createImageBitmap(). Also depends on Replace 'http-server' with 'serve'. #12455.
  2. Fetch and create the ImageBitmap in a worker by default. I believe (and please correct me if I'm wrong) there isn't much value in creating an ImageBitmap on the main thread.

UPDATE: The performance measurements below were done in Chrome using a WebWorker. Because it turns out createImageBitmap is already async, that Web Worker just adds overhead, and so actual results should be at least as good what I've reported here.

This can be used with any loader through a THREE.Loader.Handlers hook:

class CustomTextureLoader {

	constructor () {

		this.loader = new THREE.ImageBitmapLoader();

	}

	load ( url, onLoad, onProgress, onError ) {

		this.loader.load( url, ( imageBitmap ) => {

			onLoad( new THREE.CanvasTexture( imageBitmap ) );

		}, onProgress, onError );

	}

}

THREE.Loader.Handlers.add( /\.(png|jpg|jpeg)$/, new CustomTextureLoader() );

Performance: Load and upload time (ms)

Preview Boombox Boombox + ImageBitmapLoader
screenshot screen shot 2017-10-20 at 10 47 29 pm screen shot 2017-10-20 at 10 47 34 pm
Preview DamagedHelmet DamagedHelmet + ImageBitmapLoader
screenshot screen shot 2017-10-20 at 10 47 38 pm screen shot 2017-10-20 at 10 47 43 pm

In all charts, green is loading duration (non-blocking), and red is duration of first render (blocking), in milliseconds. Results measured in Chrome.

So far the results look promising. As expected, loading duration is much longer (2-3x). Texture upload is much faster, and the overall time to first render is only slightly slower (1.1x to 1.15x). But — and the key thing for WebXR purposes — parsing glTF does not typically block the main thread. In these examples, the only significant blocking happens during texture upload. With the ImageBitmap, we spend 60-70% less time uploading textures, and drop fewer frames as a result.

These aren't necessarily representative models, they're just two that had obvious render jank among glTF samples. But I'd be curious for ideas on how to proceed with this. Before merging there would probably need to be some support for pooling the workers to avoid browser limits.

@spite — Thanks for adding ImageBitmapLoader! Any opinion on the changes proposed?

@kaisalmen — I wasn't sure how to use THREE.LoaderSupport.WorkerSupport. Is there a better way to create the web worker using that? Or is it meant for use by model loaders more than texture loaders?

@ngokevin — FYI I'm imagining A-Frame could enable this mode if the user loads a model after VR presentation has begun.

P.S. To anyone reading the diff, easier with ?w=1.

@jbaicoianu
Copy link
Contributor

@donmccurdy one thing I noticed when implementing pooled worker loaders is that there's actually a big benefit to loading the files in a separate queue, and then passing the binary data to the worker when it's ready to be processed. Fetching in the worker means that your workers are bound by network IO most of the time, so if you have a pool of 4 asset loaders, you'll end up blocking much longer overall as you work your way through the queue.

If you have a separate download queue, your browser can also take better advantage of http pipelining.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Oct 21, 2017

@jbaicoianu Thanks — I didn't do it here, but with a pool of 4 workers couldn't you keep sending requests for ImageBitmaps to the same workers, even before they've finished the last task? Or is there some particular advantage to all of the network IO happening on the main thread?

Maybe I should be using THREE.LoaderSupport.WorkerSupport to deal with this sort of thing. 😁

@kaisalmen
Copy link
Contributor

@donmccurdy Need to look at your code in detail, but THREE.LoaderSupport.WorkerSupport should
be usable here (or with minor adjustments). @jbaicoianu THREE.LoaderSupport.WorkerDirector with some enhancements could delegate the loaded data to the pool of workers it spawned, I think. I will provide more detailed feedback latest tomorrow. Easiest example to see it all working with a "faked" loader/parser ist the MeshSpray example.

@kaisalmen
Copy link
Contributor

WIP: This is not fully working, yet, but almost all pieces are there. You can already get the idea:
https://github.com/kaisalmen/three.js/tree/feat-imagebitmaploader-worker

@donmccurdy
Copy link
Collaborator Author

@kaisalmen thanks! Are the fixes to LoaderSupport changes that make sense from your perspective? Hopefully loading images isn't too much a one-off... Does LoaderSupport handle worker pooling this way?

@kaisalmen
Copy link
Contributor

kaisalmen commented Oct 23, 2017

@donmccurdy I will likely revert the split of run into prepare and run as it not needed. I realized this morning. The run (=load in ImageBitmapLoader case) method called by THREE.LoaderSupport.WorkerDirector could deal with the async file loading and then message the data to the worker once available and block execution for the time being. A list of image load instructions needs to be fed to the WorkerDirector. This is still missing on my branch.

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Oct 23, 2017
kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Oct 23, 2017
kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Oct 23, 2017
@kaisalmen
Copy link
Contributor

kaisalmen commented Oct 23, 2017

@donmccurdy ImageBitmapLoader now uses WorkerSupport. Changes to LoaderSupport classes ok as there are no public interfaces changed now. Contract between worker and user of worker is expanded, but that is fine.
Verdict: Code is not shorter due to the shortness/low complexity of your original worker and WorkerSupport comes with some overhead (utility functions and communication contract), but usage in this context is now easily possible.

I would like to expand the example with a button to load n cubes (e.g. 256) with WorkerDirector orchestrating 4 parallel workers (ideally without any caching) to see how this performs.

Let me know what you think?

Edit: Do you know if a Blob is a transferable? Otherwise the postMessage is inefficient and an ArrayBuffer must be used. It can be returned by the fetch, but not feed to createImageBitmap, correct?

@mrdoob
Copy link
Owner

mrdoob commented Oct 23, 2017

@spite looks good?

@spite
Copy link
Contributor

spite commented Oct 23, 2017

Fetch and create the ImageBitmap in a worker by default

I had a version that offloaded to a worker. I didn´t see a lot of difference, and I think the idea for most APIs dealing with downloads and image decoding the be moved outside of the main thread by the browser itself. So I didn't want to complicate the code with a Web Worker dependency, mostly because I usually ship an ES5 and an ES6 version of the code, which makes worker loading syntax a bit more verbose. But if it´s working fine, awesome! 🤘🏽

LGTM

@donmccurdy
Copy link
Collaborator Author

I had a version that offloaded to a worker. I didn´t see a lot of difference, and I think the idea for most APIs dealing with downloads and image decoding the be moved outside of the main thread by the browser itself.

Oh! That would be great if the browser goes off-thread already... Will try this in Chrome and Firefox and report back...

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Oct 29, 2017
@kaisalmen
Copy link
Contributor

kaisalmen commented Oct 29, 2017

@donmccurdy Finally had time to create the WorkerDirector proof of concept. The adjusted example requires 1.5GB of spare graphics memory otherwise the browser will crash. Even with enough graphics memory available Chrome crashes now and then. Workers are destroyed properly, though. Let me know what you think.

Edit: Workers are created once and are then re-used. If the worker usage makes sense in this particular use case needs to be decided. Anyway a useful result of this experiment is that the overall concept of a common, instruction-based and repeatable worker usage can be applied to this class of problem.
Edit2: Amended commit with latest updates from main branch and fixed with spaces

@MJCD
Copy link

MJCD commented Oct 30, 2017 via email

@MJCD
Copy link

MJCD commented Oct 30, 2017 via email

kaisalmen added a commit to kaisalmen/three.js that referenced this pull request Oct 30, 2017
…Director. Adds latest updates from main branch to LoaderSupport and OBJLoader2.
@donmccurdy
Copy link
Collaborator Author

I agree completely with Juame - any XHR style request by default is already
largely non-blocking to the main thread - so before you all get bogged down
in the gritty of how to do it can we answer 'why' specifically?

It is not the request I am trying to offload from the main thread, but the image decoding triggered by the createImageBitmap() call. I still haven't had the chance to verify whether that is actually blocking in Chrome and Firefox, but I'd assumed it was based on this blog post from Chrome devrel. If so, that would cause an unacceptable number of dropped frames for WebVR applications and should be done by a worker.

Will report results when I've verified this. It would certainly be more convenient if the worker is not needed.

@donmccurdy
Copy link
Collaborator Author

Sure there's some stats but that's not really indicative of much..

60-70% fewer dropped frames during texture upload is quite noticeable when the screen is strapped to your face. 😉

@spite
Copy link
Contributor

spite commented Oct 30, 2017

I've been talking to @paullewis about the relevance of the article, since it's from Jan 2016 and my experience from a few months ago showed that it might not be necessary any more. So in the time of the article, createImageBitmap would still occur in the main thread, but it's moved off it since then. The best way to show it, some profiling when clicking "Add ImageBitmap":

screenshot from 2017-10-30 16 18 08

You can see that Image Decode happens in a thread. If you profile with "Add Image", you'll see it happens in the main thread.

So this is almost as good as it gets, no need for workers on Chrome. Can't say about other browsers -haven't tested- but I'm certain they'll all end up implementing async image decoding.

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Oct 31, 2017

Thanks @spite and all! Measuring here I get the same result: createImageBitmap is asynchronous. Bit hard to read Firefox's performance measurements but it looks asynchronous there too. So, I've removed the Web Worker part of this PR, leaving only the support for Firefox's (partial) createImageBitmap implementation. Whether that is worth merging, or whether we require those options for this to be useful, I'm not sure.

@kaisalmen thanks for your help and pointers! I will be wanting to use WorkerSupport soon for decoding DRACO-compressed assets in GLTFLoader, which definitely will benefit from a Web Worker.

@donmccurdy donmccurdy changed the title [WIP] ImageBitmapLoader: Use web worker by default. ImageBitmapLoader: Support Firefox's partial createImageBitmap implementation. Oct 31, 2017
@spite
Copy link
Contributor

spite commented Oct 31, 2017

For now Chrome's createImageBitmap does image decoding off the main thread when the source is a blob, and it doesn't when it's an ImageElement or SVGElement. For the use case we have, i think it's fine with blobs.

This is more or less what I hope we'll see in the future: https://www.chromestatus.com/features/5637156160667648

@donmccurdy
Copy link
Collaborator Author

ImageBitmap just landed on Safari Technical Preview: https://webkit.org/blog/8016/release-notes-for-safari-technology-preview-43/

It appears to support options, but is currently failing on https://threejs.org/examples/?q=imagebitmap#webgl_loader_imagebitmap.

@mrdoob mrdoob merged commit 620267b into mrdoob:dev Nov 1, 2017
@mrdoob
Copy link
Owner

mrdoob commented Nov 1, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants