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

Avoid unnecessary image decode. #7550

Merged
merged 2 commits into from
Nov 8, 2018
Merged

Conversation

dcervelli
Copy link
Contributor

image_source.js unnecessarily decodes image data via JavaScript before creating a Texture from the image data.

The Texture class can handle a native HTMLImageElement directly and then the browser can decode the image much more quickly and with less memory.

For my use case, this results in about a 10-20% reduction in scripting time.

@mourner
Copy link
Member

mourner commented Nov 5, 2018

It was originally like this but changed in this commit 2d359e9 — can you make sure this won't break the use cases that necessiated that change?

@dcervelli
Copy link
Contributor Author

I pored over that commit and a few other related ones to determine what the intent was behind that change but don't have a definitive answer.

It seems like the change to image_source.js is unrelated to the broader commit (changing the public API of Map#addImage).

A couple of threads I tried to track down:

  • Was the browser.getImageData call added to correspond to the desired change to the type in _prepareImage? This doesn't make a ton of sense though since the original HTMLImageElement should have been compatible with TexImageSource.
  • Was the browser.getImageData call needed because most of the guts of dealing with texture were in image_source.js at the time (as opposed to texture.js, where they are now)? Again, this isn't an adequate explanation because the HTMLImageElement appears to be a sufficient type for the texture methods.

Given that image is private, the types are all compatible, and the changed line appears to be unrelated to the commit message where it was changed, this seems like a safe change.

@jfirebaugh, any background you could add?

Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

I don't see why would it break anything too, so I think we should just merge — we can always revert later if anything comes up.

@mourner mourner merged commit 9232211 into mapbox:master Nov 8, 2018
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 14, 2019
* Avoid unnecessary image decode.

* Remove unused import.
pirxpilot pushed a commit to pirxpilot/mapbox-gl-js that referenced this pull request Jun 18, 2019
* Avoid unnecessary image decode.

* Remove unused import.
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.

2 participants