Change default Resource.fetchImage flipY to false

[OmarShehata]

In 1.56 we added ImageBitmap support, which cannot be flipped during texture upload, so I exposed an option to flipY on fetch instead. Since textures in Cesium are flipped by default, I set the default flipY to true. This now means that anyone who used fetchImage() to do anything other than render it in Cesium (such as read pixels, or draw to another canvas), it was now upside down with this release.

This PR changes the flipY default on fetchImage to false. This is still a breaking change compared to 1.55. If someone uses fetchImage in Cesium, then passes that image to something like a material that uploads it as a texture, it'll now be upside down in 1.56.1 (this PR). If this is a less common case, then I think this breaking change is better than the breaking change in 1.56.

Testing this

The general rule is, if an image is going to be uploaded as a texture, it should be flipped on fetch. If ImageBitmap is NOT supported, then the image will be flipped during upload. If ImageBitmap is supported, then it will be flipped during fetch.

Now that the default flip on fetch is false, we must flip images everywhere when we fetch if we expect to render them in the WebGL context (with the exception of 3D models which don't get flipped on upload). This means:

• All imagery.
  • I tested this with by cycling through all the imagery layers in the widget picket in the Hello World Sandcastle.
  • By confirming all the layers look the same as in 1.55 in the Imagery Layers Sandcastle.
• Materials with images
  • Check the "image" dropdown in the Materials Sandcastle.
  • Check the Ground Materials for good measure.
• CubeMap materials and CubeMaps
  • I couldn't find a good test case for this? See Material.js:880 and loadCubeMap.js
• The PinBuilder
  • This is subtle, but it's actually upside down in 1.56. See Map Pins Sandcastle.
  • Fixing this involved flipping in TextureAtlas, which should fix the Billboards too.
• PostProcess effects with image uniforms
  • This one has no Sandcastle. So I created one. The Cesium logo should be right side up in full screen.

It's also important to test these in Firefox, where ImageBitmap is not supported.

• All the tests above work in Firefox.

[cesium-concierge]

Thanks for the pull request @OmarShehata!

• ✔️ Signed CLA found.

Reviewers, don't forget to make sure that:

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

[OmarShehata]

@lilleyse this is good to look at. Consider if this breaking change is indeed the behavior we want over the current behavior.

[lilleyse]

PostProcess effects with image uniforms

Lens flare might have them. Though your Sandcastle is more obviously correct.

[lilleyse]

The code looks fine. Chrome and Firefox sandcastle demos look good. I think the breaking change is acceptable, it's not super common behavior to see and seems unavoidable with ImageBitmap. I'm going to wait to merge until tomorrow in case others want to comment.

[OmarShehata]

Just to be perfectly clear what the breaking change here is, consider the following psuedocode:

var image = resource.fetchImage();
var texture = new Texture({ source: image });
gl.draw(texture);

• In 1.55, this renders right side up
• In 1.56, this renders right side up
• In 1.56.1, this renders upside down, but only in Chrome

This flipped-but-only-in-chrome (aka when ImageBitmap is supported) issue is what I thought is strange behavior that I was trying to avoid. This happens because the default orientation for fetching an image is not the same as for uploading a texture anymore.

On the bright side, here's what it does fix. Consider this psuedocode:

var image = resource.fetchImage()
image.readPixels();

The pixels are going to be:

• In 1.55, right side up
• In 1.56, upside down, but only in Chrome.
• In 1.56.1, right side up.

So I guess either way there's a "flipped but only when ImageBitmap is supported" issue, but this seems like the lesser of the evils. The confusion in 1.56.1 would be in our own codebase vs exposed to the user.

One thing we could do make it cleaner after this is change the default flipY in Texture to match fetchImage. Ideally it'd be nice to fix imagery so it's flipped in the shader, but that's a larger change, so this would at least make it explicit that when you're uploading a texture that you know needs to be flipped, the image orientation during fetch should also match.

[OmarShehata]

@mramato did you want to weigh in on this behavior? It's summarized in #7701 (comment).

[mramato]

Overall, we need to have consistent behavior across all browsers, otherwise it's a leaky abstraction and a bad API. If I'm understanding this PR correctly, this fixes the inconsistent behavior from an external/users point of view and makes it consistent with 1.55.

If that's the case, I would not consider this a breaking change for 1.56.1 but instead it fixes a bug in 1.56.0 (the bug being inconsistent behavior across browsers)

Texture is a private class and not part of the Cesium API, so I'm not worried about it's usage.

Assuming @lilleyse is happy with everything, I guess the only question I have is: Using the official API, are there any places in the code where a user needs to be aware of texture flipping or special case handling if ImageBitmap is supported? I hope the answer is no (or at least no more than 1.55). If the answer is yes, then what are those cases and how can we fix it so the answer becomes no?

[OmarShehata]

Using the official API, are there any places in the code where a user needs to be aware of texture flipping or special case handling if ImageBitmap is supported?

This is what we were trying to figure out last night. Most of our codepaths that are user-facing take an image string, and fetch it (so it will flip because it knows it's about to upload as a texture).

The only way this would break is if a user is fetching the image themselves using resource.fetchImage() and passing it in. In this case, it would be too late to flip the image, and it will appear upside down.

[OmarShehata]

If we want the safest possible approach that makes no breaking change, we could add an option to choose whether to decode using ImageBitmap, make it false by default, and then enable it throughout our codebase.

[OmarShehata]

@lilleyse here's a Sandcastle to demonstrate this.

This will work in Firefox but not in Chrome. This is because Material doesn't take in an ImageBitmap.

We can allow Material to take an ImageBitmap, but then it will be flipped.

The only way to make this not a breaking change is to have fetchImage just not use createImageBitmap by default. Then we can use it internally in the codebase, but doesn't change the public interface in any way, and everyone is happy.

[mramato]

If we want the safest possible approach that makes no breaking change, we could add an option to choose whether to decode using ImageBitmap, make it false by default, and then enable it throughout our codebase.

This sounds like the right approach, perhaps a preferImageBitmap option similar to preferBlob?

[OmarShehata]

We actually considered doing this when we acknowledged that this was "technically a breaking change":

#7579 (comment)

Looks like it slipped due to my lack of foresight here:

I contemplated adding an option to say disableImageBitmap or something but I can't think of any good reason why you'd want this in the real world. I will update CHANGES.md to mention it's a breaking change.

[mramato]

@OmarShehata don't beat yourself up too much, the important thing is we have a fix and we all learned something.

@lilleyse what do you think about the proposed solution? Makes sense to me and will be relatively painless for us internally. We can always make a larger (and planned) breaking change in the future if we decide it will allow us to simplify the API.

[lilleyse]

@mramato I think it makes perfect sense.

[OmarShehata]

@lilleyse this should be good now. By default, fetchImage() now returns an Image just like before. Materials, Billboards and Labels don't use preferImateBitmap: true. Anything that takes an image from a user (the post process states too) does this. So this fixes #7700 as well.

[lilleyse]

There are a few places including here that don't pass in preferBlob: true. Should they?

[lilleyse]

Especially curious about ImageryProvider not using preferBlob since preferBlob is helpful for ImageBitmap.

[OmarShehata]

I'm actually not sure when in general we want to set preferBlob: true. @mramato do you have any insight here?

In terms of using ImageBitmap, preferImageBitmap forces it to fetch it as a blob, otherwise decoding is locked to the main thread.

[mramato]

preferBlob is something you don't want to set unless you are working around limitations inherent in using an Image element.

Some of these include:

1. Sending headers with the request, for example an Authorization header with an access token in it for secured urls, can't be accomplished with an Image element.
2. Getting the size/length of the resulting data DiscardMissingTileImagePolicy does this.

It's possible I'm forgetting something, but I think those are the only 2. If preferImageBitmap implies preferBlob is true, that's fine and the end result is transparent to the user.

[OmarShehata]

So, if you set preferBlob: true, it still gives you back an Image. It just stores the blob to it as well so you can do things like read the bytes or size etc. right?

The image.blob = blob is not set when using ImageBitmap, so I'm surprised nothing has broken there. Also, it looks like if an ImageryProvider has a discard policy, it's going to load every tile with its blob?

https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/ImageryProvider.js#L347-L349

[OmarShehata]

It looks like this is necessary because when you first create the DiscardMissingTileImagePolicy, it does one request, to check what the missing tile looks like? And then that's why we need preferBlob: true on every subsequent request, to check if what we got is a missing tile or a real tile, if I'm reading this right.

[OmarShehata]

Ok, so looks like all we need to do here is make sure when you set preferBlob: true you can access the blob, regardless of whether it's loading using Image or ImageBitmap. But when you want to use preferBlob and when you want to use preferImageBitmap are unrelated.

[lilleyse]

And part of that was my confusion because I didn't notice that ImageBitmap was always requesting a blob regardless of the preferBlob setting. So preferBlob is orthogonal and its main purpose is to just attach a blob to the Image/ImageBitmap to be used by DiscardMissingTileImagePolicy.

[mramato]

Yes, but also to send headers. If preferBlob is not set and there are headers with the request, we use a blob.

[lilleyse]

I pushed some doc tweaks: 22f1943

[OmarShehata]

@lilleyse the last commit makes sure ImageBitmap keeps a reference to .blob in the same way image.blob was saved before.

I also made sure to flipY: true in the DiscardImage policy fetch, since we flipY for any imagery request.

[lilleyse]

Code and Sandcastle demos look good.