Use createImageBitmap when available

[OmarShehata]

This PR adds support for using createImageBitmap to move image decoding off the main thread. This doesn't make loading any particular image faster, but it greatly improves frame stutters by avoiding the expensive blocking image decode that happens on texture upload. It might also improve loading scene time since images can be decoded in parallel.

This builds off the other PR #6625. The problem there was that images created with createImageBitmap can be flipped on texture upload only when passing ImageBitmapOptions to the createImageBitmap was not supported. This is because it's faster to flip the image during decode. So I had to add a way to flip an image when fetching it.

@mramato to make this easier to review, I recommend reading each commit separately:

• 96cbc18 adds the new feature detection functions needed for supporting this. The new spec show how they're expected to be used.
• b772d52 this is the meat of this PR. This adds flipImage on Resource, and uses createImageBitmap on all image requests, as well as in loadImageFromTypedArray. It also kicks off the support check in the Viewer constructor.
• dca9ed1 adds tests to verify that createImageBitmap behaves how we expect it to. Specifically, it tests that it's possible to flip the image either on fetch or on texture upload as expected, and reads back the pixels since this is the only way to know if the operation truly worked.
• Finally, 824ceb1 updates all the ImageryProvider specs to check for ImageBitmap instead of Image when it's supported.

Why it's necessary to expose flipY on fetchImage

We can't make decisions about whether to flip inside Resource because whether we flip or not depends on what we're going to do with this image. All textures in CesiumJS are flipped by default during texture upload. This seems to be because imagery expects this orientation.

Models with textures will disable this flipping to get the correct orientation. This means that there's no one correct orientation. If we always flip, models will be upside down. If we never flip, imagery will be upside down. I spent some time trying to see if I could get all imagery to be flipped in the shader so we can just simply never have to worry about flipping during texture upload but I couldn't get it to work correctly with the projection.

Tests & Results

To compare before/after, simply add return false to the first line of FeatureDetection.supportsCreateImageBitmap.

This bike model is a good test, since it has huge textures that severely block the main thread when loading in.

If the glTF model is in SampleData/models/Bike you can use this local Sandcastle to see it loading while rotating, which makes the frame jank obvious.

Image bitmap on the left:

Image bitmap on top:

It does also make a big difference with tilesets, which often load in lots of images during camera fly throughs.

Another test is on the Melbourne 3D Tileset, during a fly through, we see the following before (top) and after:

[cesium-concierge]

Thanks for the pull request @OmarShehata!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

[OmarShehata]

Ok @mramato I've refactored it to use Resource.fetchBlob and remove flipImage from Resource. I've updated the PR description to document why we expose flipY on fetchImage.

The only outstanding issue is #7579 (comment). I'm still using the fetch API in FeatureDetection to avoid a circular dependency. I started changing it to take Resource as an argument but realized it was a lot simpler to just using fetch there since it's supported wherever createImageBitmap is. Otherwise I'd have to include Resource in a bunch of new places that don't need it just to pass it to FeatureDetection. I'm open to suggestions here.

[mramato]

Otherwise I'd have to include Resource in a bunch of new places that don't need it just to pass it to FeatureDetection. I'm open to suggestions here.

One idea would be to move the feature detection code into Resource instead of having it on FeatureDetection. Since all image loads are async anyway, this also means you can check for support in one spot the first time an image is loaded. It might actually simplify the code further. If you don't like this idea, then using fetch might be okay (would need to look into a little more), but I kind of like the idea of putting the feature check closer to the use case anyway.

[OmarShehata]

Ok, the good news is that the new approach works and not only simplifies the code but things are functionally better too. Here's some highlights:

• There will no longer be some images that'll slip through and load using the old code-path just because they were requested before supportsImageBitmapOptions finishes. It'll just wait for that promise to resolve.
  • So we no longer have to kick this off in Viewer.js.
  • This also means we no longer have to wait for supportsImageBitmapOptions in the beforeAll of most specs! That's automatically handled now.
• I discovered if prefersBlob was true, it would fetch a blob, create a URL, then fetch that again, and create and ImageBitmap. This can now be avoided by directly creating an image bitmap from the blob.
• I added a deprecationWarning and discovered some areas in the code that I updated to the new function signature.

The other good news is that I found out why there was an extra parameter being passed to fetchImage that wasn't being used. IonResource.fetchImage has a crossOriginAllowed parameter. To fix this I either had to make fetchImage pass it down to here:

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Resource.js#L911-L916

