[embedder] Add FBO callback that takes frame info #20617

iskakaushik · 2020-08-19T00:27:14Z

Flutter conflates the size of the frame that is rendered by the framework and the size of the render surface itself. While these are usually the same, this breaks down in cases where the render surface size is being updated constantly.

To this effect, we will be adding a variant of fbo_callback that takes a FrameInfo struct. fbo_callback_with_frame_info. FrameInfo will contain the size of the layer tree that Flutter plans on rendering to the FBO. This lets us create and return an FBO of the requested size along with any additional bookkeeping.

See EC-1 in flutter.dev/go/double-buffered-window-resize and flutter/flutter#44136

See EC-1 in flutter.dev/go/double-buffered-window-resize

stuartmorgan-g

Functionality LGTM (although @chinmaygarde should definitely review as well), but some nits.

stuartmorgan-g · 2020-08-19T18:57:56Z

shell/gpu/gpu_surface_gl.cc

  }

+  GLFrameInfo frame_info = {.width = static_cast<uint32_t>(size.width()),
+                            .height = static_cast<uint32_t>(size.height())};


Labelled aggregate initialization is non-standard until C++20; please don't use it at this point. (Applies throughout the PR.)

stuartmorgan-g · 2020-08-19T18:58:36Z

shell/gpu/gpu_surface_gl.cc

    return true;
  }

