From 9e1e268f1b0ce7e397865b1df627f9ea462c9b34 Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Tue, 18 Aug 2020 16:41:07 -0700 Subject: [PATCH 1/3] [embedder] Add FBO callback that takes frame info See EC-1 in flutter.dev/go/double-buffered-window-resize --- shell/common/shell_test_platform_view_gl.cc | 2 +- shell/common/shell_test_platform_view_gl.h | 2 +- shell/gpu/gpu_surface_gl.cc | 23 ++++++---- shell/gpu/gpu_surface_gl_delegate.h | 11 +++-- shell/platform/android/android_surface_gl.cc | 2 +- shell/platform/android/android_surface_gl.h | 2 +- .../android/surface/android_surface_mock.cc | 2 +- .../android/surface/android_surface_mock.h | 2 +- shell/platform/darwin/ios/ios_surface_gl.h | 2 +- shell/platform/darwin/ios/ios_surface_gl.mm | 2 +- shell/platform/embedder/embedder.cc | 29 ++++++++++-- shell/platform/embedder/embedder.h | 21 +++++++++ .../platform/embedder/embedder_surface_gl.cc | 4 +- shell/platform/embedder/embedder_surface_gl.h | 4 +- .../embedder/tests/embedder_config_builder.cc | 8 +++- .../embedder/tests/embedder_test_context.cc | 7 ++- .../embedder/tests/embedder_test_context.h | 5 ++- .../embedder/tests/embedder_unittests.cc | 44 +++++++++++++++++++ 18 files changed, 141 insertions(+), 31 deletions(-) diff --git a/shell/common/shell_test_platform_view_gl.cc b/shell/common/shell_test_platform_view_gl.cc index 0b7b1f588c71d..2adcbe6b5dfda 100644 --- a/shell/common/shell_test_platform_view_gl.cc +++ b/shell/common/shell_test_platform_view_gl.cc @@ -60,7 +60,7 @@ bool ShellTestPlatformViewGL::GLContextPresent() { } // |GPUSurfaceGLDelegate| -intptr_t ShellTestPlatformViewGL::GLContextFBO() const { +intptr_t ShellTestPlatformViewGL::GLContextFBO(GLFrameInfo frame_info) const { return gl_surface_.GetFramebuffer(); } diff --git a/shell/common/shell_test_platform_view_gl.h b/shell/common/shell_test_platform_view_gl.h index e33b84fed7814..5ca969531751d 100644 --- a/shell/common/shell_test_platform_view_gl.h +++ b/shell/common/shell_test_platform_view_gl.h @@ -56,7 +56,7 @@ class ShellTestPlatformViewGL : public ShellTestPlatformView, bool GLContextPresent() override; // |GPUSurfaceGLDelegate| - intptr_t GLContextFBO() const override; + intptr_t GLContextFBO(GLFrameInfo frame_info) const override; // |GPUSurfaceGLDelegate| GLProcResolver GetGLProcResolver() const override; diff --git a/shell/gpu/gpu_surface_gl.cc b/shell/gpu/gpu_surface_gl.cc index c492a456bb812..99f1aff328f35 100644 --- a/shell/gpu/gpu_surface_gl.cc +++ b/shell/gpu/gpu_surface_gl.cc @@ -191,6 +191,9 @@ bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) { return true; } + GLFrameInfo frame_info = {.width = static_cast(size.width()), + .height = static_cast(size.height())}; + // We need to do some updates. TRACE_EVENT0("flutter", "UpdateSurfacesSize"); @@ -205,9 +208,9 @@ bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) { sk_sp onscreen_surface; onscreen_surface = - WrapOnscreenSurface(context_.get(), // GL context - size, // root surface size - delegate_->GLContextFBO() // window FBO ID + WrapOnscreenSurface(context_.get(), // GL context + size, // root surface size + delegate_->GLContextFBO(frame_info) // window FBO ID ); if (onscreen_surface == nullptr) { @@ -287,13 +290,17 @@ bool GPUSurfaceGL::PresentSurface(SkCanvas* canvas) { auto current_size = SkISize::Make(onscreen_surface_->width(), onscreen_surface_->height()); + GLFrameInfo frame_info = { + .width = static_cast(current_size.width()), + .height = static_cast(current_size.height())}; + // The FBO has changed, ask the delegate for the new FBO and do a surface // re-wrap. - auto new_onscreen_surface = - WrapOnscreenSurface(context_.get(), // GL context - current_size, // root surface size - delegate_->GLContextFBO() // window FBO ID - ); + auto new_onscreen_surface = WrapOnscreenSurface( + context_.get(), // GL context + current_size, // root surface size + delegate_->GLContextFBO(frame_info) // window FBO ID + ); if (!new_onscreen_surface) { return false; diff --git a/shell/gpu/gpu_surface_gl_delegate.h b/shell/gpu/gpu_surface_gl_delegate.h index 9fb9765ac9be2..f8d9cd69304a9 100644 --- a/shell/gpu/gpu_surface_gl_delegate.h +++ b/shell/gpu/gpu_surface_gl_delegate.h @@ -14,6 +14,11 @@ namespace flutter { +struct GLFrameInfo { + uint32_t width; + uint32_t height; +}; + class GPUSurfaceGLDelegate : public GPUSurfaceDelegate { public: ~GPUSurfaceGLDelegate() override; @@ -33,13 +38,13 @@ class GPUSurfaceGLDelegate : public GPUSurfaceDelegate { virtual bool GLContextPresent() = 0; // The ID of the main window bound framebuffer. Typically FBO0. - virtual intptr_t GLContextFBO() const = 0; + virtual intptr_t GLContextFBO(GLFrameInfo frame_info) const = 0; // The rendering subsystem assumes that the ID of the main window bound // framebuffer remains constant throughout. If this assumption in incorrect, // embedders are required to return true from this method. In such cases, - // GLContextFBO() will be called again to acquire the new FBO ID for rendering - // subsequent frames. + // GLContextFBO(frame_info) will be called again to acquire the new FBO ID for + // rendering subsequent frames. virtual bool GLContextFBOResetAfterPresent() const; // Indicates whether or not the surface supports pixel readback as used in diff --git a/shell/platform/android/android_surface_gl.cc b/shell/platform/android/android_surface_gl.cc index a3356653df9a8..7f692f007634a 100644 --- a/shell/platform/android/android_surface_gl.cc +++ b/shell/platform/android/android_surface_gl.cc @@ -115,7 +115,7 @@ bool AndroidSurfaceGL::GLContextPresent() { return onscreen_surface_->SwapBuffers(); } -intptr_t AndroidSurfaceGL::GLContextFBO() const { +intptr_t AndroidSurfaceGL::GLContextFBO(GLFrameInfo frame_info) const { FML_DCHECK(IsValid()); // The default window bound framebuffer on Android. return 0; diff --git a/shell/platform/android/android_surface_gl.h b/shell/platform/android/android_surface_gl.h index 281a273351908..1ebbf8f60b1a5 100644 --- a/shell/platform/android/android_surface_gl.h +++ b/shell/platform/android/android_surface_gl.h @@ -59,7 +59,7 @@ class AndroidSurfaceGL final : public GPUSurfaceGLDelegate, bool GLContextPresent() override; // |GPUSurfaceGLDelegate| - intptr_t GLContextFBO() const override; + intptr_t GLContextFBO(GLFrameInfo frame_info) const override; // |GPUSurfaceGLDelegate| ExternalViewEmbedder* GetExternalViewEmbedder() override; diff --git a/shell/platform/android/surface/android_surface_mock.cc b/shell/platform/android/surface/android_surface_mock.cc index 5b0e7adad8cdd..f7098f7c56b74 100644 --- a/shell/platform/android/surface/android_surface_mock.cc +++ b/shell/platform/android/surface/android_surface_mock.cc @@ -18,7 +18,7 @@ bool AndroidSurfaceMock::GLContextPresent() { return true; } -intptr_t AndroidSurfaceMock::GLContextFBO() const { +intptr_t AndroidSurfaceMock::GLContextFBO(GLFrameInfo frame_info) const { return 0; } diff --git a/shell/platform/android/surface/android_surface_mock.h b/shell/platform/android/surface/android_surface_mock.h index 688681e01c8c0..7d5fddbf3d922 100644 --- a/shell/platform/android/surface/android_surface_mock.h +++ b/shell/platform/android/surface/android_surface_mock.h @@ -48,7 +48,7 @@ class AndroidSurfaceMock final : public GPUSurfaceGLDelegate, bool GLContextPresent() override; // |GPUSurfaceGLDelegate| - intptr_t GLContextFBO() const override; + intptr_t GLContextFBO(GLFrameInfo frame_info) const override; // |GPUSurfaceGLDelegate| ExternalViewEmbedder* GetExternalViewEmbedder() override; diff --git a/shell/platform/darwin/ios/ios_surface_gl.h b/shell/platform/darwin/ios/ios_surface_gl.h index e6433eb83ef56..745b5d0070dc4 100644 --- a/shell/platform/darwin/ios/ios_surface_gl.h +++ b/shell/platform/darwin/ios/ios_surface_gl.h @@ -43,7 +43,7 @@ class IOSSurfaceGL final : public IOSSurface, public GPUSurfaceGLDelegate { bool GLContextPresent() override; // |GPUSurfaceGLDelegate| - intptr_t GLContextFBO() const override; + intptr_t GLContextFBO(GLFrameInfo frame_info) const override; // |GPUSurfaceGLDelegate| bool SurfaceSupportsReadback() const override; diff --git a/shell/platform/darwin/ios/ios_surface_gl.mm b/shell/platform/darwin/ios/ios_surface_gl.mm index 3531f61c3c0df..9da8d9ea50088 100644 --- a/shell/platform/darwin/ios/ios_surface_gl.mm +++ b/shell/platform/darwin/ios/ios_surface_gl.mm @@ -44,7 +44,7 @@ } // |GPUSurfaceGLDelegate| -intptr_t IOSSurfaceGL::GLContextFBO() const { +intptr_t IOSSurfaceGL::GLContextFBO(GLFrameInfo frame_info) const { return IsValid() ? render_target_->GetFramebuffer() : GL_NONE; } diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index e260c0b1bc71f..c8186bdd92b4a 100644 --- a/shell/platform/embedder/embedder.cc +++ b/shell/platform/embedder/embedder.cc @@ -93,8 +93,19 @@ static bool IsOpenGLRendererConfigValid(const FlutterRendererConfig* config) { if (SAFE_ACCESS(open_gl_config, make_current, nullptr) == nullptr || SAFE_ACCESS(open_gl_config, clear_current, nullptr) == nullptr || - SAFE_ACCESS(open_gl_config, present, nullptr) == nullptr || - SAFE_ACCESS(open_gl_config, fbo_callback, nullptr) == nullptr) { + SAFE_ACCESS(open_gl_config, present, nullptr) == nullptr) { + return false; + } + + bool fbo_callback_exists = + SAFE_ACCESS(open_gl_config, fbo_callback, nullptr) != nullptr; + bool fbo_with_frame_info_callback_exists = + SAFE_ACCESS(open_gl_config, fbo_with_frame_info_callback, nullptr) != + nullptr; + // only one of these callbacks must exist. + if (fbo_callback_exists == fbo_with_frame_info_callback_exists) { + LOG_EMBEDDER_ERROR(kInternalInconsistency, + "OpenGL fbo callback configuration was invalid."); return false; } @@ -168,8 +179,18 @@ InferOpenGLPlatformViewCreationCallback( return ptr(user_data); }; - auto gl_fbo_callback = [ptr = config->open_gl.fbo_callback, - user_data]() -> intptr_t { return ptr(user_data); }; + auto gl_fbo_callback = + [fbo_callback = config->open_gl.fbo_callback, + fbo_with_frame_info_callback = + config->open_gl.fbo_with_frame_info_callback, + user_data](flutter::GLFrameInfo frame_info) -> intptr_t { + if (fbo_callback) { + return fbo_callback(user_data); + } else { + return fbo_with_frame_info_callback( + user_data, {.width = frame_info.width, .height = frame_info.height}); + } + }; const FlutterOpenGLRendererConfig* open_gl_config = &config->open_gl; std::function gl_make_resource_current_callback = nullptr; diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index 3a6126300f8b7..852d7c760fef5 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -306,12 +306,26 @@ typedef bool (*TextureFrameCallback)(void* /* user data */, FlutterOpenGLTexture* /* texture out */); typedef void (*VsyncCallback)(void* /* user data */, intptr_t /* baton */); +typedef struct { + /// The width of the surface that will be backed by the fbo. + uint32_t width; + /// The height of the surface that will be backed by the fbo. + uint32_t height; +} FlutterFrameInfo; + +typedef uint32_t (*FBOWithFrameInfoCallBack)(void* /* user data */, + FlutterFrameInfo /* frame info*/); + typedef struct { /// The size of this struct. Must be sizeof(FlutterOpenGLRendererConfig). size_t struct_size; BoolCallback make_current; BoolCallback clear_current; BoolCallback present; + /// This is an optional callback. Exactly one of + /// `fbo_with_frame_info_callback` or `fbo_callback` must be provided. The + /// return value indicates the id of the frame buffer object that flutter will + /// obtain the gl surface from. UIntCallback fbo_callback; /// This is an optional callback. Flutter will ask the emebdder to create a GL /// context current on a background thread. If the embedder is able to do so, @@ -342,6 +356,13 @@ typedef struct { /// that external texture details can be supplied to the engine for subsequent /// composition. TextureFrameCallback gl_external_texture_frame_callback; + /// This is an optional callback. Exactly one of + /// `fbo_with_frame_info_callback` or `fbo_callback` must provided. The + /// return value indicates the id of the frame buffer object (fbo) that + /// flutter will obtain the gl surface from. When using this variant, the + /// embedder is passed a `FlutterFrameInfo` struct that indicates the + /// properties of the surface that flutter will acquire from the returned fbo. + FBOWithFrameInfoCallBack fbo_with_frame_info_callback; } FlutterOpenGLRendererConfig; typedef struct { diff --git a/shell/platform/embedder/embedder_surface_gl.cc b/shell/platform/embedder/embedder_surface_gl.cc index 3deb7b5f3032c..4a28d68ecb295 100644 --- a/shell/platform/embedder/embedder_surface_gl.cc +++ b/shell/platform/embedder/embedder_surface_gl.cc @@ -50,8 +50,8 @@ bool EmbedderSurfaceGL::GLContextPresent() { } // |GPUSurfaceGLDelegate| -intptr_t EmbedderSurfaceGL::GLContextFBO() const { - return gl_dispatch_table_.gl_fbo_callback(); +intptr_t EmbedderSurfaceGL::GLContextFBO(GLFrameInfo frame_info) const { + return gl_dispatch_table_.gl_fbo_callback(frame_info); } // |GPUSurfaceGLDelegate| diff --git a/shell/platform/embedder/embedder_surface_gl.h b/shell/platform/embedder/embedder_surface_gl.h index 2a3a76748db17..6a4b8ed94703d 100644 --- a/shell/platform/embedder/embedder_surface_gl.h +++ b/shell/platform/embedder/embedder_surface_gl.h @@ -19,7 +19,7 @@ class EmbedderSurfaceGL final : public EmbedderSurface, std::function gl_make_current_callback; // required std::function gl_clear_current_callback; // required std::function gl_present_callback; // required - std::function gl_fbo_callback; // required + std::function gl_fbo_callback; // required std::function gl_make_resource_current_callback; // optional std::function gl_surface_transformation_callback; // optional @@ -59,7 +59,7 @@ class EmbedderSurfaceGL final : public EmbedderSurface, bool GLContextPresent() override; // |GPUSurfaceGLDelegate| - intptr_t GLContextFBO() const override; + intptr_t GLContextFBO(GLFrameInfo frame_info) const override; // |GPUSurfaceGLDelegate| bool GLContextFBOResetAfterPresent() const override; diff --git a/shell/platform/embedder/tests/embedder_config_builder.cc b/shell/platform/embedder/tests/embedder_config_builder.cc index 1c08b98fde1f9..1f6f2d85f820d 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.cc +++ b/shell/platform/embedder/tests/embedder_config_builder.cc @@ -35,8 +35,12 @@ EmbedderConfigBuilder::EmbedderConfigBuilder( opengl_renderer_config_.present = [](void* context) -> bool { return reinterpret_cast(context)->GLPresent(); }; - opengl_renderer_config_.fbo_callback = [](void* context) -> uint32_t { - return reinterpret_cast(context)->GLGetFramebuffer(); + opengl_renderer_config_.fbo_with_frame_info_callback = + [](void* context, FlutterFrameInfo frame_info) -> uint32_t { + return reinterpret_cast(context)->GLGetFramebuffer({ + .width = frame_info.width, + .height = frame_info.height, + }); }; opengl_renderer_config_.make_resource_current = [](void* context) -> bool { return reinterpret_cast(context) diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc index be7f7641cc586..048d5d9c14111 100644 --- a/shell/platform/embedder/tests/embedder_test_context.cc +++ b/shell/platform/embedder/tests/embedder_test_context.cc @@ -197,11 +197,16 @@ bool EmbedderTestContext::GLPresent() { return true; } -uint32_t EmbedderTestContext::GLGetFramebuffer() { +uint32_t EmbedderTestContext::GLGetFramebuffer(FlutterFrameInfo frame_info) { FML_CHECK(gl_surface_) << "GL surface must be initialized."; + gl_surface_fbo_frame_infos_.push_back(frame_info); return gl_surface_->GetFramebuffer(); } +std::vector EmbedderTestContext::GetGLFBOFrameInfos() { + return gl_surface_fbo_frame_infos_; +} + bool EmbedderTestContext::GLMakeResourceCurrent() { FML_CHECK(gl_surface_) << "GL surface must be initialized."; return gl_surface_->MakeResourceCurrent(); diff --git a/shell/platform/embedder/tests/embedder_test_context.h b/shell/platform/embedder/tests/embedder_test_context.h index 56a44f6b5efe6..9954f3bd3eb94 100644 --- a/shell/platform/embedder/tests/embedder_test_context.h +++ b/shell/platform/embedder/tests/embedder_test_context.h @@ -79,6 +79,8 @@ class EmbedderTestContext { size_t GetSoftwareSurfacePresentCount() const; + std::vector GetGLFBOFrameInfos(); + private: // This allows the builder to access the hooks. friend class EmbedderConfigBuilder; @@ -101,6 +103,7 @@ class EmbedderTestContext { std::unique_ptr compositor_; NextSceneCallback next_scene_callback_; SkMatrix root_surface_transformation_; + std::vector gl_surface_fbo_frame_infos_; size_t gl_surface_present_count_ = 0; size_t software_surface_present_count_ = 0; @@ -133,7 +136,7 @@ class EmbedderTestContext { bool GLPresent(); - uint32_t GLGetFramebuffer(); + uint32_t GLGetFramebuffer(FlutterFrameInfo frame_info); bool GLMakeResourceCurrent(); diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc index 558669ddb550c..c4c19cb5c4eb5 100644 --- a/shell/platform/embedder/tests/embedder_unittests.cc +++ b/shell/platform/embedder/tests/embedder_unittests.cc @@ -4352,5 +4352,49 @@ TEST_F(EmbedderTest, CanLaunchAndShutdownWithAValidElfSource) { engine.reset(); } +TEST_F(EmbedderTest, FrameInfoContainsValidWidthAndHeight) { + auto& context = GetEmbedderContext(); + + EmbedderConfigBuilder builder(context); + builder.SetOpenGLRendererConfig(SkISize::Make(600, 1024)); + builder.SetDartEntrypoint("push_frames_over_and_over"); + + const auto root_surface_transformation = + SkMatrix().preTranslate(0, 1024).preRotate(-90, 0, 0); + + context.SetRootSurfaceTransformation(root_surface_transformation); + + auto engine = builder.LaunchEngine(); + + // Send a window metrics events so frames may be scheduled. + FlutterWindowMetricsEvent event = {}; + event.struct_size = sizeof(event); + event.width = 1024; + event.height = 600; + event.pixel_ratio = 1.0; + ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), + kSuccess); + ASSERT_TRUE(engine.is_valid()); + + constexpr size_t frames_expected = 10; + fml::CountDownLatch frame_latch(frames_expected); + size_t frames_seen = 0; + context.AddNativeCallback("SignalNativeTest", + CREATE_NATIVE_ENTRY([&](Dart_NativeArguments args) { + frames_seen++; + frame_latch.CountDown(); + })); + frame_latch.Wait(); + + ASSERT_EQ(frames_expected, frames_seen); + ASSERT_EQ(context.GetGLFBOFrameInfos().size(), frames_seen); + + for (FlutterFrameInfo frame_info : context.GetGLFBOFrameInfos()) { + // width and height are rotated by 90 deg + ASSERT_EQ(frame_info.width, event.height); + ASSERT_EQ(frame_info.height, event.width); + } +} + } // namespace testing } // namespace flutter From 7c2e7c2287dc241da520bdbc9ac1511fed23c0ae Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Wed, 19 Aug 2020 13:09:41 -0700 Subject: [PATCH 2/3] Address CR feedback. --- shell/gpu/gpu_surface_gl.cc | 10 ++-- shell/gpu/gpu_surface_gl_delegate.h | 2 + shell/platform/embedder/embedder.cc | 10 ++-- shell/platform/embedder/embedder.h | 55 ++++++++++++------- .../embedder/tests/embedder_config_builder.cc | 26 +++++++-- .../embedder/tests/embedder_config_builder.h | 6 ++ .../embedder/tests/embedder_test_context.h | 1 + .../embedder/tests/embedder_unittests.cc | 15 ++++- 8 files changed, 87 insertions(+), 38 deletions(-) diff --git a/shell/gpu/gpu_surface_gl.cc b/shell/gpu/gpu_surface_gl.cc index 99f1aff328f35..5386683a27474 100644 --- a/shell/gpu/gpu_surface_gl.cc +++ b/shell/gpu/gpu_surface_gl.cc @@ -191,9 +191,6 @@ bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) { return true; } - GLFrameInfo frame_info = {.width = static_cast(size.width()), - .height = static_cast(size.height())}; - // We need to do some updates. TRACE_EVENT0("flutter", "UpdateSurfacesSize"); @@ -207,6 +204,8 @@ bool GPUSurfaceGL::CreateOrUpdateSurfaces(const SkISize& size) { sk_sp onscreen_surface; + GLFrameInfo frame_info = {static_cast(size.width()), + static_cast(size.height())}; onscreen_surface = WrapOnscreenSurface(context_.get(), // GL context size, // root surface size @@ -290,9 +289,8 @@ bool GPUSurfaceGL::PresentSurface(SkCanvas* canvas) { auto current_size = SkISize::Make(onscreen_surface_->width(), onscreen_surface_->height()); - GLFrameInfo frame_info = { - .width = static_cast(current_size.width()), - .height = static_cast(current_size.height())}; + GLFrameInfo frame_info = {static_cast(current_size.width()), + static_cast(current_size.height())}; // The FBO has changed, ask the delegate for the new FBO and do a surface // re-wrap. diff --git a/shell/gpu/gpu_surface_gl_delegate.h b/shell/gpu/gpu_surface_gl_delegate.h index f8d9cd69304a9..7fd111dd3ebe5 100644 --- a/shell/gpu/gpu_surface_gl_delegate.h +++ b/shell/gpu/gpu_surface_gl_delegate.h @@ -14,6 +14,8 @@ namespace flutter { +// A structure to represent the frame information which is passed to the +// embedder when requesting a frame buffer object. struct GLFrameInfo { uint32_t width; uint32_t height; diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index c8186bdd92b4a..b8c95ce674f34 100644 --- a/shell/platform/embedder/embedder.cc +++ b/shell/platform/embedder/embedder.cc @@ -104,8 +104,6 @@ static bool IsOpenGLRendererConfigValid(const FlutterRendererConfig* config) { nullptr; // only one of these callbacks must exist. if (fbo_callback_exists == fbo_with_frame_info_callback_exists) { - LOG_EMBEDDER_ERROR(kInternalInconsistency, - "OpenGL fbo callback configuration was invalid."); return false; } @@ -183,12 +181,14 @@ InferOpenGLPlatformViewCreationCallback( [fbo_callback = config->open_gl.fbo_callback, fbo_with_frame_info_callback = config->open_gl.fbo_with_frame_info_callback, - user_data](flutter::GLFrameInfo frame_info) -> intptr_t { + user_data](flutter::GLFrameInfo gl_frame_info) -> intptr_t { if (fbo_callback) { return fbo_callback(user_data); } else { - return fbo_with_frame_info_callback( - user_data, {.width = frame_info.width, .height = frame_info.height}); + FlutterFrameInfo frame_info; + frame_info.struct_size = sizeof(FlutterFrameInfo); + frame_info.size = {gl_frame_info.width, gl_frame_info.height}; + return fbo_with_frame_info_callback(user_data, &frame_info); } }; diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index 852d7c760fef5..7b3934628fa16 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -306,15 +306,33 @@ typedef bool (*TextureFrameCallback)(void* /* user data */, FlutterOpenGLTexture* /* texture out */); typedef void (*VsyncCallback)(void* /* user data */, intptr_t /* baton */); +/// A structure to represent the width and height. +typedef struct { + double width; + double height; +} FlutterSize; + +/// A structure to represent the width and height. +/// +/// See: \ref FlutterSize when the value are not integers. typedef struct { - /// The width of the surface that will be backed by the fbo. uint32_t width; - /// The height of the surface that will be backed by the fbo. uint32_t height; +} FlutterUIntSize; + +/// This information is passed to the embedder when requesting a frame buffer +/// object. +/// +/// See: \ref FlutterSoftwareRendererConfig.fbo_with_frame_info_callback. +typedef struct { + /// The size of this struct. Must be sizeof(FlutterFrameInfo). + size_t struct_size; + /// The size of the surface that will be backed by the fbo. + FlutterUIntSize size; } FlutterFrameInfo; -typedef uint32_t (*FBOWithFrameInfoCallBack)(void* /* user data */, - FlutterFrameInfo /* frame info*/); +typedef uint32_t (*UIntFrameInfoCallback)(void* /* user data */, + FlutterFrameInfo* /* frame info */); typedef struct { /// The size of this struct. Must be sizeof(FlutterOpenGLRendererConfig). @@ -322,10 +340,11 @@ typedef struct { BoolCallback make_current; BoolCallback clear_current; BoolCallback present; - /// This is an optional callback. Exactly one of - /// `fbo_with_frame_info_callback` or `fbo_callback` must be provided. The - /// return value indicates the id of the frame buffer object that flutter will - /// obtain the gl surface from. + /// Specifying one (and only one) of the `fbo_callback` or + /// `fbo_with_frame_info_callback` is required. Specifying both is an error + /// and engine intialization will be terminated. The return value indicates + /// the id of the frame buffer object that flutter will obtain the gl surface + /// from. UIntCallback fbo_callback; /// This is an optional callback. Flutter will ask the emebdder to create a GL /// context current on a background thread. If the embedder is able to do so, @@ -356,13 +375,14 @@ typedef struct { /// that external texture details can be supplied to the engine for subsequent /// composition. TextureFrameCallback gl_external_texture_frame_callback; - /// This is an optional callback. Exactly one of - /// `fbo_with_frame_info_callback` or `fbo_callback` must provided. The - /// return value indicates the id of the frame buffer object (fbo) that - /// flutter will obtain the gl surface from. When using this variant, the - /// embedder is passed a `FlutterFrameInfo` struct that indicates the - /// properties of the surface that flutter will acquire from the returned fbo. - FBOWithFrameInfoCallBack fbo_with_frame_info_callback; + /// Specifying one (and only one) of the `fbo_callback` or + /// `fbo_with_frame_info_callback` is required. Specifying both is an error + /// and engine intialization will be terminated. The return value indicates + /// the id of the frame buffer object (fbo) that flutter will obtain the gl + /// surface from. When using this variant, the embedder is passed a + /// `FlutterFrameInfo` struct that indicates the properties of the surface + /// that flutter will acquire from the returned fbo. + UIntFrameInfoCallback fbo_with_frame_info_callback; } FlutterOpenGLRendererConfig; typedef struct { @@ -523,11 +543,6 @@ typedef struct { double y; } FlutterPoint; -typedef struct { - double width; - double height; -} FlutterSize; - typedef struct { FlutterRect rect; FlutterSize upper_left_corner_radius; diff --git a/shell/platform/embedder/tests/embedder_config_builder.cc b/shell/platform/embedder/tests/embedder_config_builder.cc index 1f6f2d85f820d..05fde3c9abe60 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.cc +++ b/shell/platform/embedder/tests/embedder_config_builder.cc @@ -36,11 +36,12 @@ EmbedderConfigBuilder::EmbedderConfigBuilder( return reinterpret_cast(context)->GLPresent(); }; opengl_renderer_config_.fbo_with_frame_info_callback = - [](void* context, FlutterFrameInfo frame_info) -> uint32_t { - return reinterpret_cast(context)->GLGetFramebuffer({ - .width = frame_info.width, - .height = frame_info.height, - }); + [](void* context, FlutterFrameInfo* frame_info) -> uint32_t { + FlutterFrameInfo tmp; + tmp.struct_size = sizeof(FlutterFrameInfo); + tmp.size = frame_info->size; + return reinterpret_cast(context)->GLGetFramebuffer( + tmp); }; opengl_renderer_config_.make_resource_current = [](void* context) -> bool { return reinterpret_cast(context) @@ -114,6 +115,21 @@ void EmbedderConfigBuilder::SetSoftwareRendererConfig(SkISize surface_size) { context_.SetupOpenGLSurface(surface_size); } +void EmbedderConfigBuilder::SetOpenGLFBOCallBack() { + // SetOpenGLRendererConfig must be called before this. + FML_CHECK(renderer_config_.type == FlutterRendererType::kOpenGL); + renderer_config_.open_gl.fbo_callback = [](void* context) -> uint32_t { + FlutterFrameInfo tmp; + // fbo_callback doesn't use the frame size information, only + // fbo_callback_with_frame_info does. + tmp.struct_size = sizeof(FlutterFrameInfo); + tmp.size.width = 0; + tmp.size.height = 0; + return reinterpret_cast(context)->GLGetFramebuffer( + tmp); + }; +} + void EmbedderConfigBuilder::SetOpenGLRendererConfig(SkISize surface_size) { renderer_config_.type = FlutterRendererType::kOpenGL; renderer_config_.open_gl = opengl_renderer_config_; diff --git a/shell/platform/embedder/tests/embedder_config_builder.h b/shell/platform/embedder/tests/embedder_config_builder.h index a386cd91d0c6d..e6f0016918776 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.h +++ b/shell/platform/embedder/tests/embedder_config_builder.h @@ -49,6 +49,12 @@ class EmbedderConfigBuilder { void SetOpenGLRendererConfig(SkISize surface_size); + // Used to explicitly set an `open_gl.fbo_callback`. Using this method will + // cause your test to fail since the ctor for this class sets + // `open_gl.fbo_callback_with_frame_info`. This method exists as a utility to + // explicitly test this behavior. + void SetOpenGLFBOCallBack(); + void SetAssetsPath(); void SetSnapshots(); diff --git a/shell/platform/embedder/tests/embedder_test_context.h b/shell/platform/embedder/tests/embedder_test_context.h index 9954f3bd3eb94..f4135c07d7f7c 100644 --- a/shell/platform/embedder/tests/embedder_test_context.h +++ b/shell/platform/embedder/tests/embedder_test_context.h @@ -79,6 +79,7 @@ class EmbedderTestContext { size_t GetSoftwareSurfacePresentCount() const; + // Returns the frame information for all the frames that were rendered. std::vector GetGLFBOFrameInfos(); private: diff --git a/shell/platform/embedder/tests/embedder_unittests.cc b/shell/platform/embedder/tests/embedder_unittests.cc index c4c19cb5c4eb5..f235613698f6a 100644 --- a/shell/platform/embedder/tests/embedder_unittests.cc +++ b/shell/platform/embedder/tests/embedder_unittests.cc @@ -4391,10 +4391,21 @@ TEST_F(EmbedderTest, FrameInfoContainsValidWidthAndHeight) { for (FlutterFrameInfo frame_info : context.GetGLFBOFrameInfos()) { // width and height are rotated by 90 deg - ASSERT_EQ(frame_info.width, event.height); - ASSERT_EQ(frame_info.height, event.width); + ASSERT_EQ(frame_info.size.width, event.height); + ASSERT_EQ(frame_info.size.height, event.width); } } +TEST_F(EmbedderTest, MustNotRunWithBothFBOCallbacksSet) { + auto& context = GetEmbedderContext(); + + EmbedderConfigBuilder builder(context); + builder.SetOpenGLRendererConfig(SkISize::Make(600, 1024)); + builder.SetOpenGLFBOCallBack(); + + auto engine = builder.LaunchEngine(); + ASSERT_FALSE(engine.is_valid()); +} + } // namespace testing } // namespace flutter From 052244810d434fd4d917e53ec582d94c8749d54a Mon Sep 17 00:00:00 2001 From: Kaushik Iska Date: Wed, 19 Aug 2020 13:47:17 -0700 Subject: [PATCH 3/3] zero init structs --- shell/platform/embedder/embedder.cc | 2 +- shell/platform/embedder/embedder.h | 5 +++-- .../embedder/tests/embedder_config_builder.cc | 17 +++++++---------- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/shell/platform/embedder/embedder.cc b/shell/platform/embedder/embedder.cc index b8c95ce674f34..8a94ee5eb2dc9 100644 --- a/shell/platform/embedder/embedder.cc +++ b/shell/platform/embedder/embedder.cc @@ -185,7 +185,7 @@ InferOpenGLPlatformViewCreationCallback( if (fbo_callback) { return fbo_callback(user_data); } else { - FlutterFrameInfo frame_info; + FlutterFrameInfo frame_info = {}; frame_info.struct_size = sizeof(FlutterFrameInfo); frame_info.size = {gl_frame_info.width, gl_frame_info.height}; return fbo_with_frame_info_callback(user_data, &frame_info); diff --git a/shell/platform/embedder/embedder.h b/shell/platform/embedder/embedder.h index 7b3934628fa16..885926eb122cc 100644 --- a/shell/platform/embedder/embedder.h +++ b/shell/platform/embedder/embedder.h @@ -331,8 +331,9 @@ typedef struct { FlutterUIntSize size; } FlutterFrameInfo; -typedef uint32_t (*UIntFrameInfoCallback)(void* /* user data */, - FlutterFrameInfo* /* frame info */); +typedef uint32_t (*UIntFrameInfoCallback)( + void* /* user data */, + const FlutterFrameInfo* /* frame info */); typedef struct { /// The size of this struct. Must be sizeof(FlutterOpenGLRendererConfig). diff --git a/shell/platform/embedder/tests/embedder_config_builder.cc b/shell/platform/embedder/tests/embedder_config_builder.cc index 05fde3c9abe60..f69161e6e23cc 100644 --- a/shell/platform/embedder/tests/embedder_config_builder.cc +++ b/shell/platform/embedder/tests/embedder_config_builder.cc @@ -36,12 +36,9 @@ EmbedderConfigBuilder::EmbedderConfigBuilder( return reinterpret_cast(context)->GLPresent(); }; opengl_renderer_config_.fbo_with_frame_info_callback = - [](void* context, FlutterFrameInfo* frame_info) -> uint32_t { - FlutterFrameInfo tmp; - tmp.struct_size = sizeof(FlutterFrameInfo); - tmp.size = frame_info->size; + [](void* context, const FlutterFrameInfo* frame_info) -> uint32_t { return reinterpret_cast(context)->GLGetFramebuffer( - tmp); + *frame_info); }; opengl_renderer_config_.make_resource_current = [](void* context) -> bool { return reinterpret_cast(context) @@ -119,14 +116,14 @@ void EmbedderConfigBuilder::SetOpenGLFBOCallBack() { // SetOpenGLRendererConfig must be called before this. FML_CHECK(renderer_config_.type == FlutterRendererType::kOpenGL); renderer_config_.open_gl.fbo_callback = [](void* context) -> uint32_t { - FlutterFrameInfo tmp; + FlutterFrameInfo frame_info = {}; // fbo_callback doesn't use the frame size information, only // fbo_callback_with_frame_info does. - tmp.struct_size = sizeof(FlutterFrameInfo); - tmp.size.width = 0; - tmp.size.height = 0; + frame_info.struct_size = sizeof(FlutterFrameInfo); + frame_info.size.width = 0; + frame_info.size.height = 0; return reinterpret_cast(context)->GLGetFramebuffer( - tmp); + frame_info); }; }