Initial checkin of the new Mozart compositor.

Initial checkin of the new Mozart compositor.

The Mozart compositor is designed to support feed forward rendering of
scene graphs using scene version codes and node combinators to
determine synchronization behaviors.

A unique characteristic of Mozart is its ability to allow clients to
specify how to resolve situations where content is missing or not ready
by supplying alternative choices for what should be rendered.  The goal
is to avoid stalls by allowing more flexibility in specifying how scene
state updates are coordinated.

The scene graph consists of nodes of various types which represent
primitive operations such as filling a rectangle, drawing an image,
or embedding another scene.

The children of each node are composed according to the node's combinator
rule.  The default MERGE combinator rule simply draws all children
sequentially and refuses to draw anying if any of the children are blocked.
Other rules, such as FALLBACK, allow for alternative choices to be
made by the compositor at snapshot time based on what is available.

In the simplest cases, a client might provide substitute content to be
used in case the primary content is not yet ready.  In more elaborate
cases, a client might describe how an older version of the primary content
can be adapted to fit current demands, perhaps scaling or cropping it
as required.

The compositor itself is designed to be relatively unopinionated about
the contents that it is compositing.  Applications publish their scenes
and Mozart composites them, that's it.  High-level functionality such
as maintaining view embedding relationships or input dispatch are
exclusively handled by other components.

Mozart also offers:

- A relatively easy to use client interface.
- Vsync based scheduling.
- Hit testing (only partially implemented in this patch).
- Support for clients which use separate threads for event processing
  and rendering.
- Multi-display support (only partially implemented in this patch).

The rasterizer is currently implemented using Skia and Ganesh and runs
on a separate thread from the main engine.

This initial patch covers most of the essentials but there's plenty
more testing, optimization, and elaboration to do later.

This patch is followed by several others which serve to update the
view manager to use the new compositor, add various helpers,
update examples, and so on.

BUG=
R=abarth@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/751f4d3c32cbd11f0bfd44c5facf6c62173310bf

[jeffbrown, 2015-12-31 11:19:51]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, jamesr@chromium.org, viettrungluu@chromium.org

[viettrungluu, 2016-01-04 21:08:24]

May as well send comments/questions now.

(Far from finished reviewing ... will continue working.)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/README.md (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/README.md:4: It provides a graphical compositor system
for used by other applications.
"used" -> "use" (also in the original)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/output.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:17: // Renders snapshotted frames of
the scene graph to a display output.
Could you comment on any thread-safety requirements of implementations of
Output? (Partly to help me understand GpuOutput better....)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:20: Output() = default;
nit: I think we'd usually just say "{}" instead of "= default;" for default
constructors (and also destructors). <shrug>

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:24: virtual Scheduler* scheduler() = 0;
nit: lowercase_c_style() is usually reserved for inlined getters/setters/etc.
(which admittedly is a stupid rule), and typically wouldn't be used for
something virtual, I think. (So this would typically be called GetScheduler().)

(I don't feel very strongly about this, and don't know if anyone else does.)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:27: virtual void SubmitFrame(const
std::shared_ptr<RenderFrame>& frame) = 0;
I wonder if this should take an std::shared_ptr<RenderFrame>&& instead (the main
advantage being that this could avoid some refcount churn). <shrug>

Or maybe an std::shared_ptr<RenderFrame>. (This has the advantage of not being
annoying for the caller when they actually want to hold on to a reference, at
the cost of easier accidental churn.)

Not a big deal in any case.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/scheduler.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:18: enum class SchedulingMode {
Maybe this should be declared inside Scheduler? (Depends on how much you
envisage it being used separately from Scheduler, I guess.)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:20: kSnapshot,
FYI: Typically, we've followed Chromium style, which uses FOO_BAR instead of
kFooBar for enum values (which is an older Google style). However, rumor has it
there are ongoing style discussions within Google (and relatedly also
Chromium)....

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:26: // Interface implemented by
schedulers.
This comment is almost useless. :) It may be helpful to say what a scheduler
is....

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:56: struct SchedulerCallbacks {
When/where is this used? (It's a bit odd that it's in this file, and not used by
Scheduler....)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/vsync_scheduler.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/vsync_scheduler.h:28: static constexpr int64_t
kMinVsyncInterval = 1000;     // 1000 Hz
You need to declare storage for these (in the .cc) file. (Otherwise, you'd
probably get a link error if someone ever needed to use
&VsyncScheduler::kMinVsyncInterval, or sometimes when calling a function that
takes a reference argument[*] at low optimization levels.)

[*] It's pretty stupid to take a |const int64_t&| argument, but you run into
this with templated stuff (e.g., STL containers).

[abarth, 2016-01-10 04:22:09]

Brain experienced EOVERFLOW.  Will continue review after TV.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
File mojo/services/gfx/composition/interfaces/hit_tests.mojom (right):

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/hit_tests.mojom:39: array<Hit> hits;
I'm not sure it matters much, but this is backwards from what we do in Flutter. 
In Flutter, we give this list from the leaf up to the root.

There are some more subtle issues that we can discuss in more detail in person
if you like.  What's you've got here is fine, but less powerful that it could be
with some small tweaks.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
File mojo/services/gfx/composition/interfaces/nodes.mojom (right):

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:91: Clip? content_clip;
Woah there.  This should just be a rectangular clip.  More complex clips (e.g.,
RRect) should go through the LayerNodeOp.  You're going to want to do all sorts
of transformations with rectangular clips that you can't do with the more
complex clips (e.g., push them down to the images to restrict the source_rects).

Also, RRect clips can't be done with proper antialiasing without an intermediate
layer anyway.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:96: // TODO(jeffbrown):
This scheme is probably too naive.
Yeah.  :)

It should be enough to get going though.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:144: mojo.Rect? image_rect;
Sometimes we call this the source_rect

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:191: struct LayerNodeOp {
Oh, interesting.  I thought this was going to be something different.  I thought
you were going to give me a way to cache pixels in the compositor by letting me
create a node that retained its intermediate state.  Then I could use that
object in layer scenes until it was freed.  Obviously that's something we can
add later.  ;)

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:201: union Clip {
I'd skip having this union and just have the clips separated.  RRect and Rect
clips are so different from each other.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:209: // Make it less silly.
I'd have used a single uint32 with a defined color channel order (RGBA_8888
probably).

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.cc:61: if
(frame_queue_->PutFrame(frame)) {
We should talk in person about whether this scheduling model is what you
actually want.  Rather than dropping frames here, you might want to apply more
back pressure earlier in the pipeline.  Obviously, scheduling is a complicated
topic and one that we'll iterate on over time, so I wouldn't worry about it too
much for this iteration.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_output.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.h:49: std::shared_ptr<RenderFrame>
TakeFrame();
Do we want to use shared_ptr in production code?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_rasterizer.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_rasterizer.cc:168: }
Usually you want some sort of signal to come out of here.  Knowing when the swap
completes lets you throttle work earlier in the pipeline.  I guess I haven't
seen where the backpressure gets applied yet.

[abarth, 2016-01-10 22:55:37]

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/compositor_engine.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:106: label.get().substr(0,
mojo::gfx::composition::kLabelMaxLength);
This pattern recurs a bunch.  Maybe factor it out somewhere?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:197:
base::TimeDelta::FromMicroseconds(presentation_time - now));
Do we need to schedule this for earlier so that it will actually be presented at
presentation_time?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:228: viewport->height < 1 ||
viewport->height > kMaxViewportHeight) {
nit: I would have tested <= 0

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:262: callback.Run(nullptr);
It's odd that there are two "failed" results here: a null HitTestResult and a
HitTestResult with an empty hits list.  Might want to reduce to just the latter.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:277: // TODO: Detect ANRs
ANRs?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:278: if
(scene_state->scene_listener())
Multiline ifs require { }

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:337: DestroyScene(scene_state);
Should we DCHECK that scene_state isn't in scenes_by_token_ anymore?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/node_def.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:18: // TODO(jeffbrown): Move these
conversion functions somewhere more useful.
There's a converters directory in mojo/common.  Maybe we should add some
converters to mojo/skia?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:20: static SkRect ToSkRect(const
mojo::Rect& rect) {
The mojo types have template magic to make these sorts of converters easier.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:32: SkRect::MakeXYWH(rrect.x, rrect.y,
rrect.width, rrect.height), radii);
Note that this routine is more expensive that setRectXY, which is by far the
more common use case.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:38: // Perhaps we need a simpler 2D
transform matrix mojom.
This is the same thing we do in Flutter.  We can talk to the Skia folks about
it, but it'll work fine for a while.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:54:
result.addRect(ToSkRect(*clip.get_rect()));
Woah there.  I haven't seen where you're using this function, but the codepath
for Rect should be very different from the RRect and Path code paths.  Rect
clips can be done much more efficiently if you use the Rect-specific codepath in
Skia.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:216: return s.str();
base::StringPrintf

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/resource_def.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/resource_def.h:47: SceneDef* referenced_scene_;
What is the lifetime relationship between this object and referenced_scene_ ? 
The raw pointer makes this very unclear.  Based on the clear_referenced_scene()
function above, it looks like this might be a weak pointer...  Should we use
some sort of pointer type to make the lifetime relationship explicit?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/resource_def.h:64: std::shared_ptr<RenderImage>
image_;
I think I raised this before, but shared_ptr is not particularly efficient.  We
probably want to use scoped_refptr or some other intrusive ref pointer that
doesn't have a built-in weak pointer confusing things.

It's ok to use shared_ptr in examples to avoid dragging in extra dependencies,
but in production code, I think we should be using a ref pointer that's more
optimized for our use cases.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/scene_def.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/scene_def.cc:167:
unavailable_sender.Run(pair.first);
This really looks like a weak pointer with a callback.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/scene_def.cc:234: // avoidance behavior. 
Basically just need better bookkeeping.
The DAG is making this harder for you.  I'm not sure how important it is to have
a DAG rather than a tree.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/snapshot.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/snapshot.cc:55: SkRect skia_viewport =
sk_viewport

We usually use "sk" rather than "skia" to refer to stuff in Skia types.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/snapshot.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/snapshot.h:86: SnapshotBuilder(std::ostream*
block_log);
explicit

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/painting.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:34: // Gets the Ganesh scope.
Kind of a useless comment.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:39: // Gets the painting cache.
Kind of a useless comment.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:44: PaintingCache* const
painting_cache_;
If this isn't nullable, consider "const PaintingCache&"

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:91:
std::owner_less<std::weak_ptr<RenderImage>>> used_images_;
No idea what this does.

map instead of unordered_map?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/render_frame.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_frame.h:37: static
std::shared_ptr<RenderFrame> New(
We usually call these functions "Create"

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/render_layer.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:73:
canvas->concat(content_transform_);
In Flutter (and in Chrome) we optimize the case where the content transform is a
2d offset by pushing offsets through the whole system and applying them at the
end in the DrawImage / DrawPicture step.  That lets us skip a ton of matrix ops
and save/restore pairs.  I'm not sure you need to do that in the first iteration
of the system, but it's something to keep in mind.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:75:
canvas->clipPath(content_clip_);
It's strange to group these two operations together.  concat just requires a
"canvas->save" but clipPath requires a "canvas->saveLayer".  I think this is
part of your treating rectangular clips and non-rectangular clips in a uniform
way.  If this were just a rectangular clip here, then it would make more sense.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:195: canvas->flush();
No need for flush here.  endRecordingAsPicture is effectively a finish.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:223:
layer_->commands_.emplace_back(new PushNodeCommand(node_id, hit_id));
This seems like it will generate a lot of extra save()/restore() pairs without
anything mutating state that's saved and restored.  That quite inefficient.  Why
not defer the save()/restore() work to the point where you mutate state that
needs to be saved and restored?

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:231:
layer_->commands_.emplace_back(new PopNodeCommand());
This file looks a bit like you're re-implementing SkPictureRecorder.  The actual
implementation of SkPictureRecorder does this with many fewer mallocs, etc.

Is there some reason you need to reify the command list here?  It might be
easier to just build up the command list inside an SkPicture.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/renderer_impl.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/renderer_impl.h:45: RendererState* const state_;
Consider const Foo& if these are non-nullable.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/renderer_state.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/renderer_state.h:90: base::WeakPtrFactory<RendererState>
weak_factory_;  // must be last
You can skip this comment.  weak_factory_ always needs to be last.  It's a
common enough pattern that everyone working on the codebase should know it
already and shouldn't need to be reminded.

[abarth, 2016-01-10 22:57:57]

This patch is way too big to review.  I'd recommend breaking it down into a
number of steps:

1) The mojom interfaces.
2) The scheduler.
3) The data structures for the scene graph and the Skia backend
4) The code that applies updates and publishes the scene graph.
5) The mojo app stuff that wires it all up.

[jeffbrown, 2016-01-10 23:55:22]

On 2016/01/10 22:57:57, abarth wrote:
> This patch is way too big to review.  I'd recommend breaking it down into a
> number of steps:
> 
> 1) The mojom interfaces.
> 2) The scheduler.
> 3) The data structures for the scene graph and the Skia backend
> 4) The code that applies updates and publishes the scene graph.
> 5) The mojo app stuff that wires it all up.

Yeah, it is a big patch but with all of the comments on it at this point I don't
think it's worth the hassle of splitting it up.  Future patches will be smaller,
I promise.  :)

Thank you very much for your review.  Lots of stuff to think about.

[abarth, 2016-01-11 03:40:12]

On 2016/01/10 at 23:55:22, jeffbrown wrote:
> Yeah, it is a big patch but with all of the comments on it at this point I
don't think it's worth the hassle of splitting it up.  Future patches will be
smaller, I promise.  :)

No worries.  If you land the mojoms separately, that can unblock landing a bunch
of your other CLs because they'll compile.  :)

> Thank you very much for your review.  Lots of stuff to think about.

My pleasure.  We also don't need to get everything perfect in the first landing.

[jeffbrown, 2016-01-16 03:28:33]

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
File mojo/services/gfx/composition/interfaces/hit_tests.mojom (right):

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/hit_tests.mojom:39: array<Hit> hits;
On 2016/01/10 04:22:08, abarth wrote:
> I'm not sure it matters much, but this is backwards from what we do in
Flutter. 
> In Flutter, we give this list from the leaf up to the root.
> 
> There are some more subtle issues that we can discuss in more detail in person
> if you like.  What's you've got here is fine, but less powerful that it could
be
> with some small tweaks.

