fix: the callback of beginFrameSubscription should pass NativeImage instead of Buffer (#17548)

* Revert "revert: frame subscriber deprecation and re-enable tests"

This reverts commit f643ce4f66.

* fix: make sure SkBitmap's pixels are copied

atom/browser/api/atom_api_web_contents.cc

@@ -1825,7 +1825,7 @@ void WebContents::BeginFrameSubscription(mate::Arguments* args) {
   }
 
   frame_subscriber_.reset(
-      new FrameSubscriber(isolate(), web_contents(), callback, only_dirty));
+      new FrameSubscriber(web_contents(), callback, only_dirty));
 }
 
 void WebContents::EndFrameSubscription() {

atom/browser/api/frame_subscriber.cc

@@ -7,11 +7,11 @@
 #include <utility>
 
 #include "atom/common/native_mate_converters/gfx_converter.h"
-#include "atom/common/node_includes.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_view.h"
 #include "media/capture/mojom/video_capture_types.mojom.h"
+#include "ui/gfx/image/image.h"
 #include "ui/gfx/skbitmap_operations.h"
 
 namespace atom {
@@ -20,12 +20,10 @@ namespace api {
 
 constexpr static int kMaxFrameRate = 30;
 
-FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
+FrameSubscriber::FrameSubscriber(content::WebContents* web_contents,
-                                 content::WebContents* web_contents,
                                  const FrameCaptureCallback& callback,
                                  bool only_dirty)
     : content::WebContentsObserver(web_contents),
-      isolate_(isolate),
       callback_(callback),
       only_dirty_(only_dirty),
       weak_ptr_factory_(this) {
@@ -148,26 +146,23 @@ void FrameSubscriber::Done(const gfx::Rect& damage, const SkBitmap& frame) {
   if (frame.drawsNothing())
     return;
 
-  v8::Locker locker(isolate_);
-  v8::HandleScope handle_scope(isolate_);
-
-  const_cast<SkBitmap&>(frame).setAlphaType(kPremul_SkAlphaType);
   const SkBitmap& bitmap = only_dirty_ ? SkBitmapOperations::CreateTiledBitmap(
                                              frame, damage.x(), damage.y(),
                                              damage.width(), damage.height())
                                        : frame;
 
-  size_t rgb_row_size = bitmap.width() * bitmap.bytesPerPixel();
+  // Copying SkBitmap does not copy the internal pixels, we have to manually
-  auto* source = static_cast<const char*>(bitmap.getPixels());
+  // allocate and write pixels otherwise crash may happen when the original
+  // frame is modified.
+  SkBitmap copy;
+  copy.allocPixels(SkImageInfo::Make(bitmap.width(), bitmap.height(),
+                                     kRGBA_8888_SkColorType,
+                                     kPremul_SkAlphaType));
+  SkPixmap pixmap;
+  bool success = bitmap.peekPixels(&pixmap) && copy.writePixels(pixmap, 0, 0);
+  CHECK(success);
 
-  v8::MaybeLocal<v8::Object> buffer =
+  callback_.Run(gfx::Image::CreateFrom1xBitmap(copy), damage);
-      node::Buffer::Copy(isolate_, source, rgb_row_size * bitmap.height());
-  auto local_buffer = buffer.ToLocalChecked();
-
-  v8::Local<v8::Value> damage_rect =
-      mate::Converter<gfx::Rect>::ToV8(isolate_, damage);
-
-  callback_.Run(local_buffer, damage_rect);
 }
 
 }  // namespace api

atom/browser/api/frame_subscriber.h

@@ -14,6 +14,10 @@
 #include "content/public/browser/web_contents_observer.h"
 #include "v8/include/v8.h"
 
+namespace gfx {
+class Image;
+}
+
 namespace atom {
 
 namespace api {
@@ -24,10 +28,9 @@ class FrameSubscriber : public content::WebContentsObserver,
                         public viz::mojom::FrameSinkVideoConsumer {
  public:
   using FrameCaptureCallback =
-      base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>;
+      base::Callback<void(const gfx::Image&, const gfx::Rect&)>;
 
-  FrameSubscriber(v8::Isolate* isolate,
+  FrameSubscriber(content::WebContents* web_contents,
-                  content::WebContents* web_contents,
                   const FrameCaptureCallback& callback,
                   bool only_dirty);
   ~FrameSubscriber() override;
@@ -51,7 +54,6 @@ class FrameSubscriber : public content::WebContentsObserver,
 
   void Done(const gfx::Rect& damage, const SkBitmap& frame);
 
-  v8::Isolate* isolate_;
   FrameCaptureCallback callback_;
   bool only_dirty_;
 

spec/api-browser-window-spec.js

@@ -2533,13 +2533,6 @@ describe('BrowserWindow module', () => {
   })
 
   describe('beginFrameSubscription method', () => {
-    before(function () {
-      // FIXME These specs crash on Linux when run in a docker container
-      if (isCI && process.platform === 'linux') {
-        this.skip()
-      }
-    })
-
     it('does not crash when callback returns nothing', (done) => {
       w.loadFile(path.join(fixtures, 'api', 'frame-subscriber.html'))
       w.webContents.on('dom-ready', () => {
@@ -2569,13 +2562,14 @@ describe('BrowserWindow module', () => {
         })
       })
     })
+
     it('subscribes to frame updates (only dirty rectangle)', (done) => {
       let called = false
       let gotInitialFullSizeFrame = false
       const [contentWidth, contentHeight] = w.getContentSize()
       w.webContents.on('did-finish-load', () => {
-        w.webContents.beginFrameSubscription(true, (data, rect) => {
+        w.webContents.beginFrameSubscription(true, (image, rect) => {
-          if (data.length === 0) {
+          if (image.isEmpty()) {
             // Chromium sometimes sends a 0x0 frame at the beginning of the
             // page load.
             return
@@ -2596,13 +2590,14 @@ describe('BrowserWindow module', () => {
           // assert(rect.width < contentWidth || rect.height < contentHeight)
           called = true
 
-          expect(data.length).to.equal(rect.width * rect.height * 4)
+          expect(image.getBitmap().length).to.equal(rect.width * rect.height * 4)
           w.webContents.endFrameSubscription()
           done()
         })
       })
       w.loadFile(path.join(fixtures, 'api', 'frame-subscriber.html'))
     })
+
     it('throws error when subscriber is not well defined', (done) => {
       w.loadFile(path.join(fixtures, 'api', 'frame-subscriber.html'))
       try {