+  GLFrameInfo frame_info = {.width = static_cast<uint32_t>(size.width()),


Please declare this closer to where it's used (per style guide).

stuartmorgan-g · 2020-08-19T18:59:30Z

shell/gpu/gpu_surface_gl_delegate.h


 namespace flutter {

+struct GLFrameInfo {


Needs a declaration comment.

stuartmorgan-g · 2020-08-19T19:03:27Z

shell/platform/embedder/embedder.h

                                     FlutterOpenGLTexture* /* texture out */);
 typedef void (*VsyncCallback)(void* /* user data */, intptr_t /* baton */);

+typedef struct {


Needs a declaration comment.

stuartmorgan-g · 2020-08-19T19:03:31Z

shell/platform/embedder/embedder.h

+  uint32_t height;
+} FlutterFrameInfo;
+
+typedef uint32_t (*FBOWithFrameInfoCallBack)(void* /* user data */,


stuartmorgan-g · 2020-08-19T19:05:25Z

shell/platform/embedder/embedder.h

  BoolCallback make_current;
  BoolCallback clear_current;
  BoolCallback present;
+  /// This is an optional callback. Exactly one of


"Optional" seems somewhat misleading here; if someone skims quickly they might think this is the same as the other optional callbacks, where it truly is optional. I would just remove this first sentence and start with the "Exactly one of ..." part. (Same below.)

stuartmorgan-g · 2020-08-19T19:05:57Z

shell/platform/embedder/embedder.h

                                     FlutterOpenGLTexture* /* texture out */);
 typedef void (*VsyncCallback)(void* /* user data */, intptr_t /* baton */);

+typedef struct {


This struct should start with a size in case we want to add more information to it in the future.

stuartmorgan-g · 2020-08-19T19:07:57Z

shell/platform/embedder/tests/embedder_test_context.h


  size_t GetSoftwareSurfacePresentCount() const;

+  std::vector<FlutterFrameInfo> GetGLFBOFrameInfos();


Please add a declaration comment, since the implementation isn't inline to make it obvious what it does. (Or, alternately, comment the ivar.)

chinmaygarde

A few nits but a good foundation.

chinmaygarde · 2020-08-19T19:05:47Z

shell/platform/embedder/tests/embedder_unittests.cc

  engine.reset();
 }

+TEST_F(EmbedderTest, FrameInfoContainsValidWidthAndHeight) {


Another test here to verify that engine initialization is aborted if both the old and new callbacks are specified by the embedder.

chinmaygarde · 2020-08-19T19:08:22Z

shell/platform/embedder/embedder.h

  BoolCallback make_current;
  BoolCallback clear_current;
  BoolCallback present;
+  /// This is an optional callback. Exactly one of


Instead of saying this is optional, can we say "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.".

chinmaygarde · 2020-08-19T19:10:34Z

shell/platform/embedder/embedder.h

+  uint32_t height;
+} FlutterFrameInfo;
+
+typedef uint32_t (*FBOWithFrameInfoCallBack)(void* /* user data */,


I don't believe prepositions are used in in the embedder API. Also, the uint32_t needing to be an FBO is not really required. How about UIntFrameInfoCallback instead to keep this consistent with the UIntCallback you are upgrading?

chinmaygarde · 2020-08-19T19:12:09Z

shell/platform/embedder/embedder.h

+  uint32_t width;
+  /// The height of the surface that will be backed by the fbo.
+  uint32_t height;
+} FlutterFrameInfo;


This seems like something we can reuse later. How about FlutterUIntSize? There is already a FlutterSize but those are doubles.

Do you foresee a case where this object will be need to expanded? If so, FBOWithFrameInfoCallBack will need to take a pointer to this. It will also need to have a struct size member.

chinmaygarde · 2020-08-19T19:13:29Z

shell/platform/embedder/embedder.cc

+      nullptr;
+  // only one of these callbacks must exist.
+  if (fbo_callback_exists == fbo_with_frame_info_callback_exists) {
+    LOG_EMBEDDER_ERROR(kInternalInconsistency,


LOG_EMBEDDER_ERROR is only used right before returning control to the caller. kInternalInconsistency isn't actually going to be returned to the caller here. Just get rid of this line as the result of IsRendererValid is used to generate the error later.

chinmaygarde

Another pass. Almost there.

chinmaygarde · 2020-08-19T20:27:53Z

shell/platform/embedder/embedder.h

+} FlutterFrameInfo;
+
+typedef uint32_t (*UIntFrameInfoCallback)(void* /* user data */,
+                                          FlutterFrameInfo* /* frame info */);


const FlutterFrameInfo* since this is not an in-out parameter.

chinmaygarde · 2020-08-19T20:29:30Z

shell/platform/embedder/tests/embedder_config_builder.cc

+  // 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;


Zero initialize structs for good measure. That makes it resilient to updates to the struct. So, FlutterFrameInfo frame_info = {}; I'd also avoid vars with terse names like tmp.

Ditto about the zero initializing structs and ivar names elsewhere.

Renamed to frame_info, and zero initialized it.

chinmaygarde · 2020-08-19T20:36:07Z

shell/platform/embedder/tests/embedder_config_builder.cc

-    return reinterpret_cast<EmbedderTestContext*>(context)->GLGetFramebuffer();
+  opengl_renderer_config_.fbo_with_frame_info_callback =
+      [](void* context, FlutterFrameInfo* frame_info) -> uint32_t {
+    FlutterFrameInfo tmp;


Unclear why you prefer this form of copying of the struct instead of just return reinterpret_cast<EmbedderTestContext*>(context)->GLGetFramebuffer(*frame_info). That is more resilient to updates to the frame info struct.

copy-pasta error.

fluttergithubbot · 2020-08-19T21:51:02Z

This pull request is not suitable for automatic merging in its current state.

This pull request has changes requested by @stuartmorgan. Please resolve those before re-applying the label.

stuartmorgan-g · 2020-08-19T22:19:26Z

shell/platform/embedder/embedder.h

  double x;
  double y;
 } FlutterPoint;



Nit: maybe move FlutterRect, FlutterPoint, and FLutterRoundedRect up as well so these similar structs all remain together.

…gine#20617)

blasten · 2020-08-21T20:00:06Z

shell/platform/embedder/tests/embedder_unittests.cc

+  frame_latch.Wait();
+
+  ASSERT_EQ(frames_expected, frames_seen);
+  ASSERT_EQ(context.GetGLFBOFrameInfos().size(), frames_seen);


This is racy. context.GetGLFBOFrameInfos() can have 9 entries.

iskakaushik requested a review from chinmaygarde August 19, 2020 00:27

flutter-dashboard bot added platform-android platform-ios labels Aug 19, 2020

googlebot added the cla: yes label Aug 19, 2020

auto-assign bot requested a review from gw280 August 19, 2020 00:27

iskakaushik force-pushed the ec-1 branch 4 times, most recently from 56b270b to 5ff79a7 Compare August 19, 2020 15:47

[embedder] Add FBO callback that takes frame info

9e1e268

See EC-1 in flutter.dev/go/double-buffered-window-resize

iskakaushik force-pushed the ec-1 branch from 5ff79a7 to 9e1e268 Compare August 19, 2020 16:53

iskakaushik requested review from stuartmorgan-g and removed request for gw280 August 19, 2020 18:50

stuartmorgan-g suggested changes Aug 19, 2020

View reviewed changes

chinmaygarde reviewed Aug 19, 2020

View reviewed changes

Address CR feedback.

7c2e7c2

iskakaushik requested review from chinmaygarde and stuartmorgan-g August 19, 2020 20:11

chinmaygarde reviewed Aug 19, 2020

View reviewed changes

zero init structs

0522448

iskakaushik requested a review from chinmaygarde August 19, 2020 21:18

chinmaygarde approved these changes Aug 19, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 19, 2020

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 19, 2020

stuartmorgan-g approved these changes Aug 19, 2020

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Aug 19, 2020

fluttergithubbot merged commit 26fe912 into flutter:master Aug 19, 2020

engine-flutter-autoroll mentioned this pull request Aug 20, 2020

Roll Engine from 74d32ecdbd46 to 26fe9125b5bf (7 revisions) flutter/flutter#64206

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 20, 2020

26fe912 [embedder] Add FBO callback that takes frame info (flutter/en…

dc473a0

…gine#20617)

engine-flutter-autoroll mentioned this pull request Aug 20, 2020

Roll Engine from 74d32ecdbd46 to 89f506ed626b (10 revisions) flutter/flutter#64213

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Aug 20, 2020

26fe912 [embedder] Add FBO callback that takes frame info (flutter/en…

4fea663

…gine#20617)

gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Aug 20, 2020

[embedder] Add FBO callback that takes frame info (flutter#20617)

f4ea7ac

iskakaushik mentioned this pull request Aug 20, 2020

All shape related structs are together #20665

Merged

blasten mentioned this pull request Aug 21, 2020

EmbedderTest.FrameInfoContainsValidWidthAndHeight failed on Mac Host Engine flutter/flutter#64344

Closed

blasten reviewed Aug 21, 2020

View reviewed changes


		size_t GetSoftwareSurfacePresentCount() const;

		std::vector<FlutterFrameInfo> GetGLFBOFrameInfos();

[embedder] Add FBO callback that takes frame info #20617

[embedder] Add FBO callback that takes frame info #20617

Uh oh!

Conversation

iskakaushik commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iskakaushik commented Aug 19, 2020 •

edited

Loading