Switch browser side audio capture path to use base time primitives.

Switch browser side audio capture path to use base time primitives.

We've been lying about bits_per_sample() all over the code base
ever since we started passing floating point AudioBus objects
around. Lets go ahead and kill the vestiges.

Included with your purchase:
- Cleans up an old AudioInputCallback interface which is long since
  been removed.
- Adds base::TimeDelta delay and base::TimeTicks delay_timestamp
  like we have on the output side. Does not wire these up on the
  renderer side completely though. Defer to folk on the input
  path for this cleanup, though they look immediately useful.
- Removes |hardware_latency_bytes| from all browser side callsites.

BUG=609890
TEST=no functional changes...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

[DaleCurtis, 2017-02-10 02:10:04]

Description was changed from

==========
Switch browser side audio capture path to use base time primitives.

We've been lying about bits_per_sample() all over the code base
ever since we started passing floating point AudioBus objects
around. Lets go ahead and kill the vestiges.

Included with your purchase:
- Cleans up an old AudioInputCallback interface which is long since
  been removed.
- Adds base::TimeDelta delay and base::TimeTicks delay_timestamp
  like we have on the output side. Does not wire these up on the
  renderer side completely though. Defer to folk on the input
  path for this cleanup, though they look immediately useful.
- Removes |hardware_latency_bytes| from all browser side callsites.

BUG=609890
TEST=no functional changes...
==========

to

==========
Switch browser side audio capture path to use base time primitives.

We've been lying about bits_per_sample() all over the code base
ever since we started passing floating point AudioBus objects
around. Lets go ahead and kill the vestiges.

Included with your purchase:
- Cleans up an old AudioInputCallback interface which is long since
  been removed.
- Adds base::TimeDelta delay and base::TimeTicks delay_timestamp
  like we have on the output side. Does not wire these up on the
  renderer side completely though. Defer to folk on the input
  path for this cleanup, though they look immediately useful.
- Removes |hardware_latency_bytes| from all browser side callsites.

BUG=609890
TEST=no functional changes...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2017-02-10 02:11:16]

dalecurtis@chromium.org changed reviewers:
+ olka@chromium.org, tommi@chromium.org

[DaleCurtis, 2017-02-10 02:11:17]

WDYT? After this we'll be able to start cleaning up the bits_per_sample on
AudioParameters. I'm torn about including delay_timestamp yet, but it looks like
it could be immediately useful to WebRTC.

[DaleCurtis, 2017-02-10 02:11:25]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 02:11:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-02-10 02:12:07]

(not done yet, WASAPI delay calculations hurt my head)

[commit-bot: I haz the power, 2017-02-10 02:23:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 02:23:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[o1ka, 2017-02-10 13:28:51]

I've never been good at these delay calculations, so I'm confused and have a lot
of questions :)
Overall, comments in the code would be really nice for an unprepared reader.

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:332:
buffer->params.delay_timestamp =
Another reason to have very different names for these two values: they look
pretty similar and are very easy to mix up.

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:164: delay_timestamp);
Should we probably adjust delay and delay_timestamp at the process border?

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:338: // TODO(olka, tommi): Do something with
|delay_timestamp|.
Looking on TimeTicks::IsConsistentAcrossProcesses() method comment: how reliable
or inconsistent this value may be?

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:338: // TODO(olka, tommi): Do something with
|delay_timestamp|.
Will we break anything if we leave it as "TODO"? Looks like we may need to
correct the delay basing on |delay_timestamp| and Now()?

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_io.h
File media/audio/audio_io.h (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_io.h#newc...
media/audio/audio_io.h:124: base::TimeDelta delay,
Are we expecting that TimeTicks::IsHighResolution() is true on all the
platforms? Should we have a check somewhere for that?

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_io.h#newc...
media/audio/audio_io.h:125: base::TimeTicks delay_timestamp,
It's hard for me to interpret what "delay timestamp" means. What is it counted
from? Is it a "buffer timestamp"?

How is |delay| calculated? Is it a time interval between the moment the physical
signal is received and the moment OnData() is called? How the user should
interpret it?

Is  |delay_timestamp| - |delay| equal to the physical time of the signal?


Would be nice to have comments explaining the contract, and more descriptive
naming (maybe |latency| and |timestamp| will do?)

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:995: fifo_.Push(audio_data,
number_of_frames, format_.mBitsPerChannel / 8);
Shouldn't we push latency and timestamp info the FIFO together with the data?

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:1003: capture_delay_bytes +=
fifo_.GetAvailableFrames() * format_.mBytesPerFrame;
Hmm.. does it really compile? :)

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:1004: sink_->OnData(this,
audio_bus, capture_latency, delay_timestamp,
Shouldn't we adjust latency and timestamp values taking into account the fifo?

https://codereview.chromium.org/2689483006/diff/1/media/audio/pulse/pulse_inp...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/pulse/pulse_inp...
media/audio/pulse/pulse_input.cc:313:
static_cast<double>(params_.sample_rate()));
I'm not sure if this is a good enough approximation, if each fifo Push() has a
different latency.

