From 3529f8a40af1d8dca0aea02533fed486035fe6d0 Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 21 Jun 2016 02:15:39 +0200 Subject: [PATCH 1/4] Frame subscriber bugfix + added only_damaged option --- atom/browser/api/atom_api_web_contents.cc | 11 +++++++-- atom/browser/api/atom_api_web_contents.h | 3 +-- atom/browser/api/frame_subscriber.cc | 30 ++++++++++++++++------- atom/browser/api/frame_subscriber.h | 10 +++++--- 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 52d24084d4b3..1a2318f4047e 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1195,11 +1195,18 @@ void WebContents::SendInputEvent(v8::Isolate* isolate, } void WebContents::BeginFrameSubscription( - const FrameSubscriber::FrameCaptureCallback& callback) { + mate::Arguments* args) { + FrameSubscriber::FrameCaptureCallback callback; + if (!args->GetNext(&callback)) + return; + + bool only_damaged = false; + args->GetNext(&only_damaged); + const auto view = web_contents()->GetRenderWidgetHostView(); if (view) { std::unique_ptr frame_subscriber(new FrameSubscriber( - isolate(), view, callback)); + isolate(), view, callback, only_damaged)); view->BeginFrameSubscription(std::move(frame_subscriber)); } } diff --git a/atom/browser/api/atom_api_web_contents.h b/atom/browser/api/atom_api_web_contents.h index 6151110887aa..dd91360c5fea 100644 --- a/atom/browser/api/atom_api_web_contents.h +++ b/atom/browser/api/atom_api_web_contents.h @@ -139,8 +139,7 @@ class WebContents : public mate::TrackableObject, void SendInputEvent(v8::Isolate* isolate, v8::Local input_event); // Subscribe to the frame updates. - void BeginFrameSubscription( - const FrameSubscriber::FrameCaptureCallback& callback); + void BeginFrameSubscription(mate::Arguments* args); void EndFrameSubscription(); // Methods for creating . diff --git a/atom/browser/api/frame_subscriber.cc b/atom/browser/api/frame_subscriber.cc index f81a8bea8b1a..364a146d7825 100644 --- a/atom/browser/api/frame_subscriber.cc +++ b/atom/browser/api/frame_subscriber.cc @@ -6,6 +6,7 @@ #include "base/bind.h" #include "atom/common/node_includes.h" +#include "atom/common/native_mate_converters/gfx_converter.h" #include "content/public/browser/render_widget_host.h" namespace atom { @@ -14,8 +15,10 @@ namespace api { FrameSubscriber::FrameSubscriber(v8::Isolate* isolate, content::RenderWidgetHostView* view, - const FrameCaptureCallback& callback) - : isolate_(isolate), view_(view), callback_(callback), weak_factory_(this) { + const FrameCaptureCallback& callback, + const bool& only_damaged) + : isolate_(isolate), view_(view), only_damaged_(only_damaged), + callback_(callback), weak_factory_(this) { } bool FrameSubscriber::ShouldCaptureFrame( @@ -27,21 +30,27 @@ bool FrameSubscriber::ShouldCaptureFrame( if (!view_ || !host) return false; - const auto size = view_->GetVisibleViewportSize(); + if (damage_rect.width() == 0 || damage_rect.height() == 0) + return false; + + gfx::Rect rect = gfx::Rect(view_->GetVisibleViewportSize()); + if (only_damaged_) + rect = damage_rect; host->CopyFromBackingStore( - gfx::Rect(size), - size, + rect, + rect.size(), base::Bind(&FrameSubscriber::OnFrameDelivered, - weak_factory_.GetWeakPtr(), callback_), + weak_factory_.GetWeakPtr(), callback_, rect), kBGRA_8888_SkColorType); return false; } void FrameSubscriber::OnFrameDelivered(const FrameCaptureCallback& callback, - const SkBitmap& bitmap, content::ReadbackResponse response) { - if (bitmap.computeSize64() == 0) + const gfx::Rect& damage_rect, const SkBitmap& bitmap, + content::ReadbackResponse response) { + if (response != content::ReadbackResponse::READBACK_SUCCESS) return; v8::Locker locker(isolate_); @@ -57,7 +66,10 @@ void FrameSubscriber::OnFrameDelivered(const FrameCaptureCallback& callback, reinterpret_cast(node::Buffer::Data(buffer.ToLocalChecked())), rgb_arr_size); - callback_.Run(buffer.ToLocalChecked()); + v8::Local damage = + mate::Converter::ToV8(isolate_, damage_rect); + + callback_.Run(buffer.ToLocalChecked(), damage); } } // namespace api diff --git a/atom/browser/api/frame_subscriber.h b/atom/browser/api/frame_subscriber.h index a803d75dff20..9ab7f1eb9bd3 100644 --- a/atom/browser/api/frame_subscriber.h +++ b/atom/browser/api/frame_subscriber.h @@ -20,11 +20,13 @@ namespace api { class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber { public: - using FrameCaptureCallback = base::Callback)>; + using FrameCaptureCallback = + base::Callback, v8::Local)>; FrameSubscriber(v8::Isolate* isolate, content::RenderWidgetHostView* view, - const FrameCaptureCallback& callback); + const FrameCaptureCallback& callback, + const bool& only_damaged); bool ShouldCaptureFrame(const gfx::Rect& damage_rect, base::TimeTicks present_time, @@ -33,11 +35,13 @@ class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber { private: void OnFrameDelivered(const FrameCaptureCallback& callback, - const SkBitmap& bitmap, content::ReadbackResponse response); + const gfx::Rect& damage_rect, const SkBitmap& bitmap, + content::ReadbackResponse response); v8::Isolate* isolate_; content::RenderWidgetHostView* view_; FrameCaptureCallback callback_; + bool only_damaged_; base::WeakPtrFactory weak_factory_; From 712141f1533b484f86fc5d80413b485ceb16617b Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 21 Jun 2016 13:35:30 +0200 Subject: [PATCH 2/4] Updated docs for beginFrameSubscription --- docs/api/web-contents.md | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index fec4708d1c60..3d953a0a270f 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -917,12 +917,14 @@ For the `mouseWheel` event, the `event` object also have following properties: * `hasPreciseScrollingDeltas` Boolean * `canScroll` Boolean -### `webContents.beginFrameSubscription(callback)` +### `webContents.beginFrameSubscription(callback, onlyDamaged)` * `callback` Function +* `onlyDamaged` Boolean Begin subscribing for presentation events and captured frames, the `callback` -will be called with `callback(frameBuffer)` when there is a presentation event. +will be called with `callback(frameBuffer, damagedRect)` when there is a +presentation event. The `frameBuffer` is a `Buffer` that contains raw pixel data. On most machines, the pixel data is effectively stored in 32bit BGRA format, but the actual @@ -930,6 +932,11 @@ representation depends on the endianness of the processor (most modern processors are little-endian, on machines with big-endian processors the data is in 32bit ARGB format). +The `damagedRect` is an object with `x, y, width, height` properties that +describes which part of the page was repainted. If `onlyDamaged` is set to +`true`, `frameBuffer` will only contain the repainted area. `onlyDamaged` +defaults to `false`. + ### `webContents.endFrameSubscription()` End subscribing for frame presentation events. From 3c92825e2aab64abfde6efeb54ac1069a0832221 Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Tue, 21 Jun 2016 14:32:22 +0200 Subject: [PATCH 3/4] Fix init list order --- atom/browser/api/frame_subscriber.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atom/browser/api/frame_subscriber.cc b/atom/browser/api/frame_subscriber.cc index 364a146d7825..db78e7e93663 100644 --- a/atom/browser/api/frame_subscriber.cc +++ b/atom/browser/api/frame_subscriber.cc @@ -17,8 +17,8 @@ FrameSubscriber::FrameSubscriber(v8::Isolate* isolate, content::RenderWidgetHostView* view, const FrameCaptureCallback& callback, const bool& only_damaged) - : isolate_(isolate), view_(view), only_damaged_(only_damaged), - callback_(callback), weak_factory_(this) { + : isolate_(isolate), view_(view), callback_(callback), + only_damaged_(only_damaged), weak_factory_(this) { } bool FrameSubscriber::ShouldCaptureFrame( From 5118def724d650a95360828c45a572f1d1ee9deb Mon Sep 17 00:00:00 2001 From: Heilig Benedek Date: Sat, 25 Jun 2016 18:23:40 +0200 Subject: [PATCH 4/4] damaged -> dirty rename, fixed misc issues, changed signature, updated docs and added tests --- atom/browser/api/atom_api_web_contents.cc | 12 +++--- atom/browser/api/frame_subscriber.cc | 12 +++--- atom/browser/api/frame_subscriber.h | 4 +- docs/api/web-contents.md | 12 +++--- spec/api-browser-window-spec.js | 45 +++++++++++++++++------ spec/fixtures/api/frame-subscriber.html | 11 ++++++ 6 files changed, 65 insertions(+), 31 deletions(-) create mode 100644 spec/fixtures/api/frame-subscriber.html diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 1a2318f4047e..bfedddce8a34 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -1197,16 +1197,18 @@ void WebContents::SendInputEvent(v8::Isolate* isolate, void WebContents::BeginFrameSubscription( mate::Arguments* args) { FrameSubscriber::FrameCaptureCallback callback; - if (!args->GetNext(&callback)) - return; + bool only_dirty = false; - bool only_damaged = false; - args->GetNext(&only_damaged); + if (!args->GetNext(&callback)) { + args->GetNext(&only_dirty); + if (!args->GetNext(&callback)) + args->ThrowTypeError("'callback' must be defined"); + } const auto view = web_contents()->GetRenderWidgetHostView(); if (view) { std::unique_ptr frame_subscriber(new FrameSubscriber( - isolate(), view, callback, only_damaged)); + isolate(), view, callback, only_dirty)); view->BeginFrameSubscription(std::move(frame_subscriber)); } } diff --git a/atom/browser/api/frame_subscriber.cc b/atom/browser/api/frame_subscriber.cc index db78e7e93663..91b6765db189 100644 --- a/atom/browser/api/frame_subscriber.cc +++ b/atom/browser/api/frame_subscriber.cc @@ -16,13 +16,13 @@ namespace api { FrameSubscriber::FrameSubscriber(v8::Isolate* isolate, content::RenderWidgetHostView* view, const FrameCaptureCallback& callback, - const bool& only_damaged) + bool only_dirty) : isolate_(isolate), view_(view), callback_(callback), - only_damaged_(only_damaged), weak_factory_(this) { + only_dirty_(only_dirty), weak_factory_(this) { } bool FrameSubscriber::ShouldCaptureFrame( - const gfx::Rect& damage_rect, + const gfx::Rect& dirty_rect, base::TimeTicks present_time, scoped_refptr* storage, DeliverFrameCallback* callback) { @@ -30,12 +30,12 @@ bool FrameSubscriber::ShouldCaptureFrame( if (!view_ || !host) return false; - if (damage_rect.width() == 0 || damage_rect.height() == 0) + if (dirty_rect.IsEmpty()) return false; gfx::Rect rect = gfx::Rect(view_->GetVisibleViewportSize()); - if (only_damaged_) - rect = damage_rect; + if (only_dirty_) + rect = dirty_rect; host->CopyFromBackingStore( rect, diff --git a/atom/browser/api/frame_subscriber.h b/atom/browser/api/frame_subscriber.h index 9ab7f1eb9bd3..b6cff3da81db 100644 --- a/atom/browser/api/frame_subscriber.h +++ b/atom/browser/api/frame_subscriber.h @@ -26,7 +26,7 @@ class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber { FrameSubscriber(v8::Isolate* isolate, content::RenderWidgetHostView* view, const FrameCaptureCallback& callback, - const bool& only_damaged); + bool only_dirty); bool ShouldCaptureFrame(const gfx::Rect& damage_rect, base::TimeTicks present_time, @@ -41,7 +41,7 @@ class FrameSubscriber : public content::RenderWidgetHostViewFrameSubscriber { v8::Isolate* isolate_; content::RenderWidgetHostView* view_; FrameCaptureCallback callback_; - bool only_damaged_; + bool only_dirty_; base::WeakPtrFactory weak_factory_; diff --git a/docs/api/web-contents.md b/docs/api/web-contents.md index 3d953a0a270f..56263780a08f 100644 --- a/docs/api/web-contents.md +++ b/docs/api/web-contents.md @@ -917,13 +917,13 @@ For the `mouseWheel` event, the `event` object also have following properties: * `hasPreciseScrollingDeltas` Boolean * `canScroll` Boolean -### `webContents.beginFrameSubscription(callback, onlyDamaged)` +### `webContents.beginFrameSubscription([onlyDirty ,]callback)` +* `onlyDirty` Boolean * `callback` Function -* `onlyDamaged` Boolean Begin subscribing for presentation events and captured frames, the `callback` -will be called with `callback(frameBuffer, damagedRect)` when there is a +will be called with `callback(frameBuffer, dirtyRect)` when there is a presentation event. The `frameBuffer` is a `Buffer` that contains raw pixel data. On most machines, @@ -932,9 +932,9 @@ representation depends on the endianness of the processor (most modern processors are little-endian, on machines with big-endian processors the data is in 32bit ARGB format). -The `damagedRect` is an object with `x, y, width, height` properties that -describes which part of the page was repainted. If `onlyDamaged` is set to -`true`, `frameBuffer` will only contain the repainted area. `onlyDamaged` +The `dirtyRect` is an object with `x, y, width, height` properties that +describes which part of the page was repainted. If `onlyDirty` is set to +`true`, `frameBuffer` will only contain the repainted area. `onlyDirty` defaults to `false`. ### `webContents.endFrameSubscription()` diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 7310cd6a9044..b5f55905589a 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -603,21 +603,42 @@ describe('browser-window module', function () { }) describe('beginFrameSubscription method', function () { - this.timeout(20000) + it('subscribes to frame updates', function (done) { + this.timeout(20000) - it('subscribes frame updates', function (done) { - let called = false - w.loadURL('file://' + fixtures + '/api/blank.html') - w.webContents.beginFrameSubscription(function (data) { - // This callback might be called twice. - if (called) return - called = true - - assert.notEqual(data.length, 0) - w.webContents.endFrameSubscription() - done() + w.loadURL('file://' + fixtures + '/api/frame-subscriber.html') + w.webContents.on('dom-ready', function () { + w.webContents.beginFrameSubscription(function (data) { + assert.notEqual(data.length, 0) + w.webContents.endFrameSubscription() + done() + }) }) }) + + it('subscribes to frame updates (only dirty rectangle)', function (done) { + this.timeout(20000) + + w.loadURL('file://' + fixtures + '/api/frame-subscriber.html') + w.webContents.on('dom-ready', function () { + w.webContents.beginFrameSubscription(true, function (data) { + assert.notEqual(data.length, 0) + w.webContents.endFrameSubscription() + done() + }) + }) + }) + + it('throws error when subscriber is not well defined', function (done) { + this.timeout(20000) + + w.loadURL('file://' + fixtures + '/api/frame-subscriber.html') + try{ + w.webContents.beginFrameSubscription(true, true) + } catch(e) { + done() + } + }) }) describe('savePage method', function () { diff --git a/spec/fixtures/api/frame-subscriber.html b/spec/fixtures/api/frame-subscriber.html new file mode 100644 index 000000000000..0d5c7b317847 --- /dev/null +++ b/spec/fixtures/api/frame-subscriber.html @@ -0,0 +1,11 @@ + + +
+ + +