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

Emit NativeImage objects in paint event #6721

Merged
merged 6 commits into from Aug 5, 2016

Conversation

Projects
None yet
2 participants
@zcbenz
Contributor

zcbenz commented Aug 4, 2016

Instead of passing data as Buffer directly in the paint event, this PR changes to emit NativeImage objects. So the API would be easier to use and test.

Regarding the performance, NativeImage stores the bitmap by simply ref-counting its underlying data, so no copying and encoding are involved, and for users wanting to access the bitmap data, calling toBitmap() is as efficient as before.

@gerhardberger @brenca Please let me know if you have concerns about this.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 4, 2016

@gerhardberger @brenca

Also I found that the whole frame is always sent instead of only the dirty areas, and from the implementation this seems to be expected.Should we fix the docs or find a way to actually only send the dirty areas? Current behavior is totally fine in my option though.

@brenca

This comment has been minimized.

Member

brenca commented Aug 4, 2016

@zcbenz I think using NativeImage is a good idea, we had some memory issues with the buffer::Copy implementation, this could solve that problem, I'm currently testing how big of an overhead it would be compared to buffer::New (which might be unsafe, but faster).
Memory consumption could also be lowered by sending only the dirty areas, the current implementation is what CEF uses, but they don't have to pump the data to node, which seems to be a bottleneck here (I might be wrong).
We'll try to cook up a solution that only sends the dirty areas, I think in that case the overhead of NativeImage (if any) would be negligible, and it would create a nice API indeed. We could make this optional, like in FrameSubscriber. What do you think?

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 4, 2016

If you are seeking for best performance, you can use node::Buffer::New and set the FreeFunction to no-op, so the copy of bitmap is avoided, and node::Buffer will not try to free the memory when garbage collected. The downside is the data is only available during the paint event, but I think it is fine as long as explicitly documented.

We can probably add a getBitmap method to NativeImage which uses node::Buffer::New to return the bitmap data, so for performance purpose users can call getBitmap() to get the bitmap data without copy, and for other purposes users can freely call methods like toPNG().

We'll try to cook up a solution that only sends the dirty areas, I think in that case the overhead of NativeImage (if any) would be negligible, and it would create a nice API indeed. We could make this optional, like in FrameSubscriber. What do you think?

Having an option for only sending dirty areas is good. But I don't know how you would use the API, if you don't need this feature we can just add it in future when someone else needs it.

@brenca

This comment has been minimized.

Member

brenca commented Aug 4, 2016

Adding a getBitmap to NativeImage is a great idea, I'll implement it, try it out and post a PR ASAP, thanks!
We discussed the dirty area thing a bit further with @gerhardberger, and came to the conclusion that it would actually slow down offscreen rendering in most cases, since the software rendering path paints the whole image into a bitmap directly, we would need to copy the relevant parts first, which is rather slow. It could speed up the GPU path, but what we would win with that wouldn't compensate for what we would lose on the software path.

@brenca

This comment has been minimized.

Member

brenca commented Aug 4, 2016

We also did some stress tests with the noop node::Buffer::New method you mentioned, and it's indeed much faster, thanks again for the tip. 👍

@zcbenz zcbenz merged commit d234187 into master Aug 5, 2016

8 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3779761 succeeded in 48s
Details
electron-linux-ia32 Build #3779762 succeeded in 46s
Details
electron-linux-x64 Build #3779763 succeeded in 81s
Details
electron-mas-x64 Build #2112 succeeded in 7 min 11 sec
Details
electron-osx-x64 Build #2109 succeeded in 7 min 9 sec
Details
electron-win-ia32 Build #1133 succeeded in 6 min 20 sec
Details
electron-win-x64 Build #1117 succeeded in 6 min 21 sec
Details

@zcbenz zcbenz deleted the osr-refactor branch Aug 5, 2016

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