Add two demos which exercise the audio server.

examples/audio_play_test/play_tone.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+
+#include "base/bind.h"
+#include "base/logging.h"
+#include "mojo/public/c/system/main.h"
+#include "mojo/public/cpp/application/application_delegate.h"
+#include "mojo/public/cpp/application/application_impl.h"
+#include "mojo/public/cpp/application/application_runner.h"
+#include "mojo/public/cpp/utility/run_loop.h"
+#include "mojo/services/media/audio/interfaces/audio_server.mojom.h"
+#include "mojo/services/media/audio/interfaces/audio_track.mojom.h"
+#include "mojo/services/media/common/cpp/circular_buffer_media_pipe_adapter.h"
+#include "mojo/services/media/common/cpp/linear_transform.h"
+#include "mojo/services/media/common/cpp/local_time.h"
+#include "mojo/services/media/common/interfaces/rate_control.mojom.h"
+
+namespace mojo {
+namespace media {
+namespace audio {
+namespace examples {
+
+static constexpr uint32_t SAMP_FREQ = 48000;
+static constexpr uint32_t CHUNK_USEC = 1000;
+static constexpr uint32_t BUF_LO_WATER_USEC = 50000;
+static constexpr uint32_t BUF_HI_WATER_USEC = BUF_LO_WATER_USEC
+                                            + (4 * CHUNK_USEC);
+static constexpr uint32_t BUF_DEPTH_USEC    = BUF_HI_WATER_USEC
+                                            + (4 * CHUNK_USEC);

[jeffbrown, 2015/11/04 19:34:43]

Is there anything significant about the fact the the constant 4 appears in both
of these definitions?

[johngro, 2015/11/05 00:25:16]

not really.  There needs to be some distance between the high and low water
mark, and there should be some distance between high water and full,
particularly if we want to move in the direction of a feed-forward timing driven
system for maintaining buffer levels instead of a full/empty water-mark driven
system.

When I was using this code to exercise the server during initial development, I
just picked some numbers which were not 1 or 2 so I would get some bursty
behavior when it came to waking up and re-buffering.

+static constexpr uint32_t FRAME_BYTES = 2;

[jeffbrown, 2015/11/04 19:34:43]

sizeof(uint16_t)?

[johngro, 2015/11/05 00:25:15]

sure; but
<pedantic>
sizeof(int16_t) since this is the actual type of the sample format being used.
</pedantic>

More seriously, however.  A better (or at least) more formal version of this
would not have the frame size as a constexpr.  Instead, it would use the
LpcmSampleFormat enum from media_types.mojom to spec what sample types should be
produced.  Issues like how to generate the tone for the given sample type
parameter, and what the resulting frame size would be should flow from this
parameter (as well as the channel count, also assumed to be 1 in this example
code).  This would then allow the example to be converted from hard coded
parameters to something command line driven so that different formats could be
easily switched between for the purposes of exercising the mixer various
different ways.

This was just a quick example, however, so there is a lot of hardcodin' going
on.  I'll switch this to sizeof for now; depending on how much I use this
particular demo for exercising the mixer, I may come back later and give it a
more formal treatment.

+
+static inline constexpr uint32_t USecToBytes(uint64_t usec) {
+  return ((usec * SAMP_FREQ) / 1000000) * FRAME_BYTES;
+}
+
+class PlayToneApp : public ApplicationDelegate {
+ public:
+  void Initialize(ApplicationImpl* app) override;
+
+ private:
+  bool GenerateToneCbk(MediaResult res);
+  void PlayTone(double freq_hz, double amplitude, double duration_sec);
+  void Cleanup();
+
+  AudioServerPtr audio_server_;
+  AudioTrackPtr  audio_track_;
+  RateControlPtr rate_control_;
+  std::unique_ptr<CircularBufferMediaPipeAdapter> pipe_;
+
+  bool     clock_started_ = false;
+  uint64_t media_time_    = 0;
+  double   freq_hz_       = 440.0;
+  double   amplitude_     = 1.0;
+};
+
+void PlayToneApp::Initialize(ApplicationImpl* app) {
+  MediaResult result = MediaResult::UNKNOWN_ERROR;
+
+  app->ConnectToService("mojo:audio_server", &audio_server_);
+  audio_server_->CreateTrack(GetProxy(&audio_track_));
+
+  do {

[jeffbrown, 2015/11/04 19:34:43]

This construct with early breaks is a little hard to follow.  Might be clearer
to refactor this block out to its own function.

[johngro, 2015/11/05 00:25:16]

yeah... Classically, I would use gotos for this pattern, but they tend to be
frowned upon which is why I messed around with the do/while(false) pattern.  The
do/while(false) pattern also avoids the silly issue of the compiler warning
which gets generated when you do a single line initialization.

<rant>
Seriously... what is up with this?  I have never figured it out.  Two
examples...

int foo() {
  if (bar()) { goto bailout; }

  int my_param = baz();
  foobar(my_param);

  return 0;
bailout:
  return -1;
}

And

int foo() {
  if (bar()) { goto bailout; }

  int my_param;
  my_param = baz();
  foobar(my_param);

  return 0;
bailout:
  return -1;
}


In the first case, the compiler will give you an error that the goto crosses the
initialization of my_param.  Specifically, clang (gcc/g++ will do the same,
different wording) will say... 

"error: cannot jump from this goto statement to its label"
then
"note: jump bypasses variable initialization"

I think this is pretty silly, because the compiler's live-ness analysis knows
goddamn well that my_param is a dead variable once you reach bailout.  Even so,
I might be able to understand provided that there were some consistency.

Enter case #2.  Exactly the same as #1 in terms of the code to be produced,
except that I have declared the variable, then initialized it in a separate
statement.  No error for this, not even a warning.  Why?  Because the compiler
knows that the variable is dead and has no destructor, so it does not matter!

Extra credit; change the bailout to return my_param.  Now clang gives you an
uninitialized variable error, but does not tell you the reason why (because the
code was reached via the jump).
</rant>

Gak...
With that out of my system...  Breaking this into functions introduces some
issues around managing local variables (not insurmountable ones, but still
things which need to be dealt with), but does not really address the fundamental
issue; it just minimizes the size of the do/while block.  If I fail to fetch the
sink descriptor, I still need to execute the cleanup steps before exiting.  So
either end up with something like what I had or, something more like...

if (!DoCriticalTask1()) {
  Cleanup();
  return ERROR;
}
...
if (!DoCriticalTaskN()) {
  Cleanup();
  return ERROR;
}

return SUCCESS;

The issue here is the cut-n-paste repetition of the cleanup code.  In this case,
I am able to wrap this all up in destructor side effects and the Cleanup
function, so its not the end of the world, but its still annoying from a pattern
perspective.

try/catch/finally is the classic purist's approach to these pattern problems,
but we don't do exceptions in our world (a decision which I support FWIW, but
feel the most when I write code like this).

Formally, given all of the icky calls to Wait, this should really be broken into
an async pattern with callbacks which are methods in the class.  In addition to
making the code harder to follow as a sequence of critical steps (IMO), this can
have alternate negative side effect of forcing you to carry your temporary state
around as class members (although I suppose that in this case, I may be able to
bind them all to lamdas, I have not really though too much about it).

Finally, I can take you suggestion and put the entire do/while block into a
function.  This will work, but has the strange feeling of wrapping the important
sequence of steps up into a a separate function just to centralize the cleanup
code.  In addition, if destructor side effects were not handling a lot of the
cleanup (they are in this case, but not in all code confronted with this
pattern), then you still have to manage cleaning up your locals/temps.

TL;DR?
The proper pattern here is exception handling.  Were it not banned, it would be
what I used here.  All of the other options can work, but they all have their
individual strangeness-es when it comes to no-cut-and-paste cleanup code (not
all of it relevant in this instance, but in other real world cases).

I'm going to go with the if (!DoImportantStep()) { Cleanup; return; } pattern
and eliminate the do/while construct for now as the compromise. 

+    // Query the sink's format capabilities.
+    AudioTrackDescriptorPtr sink_desc;
+    auto desc_cbk = [&sink_desc](AudioTrackDescriptorPtr desc) {
+      sink_desc = desc.Pass();
+    };
+    audio_track_->Describe(AudioTrack::DescribeCallback(desc_cbk));
+
+    // TODO(johngro): this pattern is awkward.  We really don't want to be
+    // calling WaitForIncomingResponse, even if we were able supply a timeout.
+    // The best practice would be to defer to a handler for the message we are
+    // expecting to eventually come back.
+    //
+    // But... what if the message never comes back?  Perhaps the service is not
+    // implemented properly, or perhaps the service is malicious.  We could
+    // queue a delayed message on our run loop which indicates a timeout, but
+    // then what happens when when the response to Describe comes back (as
+    // expected).  We don't really have a good way to cancel the "timeout"
+    // message once we have queued it.  Maintaining all of the bookkeeping
+    // required to nerf the callback when it happens and is discovered to be
+    // useless is going to get very old, very fast.
+    //
+    // For now, we just do the evil thing and block during init, but I sure do
+    // wish there was something nicer we could do.
+    if (!audio_track_.WaitForIncomingResponse()) {

[jeffbrown, 2015/11/04 19:34:43]

The pattern you're looking for here is to do two things:

1. Pass a callback for a function to continue the work after the describe
function completes.
2. Set a connection error handler to clean up in case the pipe gets closed.

No clean way to handle timeouts which is a bit annoying.  See
https://github.com/domokit/mojo/issues/485

[johngro, 2015/11/05 00:25:16]

Acknowledged.

We should talk sometime about this.  In addition to this, I feel that this
pattern is something which can become burdensome to developers in general. 
While a fully async programming model can force developers to explicitly
recognize points in code where blocking can happen and yield back to a central
framework, it does not force them to handle errors properly from all states.  I
think the place where this shows up the most is patterns like this, where
developers need to break things up into a sequence of states punctuated with
yield points, every one of which has an edge to the next state, as well as to an
error state.  Basically...

A->B->C->...->Z->Success

where all of the letters also have an edge to Failure.

Forcing developers to explicitly recognize their yield points is good in
principal, but introduces other hazards and readability issues as well (not
going into detail here, I'm ranting way too much already).  It would be
fantastic if we could provide some C/C++ specific patterns which would allow
users to easily follow the model, but also program in a sequential style even
when the code is actually reduced to a co-operative multitasking pattern under
the hood.  This (IMO) is a generally difficult problem.  While I have some ideas
of things which could be done, I'm not sure that they are good ideas or that
they should be done.

TL;DR
I agree, we should talk about ways to make this nicer for people.

+      LOG(ERROR) << "Failed to fetch sync capabilities; no response received.";
+      break;
+    }
+
+    // TODO(johngro): do something useful with our capabilities description.
+    sink_desc.reset();
+
+    // Grab the rate control interface for our audio renderer.
+    auto get_rc_cbk = [&result](MediaResult res) { result = res; };
+    audio_track_->GetRateControl(GetProxy(&rate_control_), get_rc_cbk);
+    if (!audio_track_.WaitForIncomingResponse()) {

[jeffbrown, 2015/11/04 19:34:43]

This is our second WaitForIncomingResponse.

I think in practice we can avoid a lot of this by designing the API to be
friendlier for pipelining.  In this case there's actually no reason to wait
since we can already start stuffing the pipe with data.

It's not clear what errors we care about here.  (Why should retrieving this
interface ever fail?)  Notice that trying to handle errors synchronously is very
painful in Mojo.  Embracing asynchronous error reporting (like connection error
handlers) will make it much simpler.

(Get rid of those MojoResults as much as you can!)

[johngro, 2015/11/05 00:25:16]

Ack.

Re: "why should retrieving this interface every fail?".  Faulty (or malicious)
implementation.  IMO - if you exit the scope of a service's method which has a
client's interface request, and you have not bound the interface to something,
that should be fatal for your connection.  Right now, you end up with a
dangling, half formed connection.  Calls made by the client on the interface
will hang forever; they will not receive a connection failure and cannot timeout
because of the current nature of the framework.

Currently, there is nothing I can do about this.  I return the error code
because there may be a case where the track has not been configured to the point
where it is able to give back a functioning rate control interface (I'm not even
sure if this is the case in the current implementation anymore) and returning an
un-bound interface, or adding a bunch of statefullness to the rate control
interface would be a bad thing as well.  Even so, the error code does not
mitigate against the poorly written/malicious app which returns OK in the
callback, but fails to bind the interface.

Really, we need what you and I have already discussed.  The ability to return
some form of diagnostic with a connection closed event, and then just hang up
the phone.  In addition, I think (at the moment at least) we should make it an
automatic connection close event if a caller requests an interface and the
service does not immediately bind the request to something.  I'm not sure I see
the advantage to allowing very-delayed-binding, and there are a number of issues
which lead to nasty timeout situations which would need to be dealt with if we
continue to allow this behavior.

For now, I'm leaving this, but I'm adding it to my growing list of
design/cleanup tasks that we need to address.

+      LOG(ERROR) <<
+        "Failed to fetch rate control interface; no response received.";
+      break;
+    }
+    if (result != MediaResult::OK) {
+      LOG(ERROR) << "Failed to get rate control interface.  (res = "
+                 << result << ")";
+      break;
+    }
+
+    // Configure our sink for 16-bit 48KHz mono.
+    AudioTrackConfigurationPtr cfg = AudioTrackConfiguration::New();
+    cfg->max_frames = USecToBytes(BUF_DEPTH_USEC) / FRAME_BYTES;
+
+    LpcmMediaTypeDetailsPtr pcm_cfg = LpcmMediaTypeDetails::New();
+    pcm_cfg->sample_format     = LpcmSampleFormat::SIGNED_16;
+    pcm_cfg->samples_per_frame = 1;
+    pcm_cfg->frames_per_second = SAMP_FREQ;
+
+    cfg->media_type = MediaType::New();
+    cfg->media_type->scheme = MediaTypeScheme::LPCM;
+    cfg->media_type->details = MediaTypeDetails::New();
+    cfg->media_type->details->set_lpcm(pcm_cfg.Pass());
+
+    MediaPipePtr pipe;
+    {
+      auto cbk = [&result](MediaResult res) {
+        result = res;
+      };
+      audio_track_->Configure(cfg.Pass(), GetProxy(&pipe), cbk);
+    }
+
+    if (!audio_track_.WaitForIncomingResponse()) {

[jeffbrown, 2015/11/04 19:34:43]

We already have the pipe so we can start using it immediately.  No need to wait
for the callback response.

If the request fails, then the audio service can simply close the pipe on us. 
So for error handling it's just a matter of attaching a connection error handler
to the pipe (which we'll need anyhow).

[johngro, 2015/11/05 00:25:16]

See above.  I will switch to this pattern, and re-write all of the interfaces
here, just as soon as I can provide diagnostic information as to why the
connection was closed.  I want to land this chain of CLs first.

+      LOG(ERROR) << "Failed to configure sink; no response received.";
+      break;
+    }
+
+    if (result != MediaResult::OK) {
+      LOG(ERROR) << "Failed to configure sink.  (res = "
+                 << result << ")";
+      break;
+    }
+
+
+    // Now that we are configured and have our media pipe, pass its interface to
+    // our circular buffer helper, set up our high/low water marks, register our
+    // callback, and start to buffer our audio.
+    pipe_.reset(new CircularBufferMediaPipeAdapter(pipe.Pass()));
+    pipe_->SetSignalCallback(

[jeffbrown, 2015/11/04 19:34:43]

Oddly enough base::Bind is cleaner for stuff like this.  But if you're going to
use lambdas, I would say you should use them consistently throughout the file. 
So this is fine.

[johngro, 2015/11/05 00:25:16]

Acknowledged.

base::Bind has its own issues.  Without going into too many of them the
important one here is that if this is code which is supposed to be published as
an example of how a 3rd party should use our media interfaces, there should be
no dependencies on base::.  James has already made this clear to me.

In light of that, I'm going to remove my dependencies on base from both of these
demos, and switch to using the MOJO_ versions of logging and DCHECK.

+      [this](MediaResult res) -> bool {

[jeffbrown, 2015/11/04 19:34:43]

Note the inherent assumption that "this" will outlive the pipe.  That's true
because the pipe is being stored as a member.  So you're good here.

[johngro, 2015/11/05 00:25:16]

Indeed.  This is one of the other issues I have with base::Callbacks.  It really
encourages you to make all of your callback target objects ref-counted, then
fire off all sorts of callbacks which hold strong references to the object
itself.  IMO, this is a lifecycle nightmare.

I would much rather have a weak_ptr model which auto nerfs the callback if its
target object has gone away.  base:: provides this, but warns you in no
uncertain terms that their weak reference implementation is not threadsafe
(unlike std::weak_ptr), which just presents a whole new set of invariants which
need to be demonstrated in your code.

Finally, base::Bind and base::Callback provide a way to bind to the naked this
pointer, but make you feel like a Bad Person (IMO) for doing so when you do it
(you need to pass base::Unretained(this) instead of passing a scoped_refptr
reference to this).

Complexity like this is just an inherent side effect of using a highly
asynchronous callback pattern with a non-garbage collected language like C++. 
As you note, encapsulation should get me out of this situation (although
technically, cannot be demonstrated... a bad framework implementation could
technically fire my callback even after the pipe has been destroyed; the proof
that this cannot happen is non-obvious, at least to me)

+        return GenerateToneCbk(res);
+      });
+    pipe_->SetWatermarks(USecToBytes(BUF_HI_WATER_USEC),
+                         USecToBytes(BUF_LO_WATER_USEC));
+    result = MediaResult::OK;
+  } while (false);
+
+  if (result != MediaResult::OK) {
+    Cleanup();
+  }
+}
+
+bool PlayToneApp::GenerateToneCbk(MediaResult res) {
+  using MappedPacket = CircularBufferMediaPipeAdapter::MappedPacket;
+  MappedPacket mapped_pkt;
+
+  DCHECK_GT(freq_hz_, 0.0);
+  DCHECK_GE(amplitude_, 0.0);
+  DCHECK_LE(amplitude_, 1.0);
+
+  if (res != MediaResult::OK) {

[jeffbrown, 2015/11/04 19:34:43]

As we discussed before, it would be more Mojo-like to just close the pipe.  Then
you will only need the connection error handler to handle all of these errors.

Note that you *still* need a connection error handler here (but don't have one)
because the pipe could be closed on you and you will have to clean up.  So
adopting a "shoot the pipe in the head" mentality leads to more consistent error
handling logic on the client side.

[johngro, 2015/11/05 00:25:16]

As we discussed before, I will clean all of this up with a smile on my face as
soon as the framework provides a proper channel for diagnostic/debugging error
propagation.  A proper timeout mechanism is a nice to have as well.  For now,
its going on my TODO list.

+    LOG(ERROR) << "Fatal error in cbuf (" << res << ").";
+    goto error;

[jeffbrown, 2015/11/04 19:34:43]

Not sure what the style guide has to say about gotos but I imagine it's nothing
good.  You could just as easily return false and have the caller invoke
Cleanup() when that happens.

[johngro, 2015/11/05 00:25:16]

The external style guide actually sidesteps the issue completely.  It talks
about goto in only one place where it says...

"""
Style rules should pull their weight
The benefit of a style rule must be large enough to justify asking all of our
engineers to remember it. The benefit is measured relative to the codebase we
would get without the rule, so a rule against a very harmful practice may still
have a small benefit if people are unlikely to do it anyway. This principle
mostly explains the rules we don’t have, rather than the rules we do: for
example, goto contravenes many of the following principles, but is already
vanishingly rare, so the Style Guide doesn’t discuss it.
"""

IMO - they go out of their way to slam goto and dismiss it as something that "no
one does", but provide no good alternatives to the pattern.  See my *extensive*
rant about issues surrounding the sequential init/unwind pattern given above. 
Also, if they think that goto is unusual, they should spend more time in low
level system/kernel code where the pattern is common (and not universally
incorrect, as is a common snarky response from folks who do not do a lot of this
style of programming).

I'm going to respond with the conclusion of that rant.  The real answer to this
pattern is try/catch/finally, but this pattern is banned (for other good
reasons).  I will resolve the issue the same way that I did before which is to
duplicate the cleanup/return false handler at the end.  All of the non-exception
solutions suck, this one just prefers cutting-and-pasting hazards to avoidance
of a useful keyword which people have an irrational fear of because of the harm
it could do (but does not in this application).

+  }
+
+  while (!pipe_->AboveHiWater()) {
+    res = pipe_->CreateMediaPacket(USecToBytes(CHUNK_USEC),
+                                   false,
+                                   &mapped_pkt);
+    if (res != MediaResult::OK) {

[jeffbrown, 2015/11/04 19:34:43]

Why would this ever fail?  Is it DCHECK worthy?

[johngro, 2015/11/05 00:25:16]

Please refer to the documentation present for this method in
circular_buffer_media_pipe_adapter.h.

Seriously... I provide 28 lines of documentation including an exhaustive list of
the error codes which may be returned as well as the reasons why they might be
returned, in a doxygen compatible format.  If the change had been checked in
already (this is patchset 7/7 in the chain, the code which introduced the method
is 1/7 in the chain), and we had automatic documentation generation as part of
the build, I would refer you to the automatically generated docs as to the
possible reasons this method might fail.

re: should we DCHECK?  Perhaps.  From a style standpoint, I am a generally fan
of an orderly shutdown whenever reasonable, particularly when interacting with
the behavior of someone else's code (which, from the perspective of this demo
code, the adapter's utility code is).  DCHECKs should generally be only for
things which  are truly invariants of the code sitting in front of you, and then
usually when the invariant is relatively simple to show.

In this case...
Given the currently documented ways for this code to return an error, I do not
expect this to fail, which is why I log an error and attempt a clean shutdown. 
This code exists outside of this project, however, and could change in the
future.  If future behavior introduces a new way this might legitimately fail, I
defend my decision to have this code log an error and cleanly shutdown instead
of shooting itself for a perceived violation of an invariant that it cannot (or
should not) actually assert.

+      LOG(ERROR) << "Unexpected error when creating media packet ("
+                 << res << ").";
+      goto error;
+    }
+
+    mapped_pkt.packet()->pts = media_time_;

[jeffbrown, 2015/11/04 19:34:43]

Avoid abbreviations.  Prefer "presentation_time".  I know it's annoying...

Also, should mapped_pkt even expose the underlying packet structure?  I see we
have accessors for data() and length() below so we could simply offer a
set_presentation_time() method too then hide the packet pointer itself.

[johngro, 2015/11/05 00:25:16]

I responded to this already, 6 patch sets ago. As I noted back then, PTS is a
term which is...
1) common in this context.
1a) I can haul out multiple industry standard which use this term all over the
place.
1b) I can also also point to popular media APIs which use this term as well.
2) Defined in the top level documentation for the media APIs (we have a glossary
section which defines this term in the Motown docs).  FWIW - so does the
docs/specs that I would bring out for 1a and 1b.
3) Defined by the comments around this code.
4) Not made intrinsically more understandable by the expansion to the phrase
presentation_time or even presentation_timestamp.  If you don't know what a pts
is, you are unlikely to know what a presentation_time is either.  You should
probably be starting with the top level documentation in either case if you are
not familiar with the term.

I can make the same argument for the abbreviation VBLANK in a video compositor,
or R146/C192 in an electrical schematic.  If you don't already know, you are
going to need more than just an expansion of the abbreviation to be comfortable
with the material.

Finally, the place for suggestions (such as implementing a class/accessory
pattern instead of a struct style pattern) is in the CL which introduces the
structure, not 6 changes down the line where the code is being used.  This is
doubly true when that patch set is still out for review (which it is).

If you really want to push for this, please do it in the appropriate patch set,
not here.  Making the comment here is akin to someone checking in code which
fills out the pts field an FFMpeg structure and telling them that FFMpeg should
have named that field something else.  Perhaps, but this is not the place to
make that argument.

+
+    for (uint32_t i = 0; i < MappedPacket::kMaxRegions; ++i) {
+      int16_t* data = reinterpret_cast<int16_t*>(mapped_pkt.data(i));

[jeffbrown, 2015/11/04 19:34:43]

Would it make sense to embed the data type into MappedPacket itself as a
convenient?

When we create the pipe adapter thingy, we already know what kind of data we're
going to be transferring through these buffers.  So we could parameterize the
type with this knowledge.  If the data is heterogeneous then the client could
use uint8_t and reinterpret_cast as seen here.  But frequently the data will be
homogeneous.

[johngro, 2015/11/05 00:25:16]

no, I don't think so.  If it would make you feel better, I could templatize data
so you have to say data<uint8_t>, or data<int16_t>, but I don't think that I see
the advantage.

Regardless, if we decide that this is the proper way to go, the comment belongs
either as a bug/issue, or as a comment for the CL which defines the structure,
not the one which uses is.

+      uint64_t len = mapped_pkt.length(i);
+      if (!data) continue;

[jeffbrown, 2015/11/04 19:34:43]

swap the above two lines

[johngro, 2015/11/05 00:25:16]

Done.

+
+      DCHECK(len && !(len % FRAME_BYTES));
+      len /= FRAME_BYTES;
+      for (uint64_t i = 0; i < len; ++i, ++media_time_) {
+        double tmp = ((M_PI * 2.0) / SAMP_FREQ) * freq_hz_ * media_time_;
+        data[i] = std::numeric_limits<int16_t>::max() * amplitude_ * sin(tmp);
+      }
+    }
+
+    res = pipe_->SendMediaPacket(&mapped_pkt);

[jeffbrown, 2015/11/04 19:34:43]

Is this blocking behind the scenes?

Kind of annoying to have to check all of these things.  If the error is bad
arguments (malformed packet) then we could DCHECK.  If the error is that the
remote end didn't accept the packet then it can just close the pipe and our
connection error handler will deal with it.

[johngro, 2015/11/05 00:25:15]

no; this is interacting with a client side helper class, not the actual mojo
interface, and it will not block.  If you want to get the result of the
SendPacket operation on the underlying pipe interface itself, you can pass a
callback along with the call to send.  The local error code is about local
state.

I suppose this is another request for the framework.  If I (the client) decide
to close the pipe myself due to some error...
1) My connection closed callback should still fire.
2) I should be able to provide the diagnostic reason for closing the connection
which is supplied to the connection closed callback I have registered.

Right now...
1) The closed callback will not fire until I take my next action and discover
that my pipe has been closed... either by myself, or some other piece of code
which has the ability to close my connection (perhaps some library code, or some
framework code).
2) There is no ability to provide any diagnostic info, regardless of which side
closes the connection.

+    if (res != MediaResult::OK) {
+      LOG(ERROR) << "Unexpected error when sending media packet ("
+                 << res << ").";
+      pipe_->CancelMediaPacket(&mapped_pkt);
+      goto error;
+    }
+  }
+
+  if (!clock_started_) {
+    // In theory, this could be done at compile time using std::ratio, but
+    // std::ratio is prohibited.
+    LinearTransform::Ratio audio_rate(SAMP_FREQ, 1);
+    LinearTransform::Ratio local_time_rate(LocalDuration::period::num,
+                                           LocalDuration::period::den);
+    LinearTransform::Ratio rate;
+    bool success = LinearTransform::Ratio::Compose(local_time_rate,
+                                                   audio_rate,
+                                                   &rate);
+    DCHECK(success);  // assert that there was no loss of precision.
+
+    LOG(INFO) << "Setting rate " << rate;
+
+    rate_control_->SetRate(rate.numerator, rate.denominator);
+    clock_started_ = true;
+  }
+
+  return true;
+
+error:
+  Cleanup();
+  return false;
+}
+
+void PlayToneApp::Cleanup() {
+  audio_track_.reset();
+  audio_server_.reset();
+  RunLoop::current()->Quit();
+}
+
+}  // namespace examples
+}  // namespace audio
+}  // namespace media
+}  // namespace mojo
+
+MojoResult MojoMain(MojoHandle app_request) {
+  mojo::ApplicationRunner runner(new mojo::media::audio::examples::PlayToneApp);
+  return runner.Run(app_request);
+}