fix: reimplement FrameSubscriber with mojo, re-enable tests

BUILD.gn

@@ -247,6 +247,7 @@ static_library("electron_lib") {
     "//components/network_session_configurator/common",
     "//components/prefs",
     "//components/spellcheck/renderer",
+    "//components/viz/host",
     "//components/viz/service",
     "//content/public/app:both",
     "//content/public/browser",
@@ -254,6 +255,7 @@ static_library("electron_lib") {
     "//content/public/common:service_names",
     "//content/shell:copy_shell_resources",
     "//gin",
+    "//media/capture/mojom:video_capture",
     "//media/mojo/interfaces",
     "//net:extras",
     "//net:net_resources",
@@ -263,6 +265,7 @@ static_library("electron_lib") {
     "//ppapi/shared_impl",
     "//services/device/public/mojom",
     "//services/proxy_resolver:lib",
+    "//services/viz/privileged/interfaces/compositing",
     "//skia",
     "//third_party/blink/public:blink",
     "//third_party/boringssl",

atom/browser/api/frame_subscriber.cc

@@ -4,13 +4,12 @@
 
 #include "atom/browser/api/frame_subscriber.h"
 
+#include <utility>
+
 #include "atom/common/native_mate_converters/gfx_converter.h"
-#include "components/viz/common/frame_sinks/copy_output_request.h"
-#include "components/viz/service/frame_sinks/frame_sink_manager_impl.h"
-#include "components/viz/service/surfaces/surface_manager.h"
-#include "content/browser/compositor/surface_utils.h"
-#include "content/browser/renderer_host/render_widget_host_view_base.h"
-#include "ui/gfx/geometry/rect_conversions.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 "ui/gfx/skbitmap_operations.h"
 
 #include "atom/common/node_includes.h"
@@ -19,6 +18,8 @@ namespace atom {
 
 namespace api {
 
+constexpr static int kMaxFrameRate = 30;
+
 FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
                                  content::WebContents* web_contents,
                                  const FrameCaptureCallback& callback,
@@ -27,48 +28,122 @@ FrameSubscriber::FrameSubscriber(v8::Isolate* isolate,
       isolate_(isolate),
       callback_(callback),
       only_dirty_(only_dirty),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  content::RenderViewHost* rvh = web_contents->GetRenderViewHost();
+  if (rvh)
+    AttachToHost(rvh->GetWidget());
+}
 
 FrameSubscriber::~FrameSubscriber() = default;
 
-gfx::Rect FrameSubscriber::GetDamageRect() {
-  auto* view = web_contents()->GetRenderWidgetHostView();
-  if (view == nullptr)
-    return gfx::Rect();
-
-  auto* view_base = static_cast<content::RenderWidgetHostViewBase*>(view);
-  viz::SurfaceManager* surface_manager =
-      content::GetFrameSinkManager()->surface_manager();
-  viz::Surface* surface =
-      surface_manager->GetSurfaceForId(view_base->GetCurrentSurfaceId());
-  if (surface == nullptr)
-    return gfx::Rect();
-
-  if (surface->HasActiveFrame()) {
-    const viz::CompositorFrame& frame = surface->GetActiveFrame();
-    viz::RenderPass* root_pass = frame.render_pass_list.back().get();
-    gfx::Size frame_size = root_pass->output_rect.size();
-    gfx::Rect damage_rect =
-        gfx::ToEnclosingRect(gfx::RectF(root_pass->damage_rect));
-    damage_rect.Intersect(gfx::Rect(frame_size));
-    return gfx::ScaleToEnclosedRect(damage_rect,
-                                    1.0f / frame.device_scale_factor());
-  } else {
-    return gfx::Rect();
+void FrameSubscriber::AttachToHost(content::RenderWidgetHost* host) {
+  host_ = host;
+
+  // The view can be null if the renderer process has crashed.
+  // (https://crbug.com/847363)
+  if (!host_->GetView())
+    return;
+
+  // Create and configure the video capturer.
+  video_capturer_ = host_->GetView()->CreateVideoCapturer();
+  video_capturer_->SetResolutionConstraints(
+      host_->GetView()->GetViewBounds().size(),
+      host_->GetView()->GetViewBounds().size(), true);
+  video_capturer_->SetAutoThrottlingEnabled(false);
+  video_capturer_->SetMinSizeChangePeriod(base::TimeDelta());
+  video_capturer_->SetFormat(media::PIXEL_FORMAT_ARGB,
+                             media::COLOR_SPACE_UNSPECIFIED);
+  video_capturer_->SetMinCapturePeriod(base::TimeDelta::FromSeconds(1) /
+                                       kMaxFrameRate);
+  video_capturer_->Start(this);
+}
+
+void FrameSubscriber::DetachFromHost() {
+  if (!host_)
+    return;
+  video_capturer_.reset();
+  host_ = nullptr;
+}
+
+void FrameSubscriber::RenderViewCreated(content::RenderViewHost* host) {
+  if (!host_)
+    AttachToHost(host->GetWidget());
+}
+
+void FrameSubscriber::RenderViewDeleted(content::RenderViewHost* host) {
+  if (host->GetWidget() == host_) {
+    DetachFromHost();
+  }
+}
+
+void FrameSubscriber::RenderViewHostChanged(content::RenderViewHost* old_host,
+                                            content::RenderViewHost* new_host) {
+  if ((old_host && old_host->GetWidget() == host_) || (!old_host && !host_)) {
+    DetachFromHost();
+    AttachToHost(new_host->GetWidget());
+  }
+}
+
+void FrameSubscriber::OnFrameCaptured(
+    base::ReadOnlySharedMemoryRegion data,
+    ::media::mojom::VideoFrameInfoPtr info,
+    const gfx::Rect& update_rect,
+    const gfx::Rect& content_rect,
+    viz::mojom::FrameSinkVideoConsumerFrameCallbacksPtr callbacks) {
+  gfx::Size view_size = host_->GetView()->GetViewBounds().size();
+  if (view_size != content_rect.size()) {
+    video_capturer_->SetResolutionConstraints(view_size, view_size, true);
+    video_capturer_->RequestRefreshFrame();
+    return;
+  }
+
+  if (!data.IsValid()) {
+    callbacks->Done();
+    return;
+  }
+  base::ReadOnlySharedMemoryMapping mapping = data.Map();
+  if (!mapping.IsValid()) {
+    DLOG(ERROR) << "Shared memory mapping failed.";
+    return;
   }
+  if (mapping.size() <
+      media::VideoFrame::AllocationSize(info->pixel_format, info->coded_size)) {
+    DLOG(ERROR) << "Shared memory size was less than expected.";
+    return;
+  }
+
+  // The SkBitmap's pixels will be marked as immutable, but the installPixels()
+  // API requires a non-const pointer. So, cast away the const.
+  void* const pixels = const_cast<void*>(mapping.memory());
+
+  // Call installPixels() with a |releaseProc| that: 1) notifies the capturer
+  // that this consumer has finished with the frame, and 2) releases the shared
+  // memory mapping.
+  struct FramePinner {
+    // Keeps the shared memory that backs |frame_| mapped.
+    base::ReadOnlySharedMemoryMapping mapping;
+    // Prevents FrameSinkVideoCapturer from recycling the shared memory that
+    // backs |frame_|.
+    viz::mojom::FrameSinkVideoConsumerFrameCallbacksPtr releaser;
+  };
+
+  SkBitmap bitmap;
+  bitmap.installPixels(
+      SkImageInfo::MakeN32(content_rect.width(), content_rect.height(),
+                           kPremul_SkAlphaType),
+      pixels,
+      media::VideoFrame::RowBytes(media::VideoFrame::kARGBPlane,
+                                  info->pixel_format, info->coded_size.width()),
+      [](void* addr, void* context) {
+        delete static_cast<FramePinner*>(context);
+      },
+      new FramePinner{std::move(mapping), std::move(callbacks)});
+  bitmap.setImmutable();
+
+  Done(content_rect, bitmap);
 }
 
-// FIXME(MarshallOfSound): Removed in C70
-// void FrameSubscriber::DidReceiveCompositorFrame() {
-//   auto* view = web_contents()->GetRenderWidgetHostView();
-//   if (view == nullptr)
-//     return;
-
-//   view->CopyFromSurface(
-//       gfx::Rect(), view->GetViewBounds().size(),
-//       base::BindOnce(&FrameSubscriber::Done, weak_ptr_factory_.GetWeakPtr(),
-//                      GetDamageRect()));
-// }
+void FrameSubscriber::OnStopped() {}
 
 void FrameSubscriber::Done(const gfx::Rect& damage, const SkBitmap& frame) {
   if (frame.drawsNothing())

atom/browser/api/frame_subscriber.h

@@ -5,13 +5,13 @@
 #ifndef ATOM_BROWSER_API_FRAME_SUBSCRIBER_H_
 #define ATOM_BROWSER_API_FRAME_SUBSCRIBER_H_
 
-#include "content/public/browser/web_contents.h"
+#include <memory>
 
 #include "base/callback.h"
 #include "base/memory/weak_ptr.h"
-#include "components/viz/common/frame_sinks/copy_output_result.h"
+#include "components/viz/host/client_frame_sink_video_capturer.h"
+#include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
-#include "ui/gfx/image/image.h"
 #include "v8/include/v8.h"
 
 namespace atom {
@@ -20,7 +20,8 @@ namespace api {
 
 class WebContents;
 
-class FrameSubscriber : public content::WebContentsObserver {
+class FrameSubscriber : public content::WebContentsObserver,
+                        public viz::mojom::FrameSinkVideoConsumer {
  public:
   using FrameCaptureCallback =
       base::Callback<void(v8::Local<v8::Value>, v8::Local<v8::Value>)>;
@@ -32,15 +33,32 @@ class FrameSubscriber : public content::WebContentsObserver {
   ~FrameSubscriber() override;
 
  private:
-  gfx::Rect GetDamageRect();
-  // FIXME(MarshallOfSound): Removed in C70
-  //   void DidReceiveCompositorFrame() override;
+  void AttachToHost(content::RenderWidgetHost* host);
+  void DetachFromHost();
+
+  void RenderViewCreated(content::RenderViewHost* host) override;
+  void RenderViewDeleted(content::RenderViewHost* host) override;
+  void RenderViewHostChanged(content::RenderViewHost* old_host,
+                             content::RenderViewHost* new_host) override;
+
+  // viz::mojom::FrameSinkVideoConsumer implementation.
+  void OnFrameCaptured(
+      base::ReadOnlySharedMemoryRegion data,
+      ::media::mojom::VideoFrameInfoPtr info,
+      const gfx::Rect& update_rect,
+      const gfx::Rect& content_rect,
+      viz::mojom::FrameSinkVideoConsumerFrameCallbacksPtr callbacks) override;
+  void OnStopped() override;
+
   void Done(const gfx::Rect& damage, const SkBitmap& frame);
 
   v8::Isolate* isolate_;
   FrameCaptureCallback callback_;
   bool only_dirty_;
 
+  content::RenderWidgetHost* host_;
+  std::unique_ptr<viz::ClientFrameSinkVideoCapturer> video_capturer_;
+
   base::WeakPtrFactory<FrameSubscriber> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(FrameSubscriber);

spec/api-browser-window-spec.js

@@ -2368,8 +2368,7 @@ describe('BrowserWindow module', () => {
     })
   })
 
-  // FIXME: Disabled with C70.
-  xdescribe('beginFrameSubscription method', () => {
+  describe('beginFrameSubscription method', () => {
     before(function () {
       // This test is too slow, only test it on CI.
       if (!isCI) {