Update the content handler to use the Mozart session API. #3887

chinmaygarde · 2017-07-17T22:30:16Z

This patch contains the following commits from chinmaygarde/flutter_engine/tree/session.

Fix erroneous call to restoreToCount(0).
Always include child paint bounds in container layer.
Trace skia resource cache usage and bytes in available surfaces.
Add surface pool stats.
Fix the canvas clear operation that was removed due to an incorrect rebase.
Do a single flush with a idle wait.
Initial commit of the vulkan surface pool in the content handler.
Optimize texture creation for frames.
Use new view manager API.
We only need scale factors on Fuchsia.
Set the correct transformation for clipping.
Render at most one frame at a time, really.
Always draw full-sized shape textures.
Generate at most one texture per physical layer.
Only draw when needed.
Correctly save/restore the canvas state.
Rasterize images to the containing physical model layer.
Fix several issues related to paint bounds calculations.
Don't bother setting paint bounds when using system compositing.
Plum through hit testing behavior.
Implement child scene embedding.
Simplify propagation of session through the layers.
Use mozart::client::ContainerNode.
Only compile SceneUpdateContext for Fuchsia builds.
Remove unwanted dependencies.
WIP: Update flow layers to use the new Session interface.

j9brown

lgtm with minor nits (mostly TODOs)

j9brown · 2017-07-17T22:42:35Z

content_handler/runtime_holder.cc

-  if (!is_ready_to_draw_)
-    return;  // Only draw once per frame.
-  is_ready_to_draw_ = false;
+  if (!frame_outstanding_ || frame_rendering_)


We probably shouldn't be discarding the layer tree here. But then, Flutter shouldn't be calling Render() if we didn't call BeginFrame(). So why does the spurious call to Render happen?

(I think we should open a bug and leave a TODO here.)

Done. MZ-193

j9brown · 2017-07-17T22:43:39Z

content_handler/runtime_holder.cc

+  // Attempt to read the device pixel ratio.
+  float pixel_ratio = 1.0;
+  if (auto& metrics = properties->display_metrics) {
+    pixel_ratio = 2.0;  // TODO(chinmaygarde): Fix metrics->device_pixel_ratio;


The metrics are being passed through correctly now. We should use them.

j9brown · 2017-07-17T22:44:37Z

content_handler/session_connection.cc

+
+void SessionConnection::OnSessionError() {
+  ASSERT_IS_GPU_THREAD;
+  // TODO: Not this.


Isn't that a TODON'T? ;)

Right. At first I thought I should be doing something smarter, but terminating the process seems good enough to me :)

j9brown · 2017-07-17T22:47:40Z

content_handler/vulkan_surface.cc

+
+  mx::vmo session_vmo;
+  mx_status_t status =
+      exported_vmo_.duplicate(MX_RIGHT_SAME_RIGHTS, &session_vmo);


Don't duplicate this handle. We don't need to keep it any further past this point.

j9brown · 2017-07-17T22:49:36Z

content_handler/vulkan_surface.cc

+  return session_image_ != nullptr;
+}
+
+uint32_t VulkanSurface::GetSessionImageID() const {


Let's just expose the Image*.

mozart::client::Image* image() { return session_image_.get(); }

j9brown · 2017-07-17T23:10:02Z

flow/layers/clip_rect_layer.cc

+void ClipRectLayer::UpdateScene(SceneUpdateContext& context) {
+  FTL_DCHECK(needs_system_composite());
+
+  // TODO(MZ-138): Need to be able to specify an origin.


I think the transform is sufficient to set the origin (and we already apply it). So let's remove this TODO here.

j9brown · 2017-07-17T23:10:26Z

flow/layers/clip_rrect_layer.cc

+  FTL_DCHECK(needs_system_composite());
+
+  // TODO(MZ-137): Need to be able to express the radii as vectors.
+  // TODO(MZ-138): Need to be able to specify an origin.


I think the transform is sufficient to set the origin (and we already apply it). So let's remove this TODO here.

j9brown · 2017-07-17T23:10:42Z

flow/layers/clip_path_layer.cc

+
+  // TODO(MZ-140): Must be able to specify paths as shapes to nodes.
+  //               Treating the shape as a rectangle for now.
+  // TODO(MZ-138): Need to be able to specify an origin.


I think the transform is sufficient to set the origin (and we already apply it). So let's remove this TODO here.

j9brown · 2017-07-17T23:16:20Z

flow/scene_update_context.cc

+  // Add a part which represents the frame's geometry for clipping purposes
+  // and possibly for its texture.
+  // TODO(MZ-137): Need to be able to express the radii as vectors.
+  // TODO(MZ-138): Need to be able to specify an origin.


I think the transform is sufficient to set the origin (and we already apply it). So let's remove this TODO here.

j9brown · 2017-07-17T23:20:21Z

flow/scene_update_context.cc

+SceneUpdateContext::Transform::Transform(SceneUpdateContext& context,
+                                         const SkMatrix& transform)
+    : Entity(context) {
+  // TODO(chinmaygarde): The perspective and shear components in the matrix


TODO(MZ-192)

j9brown

lgtm, minor nits

j9brown · 2017-07-18T01:36:54Z

content_handler/vulkan_surface.h


  // |flow::SceneUpdateContext::SurfaceProducerSurface|
-  uint32_t GetSessionImageID() const override;
+  mozart::client::Image* image() override;


Why is this virtual anyhow? But since it is, per Google C++ style guide, this should be called GetImage(). Sorry for the churn.

The interface is defined in the flow library and there used to be two implementations in the content handler (one for Vulkan and one for software rasterization). The flow layer not know anything about the specific client rendering library used. Hence the virtual methods. I'll fix the signature.

j9brown · 2017-07-18T01:38:01Z

flow/scene_update_context.cc

  }

-  auto session_image_id = surface->GetSessionImageID();
+  auto session_image_id = surface->image()->id();


Return the Image* from this function instead.

chinmaygarde requested review from abarth, freiling, j9brown, jason-simmons, konkers, mikejurka and schwa423 July 17, 2017 22:30

googlebot added the cla: yes label Jul 17, 2017

j9brown reviewed Jul 17, 2017

View reviewed changes

j9brown approved these changes Jul 18, 2017

View reviewed changes

Update the content handler to use the Mozart session API.

bd0ee31

chinmaygarde force-pushed the session_review branch from 4c8e450 to bd0ee31 Compare July 18, 2017 22:31

Update licenses again.

55c8090

j9brown merged commit 925298d into flutter:master Jul 18, 2017

chinmaygarde deleted the session_review branch August 8, 2017 01:10

Update the content handler to use the Mozart session API. #3887

Update the content handler to use the Mozart session API. #3887

Uh oh!

Conversation

chinmaygarde commented Jul 17, 2017

Uh oh!

j9brown 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!

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!

j9brown 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants