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

Fixes buffer size in offscreen mode #6713

Merged
merged 4 commits into from Aug 4, 2016

Conversation

Projects
None yet
4 participants
@gerhardberger
Member

gerhardberger commented Aug 3, 2016

This PR fixes the buffer size issue in the 'paint' event in offscreen mode. #6704

Also resets some properties in OffScreenRenderWidgetHostView's destructor.

@@ -379,6 +379,13 @@ OffScreenRenderWidgetHostView::~OffScreenRenderWidgetHostView() {
#if defined(OS_MACOSX)
DestroyPlatformWidget();
#endif
if (copy_frame_generator_.get())
copy_frame_generator_.reset(NULL);

This comment has been minimized.

@miniak

miniak Aug 3, 2016

Contributor

Please use nullptr instead

mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate());
dict.Set("width", bitmap_size.width());
dict.Set("height", bitmap_size.height());
dict.Set("pixel", pixel_size);

This comment has been minimized.

@miniak

miniak Aug 3, 2016

Contributor

This should be named bytesPerPixel, just pixel is confusing

This comment has been minimized.

@miniak

miniak Aug 3, 2016

Contributor

But also it's much more common to have bits per pixel (8/16/24/32) not (1/2/3/4).

@gerhardberger gerhardberger referenced this pull request Aug 3, 2016

Closed

Problems with offscreen mode #6704

5 of 5 tasks complete
@@ -1345,11 +1345,19 @@ bool WebContents::IsOffScreen() const {
void WebContents::OnPaint(const gfx::Rect& dirty_rect,
const gfx::Size& bitmap_size,
const int pixel_size,
void* bitmap_pixels) {
v8::MaybeLocal<v8::Object> buffer = node::Buffer::New(

This comment has been minimized.

@zcbenz

zcbenz Aug 3, 2016

Contributor

Calling node::Buffer::New means the data is transferred to Node to manage, so once the returned data is garbage collected the bitmap_pixels would be deleted, while it is still being managed by someone class. I think we should do node::Buffer::Copy here.

delegated_frame_host_.reset(nullptr);
compositor_.reset(nullptr);
root_layer_.reset(nullptr);

This comment has been minimized.

@zcbenz

zcbenz Aug 3, 2016

Contributor

These classes are managed by unique_ptrs, so they are automatically deleted and we don't need to manually reset them here.

mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate());
dict.Set("width", bitmap_size.width());
dict.Set("height", bitmap_size.height());
dict.Set("bytesPerPixel", pixel_size);

This comment has been minimized.

@zcbenz

zcbenz Aug 4, 2016

Contributor

We probably don't need to expose this, the Buffer records its own length, so anyone who needs this can simply calculate it themselves. My concern is for other APIs the size is always an object with width and height, this breaks the consistency.

@@ -1345,11 +1345,19 @@ bool WebContents::IsOffScreen() const {
void WebContents::OnPaint(const gfx::Rect& dirty_rect,
const gfx::Size& bitmap_size,
const int pixel_size,

This comment has been minimized.

@zcbenz

zcbenz Aug 4, 2016

Contributor

For primitive types there is no need to add const, since they are just copied, using int directly is preferred.

removed bpp (it can be calculated)
fixed buffer size calculation (we actually send the whole image, not just the dirty part)
removed the unnecessary resets and const
now we use Copy instead of New
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 4, 2016

Thanks!

@zcbenz zcbenz merged commit 32d9382 into electron:master Aug 4, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment