OSR SendExternalBeginFrame does not call OnPaint as expected

magreenblatt commented 5 years ago

Original report by me.

What steps will reproduce the problem?

Create a minimal OSR app that:

Sets CefSettings.windowless_rendering_enabled = true.
Creates a browser with CefWindowInfo.SetAsWindowless and CefWindowInfo.external_begin_frame_enabled = true.
Returns a fixed size from CefRenderHandler::GetViewRect and GetScreenInfo.
Loads a static HTML file with LoadURL.
Calls WasResized and SendExternalBeginFrame.

What is the expected output? What do you see instead?

The expected image should be delivered to the OnPaint callback. Instead, any of the following may occur:

The OnPaint callback is never executed.
The OnPaint callback receives a completely black image of the correct size.

What version of the product are you using? On what operating system?

The problem reproduces with CEF v78 and v79 on Windows 10 and Linux. It can occur both with GPU enabled and disabled.

The problem does not reproduce with v73 (version that added begin-frame support prior to the Viz refactoring).

‌

magreenblatt commented 5 years ago

I’m not sure how this is supposed to work. SendExternalBeginFrame results in a call to ExternalBeginFrameSourceMojo::IssueExternalBeginFrame in the GPU process which results in a call to AsyncLayerTreeFrameSink::OnBeginFrame in the renderer process. That call does nothing because cc::Scheduler doesn’t implement OnBeginFrame.

The call to SetNeedsOneBeginFrame from ExternalBeginFrameSourceMojo::IssueExternalBeginFrame does result in a call to CefRenderWidgetHostViewOSR::OnDisplayDidFinishFrame as the code comments suggest, but that method implementation is currently empty in CEF.

magreenblatt commented 5 years ago

Here’s the proposed unit test for this functionality against current master. Once this is working I would expect OnPaint to be called and the existing IsBackgroundInBuffer check to pass, but that may need further adjustment.

diff --git tests/ceftests/os_rendering_unittest.cc tests/ceftests/os_rendering_unittest.cc
index [cae19438 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/cae19438)..794d4b33 [100644 (bb)](https://bitbucket.org/chromiumembedded/cef/commits/100644)
--- tests/ceftests/os_rendering_unittest.cc
+++ tests/ceftests/os_rendering_unittest.cc
@@ -170,6 +170,8 @@ enum OSRTestType {
   OSR_TEST_TOUCH_CANCEL,
   // CEF_POINTER_TYPE_PEN is mapped to pen pointer event
   OSR_TEST_PEN,
+  // Test external begin frame.
+  OSR_TEST_EXTERNAL_BEGIN_FRAME,
   // Define the range for popup tests.
   OSR_TEST_POPUP_FIRST = OSR_TEST_POPUP_PAINT,
   OSR_TEST_POPUP_LAST = OSR_TEST_POPUP_SCROLL_INSIDE,
@@ -208,8 +210,15 @@ class OSRTestHandler : public RoutingTestHandler,
   void OnLoadEnd(CefRefPtr<CefBrowser> browser,
                  CefRefPtr<CefFrame> frame,
                  int httpStatusCode) override {
-    if (!started())
+    if (!started()) {
+      if (test_type_ == OSR_TEST_EXTERNAL_BEGIN_FRAME) {
+        // This should trigger a call to OnPaint().
+        browser->GetHost()->WasResized();
+        browser->GetHost()->SendExternalBeginFrame();
+      }
+
       return;
+    }

     switch (test_type_) {
       case OSR_TEST_KEY_EVENTS: {
@@ -526,6 +535,7 @@ class OSRTestHandler : public RoutingTestHandler,
     // Send events after the first full repaint
     switch (test_type_) {
       case OSR_TEST_PAINT:
+      case OSR_TEST_EXTERNAL_BEGIN_FRAME:
         if (StartTest()) {
           // test that we have a full repaint
           EXPECT_EQ(dirtyRects.size(), 1U);
@@ -1324,6 +1334,10 @@ class OSRTestHandler : public RoutingTestHandler,
       settings.background_color = CefColorSetARGB(255, 255, 255, 255);
     }

+    if (test_type_ == OSR_TEST_EXTERNAL_BEGIN_FRAME) {
+      windowInfo.external_begin_frame_enabled = true;
+    }
+
 #if defined(OS_WIN)
     windowInfo.SetAsWindowless(GetDesktopWindow());
 #elif defined(OS_MACOSX)
@@ -1603,3 +1617,5 @@ OSR_TEST(TouchEnd2X, OSR_TEST_TOUCH_END, 2.0f)
 OSR_TEST(TouchCancel, OSR_TEST_TOUCH_CANCEL, 1.0f)
 OSR_TEST(TouchCancel2X, OSR_TEST_TOUCH_CANCEL, 2.0f)
 OSR_TEST(PenEvent, OSR_TEST_PEN, 1.0f)
+OSR_TEST(ExternalBeginFrame, OSR_TEST_EXTERNAL_BEGIN_FRAME, 1.0f)
+OSR_TEST(ExternalBeginFrame2X, OSR_TEST_EXTERNAL_BEGIN_FRAME, 2.0f)

‌

magreenblatt commented 5 years ago

@{557058:f07fe0c3-2eef-4563-993e-dcdb7d76e546} Any suggestions on what might be happening here? Thanks!

magreenblatt commented 5 years ago

Testing with cefclient.exe --off-screen-rendering-enabled --external-begin-frame-enabled there appear to be many SendExternalBeginFrame() calls before the first resulting call to OnPaint(). The callback to CefLayeredWindowUpdaterOSR::Draw arrives via this callstack in the GPU process:

>   service.dll!viz::mojom::LayeredWindowUpdaterProxy::Draw(const gfx::Rect & in_damage_rect, base::OnceCallback<void ()> callback) Line 123    C++
    service.dll!viz::SoftwareOutputDeviceProxy::EndPaint() Line 127 C++
    service.dll!viz::SoftwareRenderer::FinishDrawingFrame() Line 96 C++
    service.dll!viz::DirectRenderer::DrawFrame(std::__1::vector<std::__1::unique_ptr<viz::RenderPass,std::__1::default_delete<viz::RenderPass>>,std::__1::allocator<std::__1::unique_ptr<viz::RenderPass,std::__1::default_delete<viz::RenderPass>>>> * render_passes_in_draw_order, float device_scale_factor, const gfx::Size & device_viewport_size, float sdr_white_level) Line 456 C++
    service.dll!viz::Display::DrawAndSwap() Line 535    C++
    service.dll!viz::DisplayScheduler::DrawAndSwap() Line 215   C++
    service.dll!viz::DisplayScheduler::AttemptDrawAndSwap() Line 487    C++
    service.dll!viz::DisplayScheduler::OnBeginFrameDeadline() Line 503  C++

The OnBeginFrameDeadline call is triggered via this callstack in the GPU process:

>   service.dll!viz::DisplayScheduler::ScheduleBeginFrameDeadline() Line 440    C++
    service.dll!viz::DisplayScheduler::OnBeginFrameDerivedImpl(const viz::BeginFrameArgs & args) Line 266   C++
    viz_common.dll!viz::BeginFrameObserverBase::OnBeginFrame(const viz::BeginFrameArgs & args) Line 92  C++
    viz_common.dll!viz::`anonymous namespace'::FilterAndIssueBeginFrame(viz::BeginFrameObserver * observer, const viz::BeginFrameArgs & args) Line 46   C++
    viz_common.dll!viz::ExternalBeginFrameSource::OnBeginFrame(const viz::BeginFrameArgs & args) Line 464   C++
    service.dll!viz::ExternalBeginFrameSourceMojo::IssueExternalBeginFrame(const viz::BeginFrameArgs & args) Line 23    C++

Which originates from the SendExternalBeginFrame call in the test:

>   content.dll!viz::mojom::ExternalBeginFrameControllerProxy::IssueExternalBeginFrame(const viz::BeginFrameArgs & in_args) Line 59 C++
    content.dll!ui::HostContextFactoryPrivate::IssueExternalBeginFrame(ui::Compositor * compositor, const viz::BeginFrameArgs & args) Line 275  C++
    libcef.dll!CefRenderWidgetHostViewOSR::SendExternalBeginFrame() Line 939    C++
    libcef.dll!CefBrowserPlatformDelegateOsr::SendExternalBeginFrame() Line 236 C++
    libcef.dll!CefBrowserHostImpl::SendExternalBeginFrame() Line 1154   C++

For the unit test DisplayScheduler::OnBeginFrameDeadline is called but not LayeredWindowUpdaterProxy::Draw.

magreenblatt commented 5 years ago

Looks like the problem for the unit test is that DisplayScheduler::ShouldDraw is returning false when called fromDisplayScheduler::AttemptDrawAndSwap due to root_frame_missing_ being true. That variable is set to false a bit later via this GPU process call stack:

>   service.dll!viz::DisplayScheduler::SetRootFrameMissing(bool missing) Line 73    C++
    service.dll!viz::Display::UpdateRootFrameMissing() Line 403 C++
    service.dll!viz::Display::SetLocalSurfaceId(const viz::LocalSurfaceId & id, float device_scale_factor) Line 240 C++
    service.dll!viz::RootCompositorFrameSinkImpl::SubmitCompositorFrame(const viz::LocalSurfaceId & local_surface_id, viz::CompositorFrame frame, base::Optional<viz::HitTestRegionList> hit_test_region_list, unsigned __int64 submit_time) Line 251   C++

Which originates from this call stack in the main process:

>   cc_mojo_embedder.dll!viz::mojom::CompositorFrameSinkProxy::SubmitCompositorFrame(const viz::LocalSurfaceId & in_local_surface_id, viz::CompositorFrame in_frame, base::Optional<viz::HitTestRegionList> in_hit_test_region_list, unsigned __int64 in_submit_time) Line 183  C++
    cc_mojo_embedder.dll!cc::mojo_embedder::AsyncLayerTreeFrameSink::SubmitCompositorFrame(viz::CompositorFrame frame, bool hit_test_data_changed, bool show_hit_test_borders) Line 252 C++
    cc.dll!cc::LayerTreeHostImpl::DrawLayers(cc::LayerTreeHostImpl::FrameData * frame) Line 2239    C++
    cc.dll!cc::SingleThreadProxy::DoComposite(cc::LayerTreeHostImpl::FrameData * frame) Line 687    C++
    cc.dll!cc::SingleThreadProxy::ScheduledActionDrawIfPossible() Line 904  C++
    cc.dll!cc::Scheduler::DrawIfPossible() Line 729 C++
    cc.dll!cc::Scheduler::ProcessScheduledActions() Line 832    C++
    cc.dll!cc::Scheduler::OnBeginImplFrameDeadline() Line 717   C++

‌

magreenblatt commented 5 years ago

I wonder if there’s some callback that we need to wait for (to guarantee that SubmitCompositorFrame has finished its setup) before calling SendExternalBeginFrame.

‌