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

Change default Resource.fetchImage flipY to false #7701

Merged
merged 7 commits into from
Apr 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
Change Log
==========

### 1.56.1 - 2019-04-02

##### Additions :tada:
* `Resource.fetchImage` now takes a `preferImageBitmap` option to use `createImageBitmap` when supported to move image decode off the main thread. This option defaults to `false`.

##### Breaking Changes :mega:
* The following breaking changes are relative to 1.56. The `Resource.fetchImage` behavior is now identical to 1.55 and earlier.
* Changed `Resource.fetchImage` back to return an `Image` by default, instead of an `ImageBitmap` when supported. Note that an `ImageBitmap` cannot be flipped during texture upload. Instead, set `flipY : true` during fetch to flip it.
* Changed the default `flipY` option in `Resource.fetchImage` to false. This only has an effect when ImageBitmap is used.

### 1.56 - 2019-04-01

##### Breaking Changes :mega:
Expand Down
1 change: 1 addition & 0 deletions Source/Core/IonResource.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,7 @@ define([
};
if (defined(userOptions)) {
options.flipY = userOptions.flipY;
options.preferImageBitmap = userOptions.preferImageBitmap;
}
}

Expand Down
87 changes: 56 additions & 31 deletions Source/Core/Resource.js
Original file line number Diff line number Diff line change
Expand Up @@ -872,12 +872,13 @@ define([

/**
* Asynchronously loads the given image resource. Returns a promise that will resolve to
* an {@link https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap|ImageBitmap} if the browser supports `createImageBitmap` or otherwise an
* an {@link https://developer.mozilla.org/en-US/docs/Web/API/ImageBitmap|ImageBitmap} if <code>preferImageBitmap</code> is true and the browser supports <code>createImageBitmap</code> or otherwise an
* {@link https://developer.mozilla.org/en-US/docs/Web/API/HTMLImageElement|Image} once loaded, or reject if the image failed to load.
*
* @param {Object} [options] An object with the following properties.
* @param {Boolean} [options.preferBlob=false] If true, we will load the image via a blob.
* @param {Boolean} [options.flipY=true] If true, image will be vertially flipped during decode. Only applies if the browser supports `createImageBitmap`.
* @param {Boolean} [options.preferImageBitmap=false] If true, image will be decoded during fetch and an <code>ImageBitmap</code> is returned.
* @param {Boolean} [options.flipY=false] If true, image will be vertically flipped during decode. Only applies if the browser supports <code>createImageBitmap</code>.
* @returns {Promise.<ImageBitmap>|Promise.<Image>|undefined} a promise that will resolve to the requested data when loaded. Returns undefined if <code>request.throttle</code> is true and the request does not have high enough priority.
*
*
Expand Down Expand Up @@ -905,8 +906,9 @@ define([
};
}
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
var preferImageBitmap = defaultValue(options.preferImageBitmap, false);
var preferBlob = defaultValue(options.preferBlob, false);
var flipY = defaultValue(options.flipY, true);
var flipY = defaultValue(options.flipY, false);

checkAndResetRequest(this.request);

Expand All @@ -916,7 +918,11 @@ define([
// 3. It's a blob URI
// 4. It doesn't have request headers and we preferBlob is false
if (!xhrBlobSupported || this.isDataUri || this.isBlobUri || (!this.hasHeaders && !preferBlob)) {
return fetchImage(this, flipY);
return fetchImage({
resource: this,
flipY: flipY,
preferImageBitmap: preferImageBitmap
});
}

var blobPromise = this.fetchBlob();
Expand All @@ -925,40 +931,46 @@ define([
}

var supportsImageBitmap;
var useImageBitmap;
var generatedBlobResource;
var generatedBlob;
return Resource.supportsImageBitmapOptions()
.then(function(result) {
supportsImageBitmap = result;
useImageBitmap = supportsImageBitmap && preferImageBitmap;
return blobPromise;
})
.then(function(blob) {
if (!defined(blob)) {
return;
}
if (supportsImageBitmap) {
generatedBlob = blob;
if (useImageBitmap) {
return Resource._Implementations.createImageBitmapFromBlob(blob, flipY);
}
generatedBlob = blob;
var blobUrl = window.URL.createObjectURL(blob);
generatedBlobResource = new Resource({
url: blobUrl
});

return fetchImage(generatedBlobResource, flipY);
return fetchImage({
resource: generatedBlobResource,
flipY: flipY,
preferImageBitmap: false
});
})
.then(function(image) {
if (!defined(image)) {
return;
}
if (supportsImageBitmap) {
return image;
}
window.URL.revokeObjectURL(generatedBlobResource.url);

// This is because the blob object is needed for DiscardMissingTileImagePolicy
// See https://github.com/AnalyticalGraphicsInc/cesium/issues/1353
image.blob = generatedBlob;
if (useImageBitmap) {
return image;
}

window.URL.revokeObjectURL(generatedBlobResource.url);
return image;
})
.otherwise(function(error) {
Expand All @@ -970,7 +982,21 @@ define([
});
};

function fetchImage(resource, flipY) {
/**
* Fetches an image and returns a promise to it.
*
* @param {Object} [options] An object with the following properties.
* @param {Resource} [options.resource] Resource object that points to an image to fetch.
* @param {Boolean} [options.preferImageBitmap] If true, image will be decoded during fetch and an <code>ImageBitmap</code> is returned.
* @param {Boolean} [options.flipY] If true, image will be vertically flipped during decode. Only applies if the browser supports <code>createImageBitmap</code>.
*
* @private
*/
function fetchImage(options) {
var resource = options.resource;
var flipY = options.flipY;
var preferImageBitmap = options.preferImageBitmap;

var request = resource.request;
request.url = resource.url;
request.requestFunction = function() {
Expand All @@ -984,7 +1010,7 @@ define([

var deferred = when.defer();

Resource._Implementations.createImage(url, crossOrigin, deferred, flipY);
Resource._Implementations.createImage(url, crossOrigin, deferred, flipY, preferImageBitmap);

return deferred.promise;
};
Expand All @@ -1008,7 +1034,11 @@ define([
request.state = RequestState.UNISSUED;
request.deferred = undefined;

return fetchImage(resource, flipY);
return fetchImage({
resource: resource,
flipY: flipY,
preferImageBitmap: preferImageBitmap
});
}

return when.reject(e);
Expand All @@ -1025,18 +1055,20 @@ define([
* @param {Object} [options.templateValues] Key/Value pairs that are used to replace template values (eg. {x}).
* @param {Object} [options.headers={}] Additional HTTP headers that will be sent.
* @param {DefaultProxy} [options.proxy] A proxy to be used when loading the resource.
* @param {Boolean} [options.flipY = true] Whether to vertically flip the image during fetch and decode. Only applies when requesting an image and the browser supports createImageBitmap.
* @param {Boolean} [options.flipY=false] Whether to vertically flip the image during fetch and decode. Only applies when requesting an image and the browser supports <code>createImageBitmap</code>.
* @param {Resource~RetryCallback} [options.retryCallback] The Function to call when a request for this resource fails. If it returns true, the request will be retried.
* @param {Number} [options.retryAttempts=0] The number of times the retryCallback should be called before giving up.
* @param {Request} [options.request] A Request object that will be used. Intended for internal use only.
* @param {Boolean} [options.preferBlob = false] If true, we will load the image via a blob.
* @param {Boolean} [options.preferBlob=false] If true, we will load the image via a blob.
* @param {Boolean} [options.preferImageBitmap=false] If true, image will be decoded during fetch and an <code>ImageBitmap</code> is returned.
* @returns {Promise.<ImageBitmap>|Promise.<Image>|undefined} a promise that will resolve to the requested data when loaded. Returns undefined if <code>request.throttle</code> is true and the request does not have high enough priority.
*/
Resource.fetchImage = function (options) {
var resource = new Resource(options);
return resource.fetchImage({
flipY: options.flipY,
preferBlob: options.preferBlob
preferBlob: options.preferBlob,
preferImageBitmap: options.preferImageBitmap
});
};

Expand Down Expand Up @@ -1848,17 +1880,17 @@ define([
image.src = url;
}

Resource._Implementations.createImage = function(url, crossOrigin, deferred, flipY) {
Resource._Implementations.createImage = function(url, crossOrigin, deferred, flipY, preferImageBitmap) {
// Passing an Image to createImageBitmap will force it to run on the main thread
// since DOM elements don't exist on workers. We convert it to a blob so it's non-blocking.
// See:
// https://bugzilla.mozilla.org/show_bug.cgi?id=1044102#c38
// https://bugs.chromium.org/p/chromium/issues/detail?id=580202#c10
Resource.supportsImageBitmapOptions()
.then(function(result) {
.then(function(supportsImageBitmap) {
// We can only use ImageBitmap if we can flip on decode.
// See: https://github.com/AnalyticalGraphicsInc/cesium/pull/7579#issuecomment-466146898
if (!result) {
if (!(supportsImageBitmap && preferImageBitmap)) {
loadImageElement(url, crossOrigin, deferred);
return;
}
Expand All @@ -1885,16 +1917,9 @@ define([
};

Resource._Implementations.createImageBitmapFromBlob = function(blob, flipY) {
return Resource.supportsImageBitmapOptions()
.then(function(result) {
if (!result) {
return createImageBitmap(blob);
}

return createImageBitmap(blob, {
imageOrientation: flipY ? 'flipY' : 'none'
});
});
return createImageBitmap(blob, {
imageOrientation: flipY ? 'flipY' : 'none'
});
};

function decodeResponse(loadWithHttpResponse, responseType) {
Expand Down
2 changes: 1 addition & 1 deletion Source/Core/VRTheWorldTerrainProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ define([
request : request
});
var promise = resource.fetchImage({
flipY : false
preferImageBitmap: true
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

});
if (!defined(promise)) {
return undefined;
Expand Down
5 changes: 3 additions & 2 deletions Source/Core/loadImageFromTypedArray.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ define([
var uint8Array = options.uint8Array;
var format = options.format;
var request = options.request;
var flipY = defaultValue(options.flipY, true);
var flipY = defaultValue(options.flipY, false);
//>>includeStart('debug', pragmas.debug);
Check.typeOf.object('uint8Array', uint8Array);
Check.typeOf.string('format', format);
Expand All @@ -37,7 +37,8 @@ define([
request: request
});
return resource.fetchImage({
flipY : flipY
flipY : flipY,
preferImageBitmap : true
})
.then(function(image) {
window.URL.revokeObjectURL(blobUrl);
Expand Down
16 changes: 10 additions & 6 deletions Source/Renderer/loadCubeMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,14 +66,18 @@ define([
//
// Also, it is perhaps acceptable to use the context here in the callbacks, but
// ideally, we would do it in the primitive's update function.
var flipOptions = {
flipY : true,
preferImageBitmap: true
};

var facePromises = [
Resource.createIfNeeded(urls.positiveX).fetchImage(),
Resource.createIfNeeded(urls.negativeX).fetchImage(),
Resource.createIfNeeded(urls.positiveY).fetchImage(),
Resource.createIfNeeded(urls.negativeY).fetchImage(),
Resource.createIfNeeded(urls.positiveZ).fetchImage(),
Resource.createIfNeeded(urls.negativeZ).fetchImage()
Resource.createIfNeeded(urls.positiveX).fetchImage(flipOptions),
Resource.createIfNeeded(urls.negativeX).fetchImage(flipOptions),
Resource.createIfNeeded(urls.positiveY).fetchImage(flipOptions),
Resource.createIfNeeded(urls.negativeY).fetchImage(flipOptions),
Resource.createIfNeeded(urls.positiveZ).fetchImage(flipOptions),
Resource.createIfNeeded(urls.negativeZ).fetchImage(flipOptions)
];

return when.all(facePromises, function(images) {
Expand Down
4 changes: 3 additions & 1 deletion Source/Scene/DiscardMissingTileImagePolicy.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,9 @@ define([
}

resource.fetchImage({
preferBlob : true
preferBlob : true,
preferImageBitmap : true,
flipY : true
}).then(success).otherwise(failure);
}

Expand Down
3 changes: 2 additions & 1 deletion Source/Scene/GoogleEarthEnterpriseImageryProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,8 @@ define([

return loadImageFromTypedArray({
uint8Array: a,
format: type
format: type,
flipY: true
});
});
};
Expand Down
9 changes: 7 additions & 2 deletions Source/Scene/ImageryProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -345,11 +345,16 @@ define([
return loadCRN(resource);
} else if (defined(imageryProvider.tileDiscardPolicy)) {
return resource.fetchImage({
preferBlob : true
preferBlob : true,
preferImageBitmap : true,
flipY : true
});
}

return resource.fetchImage();
return resource.fetchImage({
preferImageBitmap : true,
flipY : true
});
};

return ImageryProvider;
Expand Down
4 changes: 1 addition & 3 deletions Source/Scene/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -1723,9 +1723,7 @@ define([
} else if (crnRegex.test(uri)) {
promise = loadCRN(imageResource);
} else {
promise = imageResource.fetchImage({
flipY: false
});
promise = imageResource.fetchImage();
}
promise.then(imageLoad(model, id, imageId)).otherwise(ModelUtility.getFailedLoadFunction(model, 'image', imageResource.url));
}
Expand Down
1 change: 1 addition & 0 deletions Source/Scene/PostProcessStage.js
Original file line number Diff line number Diff line change
Expand Up @@ -670,6 +670,7 @@ define([
var resource = new Resource({
url : stageNameUrlOrImage
});

promises.push(resource.fetchImage().then(createLoadImageFunction(stage, name)));
} else {
stage._texturesToCreate.push({
Expand Down
2 changes: 1 addition & 1 deletion Source/Scene/SingleTileImageryProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ define([
}

function doRequest() {
when(resource.fetchImage(), success, failure);
resource.fetchImage().then(success).otherwise(failure);
}

doRequest();
Expand Down
Loading