Or just remove that parameter from IonResource. I picked the latter since it was never working before for however long and this way it's consistent with Resource's fetchImage. Let me know if you think this wasn't the right choice.

I will bump this when CI passes.

[OmarShehata]

@mramato this is good to look at. See #7579 (comment) for a summary of the changes.

[mramato]

Nice, thanks again @OmarShehata. I'll do another pass on this either later tonight or first thing tomorrow.

[mramato]

What's the status of this PR? Last I heard we were trying to get some info/advisement from the Firefox team?

Do we want to get this into the Friday release?

[OmarShehata]

What's the status of this PR? Last I heard we were trying to get some info/advisement from the Firefox team?

I still haven't heard back. Perhaps this might be better to merge after release at this point?

[mramato]

Merging after is fine. I would re-recommend doing 2 and then making additional changes once we hear back from the FF team. Just bump this when it's ready. Thanks!

[OmarShehata]

We didn't get a definitive answer back, but it looks like there are no known plans for those bugs. The consensus in this ThreeJS thread is to just disable for Firefox. We're already detecting it by actually calling it to see if it works, so no need to detect and disable for any specific browser.

I've updated the code to only use createImageBitmap when ImageBitmapOptions is supported. This does simplify the calling code a bit since we no longer need to account for the two paths.

@mramato no rush on this review at the moment, but would be good to get this far-reaching in sometime this/next week.

I did one final series of tests to make sure models and imagery still all loaded correctly with a couple of Sandcastle examples on Chrome, Firefox and Edge.

[mramato]

This should have a return statement and then you should move all of the following then calls up a level.

[OmarShehata]

Can you explain why? I'm assuming this is what you mean:

Resource.supportsImageBitmapOptions()
    .then(function(result) {
        if (!result) {
            loadImageElement(url, crossOrigin, deferred);
            return;
        }

        return Resource.fetchBlob({
            url: url
        });
    })
    .then(function(blob) {
        return Resource._Implementations.createImageBitmapFromBlob(blob, flipY);
    })
    .then(deferred.resolve).otherwise(deferred.reject);

This will run the .then(function(blob) anyway even if we loadImageElement and don't fetch the blob, and then blob will be undefined, which we don't want, right?

I can just move the .then(deferred.resolve).otherwise(deferred.reject); outside and I think that should work fine.

[OmarShehata]

I can see Resource.prototype.fetchImage checks if the result is defined at each step in the promise chain to make sure it doesn't run when it terminates early like that. I can follow that pattern.

[mramato]

Because if for some reason supportsImageBitmapOptions rejected, you would never call deferred.reject (or deferred.resolve).

[mramato]

I can see Resource.prototype.fetchImage checks if the result is defined at each step in the promise chain to make sure it doesn't run when it terminates early like that. I can follow that pattern.

I'm not sure what you mean by this, please stop by if you still have questions.

[OmarShehata]

Thanks for the explanation. I'm pretty sure I've got it locked down now. What I meant was, Resource has a similar promise chain when fetching an image. When we return undefined, all the subsequent then's are still going to run. So you need to add a check at each subsequent then() that the result is defined.

Here is where Resource is doing this:

https://github.com/AnalyticalGraphicsInc/cesium/blob/8bc364ce5eca3c42d84fe21386b0d5a50be82c2d/Source/Core/Resource.js#L886-L889

I just followed a similar pattern in createImage:

https://github.com/AnalyticalGraphicsInc/cesium/blob/a6324e2bcf8b3547b4adb589e7ee789321997678/Source/Core/Resource.js#L1855-L1882

Notice that deferred.resolve is only called in the ImageBitmap codepath. Since loadImageElement calls it on the Image on onload.

Will bump when CI passes.

[mramato]

I think this whole file could probably use some clean-up. Some of the (old) if checks make reading the code pretty confusing. No action on your part, and this is nothing you did; just mentioning it because I may open a small clean-up PR to take a pass (after merging this).

[OmarShehata]

@mramato all good now. Last commit makes all testing rely just on the Resource.supportsImageBitmapOptions promise. Tested again on Chrome, Edge, Firefox and IE11.

[mramato]

This would be better as a custom Jasmine matcher than a standlone function, I'll just submit a fix to master after merging.

[mramato]

Thanks @OmarShehata! I think we can finally call this done. Hopefully Firefox and Safari will get in line and add full support so we can benefit there as well.