Pass delay and timestamp to AudioSourceCallback::OnMoreData.

media/audio/mac/audio_auhal_mac.cc

Index: media/audio/mac/audio_auhal_mac.cc
diff --git a/media/audio/mac/audio_auhal_mac.cc b/media/audio/mac/audio_auhal_mac.cc
index c9134023924e8b74ecffd7346b304d0dccd64f96..d45d58459ea2fcf3b983bff426b386f89cdb5a98 100644
--- a/media/audio/mac/audio_auhal_mac.cc
+++ b/media/audio/mac/audio_auhal_mac.cc
@@ -6,13 +6,15 @@
 
 #include <CoreServices/CoreServices.h>
 
+#include <algorithm>
+#include <string>
+
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/logging.h"
 #include "base/mac/mac_logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/stringprintf.h"
-#include "base/time/time.h"
 #include "base/trace_event/trace_event.h"
 #include "media/audio/mac/audio_manager_mac.h"
 #include "media/base/audio_pull_fifo.h"
@@ -236,10 +238,7 @@ OSStatus AUHALStream::Render(
   // Make |output_bus_| wrap the output AudioBufferList.
   WrapBufferList(data, output_bus_.get(), number_of_frames);
 
-  // Update the playout latency.
-  const double playout_latency_frames = GetPlayoutLatency(output_time_stamp);
-  current_hardware_pending_bytes_ = static_cast<uint32_t>(
-      (playout_latency_frames + 0.5) * params_.GetBytesPerFrame());
+  current_target_playout_time_ = GetTargetPlayoutTime(output_time_stamp);
 
   if (audio_fifo_)
     audio_fifo_->Consume(output_bus_.get(), output_bus_->frames());
@@ -258,10 +257,13 @@ void AUHALStream::ProvideInput(int frame_delay, AudioBus* dest) {
     return;
   }
 
+  const base::TimeDelta delay = base::TimeDelta::FromMicroseconds(
+      frame_delay * base::Time::kMicrosecondsPerSecond / params_.sample_rate());
+  const base::TimeTicks target_playout_time =
+      current_target_playout_time_ + delay;

[jameswest, 2016/09/16 21:59:43]

I realized I'm no longer adding hardware_latency_frames_ that was being added in
GetPlayoutLatency. I can't find any good documentation for this, but should we
add both frame_delay and hardware_latency_frames_ to
current_target_playout_time_ to get target_playout_time?

[miu, 2016/09/16 23:38:28]

I think the math you have is correct: First, note that the
|current_target_playout_time_| already accounts for the
hardware_latency_frames_. What it does not account for is the delay due to the
frames inside the FIFO buffer (the |frame_delay| argument). So, you need to add
the two to get the target playout time to pass to the OnMoreData() call.

Said another way, |current_target_playout_time_| would be the
|target_playout_time| you'd use if there was no additional delay from the FIFO
buffer. You could rename |current_target_playout_time_| to
|current_target_playout_time_minus_fifo_delay_| to clarify this.

[jameswest, 2016/09/17 00:06:50]

|hardware_latency_frames_| is no longer used with my change. It was being added
to the delay frames in GetPlayoutLatency, but I changed that function to just
return |output_time_stamp->mHostTime| converted to TimeTicks, which I think
changed the behavior.

With the old logic, it looks like the real target playout time was
|output_time_stamp->mHostTime| + |hardware_latency_frames_|. When I change
OnMoreData to take a delay, I believe the math equivalent to what is there now
will be as follows:

TimeTicks delay = TimeTicks(output_time_stamp->mHostTime) +
TimeDelta(hardware_latency_frames_) + TimeDelta(frame_delay) - TimeTicks::Now()

[miu, 2016/09/17 00:33:56]

OIC! I was confused (I thought you were referring to
|current_hardware_pending_bytes_|, from the old code). I think you're right.
Though, would it be better to account for this in GetTargetPlayoutTime() instead
of here?

[jameswest, 2016/09/19 23:32:36]

I changed GetHardwareLatency() to return TimeDelta instead of frames and added
it to the TimeTicks returned by GetPlayoutTime().

+
   // Supply the input data and render the output data.
-  source_->OnMoreData(dest, current_hardware_pending_bytes_ +
-                                frame_delay * params_.GetBytesPerFrame(),
-                      current_lost_frames_);
+  source_->OnMoreData(target_playout_time, current_lost_frames_, dest);
   dest->Scale(volume_);
   current_lost_frames_ = 0;
 }
@@ -334,27 +336,18 @@ double AUHALStream::GetHardwareLatency() {
       output_format_.mSampleRate) + device_latency_frames);
 }
 
-double AUHALStream::GetPlayoutLatency(
+base::TimeTicks AUHALStream::GetTargetPlayoutTime(
     const AudioTimeStamp* output_time_stamp) {
-  // Ensure mHostTime is valid.
+  // A platform bug has been observed where the platform sometimes reports that
+  // the next frames will be output at an invalid time or a time in the past.
+  // Because the target playout time cannot be invalid or in the past, return
+  // "now" in these cases.
   if ((output_time_stamp->mFlags & kAudioTimeStampHostTimeValid) == 0)
-    return 0;
-
-  // Get the delay between the moment getting the callback and the scheduled
-  // time stamp that tells when the data is going to be played out.
-  UInt64 output_time_ns = AudioConvertHostTimeToNanos(
-      output_time_stamp->mHostTime);
-  UInt64 now_ns = AudioConvertHostTimeToNanos(AudioGetCurrentHostTime());
-
-  // Prevent overflow leading to huge delay information; occurs regularly on
-  // the bots, probably less so in the wild.
-  if (now_ns > output_time_ns)
-    return 0;
-
-  double delay_frames = static_cast<double>
-      (1e-9 * (output_time_ns - now_ns) * output_format_.mSampleRate);
+    return base::TimeTicks::Now();

[miu, 2016/09/16 18:35:58]

base::TimeTicks::Now() is being called twice in this method. Could you just call
it once and then use a var in two places:

  const base::TimeTicks now = base::TimeTicks::Now();
  if (...not valid...)
    return now;
  return std::max(...FromMachTime..., now);

[jameswest, 2016/09/16 21:59:43]

I'm not opposed to this, but I'm curious what the rationale is when both
statements can't be executed during the same function call.

[miu, 2016/09/16 23:38:28]

Oh, I missed that. Yeah, no change necessary here. :)

[jameswest, 2016/09/19 23:32:36]

Acknowledged.

 
-  return (delay_frames + hardware_latency_frames_);
+  return std::max(
+      base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime),
+      base::TimeTicks::Now());
 }
 
 void AUHALStream::UpdatePlayoutTimestamp(const AudioTimeStamp* timestamp) {