Good point.  I changed the description but I'll come back to this after Mozart
is landed.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
File mojo/services/gfx/composition/interfaces/nodes.mojom (right):

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:91: Clip? content_clip;
On 2016/01/10 04:22:08, abarth wrote:
> Woah there.  This should just be a rectangular clip.  More complex clips
(e.g.,
> RRect) should go through the LayerNodeOp.  You're going to want to do all
sorts
> of transformations with rectangular clips that you can't do with the more
> complex clips (e.g., push them down to the images to restrict the
source_rects).
> 
> Also, RRect clips can't be done with proper antialiasing without an
intermediate
> layer anyway.

I need to put some thought into whether nodes have strict visual containment
too.  For now, I'll just drop RRects entirely.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:144: mojo.Rect? image_rect;
On 2016/01/10 04:22:09, abarth wrote:
> Sometimes we call this the source_rect

Right.  I adopted this convention to line up with |image_resource_id| below to
make it clearer that they go hand-in-hand.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:191: struct LayerNodeOp {
On 2016/01/10 04:22:08, abarth wrote:
> Oh, interesting.  I thought this was going to be something different.  I
thought
> you were going to give me a way to cache pixels in the compositor by letting
me
> create a node that retained its intermediate state.  Then I could use that
> object in layer scenes until it was freed.  Obviously that's something we can
> add later.  ;)

Technically you can get that behavior simply by embedding the same layer node
multiple times in different places (a big benefit of having a DAG).  But we
might want to do something different here in the long run.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:201: union Clip {
On 2016/01/10 04:22:09, abarth wrote:
> I'd skip having this union and just have the clips separated.  RRect and Rect
> clips are so different from each other.

Done.

https://codereview.chromium.org/1552963002/diff/20001/mojo/services/gfx/compo...
mojo/services/gfx/composition/interfaces/nodes.mojom:209: // Make it less silly.
On 2016/01/10 04:22:08, abarth wrote:
> I'd have used a single uint32 with a defined color channel order (RGBA_8888
> probably).

Yeah, I'll probably land there.  Will leave it for later.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/README.md (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/README.md:4: It provides a graphical compositor system
for used by other applications.
On 2016/01/04 21:08:23, viettrungluu wrote:
> "used" -> "use" (also in the original)

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.cc:61: if
(frame_queue_->PutFrame(frame)) {
On 2016/01/10 04:22:09, abarth wrote:
> We should talk in person about whether this scheduling model is what you
> actually want.  Rather than dropping frames here, you might want to apply more
> back pressure earlier in the pipeline.  Obviously, scheduling is a complicated
> topic and one that we'll iterate on over time, so I wouldn't worry about it
too
> much for this iteration.

Agreed.  This needs backpressure.  I'll fix that later.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_output.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.h:49: std::shared_ptr<RenderFrame>
TakeFrame();
On 2016/01/10 04:22:09, abarth wrote:
> Do we want to use shared_ptr in production code?

I don't have strong opinions on this.  shared_ptr does not have any additional
heap overhead as long as every instance is allocated using make_shared.  Works
fine for me.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/gpu_rasterizer.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_rasterizer.cc:168: }
On 2016/01/10 04:22:09, abarth wrote:
> Usually you want some sort of signal to come out of here.  Knowing when the
swap
> completes lets you throttle work earlier in the pipeline.  I guess I haven't
> seen where the backpressure gets applied yet.

It doesn't.  :)

I'll fix this later, as we discussed.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/output.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:17: // Renders snapshotted frames of
the scene graph to a display output.
On 2016/01/04 21:08:23, viettrungluu wrote:
> Could you comment on any thread-safety requirements of implementations of
> Output? (Partly to help me understand GpuOutput better....)

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:20: Output() = default;
On 2016/01/04 21:08:23, viettrungluu wrote:
> nit: I think we'd usually just say "{}" instead of "= default;" for default
> constructors (and also destructors). <shrug>