And I don't really understand if and how pulse::GetHardwareLatency() and/or
other calculations take into account Sleep() call at l.320.

Comments would be very helpful.

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters.h
File media/base/audio_parameters.h (right):

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters...
media/base/audio_parameters.h:46: int64_t delay_timestamp;  // base::TimeTicks
in microseconds.
These comments look confusing to me.

[tommi (sloooow) - chröme, 2017-02-10 14:13:51]

On 2017/02/10 02:12:07, DaleCurtis wrote:
> (not done yet, WASAPI delay calculations hurt my head)

Indeed.  I took a brief look at it several months ago and iirc it turns out that
the timestamps for shared streams, are relative to when the stream started and
not to the device.  Furthermore trying to detect glitches using the clock for
the stream, doesn't work since the clock associated with the stream is somehow
overlayed on top of the actual underlying clock.  I can see if I can dig up some
of my findings and forward them to you in case its useful.

[DaleCurtis, 2017-02-11 00:00:00]

The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-11 00:00:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-11 00:33:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-11 00:33:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[DaleCurtis, 2017-02-11 01:43:13]

Just comments, didn't have time to play astrophysicist in the WASAPI timing
code.

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:332:
buffer->params.delay_timestamp =
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> Another reason to have very different names for these two values: they look
pretty similar and are very easy to mix up.

For now I'll keep the names the same since that's what we use on the output
side. They already had a really long bike-shed over this, so I don't think it's
worth changing at this time.

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_con...
File media/audio/audio_input_controller.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_con...
media/audio/audio_input_controller.cc:164: delay_timestamp);
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> Should we probably adjust delay and delay_timestamp at the process border?

delay should always be provided relative to delay_timestamp, so we shouldn't
need to do any adjustment to delay_timestamp, if there's buffering taking place
it should change |delay| though. For now I'm trying not to change anything
behaviorally though, just s/hardware_delay_bytes/delay/.

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:338: // TODO(olka, tommi): Do something with
|delay_timestamp|.
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> Looking on TimeTicks::IsConsistentAcrossProcesses() method comment: how
reliable or inconsistent this value may be?

Good question, looking at the implementations, this is always true on POSIX, but
on Windows depends on whether the high resolution timer is in effect or not. We
always use the high resolution timer when QPC is present, and a working QPC is
required for WASAPI delay information.

To your second question, no if you never do anything with this TODO, it won't
matter. I took a brief glance through the WebRTC capture code though and it
looked like it was trying to use the delay information relative to a timestamp,
so I thought this would be immediately useful.

If it's not useful or you don't plan to use it, I can remove it from this CL. In
fact this is the main reason I sent this CL early, to figure out if
delay_timestamp is actually interesting to you.

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac.cc:1003: capture_delay_bytes +=
fifo_.GetAvailableFrames() * format_.mBytesPerFrame;
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> Hmm.. does it really compile? :)

Like I said, not done yet :)

https://codereview.chromium.org/2689483006/diff/1/media/audio/pulse/pulse_inp...
File media/audio/pulse/pulse_input.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/pulse/pulse_inp...
media/audio/pulse/pulse_input.cc:313:
static_cast<double>(params_.sample_rate()));
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> I'm not sure if this is a good enough approximation, if each fifo Push() has a
different latency.
> 
> And I don't really understand if and how pulse::GetHardwareLatency() and/or
other calculations take into account Sleep() call at l.320.
> 
> Comments would be very helpful.

It's a good question, the answer is they don't. I've moved them inside the loop
so they are correctly compensating. Probably this code can be removed now that
we have a circular buffer at the process boundary.

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters.h
File media/base/audio_parameters.h (right):

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters...
media/base/audio_parameters.h:46: int64_t delay_timestamp;  // base::TimeTicks
in microseconds.
On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> These comments look confusing to me.

Do you have a suggestion? I was trying to indicate exactly what was in the
int64_t values, since they had zero explanation before :)

[miu, 2017-02-12 04:21:36]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2017-02-12 04:21:37]

All: For reference, it may help to refer to the "output" version of this change
from several months back: https://codereview.chromium.org/2517503003/

https://codereview.chromium.org/2689483006/diff/20001/media/audio/mac/audio_l...
File media/audio/mac/audio_low_latency_input_mac.cc (right):

https://codereview.chromium.org/2689483006/diff/20001/media/audio/mac/audio_l...
media/audio/mac/audio_low_latency_input_mac.cc:1006: sink_->OnData(this,
audio_bus, capture_latency, delay_timestamp,
Should |capture_latency| be |delay| here? Otherwise, it seems you are
incrementing |delay| but never using the value anywhere.

https://codereview.chromium.org/2689483006/diff/20001/media/audio/win/audio_l...
File media/audio/win/audio_low_latency_input_win.cc (right):

https://codereview.chromium.org/2689483006/diff/20001/media/audio/win/audio_l...
media/audio/win/audio_low_latency_input_win.cc:408: // TODO(dalecurtis): This is
all wrong and brain is mush, fix tomorrow.
It may help to consult what was done for this same change in the
audio_low_latency_OUTPUT_win.cc. You may also want to loop-in the developer that
made the output interface bytes-->TimeXXX changes to take a quick look at this
CL.

[o1ka, 2017-02-13 14:00:21]

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/media/audio_input_sync_writer.cc (right):

https://codereview.chromium.org/2689483006/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/media/audio_input_sync_writer.cc:332:
buffer->params.delay_timestamp =
On 2017/02/11 01:43:12, DaleCurtis wrote:
> On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> > Another reason to have very different names for these two values: they look
> pretty similar and are very easy to mix up.
> 
> For now I'll keep the names the same since that's what we use on the output
> side. They already had a really long bike-shed over this, so I don't think
it's
> worth changing at this time.

I'm ok with naming now - I got the reasons why they are called this way.

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
File media/audio/audio_input_device.cc (right):

https://codereview.chromium.org/2689483006/diff/1/media/audio/audio_input_dev...
media/audio/audio_input_device.cc:338: // TODO(olka, tommi): Do something with
|delay_timestamp|.
On 2017/02/11 01:43:13, DaleCurtis wrote:
> On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> > Looking on TimeTicks::IsConsistentAcrossProcesses() method comment: how
> reliable or inconsistent this value may be?
> 
> Good question, looking at the implementations, this is always true on POSIX,
but
> on Windows depends on whether the high resolution timer is in effect or not.
We
> always use the high resolution timer when QPC is present, and a working QPC is
> required for WASAPI delay information.
> 

Can we have this documented in the code?

> To your second question, no if you never do anything with this TODO, it won't
> matter. I took a brief glance through the WebRTC capture code though and it
> looked like it was trying to use the delay information relative to a
timestamp,
> so I thought this would be immediately useful.
> 
> If it's not useful or you don't plan to use it, I can remove it from this CL.
In
> fact this is the main reason I sent this CL early, to figure out if
> delay_timestamp is actually interesting to you.

I see, I need to do my homework on this - will be back tomorrow.

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters.h
File media/base/audio_parameters.h (right):

https://codereview.chromium.org/2689483006/diff/1/media/base/audio_parameters...
media/base/audio_parameters.h:46: int64_t delay_timestamp;  // base::TimeTicks
in microseconds.
On 2017/02/11 01:43:13, DaleCurtis wrote:
> On 2017/02/10 at 13:28:50, o1ka (CET) wrote:
> > These comments look confusing to me.
> 
> Do you have a suggestion? I was trying to indicate exactly what was in the
> int64_t values, since they had zero explanation before :)

Now basing on your other replies I got the concept. Something like
"delay in microseconds, relative to |delay_timestamp|"? This would clarify the
relations and differences between the two.

[miu, 2017-08-04 21:53:21]

Description was changed from

==========
Switch browser side audio capture path to use base time primitives.

We've been lying about bits_per_sample() all over the code base
ever since we started passing floating point AudioBus objects
around. Lets go ahead and kill the vestiges.

Included with your purchase:
- Cleans up an old AudioInputCallback interface which is long since
  been removed.
- Adds base::TimeDelta delay and base::TimeTicks delay_timestamp
  like we have on the output side. Does not wire these up on the
  renderer side completely though. Defer to folk on the input
  path for this cleanup, though they look immediately useful.
- Removes |hardware_latency_bytes| from all browser side callsites.

BUG=609890
TEST=no functional changes...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Switch browser side audio capture path to use base time primitives.

We've been lying about bits_per_sample() all over the code base
ever since we started passing floating point AudioBus objects
around. Lets go ahead and kill the vestiges.

Included with your purchase:
- Cleans up an old AudioInputCallback interface which is long since
  been removed.
- Adds base::TimeDelta delay and base::TimeTicks delay_timestamp
  like we have on the output side. Does not wire these up on the
  renderer side completely though. Defer to folk on the input
  path for this cleanup, though they look immediately useful.
- Removes |hardware_latency_bytes| from all browser side callsites.

BUG=609890
TEST=no functional changes...
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[miu, 2017-08-04 21:53:39]

I closed this, since we're doing it here instead:
https://chromium-review.googlesource.com/c/592403/