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

Frame subscriber improvements #7163

Merged
merged 5 commits into from Sep 16, 2016

Conversation

Projects
None yet
4 participants
@gerhardberger
Copy link
Member

gerhardberger commented Sep 9, 2016

This PR fixes the scale factor issue that was present when enabling frame subscription in <webview>s. #7158

Also it simplifies the frame subscription API exposing a NativeImage instead of a Buffer.

@IsaiahJTurner

This comment has been minimized.

Copy link

IsaiahJTurner commented Sep 9, 2016

Two questions:

  • From what I know NativeImage is not backwards compatible with Buffer so this would be a breaking change? Would the change for upgrading legacy code be as simple as calling getBitmap on image?
  • Are there any performance impacts from using NativeImage?

My current usage of beginFrameSubscription does all the image rendering in C on another thread. I do end up converting it to JPEG but this gives me fine control over optimizing that. I would likely end up calling getBitmap so I can continue this. However, I do see huge advantages to NativeImage since the work involved to even start using beginFrameSubscription is extremely difficult at the moment.

@gerhardberger

This comment has been minimized.

Copy link
Member Author

gerhardberger commented Sep 9, 2016

@IsaiahJTurner using NativeImage has no performance impact, if you use toBitmap() of NativeImage you get the same Buffer as it passed previously.

if you use getBitmap() its even faster, because it doesn't copy the data to a new buffer, but this way you are not in control of it.

yes, I also think this is a more versatile and simple-to-use API for users.

@brenca

This comment has been minimized.

Copy link
Member

brenca commented Sep 9, 2016

@IsaiahJTurner can confirm, a relatively recent change in NativeImage API does allow you to get the raw data without a performance impact.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Sep 16, 2016

Changing the callback type is a breaking API change, and since beginFrameSubscription is already a commonly used API, we have to postpone the API change to 2.0, which would take a very long time.

So can you keep the API unchanged, or add an option to make the behavior optional?

@gerhardberger

This comment has been minimized.

Copy link
Member Author

gerhardberger commented Sep 16, 2016

@zcbenz thanks, I reverted the API to the old one, but I kept it on a branch in my fork for later, because it is a nicer and simpler API I think.

@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Sep 16, 2016

👍

@zcbenz zcbenz merged commit da677b6 into electron:master Sep 16, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.