Yeah.  I'm experimenting with the style mostly.  Using "default" feels a little
nicer for abstract classes that only contain pure virtual methods.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:24: virtual Scheduler* scheduler() = 0;
On 2016/01/04 21:08:23, viettrungluu wrote:
> nit: lowercase_c_style() is usually reserved for inlined getters/setters/etc.
> (which admittedly is a stupid rule), and typically wouldn't be used for
> something virtual, I think. (So this would typically be called
GetScheduler().)
> 
> (I don't feel very strongly about this, and don't know if anyone else does.)

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/output.h:27: virtual void SubmitFrame(const
std::shared_ptr<RenderFrame>& frame) = 0;
On 2016/01/04 21:08:23, viettrungluu wrote:
> I wonder if this should take an std::shared_ptr<RenderFrame>&& instead (the
main
> advantage being that this could avoid some refcount churn). <shrug>
> 
> Or maybe an std::shared_ptr<RenderFrame>. (This has the advantage of not being
> annoying for the caller when they actually want to hold on to a reference, at
> the cost of easier accidental churn.)
> 
> Not a big deal in any case.

I think the style guide says we should only use && for move constructors. 
Passing a const reference at least avoids potentially creating a temporary for
no reason as is common practice for many other reference counted objects.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/scheduler.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:18: enum class SchedulingMode {
On 2016/01/04 21:08:24, viettrungluu wrote:
> Maybe this should be declared inside Scheduler? (Depends on how much you
> envisage it being used separately from Scheduler, I guess.)

I went back and forth on this.  I'll move it back in.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:20: kSnapshot,
On 2016/01/04 21:08:24, viettrungluu wrote:
> FYI: Typically, we've followed Chromium style, which uses FOO_BAR instead of
> kFooBar for enum values (which is an older Google style). However, rumor has
it
> there are ongoing style discussions within Google (and relatedly also
> Chromium)....

Apparently the preferred style is now "kFoo" like other constants.

https://google.github.io/styleguide/cppguide.html#Enumerator_Names
"New code should prefer constant-style naming if possible."

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:26: // Interface implemented by
schedulers.
On 2016/01/04 21:08:24, viettrungluu wrote:
> This comment is almost useless. :) It may be helpful to say what a scheduler
> is....

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/scheduler.h:56: struct SchedulerCallbacks {
On 2016/01/04 21:08:23, viettrungluu wrote:
> When/where is this used? (It's a bit odd that it's in this file, and not used
by
> Scheduler....)

Added docs to make this clearer.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/backend/vsync_scheduler.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/vsync_scheduler.h:28: static constexpr int64_t
kMinVsyncInterval = 1000;     // 1000 Hz
On 2016/01/04 21:08:24, viettrungluu wrote:
> You need to declare storage for these (in the .cc) file. (Otherwise, you'd
> probably get a link error if someone ever needed to use
> &VsyncScheduler::kMinVsyncInterval, or sometimes when calling a function that
> takes a reference argument[*] at low optimization levels.)
> 
> [*] It's pretty stupid to take a |const int64_t&| argument, but you run into
> this with templated stuff (e.g., STL containers).

Hah!  You're right.  I'd interpreted constexpr as meaning a "compile time only
constant expression" but it turns out it needs storage just like "const".  Oh
well.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/compositor_engine.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:106: label.get().substr(0,
mojo::gfx::composition::kLabelMaxLength);
On 2016/01/10 22:55:35, abarth wrote:
> This pattern recurs a bunch.  Maybe factor it out somewhere?

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:197:
base::TimeDelta::FromMicroseconds(presentation_time - now));
On 2016/01/10 22:55:35, abarth wrote:
> Do we need to schedule this for earlier so that it will actually be presented
at
> presentation_time?

