Pass delay and timestamp to AudioSourceCallback::OnMoreData.

media/audio/audio_output_controller.cc

 // Copyright (c) 2012 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 "media/audio/audio_output_controller.h"
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <limits>
 
 #include "base/bind.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/task_runner_util.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 
@@ skipping 266 matching lines @@
   if (stream_ != diverting_to_stream_)
     OnDeviceChange();
 }
 
 void AudioOutputController::DoReportError() {
   DCHECK(message_loop_->BelongsToCurrentThread());
   if (state_ != kClosed)
     handler_->OnError();
 }
 
-int AudioOutputController::OnMoreData(AudioBus* dest,
-                                      uint32_t total_bytes_delay,
-                                      uint32_t frames_skipped) {
+int AudioOutputController::OnMoreData(base::TimeTicks target_playout_time,
+                                      int prior_frames_skipped,
+                                      AudioBus* dest) {
   TRACE_EVENT0("audio", "AudioOutputController::OnMoreData");
 
   // Indicate that we haven't wedged (at least not indefinitely, WedgeCheck()
   // may have already fired if OnMoreData() took an abnormal amount of time).
   // Since this thread is the only writer of |on_more_io_data_called_| once the
   // thread starts, its safe to compare and then increment.
   if (base::AtomicRefCountIsZero(&on_more_io_data_called_))
     base::AtomicRefCountInc(&on_more_io_data_called_);
 
   sync_reader_->Read(dest);
 
+  const base::TimeDelta delay = target_playout_time - base::TimeTicks::Now();

[DaleCurtis, 2016/07/27 18:25:55]

Hmm, if you don't pipe this through to the renderer I don't understand the point
of this change. Or are you really saying that you actually see noticeable
differences between the output device and this point? I can't imagine more than
a handle of microseconds have passed prior to this point.

[jameswest, 2016/07/27 23:07:50]

We don't use this renderer for Cast multizone audio playback. My only goal with
this CL is to change the AudioOutputStream API so that our multizone code can
use CastAudioOutputStream instead of using the underlying media pipeline backend
directly. In order to do that, we need to pass the accurate timing data from the
media pipeline backend up to our multizone code through CastAudioOutputStream. I
was trying to change the API without changing any existing behavior in Chromium.

[DaleCurtis, 2016/07/28 01:21:47]

I thought this API was still used with WebAudio? Or is that unsupported on Cast
still?

[jameswest, 2016/07/28 02:09:39]

It's not used for multizone audio playback, but I can't say for certain that
this isn't used at all on any Cast device because some audio that isn't
synchronized across devices is output differently. If we do use it for
something, improving the accuracy isn't important.

I'm not opposed to passing target_playout_time into the renderer if you think
it's best to, but we don't need it for our use case.

[chcunningham, 2016/07/29 01:21:09]

Should we be gaurding against negative values here. delay = max(math above, 0)? 

Miu, Dale and I had conversations about negative delay in the past. We never
really reached consensus, but Dale/my position is that it isn't reasonable. See
here https://codereview.chromium.org/2004283002/

I think that small values of negative delay may creep in without a guard for
output streams that don't have delay (and use an earlier call to Now() for their
target_playout_time) - see my comments in web_contents_audio_muter.cc

[miu, 2016/07/30 21:17:02]

If negative delay creeps in, for output streams where it does not make sense,
that would indicate that there are bugs in the code (e.g., erroneous time math,
rounding errors, etc.). Do we want a guard, which might mask an underlying
problem? Perhaps the guard should be in the form of a DCHECK instead?

[jameswest, 2016/08/26 02:08:47]

There are some cases where the delay will be negative because we calculated a
delay of zero and passed TimeTicks::Now to OnMoreData and the clock has moved
forward by at least one tick. Alternatively, we could pass TimeTicks() with a
zero value and treat that as a sentinel value indicating no delay. If we use
TimeTicks::Now, we'll have to tolerate negative values and guard against them.

[chcunningham, 2016/08/27 00:36:18]

I'm fine with either approach. I slightly favor the current approach.

+  const int total_bytes_delay = delay.InSeconds() * params_.GetBytesPerSecond();
   const int frames = dest->frames();
   sync_reader_->UpdatePendingBytes(
-      total_bytes_delay + frames * params_.GetBytesPerFrame(), frames_skipped);
+      total_bytes_delay + frames * params_.GetBytesPerFrame(),
+      prior_frames_skipped);
 
   bool need_to_duplicate = false;
   {
     base::AutoLock lock(duplication_targets_lock_);
     need_to_duplicate = !duplication_targets_.empty();
   }
   if (need_to_duplicate) {
-    const base::TimeTicks reference_time =
-        base::TimeTicks::Now() +
-        base::TimeDelta::FromMicroseconds(base::Time::kMicrosecondsPerSecond *
-                                          total_bytes_delay /
-                                          params_.GetBytesPerSecond());
     std::unique_ptr<AudioBus> copy(AudioBus::Create(params_));
     dest->CopyTo(copy.get());
     message_loop_->PostTask(
         FROM_HERE,
         base::Bind(&AudioOutputController::BroadcastDataToDuplicationTargets,
-                   this, base::Passed(&copy), reference_time));
+                   this, base::Passed(&copy), target_playout_time));
   }
 
   if (will_monitor_audio_levels())
     power_monitor_.Scan(*dest, frames);
 
   return frames;
 }
 
 void AudioOutputController::BroadcastDataToDuplicationTargets(
     std::unique_ptr<AudioBus> audio_bus,
@@ skipping 165 matching lines @@
   DCHECK(message_loop_->BelongsToCurrentThread());
 
   // If we should be playing and we haven't, that's a wedge.
   if (state_ == kPlaying) {
     UMA_HISTOGRAM_BOOLEAN("Media.AudioOutputControllerPlaybackStartupSuccess",
                           base::AtomicRefCountIsOne(&on_more_io_data_called_));
   }
 }
 
 }  // namespace media