Skip to content

ImageData WebGL Textures #414

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

Merged
merged 2 commits into from
Mar 26, 2019

Conversation

mzgoddard
Copy link
Contributor

Resolves

Load faster and use less memory.

Proposed Changes

  • Support ImageData as an argument to Silhouette.update
  • Get image data out of input canvases to BitmapSkin.setBitmap and SVGSkin.setSVG. And pass image data to webgl when creating or updating the texture.

Reason for Changes

Getting the ImageData in setBitmap and setSVG lets us skip an extra draw operation to Silhouette's update canvas. It likely also skips a step browsers perform to do a operation similar to getImageData and instead use ImageData directly. This provides a significant reduction in the time it takes to turn asset data in Storage into a Skin.

Benchmark Data

Pending ...

@thisandagain
Copy link
Contributor

Thanks @mzgoddard !

/cc @kchadha @BryceLTaylor

Given ImageData we can skip drawing the input and getting image data.
This can help if update's color can also use the ImageData directly.
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

This looks great! There's one line (in two places) that I have questions about, and one possible typo, but I really like the idea of using ImageData when possible.

When possible pass ImageData to texture creation and updating to help
remove chance of references that keep canvas and underlying data from
being garbage collected.
@mzgoddard mzgoddard force-pushed the image-data-texture branch from eaeb183 to e31934f Compare March 19, 2019 21:52
Copy link
Contributor

@cwillisf cwillisf left a comment

Choose a reason for hiding this comment

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

👍 LGTM!

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

Successfully merging this pull request may close these issues.

3 participants