It actually doesn't matter here.  This only happens if the scene isn't part of a
visible scene graph as otherwise it would get snapshotted earlier.  I'll add a
comment.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:228: viewport->height < 1 ||
viewport->height > kMaxViewportHeight) {
On 2016/01/10 22:55:35, abarth wrote:
> nit: I would have tested <= 0

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:262: callback.Run(nullptr);
On 2016/01/10 22:55:35, abarth wrote:
> It's odd that there are two "failed" results here: a null HitTestResult and a
> HitTestResult with an empty hits list.  Might want to reduce to just the
latter.

Good point.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:277: // TODO: Detect ANRs
On 2016/01/10 22:55:35, abarth wrote:
> ANRs?

"Application Not Responding"

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:278: if
(scene_state->scene_listener())
On 2016/01/10 22:55:35, abarth wrote:
> Multiline ifs require { }

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_engine.cc:337: DestroyScene(scene_state);
On 2016/01/10 22:55:35, abarth wrote:
> Should we DCHECK that scene_state isn't in scenes_by_token_ anymore?

You mean after DestroyScene returns?  I don't think that's necessary since this
function doesn't really care after this point.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/node_def.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:18: // TODO(jeffbrown): Move these
conversion functions somewhere more useful.
On 2016/01/10 22:55:36, abarth wrote:
> There's a converters directory in mojo/common.  Maybe we should add some
> converters to mojo/skia?

Yeah... but maybe later.  :)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:32: SkRect::MakeXYWH(rrect.x, rrect.y,
rrect.width, rrect.height), radii);
On 2016/01/10 22:55:35, abarth wrote:
> Note that this routine is more expensive that setRectXY, which is by far the
> more common use case.

Yes, but we don't know whether we're in the fast case yet without checking. 
Fortunately Skia checks this for us so I think it's a wash.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:38: // Perhaps we need a simpler 2D
transform matrix mojom.
On 2016/01/10 22:55:36, abarth wrote:
> This is the same thing we do in Flutter.  We can talk to the Skia folks about
> it, but it'll work fine for a while.

Acknowledged.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:54:
result.addRect(ToSkRect(*clip.get_rect()));
On 2016/01/10 22:55:36, abarth wrote:
> Woah there.  I haven't seen where you're using this function, but the codepath
> for Rect should be very different from the RRect and Path code paths.  Rect
> clips can be done much more efficiently if you use the Rect-specific codepath
in
> Skia.

Dropping RRect clips for now.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/node_def.cc:216: return s.str();
On 2016/01/10 22:55:35, abarth wrote:
> base::StringPrintf

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/resource_def.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/resource_def.h:47: SceneDef* referenced_scene_;
On 2016/01/10 22:55:36, abarth wrote:
> What is the lifetime relationship between this object and referenced_scene_ ? 
> The raw pointer makes this very unclear.  Based on the
clear_referenced_scene()
> function above, it looks like this might be a weak pointer...  Should we use
> some sort of pointer type to make the lifetime relationship explicit?

I'm using bare pointers because I can guarantee that they aren't left dangling
by mutations, I don't want reference counting, and I need more control than a
weak pointer offers.

I agree it's a bit more opaque this way so I may revisit it later.  I'll add a
comment here.

See SceneDef::UnlinkReferencedScene.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/resource_def.h:64: std::shared_ptr<RenderImage>
image_;
On 2016/01/10 22:55:36, abarth wrote:
> I think I raised this before, but shared_ptr is not particularly efficient. 
We
> probably want to use scoped_refptr or some other intrusive ref pointer that
> doesn't have a built-in weak pointer confusing things.
> 
> It's ok to use shared_ptr in examples to avoid dragging in extra dependencies,
> but in production code, I think we should be using a ref pointer that's more
> optimized for our use cases.

I am actually using weak_ptr semantics here.  It's actually a good fit for this
case.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/scene_def.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/scene_def.cc:167:
unavailable_sender.Run(pair.first);
On 2016/01/10 22:55:36, abarth wrote:
> This really looks like a weak pointer with a callback.

Perhaps, but I don't see a way to set a callback on a base::WeakPtr.  And in any
case, there are side-effects that must be orchestrated by the engine.  I'm going
to leave this one to ponder for later together with whether I want to
encapsulate SceneDef cross-references in some way.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/scene_def.cc:234: // avoidance behavior. 
Basically just need better bookkeeping.
On 2016/01/10 22:55:36, abarth wrote:
> The DAG is making this harder for you.  I'm not sure how important it is to
have
> a DAG rather than a tree.

I think the DAG will be quite useful for cross-application transitions, saved
layers, and multi-display support.  That said, I may well change the policy for
handling cycles.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/snapshot.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/snapshot.cc:55: SkRect skia_viewport =
On 2016/01/10 22:55:36, abarth wrote:
> sk_viewport
> 
> We usually use "sk" rather than "skia" to refer to stuff in Skia types.

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/graph/snapshot.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/graph/snapshot.h:86: SnapshotBuilder(std::ostream*
block_log);
On 2016/01/10 22:55:36, abarth wrote:
> explicit

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/painting.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:34: // Gets the Ganesh scope.
On 2016/01/10 22:55:36, abarth wrote:
> Kind of a useless comment.

Yeah, habits from documenting public APIs.  I'll remove this.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:39: // Gets the painting cache.
On 2016/01/10 22:55:36, abarth wrote:
> Kind of a useless comment.

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:44: PaintingCache* const
painting_cache_;
On 2016/01/10 22:55:36, abarth wrote:
> If this isn't nullable, consider "const PaintingCache&"

It can't be const though and non-const references are verboten.  Alas.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:91:
std::owner_less<std::weak_ptr<RenderImage>>> used_images_;
On 2016/01/10 22:55:36, abarth wrote:
> No idea what this does.
> 
> map instead of unordered_map?

http://en.cppreference.com/w/cpp/memory/owner_less

But now that you mention it, an unordered map might work fine here...

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/render_frame.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_frame.h:37: static
std::shared_ptr<RenderFrame> New(
On 2016/01/10 22:55:36, abarth wrote:
> We usually call these functions "Create"

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/render_layer.cc (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:73:
canvas->concat(content_transform_);
On 2016/01/10 22:55:36, abarth wrote:
> In Flutter (and in Chrome) we optimize the case where the content transform is
a
> 2d offset by pushing offsets through the whole system and applying them at the
> end in the DrawImage / DrawPicture step.  That lets us skip a ton of matrix
ops
> and save/restore pairs.  I'm not sure you need to do that in the first
iteration
> of the system, but it's something to keep in mind.

Yeah, this is just to get us off the ground.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:75:
canvas->clipPath(content_clip_);
On 2016/01/10 22:55:36, abarth wrote:
> It's strange to group these two operations together.  concat just requires a
> "canvas->save" but clipPath requires a "canvas->saveLayer".  I think this is
> part of your treating rectangular clips and non-rectangular clips in a uniform
> way.  If this were just a rectangular clip here, then it would make more
sense.

Eliminating rounded clips for now.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:195: canvas->flush();
On 2016/01/10 22:55:36, abarth wrote:
> No need for flush here.  endRecordingAsPicture is effectively a finish.

Done.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:223:
layer_->commands_.emplace_back(new PushNodeCommand(node_id, hit_id));
On 2016/01/10 22:55:36, abarth wrote:
> This seems like it will generate a lot of extra save()/restore() pairs without
> anything mutating state that's saved and restored.  That quite inefficient. 
Why
> not defer the save()/restore() work to the point where you mutate state that
> needs to be saved and restored?

Oh yeah, it's horrible.  Not to mention all of the extra allocations.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/render_layer.cc:231:
layer_->commands_.emplace_back(new PopNodeCommand());
On 2016/01/10 22:55:36, abarth wrote:
> This file looks a bit like you're re-implementing SkPictureRecorder.  The
actual
> implementation of SkPictureRecorder does this with many fewer mallocs, etc.
> 
> Is there some reason you need to reify the command list here?  It might be
> easier to just build up the command list inside an SkPicture.

I didn't know about SkImageGenerator.  So... next patch will delete all of this
stuff.  :)

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/renderer_impl.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/renderer_impl.h:45: RendererState* const state_;
On 2016/01/10 22:55:36, abarth wrote:
> Consider const Foo& if these are non-nullable.

I need a non-const reference here.  So the best I can do is to say that the
pointer itself is const though that's perhaps of somewhat dubious value.  Ahh
C++ modifiers...

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/renderer_state.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/renderer_state.h:90: base::WeakPtrFactory<RendererState>
weak_factory_;  // must be last
On 2016/01/10 22:55:36, abarth wrote:
> You can skip this comment.  weak_factory_ always needs to be last.  It's a
> common enough pattern that everyone working on the codebase should know it
> already and shouldn't need to be reminded.

Done.

[jeffbrown, 2016-01-16 03:43:40]

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/render/painting.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/render/painting.h:91:
std::owner_less<std::weak_ptr<RenderImage>>> used_images_;
On 2016/01/16 03:28:32, jeffbrown wrote:
> On 2016/01/10 22:55:36, abarth wrote:
> > No idea what this does.
> > 
> > map instead of unordered_map?
> 
> http://en.cppreference.com/w/cpp/memory/owner_less
> 
> But now that you mention it, an unordered map might work fine here...

Ahh sadly there's no std::hash<> defined for std::weak_ptr.

[jeffbrown, 2016-01-26 09:14:39]

apply review feedback

[abarth, 2016-01-26 22:44:44]

LGTM

We'll obviously need to continue working on this system, but this patch seems
like a good starting point.

[jeffbrown, 2016-01-26 23:51:05]

Description was changed from

==========
Initial checkin of the new Mozart compositor.

The Mozart compositor is designed to support feed forward rendering of
scene graphs using scene version codes and node combinators to
determine synchronization behaviors.

A unique characteristic of Mozart is its ability to allow clients to
specify how to resolve situations where content is missing or not ready
by supplying alternative choices for what should be rendered.  The goal
is to avoid stalls by allowing more flexibility in specifying how scene
state updates are coordinated.

The scene graph consists of nodes of various types which represent
primitive operations such as filling a rectangle, drawing an image,
or embedding another scene.

The children of each node are composed according to the node's combinator
rule.  The default MERGE combinator rule simply draws all children
sequentially and refuses to draw anying if any of the children are blocked.
Other rules, such as FALLBACK, allow for alternative choices to be
made by the compositor at snapshot time based on what is available.

In the simplest cases, a client might provide substitute content to be
used in case the primary content is not yet ready.  In more elaborate
cases, a client might describe how an older version of the primary content
can be adapted to fit current demands, perhaps scaling or cropping it
as required.

The compositor itself is designed to be relatively unopinionated about
the contents that it is compositing.  Applications publish their scenes
and Mozart composites them, that's it.  High-level functionality such
as maintaining view embedding relationships or input dispatch are
exclusively handled by other components.

Mozart also offers:

- A relatively easy to use client interface.
- Vsync based scheduling.
- Hit testing (only partially implemented in this patch).
- Support for clients which use separate threads for event processing
  and rendering.
- Multi-display support (only partially implemented in this patch).

The rasterizer is currently implemented using Skia and Ganesh and runs
on a separate thread from the main engine.

This initial patch covers most of the essentials but there's plenty
more testing, optimization, and elaboration to do later.

This patch is followed by several others which serve to update the
view manager to use the new compositor, add various helpers,
update examples, and so on.

BUG=
==========

to

==========
Initial checkin of the new Mozart compositor.

The Mozart compositor is designed to support feed forward rendering of
scene graphs using scene version codes and node combinators to
determine synchronization behaviors.

A unique characteristic of Mozart is its ability to allow clients to
specify how to resolve situations where content is missing or not ready
by supplying alternative choices for what should be rendered.  The goal
is to avoid stalls by allowing more flexibility in specifying how scene
state updates are coordinated.

The scene graph consists of nodes of various types which represent
primitive operations such as filling a rectangle, drawing an image,
or embedding another scene.

The children of each node are composed according to the node's combinator
rule.  The default MERGE combinator rule simply draws all children
sequentially and refuses to draw anying if any of the children are blocked.
Other rules, such as FALLBACK, allow for alternative choices to be
made by the compositor at snapshot time based on what is available.

In the simplest cases, a client might provide substitute content to be
used in case the primary content is not yet ready.  In more elaborate
cases, a client might describe how an older version of the primary content
can be adapted to fit current demands, perhaps scaling or cropping it
as required.

The compositor itself is designed to be relatively unopinionated about
the contents that it is compositing.  Applications publish their scenes
and Mozart composites them, that's it.  High-level functionality such
as maintaining view embedding relationships or input dispatch are
exclusively handled by other components.

Mozart also offers:

- A relatively easy to use client interface.
- Vsync based scheduling.
- Hit testing (only partially implemented in this patch).
- Support for clients which use separate threads for event processing
  and rendering.
- Multi-display support (only partially implemented in this patch).

The rasterizer is currently implemented using Skia and Ganesh and runs
on a separate thread from the main engine.

This initial patch covers most of the essentials but there's plenty
more testing, optimization, and elaboration to do later.

This patch is followed by several others which serve to update the
view manager to use the new compositor, add various helpers,
update examples, and so on.

BUG=
R=abarth@google.com

Committed:
https://chromium.googlesource.com/external/mojo/+/751f4d3c32cbd11f0bfd44c5fac...
==========

[jeffbrown, 2016-01-26 23:51:07]

Committed patchset #4 (id:60001) manually as
751f4d3c32cbd11f0bfd44c5facf6c62173310bf (presubmit successful).

[mesch, 2016-01-29 03:01:15]

mesch@google.com changed reviewers:
+ mesch@google.com

[mesch, 2016-01-29 03:01:16]

Totally tangential drive by nitpick for the record. Feel free to ignore.

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
File services/gfx/compositor/compositor_app.h (right):

https://codereview.chromium.org/1552963002/diff/20001/services/gfx/compositor...
services/gfx/compositor/compositor_app.h:39: mojo::ApplicationImpl* app_impl_;
You can initialize this to nullptr inline if we speak C++11 here
<http://www.stroustrup.com/C++11FAQ.html#member-init>.

And then you don't need the default constructor anymore.