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

refactor: use typeutils for nativeImage serialization #23693

Merged
merged 1 commit into from
May 22, 2020

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented May 21, 2020

Description of Change

Follow-up to #23666, as stated in PR body. Uses existing typeUtils to serialize nativeImages and improves perf by using a buffer to construct the nativeImage if there is only one representation.

cc @miniak @MarshallOfSound

Checklist

Release Notes

Notes: none

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 21, 2020
const { size, scaleFactor, dataURL } = rep;

// Use Buffer when there's only one representation for better perf.
// This avoids compressing to/from PNG where it's not necessary to
Copy link
Member

Choose a reason for hiding this comment

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

Interesting in knowing more about this, why can't multiple representations use the bitmap buffer?

Copy link
Member Author

Choose a reason for hiding this comment

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

If one uses a buffer to construct an image with multiple representation from multiple dataURLs, the dataURLs themselves are not preserved. If you test:

    const image = nativeImage.createEmpty();

    const dataURL1 = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAAC0lEQVQYlWNgAAIAAAUAAdafFs0AAAAASUVORK5CYII=';
    image.addRepresentation({ scaleFactor: 1.0, dataURL: dataURL1 });

    const dataURL2 = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAIAAAACCAIAAAD91JpzAAAAFklEQVQYlWP8//8/AwMDEwMDAwMDAwAkBgMBBMzldwAAAABJRU5ErkJggg==';
    image.addRepresentation({ scaleFactor: 2.0, dataURL: dataURL2 });

    const serializedImage = serialize(image);
    const nonEmpty = deserialize(serializedImage);

    expect(nonEmpty.toDataURL({ scaleFactor: 1.0 })).to.equal(dataURL1);
    expect(nonEmpty.toDataURL({ scaleFactor: 2.0 })).to.equal(dataURL2);

expect(nonEmpty.toDataURL({ scaleFactor: 2.0 })).to.equal(dataURL2); will fail, as the only dataURL associated with the image will be dataURL1. However, there's a perf boost with buffer serialization (see: #23549) so we want to allow it where possible.

@codebytere codebytere force-pushed the use-typeutils-remote branch 2 times, most recently from 45cb617 to 14121d1 Compare May 21, 2020 16:42
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 22, 2020
@codebytere codebytere merged commit 762f7bc into master May 22, 2020
@release-clerk
Copy link

release-clerk bot commented May 22, 2020

No Release Notes

@trop
Copy link
Contributor

trop bot commented May 27, 2020

@codebytere has manually backported this PR to "10-x-y", please check out #23794

@trop
Copy link
Contributor

trop bot commented May 27, 2020

@codebytere has manually backported this PR to "9-x-y", please check out #23796

@trop
Copy link
Contributor

trop bot commented May 27, 2020

@codebytere has manually backported this PR to "8-x-y", please check out #23797

codebytere added a commit that referenced this pull request May 28, 2020
* refactor: use typeutils for nativeImage serialization (#23693)

* fix: ensure nativeImage serialization main->renderer
@ckerr ckerr mentioned this pull request Jun 8, 2020
6 tasks
@trop
Copy link
Contributor

trop bot commented Jun 8, 2020

@ckerr has manually backported this PR to "7-3-x", please check out #24021

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.

None yet

3 participants