Mozart: Improve tracing and backpressure.

Mozart: Improve tracing and backpressure.

Refactored the GPU Output to clarify the separation of concerns
between the output and rasterizer classes.  GpuOutput maintains
the queue of frames to draw and keeps track of how many frames
are outstanding.  GpuRasterizer manages the GL context and issues
the actual drawing commands.

Simplified the policy for dropping frames so it only happen in
one spot in the compositor.  The compositor maintains a queue
of drawn frames awaiting finish.  When that queue's size exceeds
the maximum allowable pipeline depth (configurable using
the --pipeline-depth argument) a pending frame will be discarded.

Added more trace events to help understand the behavior of the
compositor.

Normalized the nomenclature for different stages in composition.
They are now called...

  - Present: accept and validate scene updates published by apps
  - Snapshot: resolve scene dependencies and capture the state
              of the scene graph for traversal
  - Paint: record drawing commands for a frame (as an SkPicture)
  - Submit: enqueue a frame to be rasterized
  - Draw: rasterize the frame

There's still much to be improved here.

BUG=
R=mikejurka@google.com

Committed: https://chromium.googlesource.com/external/mojo/+/f36e23b2e279ce47c7cd0e2149048dd9b1b5af4e

[jeffbrown, 2016-05-19 00:13:45]

jeffbrown@google.com changed reviewers:
+ abarth@google.com, mikejurka@google.com

