Pass delay and timestamp to AudioSourceCallback::OnMoreData.

media/audio/mac/audio_auhal_mac.cc

 // Copyright 2013 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/mac/audio_auhal_mac.h"
 
 #include <CoreServices/CoreServices.h>
 
+#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"
 
 namespace media {
 
 static void WrapBufferList(AudioBufferList* buffer_list,
                            AudioBus* bus,
                            int frames) {
   DCHECK(buffer_list);
@@ skipping 203 matching lines @@
       audio_fifo_.reset(new AudioPullFifo(
           output_channels_,
           number_of_frames_,
           base::Bind(&AUHALStream::ProvideInput, base::Unretained(this))));
     }
   }
 
   // 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());
   else
     ProvideInput(0, output_bus_.get());
 
   last_number_of_frames_ = number_of_frames;
 
   return noErr;
 }
 
 void AUHALStream::ProvideInput(int frame_delay, AudioBus* dest) {
   base::AutoLock auto_lock(source_lock_);
   if (!source_) {
     dest->Zero();
     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;
+
   // 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;
 }
 
 // AUHAL callback.
 OSStatus AUHALStream::InputProc(
     void* user_data,
     AudioUnitRenderActionFlags* flags,
     const AudioTimeStamp* output_time_stamp,
     UInt32 bus_number,
@@ skipping 52 matching lines @@
       &device_latency_frames);
   if (result != noErr) {
     OSSTATUS_DLOG(WARNING, result) << "Could not get audio device latency";
     return 0.0;
   }
 
   return static_cast<double>((audio_unit_latency_sec *
       output_format_.mSampleRate) + device_latency_frames);
 }
 
-double AUHALStream::GetPlayoutLatency(
+base::TimeTicks AUHALStream::GetTargetPlayoutTime(
     const AudioTimeStamp* output_time_stamp) {
   // Ensure mHostTime is valid.
   if ((output_time_stamp->mFlags & kAudioTimeStampHostTimeValid) == 0)
     return 0;

[miu, 2016/08/31 23:26:54]

The two "return 0" statements in this method won't compile since a
base::TimeTicks type is required. Based on the way the old code behaved, I think
they need to be base::TimeTicks::Now().

Side point: FWICT, it seems like the old code is purposely masking some
potentially huge problems here. That's bad for users, and would make bugs very
hard to resolve. For instance, one would hope the |output_time_stamp| always has
its |TimeValid| flag set. If we're certain it never should be, then this could
become a DCHECK().

Also, the "Prevent overflow leading to huge delay" below is a total hack that
could only mask bugs from the platform CoreAudio library, and should not be
necessary. FWIW, if there ever was a problem, it's very likely to have been
fixed over the years, as this would ruin audio in all apps on MacOS.

So, could you put a TODO comment here for someone to revisit this code? I'd
suggest looking at `git blame` history to see who wrote the code originally, and
send them an e-mail. Feel free to cc me, if they need further guidance. ;-)

[jameswest, 2016/09/07 21:52:16]

I'll add the TODO, but it looks like it was written by crogers@google.com. I
can't find him in Teams, so I don't think he works here anymore.

[miu, 2016/09/07 22:19:36]

That's correct. He left several years ago. Hmm...Go ahead and assign the
TODO/crbug to me, and then it'll be on me to find someone to take ownership from
our wider "Chrome Media" group.

[miu, 2016/09/08 20:56:23]

Looking at this again, I think it hides a bug, but Dale pointed out to me (on
the side) that there's probably nothing we can do if this platform bug arises.

What this code is doing: If the output_timestamp for the next frames is bogus
(because it was in the past), we can only say the delay is zero (because delays
cannot be negative--that's meaningless).

The comments, as written, are misleading because they talk about "preventing
overflow" and "huge delay information" and that's not accurate.

Thus, could you just fix the comments to say something like: A platform bug has
been observed where sometimes the platform reports the next frames will be
output at a time in the past. Since delay-until-playout cannot be negative,
assume it is zero and return "now" as the target playout time.

[James West, 2016/09/13 07:40:50]

Done.

 
   // 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 (delay_frames + hardware_latency_frames_);
+  // TODO(jameswest): Find an alternative to FromInternalValue.
+  return base::TimeTicks::FromInternalValue(

[miu, 2016/08/31 23:26:54]

I'd suggest adding a TimeTicks::FromMachAbsoluteTime(uint64_t
mach_absolute_ticks) method. We had to do a similar thing for Windows
(TimeTicks::FromQPCValue()) a couple years back for an analogous use case.

Note that the implementation of this new method can just re-use most of this
code (in base/time/time_mac.cc):
https://cs.chromium.org/chromium/src/base/time/time_mac.cc?rcl=0&l=44

Also, note that AudioConvertHostTimeToNanos() is just doing the conversion from
mach_absolute_time() tick units to nanoseconds. So, this and the prior ~12 lines
of code can just become:

  const base::TimeTicks playout_time =
      base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);
  const base::TimeTicks now = base::TimeTicks::Now();

  if (now > playout_time)  // HACK
    return now;

  return playout_time;

...or, if you're feeling saucy and agree with my prior comment about the hack
being bogus, all those lines of code become just one:

  return base::TimeTicks::FromMachAbsoluteTime(output_time_stamp->mHostTime);

[James West, 2016/09/13 07:40:50]

Done.

+      output_time_ns / base::Time::kNanosecondsPerMicrosecond);
 }
 
 void AUHALStream::UpdatePlayoutTimestamp(const AudioTimeStamp* timestamp) {
   if ((timestamp->mFlags & kAudioTimeStampSampleTimeValid) == 0)
     return;
 
   if (last_sample_time_) {
     DCHECK_NE(0U, last_number_of_frames_);
     UInt32 diff =
         static_cast<UInt32>(timestamp->mSampleTime - last_sample_time_);
@@ skipping 187 matching lines @@
   OSStatus result = AudioUnitUninitialize(audio_unit_);
   OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
       << "AudioUnitUninitialize() failed.";
   result = AudioComponentInstanceDispose(audio_unit_);
   OSSTATUS_DLOG_IF(ERROR, result != noErr, result)
       << "AudioComponentInstanceDispose() failed.";
   audio_unit_ = 0;
 }
 
 }  // namespace media