[abarth, 2016-05-19 00:49:57]

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.h (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:45: int64_t wait_time;
Should these be base::TimeTicks?

[mikejurka, 2016-05-19 23:26:19]

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:23: constexpr uint32_t
kMaxPipelineDepth = 10u;
why should we ever be more than 3 frames ahead?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:52: << str << "\"";
is there a way to exit/crash here? should we call PostErrorCallback() here?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:74: rasterizer_thread_->Stop();
does ->Stop() block until the thread is done?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:87: scoped_refptr<FrameData>
frame_data(
is there a way we can avoid using a scoped_refptr here (using scoped_ptr
instead) ? 

also, like mentioned before, FrameData already inherits from
RefCountedThreadSafe -- why do we need to wrap it again?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:94: if (frame_data &&
!frame_data->draw_time) {
what does it mean if draw_time is 0? does it mean this frame was not drawn? a
comment would be helpful.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:260: if
(!shared_state_.current_frame_data->draw_time)
again, what does it mean if draw_time is 0? how does that differ from
"presented"?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.h (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:70:
scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
should we be using scoped_refptr or std::unique_ptr across mojo? they seem
equivalent and it seems like we should stick to one or the other, at least
within modules.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:90: scoped_refptr<FrameData>
current_frame_data;
If FrameData already inherits from RefCountedThreadSafe, do we need to wrap it
in yet another scoped_refptr?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_rasterizer.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_rasterizer.cc:105: DrawFinished(false
/*presented*/);
what is */ presented */ ? is there a missing todo here?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
File services/gfx/compositor/compositor_engine.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
services/gfx/compositor/compositor_engine.cc:442: if
(renderer_state->visible_snapshot()) {
could you add a comment explaining this if/else branch? does it have to do with
whether something is ready to be painted or not?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
File services/gfx/compositor/compositor_engine.h (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
services/gfx/compositor/compositor_engine.h:92: void
ComposeRenderer(RendererState* renderer_state,
Compose/Present/Paint all sound relatively similar--- could you write comments
explaining the differences here?

[jeffbrown, 2016-05-20 01:09:48]

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:23: constexpr uint32_t
kMaxPipelineDepth = 10u;
On 2016/05/19 23:26:18, mikejurka wrote:
> why should we ever be more than 3 frames ahead?

This is sort of arbitrary.  The limit only exists for experimentation.  A depth
of 10 would be ludicrous!

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:52: << str << "\"";
On 2016/05/19 23:26:18, mikejurka wrote:
> is there a way to exit/crash here? should we call PostErrorCallback() here?

Hmm.  Not a bad idea.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:74: rasterizer_thread_->Stop();
On 2016/05/19 23:26:18, mikejurka wrote:
> does ->Stop() block until the thread is done?

Yes.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:87: scoped_refptr<FrameData>
frame_data(
On 2016/05/19 23:26:18, mikejurka wrote:
> is there a way we can avoid using a scoped_refptr here (using scoped_ptr
> instead) ? 
> 
> also, like mentioned before, FrameData already inherits from
> RefCountedThreadSafe -- why do we need to wrap it again?

These two things go together.  There's no wrapping involved.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:94: if (frame_data &&
!frame_data->draw_time) {
On 2016/05/19 23:26:18, mikejurka wrote:
> what does it mean if draw_time is 0? does it mean this frame was not drawn? a
> comment would be helpful.

Added a comment to the header file.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:260: if
(!shared_state_.current_frame_data->draw_time)
On 2016/05/19 23:26:18, mikejurka wrote:
> again, what does it mean if draw_time is 0? how does that differ from
> "presented"?

It's set in OnDraw() above.  Basically means DrawFrame was called.  It seemed
expedient to rely on the fact that time ticks are essentially always positive
after boot but I guess it wouldn't hurt to make the flag explicit so I'll do
that for clarity.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.h (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:45: int64_t wait_time;
On 2016/05/19 00:49:56, abarth wrote:
> Should these be base::TimeTicks?

Perhaps.  Technically they are MojoTimeTicks but in the mojom files for
FrameInfo they appear as int64.  It was getting confusing to intermix these
types but perhaps I should be more rigorous.  I'll open an issue to look into it
later.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:70:
scoped_refptr<base::SingleThreadTaskRunner> compositor_task_runner_;
On 2016/05/19 23:26:19, mikejurka wrote:
> should we be using scoped_refptr or std::unique_ptr across mojo? they seem
> equivalent and it seems like we should stick to one or the other, at least
> within modules.

A better substitute for scoped_refptr is std::shared_ptr though the C++
committee made some unfortunate design decisions that make it a bit less
attractive.  That said, we do prefer using std::unique_ptr over scoped_ptr (non
ref-counted) although there are some idiosyncratic cases where scoped_ptr must
be used for interoperation with other things in //base.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.h:90: scoped_refptr<FrameData>
current_frame_data;
On 2016/05/19 23:26:18, mikejurka wrote:
> If FrameData already inherits from RefCountedThreadSafe, do we need to wrap it
> in yet another scoped_refptr?

scoped_refptr is a smart pointer which holds a ref counted pointer so the two go
hand in hand.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_rasterizer.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_rasterizer.cc:105: DrawFinished(false
/*presented*/);
On 2016/05/19 23:26:19, mikejurka wrote:
> what is */ presented */ ? is there a missing todo here?

/*presented*/ is the name of the parameter.  This is a common documentation
convention to help readers understand what is being passed.  I often use it for
hard-coded boolean values where the meaning is unclear from the context.

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
File services/gfx/compositor/compositor_engine.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
services/gfx/compositor/compositor_engine.cc:442: if
(renderer_state->visible_snapshot()) {
On 2016/05/19 23:26:19, mikejurka wrote:
> could you add a comment explaining this if/else branch? does it have to do
with
> whether something is ready to be painted or not?

ok

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
File services/gfx/compositor/compositor_engine.h (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/com...
services/gfx/compositor/compositor_engine.h:92: void
ComposeRenderer(RendererState* renderer_state,
On 2016/05/19 23:26:19, mikejurka wrote:
> Compose/Present/Paint all sound relatively similar--- could you write comments
> explaining the differences here?

Sure!

[mikejurka, 2016-05-20 01:49:23]

we didn't necessarily need to add a "bool drawn" member, i just meant to
document cases where draw_time is used in a boolean expression to make it
clearer. 

as far as FrameData -- i guess i'm not familiar with all the datastructures that
will be holding it, but just wanted to double check if there isn't an easy-ish
way to avoid using a refptr. from the chromium smart pointer guidelines:

"Ref-counted objects - use scoped_refptr<>, but better yet, rethink your design.
Reference-counted objects make it difficult to understand ownership and
destruction order, especially when multiple threads are involved. There is
almost always another way to design your object hierarchy to avoid refcounting."

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:291: : frame(frame),
submit_time(submit_time), draw_time(0), wait_time(0) {}
shouldn't we still initialize these guys to 0?

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

https://codereview.chromium.org/1995873002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.h:44: bool drawn = false;     // set
when DrawFrame is called
this works. but it wasn't my intention to force you to add a boolean, just to
add comments when you checked draw_time in a boolean expression.

[jeffbrown, 2016-05-20 19:31:36]

can haz lgtm?

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
File services/gfx/compositor/backend/gpu_output.cc (right):

https://codereview.chromium.org/1995873002/diff/1/services/gfx/compositor/bac...
services/gfx/compositor/backend/gpu_output.cc:291: : frame(frame),
submit_time(submit_time), draw_time(0), wait_time(0) {}
On 2016/05/20 01:49:23, mikejurka wrote:
> shouldn't we still initialize these guys to 0?

It's done in the header file.

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

https://codereview.chromium.org/1995873002/diff/20001/services/gfx/compositor...
services/gfx/compositor/backend/gpu_output.h:44: bool drawn = false;     // set
when DrawFrame is called
On 2016/05/20 01:49:23, mikejurka wrote:
> this works. but it wasn't my intention to force you to add a boolean, just to
> add comments when you checked draw_time in a boolean expression. 

It's fine.  Technically this is better anyhow since a zero timestamp could
hypothetically occur though in general the time ticks are anchored to 0 at boot
time.

[mikejurka, 2016-05-20 19:40:31]

lgtm

[mikejurka, 2016-05-20 19:41:17]

but would appreciate a comment on whether we absolutely need shared_ptrs for
FrameData

[mikejurka, 2016-05-20 19:42:26]

On 2016/05/20 19:41:17, mikejurka wrote:
> but would appreciate a comment on whether we absolutely need shared_ptrs for
> FrameData

or rather, whether we absolutely need scoped_refptrs as opposed to a design
using scoped_ptr

[jeffbrown, 2016-05-20 20:17:46]

Description was changed from

==========
Mozart: Improve tracing and backpressure.

Refactored the GPU Output to clarify the separation of concerns
between the output and rasterizer classes.  GpuOutput maintains
the queue of frames to draw and keeps track of how many frames
are outstanding.  GpuRasterizer manages the GL context and issues
the actual drawing commands.

Simplified the policy for dropping frames so it only happen in
one spot in the compositor.  The compositor maintains a queue
of drawn frames awaiting finish.  When that queue's size exceeds
the maximum allowable pipeline depth (configurable using
the --pipeline-depth argument) a pending frame will be discarded.

Added more trace events to help understand the behavior of the
compositor.

Normalized the nomenclature for different stages in composition.
They are now called...

  - Present: accept and validate scene updates published by apps
  - Snapshot: resolve scene dependencies and capture the state
              of the scene graph for traversal
  - Paint: record drawing commands for a frame (as an SkPicture)
  - Submit: enqueue a frame to be rasterized
  - Draw: rasterize the frame

There's still much to be improved here.

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

to

==========
Mozart: Improve tracing and backpressure.

Refactored the GPU Output to clarify the separation of concerns
between the output and rasterizer classes.  GpuOutput maintains
the queue of frames to draw and keeps track of how many frames
are outstanding.  GpuRasterizer manages the GL context and issues
the actual drawing commands.

Simplified the policy for dropping frames so it only happen in
one spot in the compositor.  The compositor maintains a queue
of drawn frames awaiting finish.  When that queue's size exceeds
the maximum allowable pipeline depth (configurable using
the --pipeline-depth argument) a pending frame will be discarded.

Added more trace events to help understand the behavior of the
compositor.

Normalized the nomenclature for different stages in composition.
They are now called...

  - Present: accept and validate scene updates published by apps
  - Snapshot: resolve scene dependencies and capture the state
              of the scene graph for traversal
  - Paint: record drawing commands for a frame (as an SkPicture)
  - Submit: enqueue a frame to be rasterized
  - Draw: rasterize the frame

There's still much to be improved here.

BUG=
R=mikejurka@google.com

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

[jeffbrown, 2016-05-20 20:17:48]

Committed patchset #2 (id:20001) manually as
f36e23b2e279ce47c7cd0e2149048dd9b1b5af4e (presubmit successful).