Support for AudioContextOptions latencyHint.

Support for AudioContextOptions latencyHint.

This adds an optional AudioContextOptions argument to AudioContext
creation with a single 'latencyHint' member taking values of either
'interactive', 'balanced, or 'playback'. Support for passing a double
for an exact latency amount will be added in a subsequent CL.

Intent: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t9SapYQiUtI/discussion

Earlier thread: https://groups.google.com/a/chromium.org/d/topic/blink-dev/FyxYC3TE8ck/discussion

Chrome feature dashboard: https://www.chromestatus.com/features/5678699475107840

BUG=564276
R=rtoy@chromium.org

Review-Url: https://codereview.chromium.org/2501863003
Cr-Commit-Position: refs/heads/master@{#449927}
Committed: https://chromium.googlesource.com/chromium/src/+/4f85155e4b7ae25e1dfb21cf686890fb8634eb6f

[Raymond Toy, 2016-11-15 18:16:17]

Just some initial comments.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:86: const
WebAudioLatencyHint& latencyHint,
Since OfflineAudioContext does not have a latency hint, it seems unexpected to
have BaseAudioContext support a latency hint. I was expecting this to be defined
only for an AudioContext.

(However, the current separation between offline and realtime contexts is not
very good, so maybe this is the best way for now.)

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl:16: };
I think this should be defined in AudioContext.idl, not here, since the latency
does not apply to BaseAudioContext because OfflineAudioContext does not have a
latency option.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp
(left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp:79:
VLOG(1) << ">>>> hardwareSampleRate = " << hardwareSampleRate;
I hope we didn't lose this log print.  It's really useful on Android to get the
sample rate displayed.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:72:
m_sampleRate(sampleRate),
Where is this initialized now?  It's really important.

[o1ka, 2016-11-15 22:44:27]

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

[o1ka, 2016-11-15 22:44:28]

Some comments on WebAudioDeviceImpl

https://codereview.chromium.org/2501863003/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webaudiodevice_impl.cc:35: :
sink_params_(params),
There is no point in passing these params. Sink will calculate them.

Instead of getting sink parameters from untrusted sources ;) (such as mixer
code) and telling them to the sink:

(1) params are not passed to constructor: only channel layout and latency  (2)
WebAudioDeviceImpl gets output parameters from AudioDevice factory, adjusts
buffer size basing on latency ("switch" part of "mixer" code), sets channel
layout - and that will be |sink_params_| (3) WebAudio gets those adjusted params
from WebAudioDevice through getters and uses them to configure rendering (fifo,
etc) (4) WebAudioDeviceImpl passes adjusted params to sink->Initialize().

Or am I missing something?

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:205: // Calculate mixer output
parameters based on mixer input parameters and
I don't think we need it here. 
And WebAudioDeviceImpl only needs a "switch" part from it, because WebAudio will
output at the sink's sample rate.

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:211: int output_sample_rate =
input_params.sample_rate();
(explanation of how this mixer code differs from what WebAudio needs)
WebAudio outputs at sink sample rate, so no input sample rate knowledge is
required

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:222: // let the browser side
handle automatic fallback.
(explanation of how this mixer code differs from what WebAudio needs)
This is relevant to mixing logic only

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:791:
GetLatencyParams(input_params, GetAudioHardwareParams(),
I think we just do not need to pass parameters to AudioDevice during creation
(see above). Only channel layout and latency hint. The rest should be calculated
in WebAudioDeviceImpl

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
I'm not sure if these two histograms have the right meaning now

[hongchan, 2016-11-15 23:46:00]

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

[hongchan, 2016-11-15 23:46:01]

Some design questions:

- Who gets to decide and keep the sample rate?
- Is it okay to keep the cached sample rate value in Blink side? (preferably in
AudioDestination?) We don't want to make an IPC call to query a double value...
- What if user changes the audio device with a different sample rate in the
middle of rendering? How can Blink respond to that?

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:86:
DCHECK(m_audioDevice);
So I guess now we have to query the sample rate via m_audioDevice. Is this your
intention?

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:88:
m_callbackBufferSize = m_audioDevice->framesPerBuffer();
o1ka@

If we do this, is this going to be an IPC call? Similarly, will
|m_audioDevice->sampleRate()| be an IPC call as well?

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/11/15 22:44:28, o1ka wrote:
> I'm not sure if these two histograms have the right meaning now

I think o1ka is right. This seems wrong and we might have to remove the first
one since we don't have that info in the Blink boundary any more.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.h (left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.h:105: float
m_sampleRate;
How do I query the sample rate of the destination then? This is the WebAudio
abstraction of the underlying device/thread, we don't want to lose the critical
info like the sample rate.

I am not sure if WebAudioLatencyHint can completely replace the m_sampleRate
variable.

[Raymond Toy, 2016-11-15 23:50:38]

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/11/15 23:46:01, hongchan wrote:
> On 2016/11/15 22:44:28, o1ka wrote:
> > I'm not sure if these two histograms have the right meaning now
> 
> I think o1ka is right. This seems wrong and we might have to remove the first
> one since we don't have that info in the Blink boundary any more.

Yeah, but I think we really want this somewhere. It's really useful information,
especially on Android, where, I think, we are extremely conservative in the
actual buffer size.  This needs to be optimized more (in a different CL).

[Andrew MacPherson, 2016-11-16 10:58:30]

Responses to earlier comments.

I will let someone else answer the design questions from hongchan@ but my
understanding is that sample rate and buffer size will now be determined in the
'content' side and not in Blink and that the calls to query them from the
WebAudioDevice do not involve IPC.

Thanks for all the help, reviewers!

https://codereview.chromium.org/2501863003/diff/1/content/renderer/media/rend...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/1/content/renderer/media/rend...
content/renderer/media/renderer_webaudiodevice_impl.cc:35: :
sink_params_(params),
On 2016/11/15 22:44:27, o1ka wrote:
> There is no point in passing these params. Sink will calculate them.
> 
> Instead of getting sink parameters from untrusted sources ;) (such as mixer
> code) and telling them to the sink:
> 
> (1) params are not passed to constructor: only channel layout and latency  (2)
> WebAudioDeviceImpl gets output parameters from AudioDevice factory, adjusts
> buffer size basing on latency ("switch" part of "mixer" code), sets channel
> layout - and that will be |sink_params_| (3) WebAudio gets those adjusted
params
> from WebAudioDevice through getters and uses them to configure rendering
(fifo,
> etc) (4) WebAudioDeviceImpl passes adjusted params to sink->Initialize().
> 
> Or am I missing something?

This makes perfect sense to me, thanks for the detailed clarification!

Should the switch statement that converts LatencyType to buffer size be made
into an AudioLatency static member since it will be used in two places now? And
do we need to pass 'channels' into the RendererWebAudioDevice constructor as
well so that it can do set_channels_for_discrete() on the params?

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:205: // Calculate mixer output
parameters based on mixer input parameters and
On 2016/11/15 22:44:27, o1ka wrote:
> I don't think we need it here. 
> And WebAudioDeviceImpl only needs a "switch" part from it, because WebAudio
will
> output at the sink's sample rate.

Sounds good, I will remove this function entirely and move the switch statement
only into the RendererWebAudioDevice.

https://codereview.chromium.org/2501863003/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:791:
GetLatencyParams(input_params, GetAudioHardwareParams(),
On 2016/11/15 22:44:27, o1ka wrote:
> I think we just do not need to pass parameters to AudioDevice during creation
> (see above). Only channel layout and latency hint. The rest should be
calculated
> in WebAudioDeviceImpl

Will fix in the next commit, thanks!

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:86: const
WebAudioLatencyHint& latencyHint,
On 2016/11/15 18:16:17, Raymond Toy wrote:
> Since OfflineAudioContext does not have a latency hint, it seems unexpected to
> have BaseAudioContext support a latency hint. I was expecting this to be
defined
> only for an AudioContext.
> 
> (However, the current separation between offline and realtime contexts is not
> very good, so maybe this is the best way for now.)

I think we need the latencyHint value to be passed into the
DefaultAudioDestinationNode so I wasn't sure if the right thing was to pass it
in here or else to move the DefaultAudioDestinationNode up to be owned by the
AudioContext (or something else). There seem to be two constructors for the
BaseAudioContext, one that's used by the AudioContext and one that's used by the
OfflineAudioContext and this should only affect the one used by the AudioContext
(and we don't store the latencyHint as a member on the BaseAudioContext). Let me
know what you think.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl:16: };
On 2016/11/15 18:16:17, Raymond Toy wrote:
> I think this should be defined in AudioContext.idl, not here, since the
latency
> does not apply to BaseAudioContext because OfflineAudioContext does not have a
> latency option.

You're right, this will be fixed in the next commit.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp
(left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp:79:
VLOG(1) << ">>>> hardwareSampleRate = " << hardwareSampleRate;
On 2016/11/15 18:16:17, Raymond Toy wrote:
> I hope we didn't lose this log print.  It's really useful on Android to get
the
> sample rate displayed.

Should I re-add the log print though we won't pass the hardwareSampleRate into
the AudioDestination or should the log print be moved to the 'content' side
where the hardware sample rate will be determined?

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:72:
m_sampleRate(sampleRate),
On 2016/11/15 18:16:17, Raymond Toy wrote:
> Where is this initialized now?  It's really important.

Based on an earlier suggestion from o1ka@ I changed it so that the sample rate
is not specified on the Blink side but can instead be queried from the
WebAudioDevice::sampleRate(). It's possible that I have misunderstood though so
let me know if so!

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:86:
DCHECK(m_audioDevice);
On 2016/11/15 23:46:01, hongchan wrote:
> So I guess now we have to query the sample rate via m_audioDevice. Is this
your
> intention?

Yes, this was the intention but I will let o1ka@ confirm.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/11/15 23:50:38, Raymond Toy wrote:
> On 2016/11/15 23:46:01, hongchan wrote:
> > On 2016/11/15 22:44:28, o1ka wrote:
> > > I'm not sure if these two histograms have the right meaning now
> > 
> > I think o1ka is right. This seems wrong and we might have to remove the
first
> > one since we don't have that info in the Blink boundary any more.
> 
> Yeah, but I think we really want this somewhere. It's really useful
information,
> especially on Android, where, I think, we are extremely conservative in the
> actual buffer size.  This needs to be optimized more (in a different CL).

Let me know if you'd like me to move this elsewhere (to the 'content' side?) or
just remove it entirely and I can take care of that.

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.h (left):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.h:105: float
m_sampleRate;
On 2016/11/15 23:46:01, hongchan wrote:
> How do I query the sample rate of the destination then? This is the WebAudio
> abstraction of the underlying device/thread, we don't want to lose the
critical
> info like the sample rate.
> 
> I am not sure if WebAudioLatencyHint can completely replace the m_sampleRate
> variable.

My understanding is that this should be queried using
m_audioDevice->sampleRate().

[o1ka, 2016-11-17 17:37:25]

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:88:
m_callbackBufferSize = m_audioDevice->framesPerBuffer();
On 2016/11/15 23:46:01, hongchan wrote:
> o1ka@
> 
> If we do this, is this going to be an IPC call? Similarly, will
> |m_audioDevice->sampleRate()| be an IPC call as well?

No extra IPC, the value is cached in audio device. Overall, the change does not
introduce any extra IPC, it just delays a call to fetch device parameters until
audio device is constructed (and thus we can ask for specific device parameters
instead of the default ones)

[Andrew MacPherson, 2016-11-28 16:15:13]

I just pushed some changes in response to reviewer comments. A few outstanding
questions/comments:

rtoy@: Should the baseLatency calculation always be 2*framesPerBuffer/sampleRate
(double buffering) or is the double buffering decided per-OS?

rtoy@: Re: the failing audionode.html unit test should this 3 argument
constructor for AudioContext still work or is the unit test incorrect?

-PASS context3 = new AudioContext(1, 44100, 44100) did not throw exception.
+FAIL context3 = new AudioContext(1, 44100, 44100) should not throw exception.
Threw exception TypeError: Failed to construct 'AudioContext': parameter 1
('contextOptions') is not an object..

rtoy@: I had to do some refactoring in the Blink layer to remove an additional
use of hardwareSampleRate(), there is still a sampleRate taken as a constructor
argument by many of the nodes but it isn't used and instead the
context()->sampleRate() is relied on in all cases. I didn't want to remove it
from the constructors without checking with you first though as this may be more
appropriate for a subsequent CL.

olka@: The refactoring to the 'content' side should be done now so let me know
if you see anything that looks off! We are not passing the number of channels
into the RendererWebAudioDeviceImpl constructor so I added a DCHECK() for the
DISCRETE case but this doesn't appear to be used by WebAudio (yet) in any case.

Thanks again everyone!

[Raymond Toy, 2016-11-28 18:41:51]

On 2016/11/28 16:15:13, Andrew MacPherson wrote:
> I just pushed some changes in response to reviewer comments. A few outstanding
> questions/comments:
> 
> rtoy@: Should the baseLatency calculation always be
2*framesPerBuffer/sampleRate
> (double buffering) or is the double buffering decided per-OS?

For all OSes, we double buffer, so it's the same everywhere.
> 
> rtoy@: Re: the failing audionode.html unit test should this 3 argument
> constructor for AudioContext still work or is the unit test incorrect?

That test is now incorrect with the addition of the dictionary parameter.
> 
> -PASS context3 = new AudioContext(1, 44100, 44100) did not throw exception.
> +FAIL context3 = new AudioContext(1, 44100, 44100) should not throw exception.
> Threw exception TypeError: Failed to construct 'AudioContext': parameter 1
> ('contextOptions') is not an object..
> 
> rtoy@: I had to do some refactoring in the Blink layer to remove an additional
> use of hardwareSampleRate(), there is still a sampleRate taken as a
constructor
> argument by many of the nodes but it isn't used and instead the
> context()->sampleRate() is relied on in all cases. I didn't want to remove it
> from the constructors without checking with you first though as this may be
more
> appropriate for a subsequent CL.

I'll need to check the code again, but it's probably ok.
> 
> olka@: The refactoring to the 'content' side should be done now so let me know
> if you see anything that looks off! We are not passing the number of channels
> into the RendererWebAudioDeviceImpl constructor so I added a DCHECK() for the
> DISCRETE case but this doesn't appear to be used by WebAudio (yet) in any
case.
> 
> Thanks again everyone!

[o1ka, 2016-11-29 13:48:30]

On 2016/11/28 16:15:13, Andrew MacPherson wrote:
> I just pushed some changes in response to reviewer comments. A few outstanding
> questions/comments:
> 
> rtoy@: Should the baseLatency calculation always be
2*framesPerBuffer/sampleRate
> (double buffering) or is the double buffering decided per-OS?
> 
> rtoy@: Re: the failing audionode.html unit test should this 3 argument
> constructor for AudioContext still work or is the unit test incorrect?
> 
> -PASS context3 = new AudioContext(1, 44100, 44100) did not throw exception.
> +FAIL context3 = new AudioContext(1, 44100, 44100) should not throw exception.
> Threw exception TypeError: Failed to construct 'AudioContext': parameter 1
> ('contextOptions') is not an object..
> 
> rtoy@: I had to do some refactoring in the Blink layer to remove an additional
> use of hardwareSampleRate(), there is still a sampleRate taken as a
constructor
> argument by many of the nodes but it isn't used and instead the
> context()->sampleRate() is relied on in all cases. I didn't want to remove it
> from the constructors without checking with you first though as this may be
more
> appropriate for a subsequent CL.
> 
> olka@: The refactoring to the 'content' side should be done now so let me know
> if you see anything that looks off! We are not passing the number of channels
> into the RendererWebAudioDeviceImpl constructor so I added a DCHECK() for the
> DISCRETE case but this doesn't appear to be used by WebAudio (yet) in any
case.
> 
> Thanks again everyone!

Thank Andrew! Unfortunately I don't have time for review today - will do it
tomorrow.

[o1ka, 2016-11-30 11:46:27]

> olka@: The refactoring to the 'content' side should be done now so let me know
> if you see anything that looks off! We are not passing the number of channels
> into the RendererWebAudioDeviceImpl constructor so I added a DCHECK() for the
> DISCRETE case but this doesn't appear to be used by WebAudio (yet) in any
case.

Commented on "content" part.
Haven't found the DCHECK - could you point me to it?
What refactoring do you refer to?

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:28: namespace {
nit: you can move unnamed namespace into |content| and avoid "content::"

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:54: :
latency_hint_(latency_hint),
It does not look like you need |latency_hint_| anywhere. Store source type
instead?

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:62: RenderFrame* const
render_frame = RenderFrame::FromWebFrame(web_frame);
Make FrameIdFromCurrentContext() helper function, or store it in the object for
reuse. (we also need to keep a comment from l.114 and make sure it is actually
valid in constructor)

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:671:
GetAudioHardwareParams().sample_rate(),
Make OverrideCreateAudioDevice() parameterless?
It's up to implementation to decide whether is needs real or mock sample
rate/buffer size.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:34:
WebAudioLatencyHint::Category latencyCategory;
This code looks like it worth a helper function.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:46:
WebAudioLatencyHint latencyHint(latencyCategory);
Why do you need a local for that?

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:128: return
m_destinationNode
It does not look like having null m_destinationNode is normal. If so, we should
not check it anywhere - just DCHECK in the constructor.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
(right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:288:
childDocument(), kLatencyHintInteractive, ASSERT_NO_EXCEPTION);
We need some unit tests for other latency hints as well.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:64: const
WebAudioLatencyHint& latencyHint,
WebAudioLatencyHint class really makes the code look heavier: const refs are not
needed if we use enum directly.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
rtoy@/hongchan@ what is your recommendation for these histograms?
What exactly does WebAudio register in them?
They may need to be moved to WebAudioDeviceImpl.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.h:82: return
m_audioDevice ? m_audioDevice->sampleRate() : 0;
Here and below: if |m_audioDevice| being nullptr is not a normal situation, we
should not mask it here by checking for nullptr; it's better to crash
immediately.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:10: class
WebAudioLatencyHint {
What is the point for using a class here as an extra level of indirection? Why
not to have a bare enum?

[Raymond Toy, 2016-11-30 20:37:29]

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/11/30 11:46:26, o1ka wrote:
> rtoy@/hongchan@ what is your recommendation for these histograms?
> What exactly does WebAudio register in them?
> They may need to be moved to WebAudioDeviceImpl.

We do definitely want to keep these around.  They're pretty important to
understand what kind of Android devices are out there;  there seems to be much
less variation in desktop values.

[Raymond Toy, 2016-11-30 21:50:27]

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:7:
AudioContextLatencyCategory latencyHint = "interactive";
Although you're not implementing the double value part yet, I think we should
define this correctly now instead of later.  For the implementation, we can just
treat float value as being the same as interactive, until it's supported.

WDYT?

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:69: virtual
int framesPerBuffer() const = 0;
Please add comment on what framesPerBuffer actually returns.  I don't know what
a "buffer" is in this context.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:599: return
framesPerBuffer() * 2 / sampleRate();
Cast sampleRate() to double so that we get double arithmetic here.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl:31: // Number of
seconds of processing latency incurred by the AudioContext passing the audio
from the AudioDestinationNode to the audio subsystem
I think this is a spec issue (for which I've filed a bug).  baseLatency doesn't
make sense for an OfflineAudioContext so it shouldn't be in the
BaseAudioContext.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:64: const
WebAudioLatencyHint& latencyHint,
On 2016/11/30 11:46:26, o1ka wrote:
> WebAudioLatencyHint class really makes the code look heavier: const refs are
not
> needed if we use enum directly.

I think this is the right approach because the latencyHint will eventually
include a double in addition to the enum.  I've proposed that the IDL should
include that, and to treat any float value as the same as interactive.  At least
for the first cut.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.h:82: return
m_audioDevice ? m_audioDevice->sampleRate() : 0;
On 2016/11/30 11:46:27, o1ka wrote:
> Here and below: if |m_audioDevice| being nullptr is not a normal situation, we
> should not mask it here by checking for nullptr; it's better to crash
> immediately.

I agree with this. Plus there's already an ASSERT in AudioDestination.cpp that
m_audioDevice is not null.

Having said that, the general style in WebAudio is to check for bad things and
if they're bad, silently return.  I don't know why that is, because I prefer bad
things to crash the renderer instead of silently proceeding to produce silence
in weird ways.

[Andrew MacPherson, 2016-12-01 12:11:56]

Thanks again olka@ and rtoy@!

> Commented on "content" part.
> Haven't found the DCHECK - could you point me to it?
> What refactoring do you refer to?

@olka: Sorry, it was a NOTREACHED(), not a DCHECK() and it's on line 708 of
renderer_blink_platform_impl.cc, so DISCRETE channel type is not supported right
now.

For the refactoring I was just referring to the changes you suggested based on
the initial commit to this CL, so you've seen them all by reviewing this patch.
Thanks!

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:28: namespace {
On 2016/11/30 11:46:26, o1ka wrote:
> nit: you can move unnamed namespace into |content| and avoid "content::"

Done.

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:54: :
latency_hint_(latency_hint),
On 2016/11/30 11:46:26, o1ka wrote:
> It does not look like you need |latency_hint_| anywhere. Store source type
> instead?

The idea with storing the latency_hint_ was that it may be necessary when we add
the EXACT_MS latency type value member, but you're right that it's not required
right now so I can change it if that's best.

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:62: RenderFrame* const
render_frame = RenderFrame::FromWebFrame(web_frame);
On 2016/11/30 11:46:26, o1ka wrote:
> Make FrameIdFromCurrentContext() helper function, or store it in the object
for
> reuse. (we also need to keep a comment from l.114 and make sure it is actually
> valid in constructor)

Done except for the check in the constructor. Should it just be a DCHECK that we
have a valid render_frame or is something else required?

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:671:
GetAudioHardwareParams().sample_rate(),
On 2016/11/30 11:46:26, o1ka wrote:
> Make OverrideCreateAudioDevice() parameterless?
> It's up to implementation to decide whether is needs real or mock sample
> rate/buffer size.

rtoy@: Do you know if we can just hardcode these to something reasonable in
layout_test_content_renderer_client.cc or if we need to retrieve actual hardware
values?

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:34:
WebAudioLatencyHint::Category latencyCategory;
On 2016/11/30 11:46:26, o1ka wrote:
> This code looks like it worth a helper function.

Done.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:46:
WebAudioLatencyHint latencyHint(latencyCategory);
On 2016/11/30 11:46:26, o1ka wrote:
> Why do you need a local for that?

That was a mistake but based on rtoy@'s suggestion of adding the 'or double'
type to AudioContextOptions I believe it's now required (see the new code).

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:7:
AudioContextLatencyCategory latencyHint = "interactive";
On 2016/11/30 21:50:27, Raymond Toy wrote:
> Although you're not implementing the double value part yet, I think we should
> define this correctly now instead of later.  For the implementation, we can
just
> treat float value as being the same as interactive, until it's supported.
> 
> WDYT?

Makes sense, I have added this now though I had to add an unnecessary typedef
for the type as it didn't seem to generate the
AudioContextLatencyCategoryOrDouble otherwise. I looked at other examples of
this kind of thing to figure out the typedef workaround.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:69: virtual
int framesPerBuffer() const = 0;
On 2016/11/30 21:50:27, Raymond Toy wrote:
> Please add comment on what framesPerBuffer actually returns.  I don't know
what
> a "buffer" is in this context.

Done.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:599: return
framesPerBuffer() * 2 / sampleRate();
On 2016/11/30 21:50:27, Raymond Toy wrote:
> Cast sampleRate() to double so that we get double arithmetic here.

Done.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:128: return
m_destinationNode
On 2016/11/30 11:46:26, o1ka wrote:
> It does not look like having null m_destinationNode is normal. If so, we
should
> not check it anywhere - just DCHECK in the constructor.

I believe an OfflineAudioContext does not have an m_destinationNode so it would
be null in that case.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.idl:31: // Number of
seconds of processing latency incurred by the AudioContext passing the audio
from the AudioDestinationNode to the audio subsystem
On 2016/11/30 21:50:27, Raymond Toy wrote:
> I think this is a spec issue (for which I've filed a bug).  baseLatency
doesn't
> make sense for an OfflineAudioContext so it shouldn't be in the
> BaseAudioContext.

Done, I've moved this to the AudioContext now.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
(right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:288:
childDocument(), kLatencyHintInteractive, ASSERT_NO_EXCEPTION);
On 2016/11/30 11:46:26, o1ka wrote:
> We need some unit tests for other latency hints as well.

I created AudioContextTest.cpp and added a basic test to just confirm that the
WebAudioDevice's buffer size values are being correctly passed to the
AudioContext, let me know if more is needed. Note that I also had to add
MODULES_EXPORT to the AudioContext class in order to make its symbols visible to
the unit tests binary.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/11/30 20:37:29, Raymond Toy wrote:
> On 2016/11/30 11:46:26, o1ka wrote:
> > rtoy@/hongchan@ what is your recommendation for these histograms?
> > What exactly does WebAudio register in them?
> > They may need to be moved to WebAudioDeviceImpl.
> 
> We do definitely want to keep these around.  They're pretty important to
> understand what kind of Android devices are out there;  there seems to be much
> less variation in desktop values.

Should I move these to the WebAudioDeviceImpl then?

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.h:82: return
m_audioDevice ? m_audioDevice->sampleRate() : 0;
On 2016/11/30 11:46:27, o1ka wrote:
> Here and below: if |m_audioDevice| being nullptr is not a normal situation, we
> should not mask it here by checking for nullptr; it's better to crash
> immediately.

Done.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:10: class
WebAudioLatencyHint {
On 2016/11/30 11:46:27, o1ka wrote:
> What is the point for using a class here as an extra level of indirection? Why
> not to have a bare enum?

The thought here is that we will be adding an "exact latency" in seconds in the
next CL and so the function APIs (hopefully) won't need to change as it will
just be a new member on this class.

[Raymond Toy, 2016-12-01 16:16:27]

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:671:
GetAudioHardwareParams().sample_rate(),
On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> On 2016/11/30 11:46:26, o1ka wrote:
> > Make OverrideCreateAudioDevice() parameterless?
> > It's up to implementation to decide whether is needs real or mock sample
> > rate/buffer size.
> 
> rtoy@: Do you know if we can just hardcode these to something reasonable in
> layout_test_content_renderer_client.cc or if we need to retrieve actual
hardware
> values?

I don't think any of the build bots have any actual audio hardware (real or
virtual) so there's nothing to be retrieved.  This is why none of the layout
tests actually use AudioContext because it doesn't really work.

[Raymond Toy, 2016-12-01 16:26:51]

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:128: return
m_destinationNode
On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> On 2016/11/30 11:46:26, o1ka wrote:
> > It does not look like having null m_destinationNode is normal. If so, we
> should
> > not check it anywhere - just DCHECK in the constructor.
> 
> I believe an OfflineAudioContext does not have an m_destinationNode so it
would
> be null in that case.

It should.  OfflineAudioContext is a subclass of BaseAudioContext, which defines
m_destinationNode.

https://codereview.chromium.org/2501863003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right):

https://codereview.chromium.org/2501863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:30:
Should("double baseLatency == default baseLatency",
context.baseLatency).beEqualTo(defaultLatency);
Please inspect the actual test output and verify that it reads nicely. I think
the output might be phrased in a funny way.

The test on line 24 might also produce something poorly phrased.

[Andrew MacPherson, 2016-12-02 09:42:55]

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/40001/content/renderer/render...
content/renderer/renderer_blink_platform_impl.cc:671:
GetAudioHardwareParams().sample_rate(),
On 2016/12/01 16:16:27, Raymond Toy wrote:
> On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> > On 2016/11/30 11:46:26, o1ka wrote:
> > > Make OverrideCreateAudioDevice() parameterless?
> > > It's up to implementation to decide whether is needs real or mock sample
> > > rate/buffer size.
> > 
> > rtoy@: Do you know if we can just hardcode these to something reasonable in
> > layout_test_content_renderer_client.cc or if we need to retrieve actual
> hardware
> > values?
> 
> I don't think any of the build bots have any actual audio hardware (real or
> virtual) so there's nothing to be retrieved.  This is why none of the layout
> tests actually use AudioContext because it doesn't really work.

Ok, I've removed the parameters from OverrideCreateAudioDevice() and have
hardcoded them to 44100 and 128 in layout_test_content_renderer_client.cc.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:128: return
m_destinationNode
On 2016/12/01 16:26:51, Raymond Toy wrote:
> On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> > On 2016/11/30 11:46:26, o1ka wrote:
> > > It does not look like having null m_destinationNode is normal. If so, we
> > should
> > > not check it anywhere - just DCHECK in the constructor.
> > 
> > I believe an OfflineAudioContext does not have an m_destinationNode so it
> would
> > be null in that case.
> 
> It should.  OfflineAudioContext is a subclass of BaseAudioContext, which
defines
> m_destinationNode.

Sorry, I see it now, the value is being set directly in OfflineAudioContext
rather than in the Base. It looks like it's possible to have a NULL
m_destinationNode though by doing AudioContext.close() from JS so without these
checks calling ctx.sampleRate for example after that would cause a crash. WDYT?

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> On 2016/11/30 20:37:29, Raymond Toy wrote:
> > On 2016/11/30 11:46:26, o1ka wrote:
> > > rtoy@/hongchan@ what is your recommendation for these histograms?
> > > What exactly does WebAudio register in them?
> > > They may need to be moved to WebAudioDeviceImpl.
> > 
> > We do definitely want to keep these around.  They're pretty important to
> > understand what kind of Android devices are out there;  there seems to be
much
> > less variation in desktop values.
> 
> Should I move these to the WebAudioDeviceImpl then?

Maybe for now the easiest thing is to just use the
Platform::audioHardwareBufferSize() here as before and this can be updated in a
subsequent CL where audioHardwareBufferSize() is removed entirely? I've updated
the code to do this now.

https://codereview.chromium.org/2501863003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right):

https://codereview.chromium.org/2501863003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:30:
Should("double baseLatency == default baseLatency",
context.baseLatency).beEqualTo(defaultLatency);
On 2016/12/01 16:26:51, Raymond Toy wrote:
> Please inspect the actual test output and verify that it reads nicely. I think
> the output might be phrased in a funny way.
> 
> The test on line 24 might also produce something poorly phrased.

Done.

[o1ka, 2016-12-02 12:02:13]

Looks good. I'm a bit worried by all the potential "division by 0" cases.

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:75:
AudioDeviceFactory::GetOutputDeviceInfo(FrameIdFromCurrentContext(),
Maybe cache render frame id in a member, instead of calling
FrameIdFromCurrentContext() here and on start?

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:29:
RendererWebAudioDeviceImpl(media::ChannelLayout layout,
Would be great if we add unit tests for RendererWebAudioDeviceImpl as well

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:27:
WebAudioLatencyHint::Category LatencyHintCategoryFromString(String category) {
Pass a string into WebAudioLatencyHint constructor and convert to category in
there?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:61: return
AudioContext::create(document, latencyCategory, exceptionState);
Is it an implicit conversion in here? This is not really readable, could you
make the constructor explicit?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
can sampleRate() be zero?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:60: return
currentSampleFrame() / static_cast<double>(sampleRate());
What behavior we prefer in case sampleRate() is zero?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.cpp:138:
return m_destination ? m_destination->sampleRate() : 0;
How valid is the case when |m_destination| is null?
There are quite a lot of places where we divide by sample rate.
Do we really want to defer the crash?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:63: int
framesPerBuffer() const override { return 0; }
Is it never expected to be called? DCHECK?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:14:
WebAudioLatencyHint(Category category) : m_category(category) {}
Not sure about WebKit, but in Chromium single-parameter constructors are always
explicit
(https://google.github.io/styleguide/cppguide.html#Implicit_Conversions)

[Raymond Toy, 2016-12-02 16:57:45]

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:128: return
m_destinationNode
On 2016/12/02 09:42:55, Andrew MacPherson wrote:
> On 2016/12/01 16:26:51, Raymond Toy wrote:
> > On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> > > On 2016/11/30 11:46:26, o1ka wrote:
> > > > It does not look like having null m_destinationNode is normal. If so, we
> > > should
> > > > not check it anywhere - just DCHECK in the constructor.
> > > 
> > > I believe an OfflineAudioContext does not have an m_destinationNode so it
> > would
> > > be null in that case.
> > 
> > It should.  OfflineAudioContext is a subclass of BaseAudioContext, which
> defines
> > m_destinationNode.
> 
> Sorry, I see it now, the value is being set directly in OfflineAudioContext
> rather than in the Base. It looks like it's possible to have a NULL
> m_destinationNode though by doing AudioContext.close() from JS so without
these
> checks calling ctx.sampleRate for example after that would cause a crash.
WDYT?

Yes, as currently implemented closed contexts have a null m_destinationNode. 
But see also crbug.com/670065.

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:96:
hardwareBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/12/02 09:42:55, Andrew MacPherson wrote:
> On 2016/12/01 12:11:56, Andrew MacPherson wrote:
> > On 2016/11/30 20:37:29, Raymond Toy wrote:
> > > On 2016/11/30 11:46:26, o1ka wrote:
> > > > rtoy@/hongchan@ what is your recommendation for these histograms?
> > > > What exactly does WebAudio register in them?
> > > > They may need to be moved to WebAudioDeviceImpl.
> > > 
> > > We do definitely want to keep these around.  They're pretty important to
> > > understand what kind of Android devices are out there;  there seems to be
> much
> > > less variation in desktop values.
> > 
> > Should I move these to the WebAudioDeviceImpl then?
> 
> Maybe for now the easiest thing is to just use the
> Platform::audioHardwareBufferSize() here as before and this can be updated in
a
> subsequent CL where audioHardwareBufferSize() is removed entirely? I've
updated
> the code to do this now.

If this preserves the current values, then I'm fine with it. I just would prefer
not to see a (short) glitch in the stats.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
On 2016/12/02 12:02:12, o1ka wrote:
> can sampleRate() be zero?

Good catch!  Yes, it can be zero here.  If the context is closed, the sample
rate will be zero, and it's valid (but meaningless?) to query the baseLatency
value on a closed context.

However, see also https://bugs.chromium.org/p/chromium/issues/detail?id=670065. 
I want to fix this soon, and there won't be a divide-by-zero issue anymore.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:60: return
currentSampleFrame() / static_cast<double>(sampleRate());
On 2016/12/02 12:02:13, o1ka wrote:
> What behavior we prefer in case sampleRate() is zero?

I believe this is safe, and infinity is ok.

This is should only be called from context()->currentTime(), and only if the
AudioDestinationNode is still active.  And if it is, sampleRate would not be 0.

[hongchan, 2016-12-02 17:40:12]

Sorry for being on and off - and thanks so much for your work here!

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:27:
WebAudioLatencyHint::Category LatencyHintCategoryFromString(String category) {
On 2016/12/02 12:02:13, o1ka wrote:
> Pass a string into WebAudioLatencyHint constructor and convert to category in
> there?

I agree with o1ka here. WebAudioLatencyHint is a part of |platform| and any
platform related task should stay out of the |modules|.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:61: return
AudioContext::create(document, latencyCategory, exceptionState);
On 2016/12/02 12:02:12, o1ka wrote:
> Is it an implicit conversion in here? This is not really readable, could you
> make the constructor explicit?

So basically these two creators are always supposed to be chained like this?
Still not sure why we cannot do this in one call.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.h:31: ExceptionState&);
This is slightly confusing. Why do we need two different static creators? Isn't
the latency hint a part of the context options?
Please do make comments why we need 2 different creators - but preferably this
needs to be refactored into one.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.idl:45: // Number of
seconds of processing latency incurred by the AudioContext passing the audio
from the AudioDestinationNode to the audio subsystem
nit: please wrap the comment by 80 cols.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:6: // not
generated
I think this is the right way - the comment is redundant here.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:37: :
AudioHandler(NodeTypeDestination, node, 0), m_currentSampleFrame(0) {
So the initial sample rate is zero? Not sure why this change is necessary here.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:69: virtual
int framesPerBuffer() const = 0;
What does this mean? Isn't it same with the 'render quantum' size? Or is it the
platform-dependent batch process size? Please make a comment.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:105: const
WebAudioLatencyHint&,
BaseAudioContext neeeds |AudioContextOptions|. |AudioContextOptions| has the
latency hint in it. This signature does not reflect the structure.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:29:
WebAudioLatencyHint::CategoryInteractive);
Can't this be a part of WebAudioLatencyHint?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.h
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.h:31:
#include "public/platform/WebAudioLatencyHint.h"
Do we need the actual inclusion here? I think the forward declaration below is
good enough.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:63: int
framesPerBuffer() const override { return 0; }
On 2016/12/02 12:02:13, o1ka wrote:
> Is it never expected to be called? DCHECK?

Or NOTREACHED().

[Andrew MacPherson, 2016-12-05 14:12:53]

Thanks again for the feedback everyone!

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.cc (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.cc:75:
AudioDeviceFactory::GetOutputDeviceInfo(FrameIdFromCurrentContext(),
On 2016/12/02 12:02:12, o1ka wrote:
> Maybe cache render frame id in a member, instead of calling
> FrameIdFromCurrentContext() here and on start?

Done. Also added a DCHECK() in the constructor to ensure that the frame ID is
not MSG_ROUTING_NONE.

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:29:
RendererWebAudioDeviceImpl(media::ChannelLayout layout,
On 2016/12/02 12:02:12, o1ka wrote:
> Would be great if we add unit tests for RendererWebAudioDeviceImpl as well

Sounds good! I will look into adding this in the next day or two.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:27:
WebAudioLatencyHint::Category LatencyHintCategoryFromString(String category) {
On 2016/12/02 17:40:11, hongchan wrote:
> On 2016/12/02 12:02:13, o1ka wrote:
> > Pass a string into WebAudioLatencyHint constructor and convert to category
in
> > there?
> 
> I agree with o1ka here. WebAudioLatencyHint is a part of |platform| and any
> platform related task should stay out of the |modules|.

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:61: return
AudioContext::create(document, latencyCategory, exceptionState);
On 2016/12/02 12:02:12, o1ka wrote:
> Is it an implicit conversion in here? This is not really readable, could you
> make the constructor explicit?

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:61: return
AudioContext::create(document, latencyCategory, exceptionState);
On 2016/12/02 17:40:11, hongchan wrote:
> On 2016/12/02 12:02:12, o1ka wrote:
> > Is it an implicit conversion in here? This is not really readable, could you
> > make the constructor explicit?
> 
> So basically these two creators are always supposed to be chained like this?
> Still not sure why we cannot do this in one call.

Done, refactored to a single constructor.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
On 2016/12/02 16:57:45, Raymond Toy wrote:
> On 2016/12/02 12:02:12, o1ka wrote:
> > can sampleRate() be zero?
> 
> Good catch!  Yes, it can be zero here.  If the context is closed, the sample
> rate will be zero, and it's valid (but meaningless?) to query the baseLatency
> value on a closed context.
> 
> However, see also
https://bugs.chromium.org/p/chromium/issues/detail?id=670065. 
> I want to fix this soon, and there won't be a divide-by-zero issue anymore.

Should that be fixed as part of this CL or should it wait for a subsequent one?
It would be straightforward to just cache the sampleRate in the BaseAudioContext
when the m_destinationNode is created if that makes sense.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.h:31: ExceptionState&);
On 2016/12/02 17:40:11, hongchan wrote:
> This is slightly confusing. Why do we need two different static creators?
Isn't
> the latency hint a part of the context options?
> Please do make comments why we need 2 different creators - but preferably this
> needs to be refactored into one.

Makes sense, I have refactored this down to 1 static constructor now. The second
one was being used by the BaseAudioContext::create() method which only appears
to be used in the BaseAudioContextTest.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.idl (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.idl:45: // Number of
seconds of processing latency incurred by the AudioContext passing the audio
from the AudioDestinationNode to the audio subsystem
On 2016/12/02 17:40:12, hongchan wrote:
> nit: please wrap the comment by 80 cols.

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:6: // not
generated
On 2016/12/02 17:40:12, hongchan wrote:
> I think this is the right way - the comment is redundant here.

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.cpp:37: :
AudioHandler(NodeTypeDestination, node, 0), m_currentSampleFrame(0) {
On 2016/12/02 17:40:12, hongchan wrote:
> So the initial sample rate is zero? Not sure why this change is necessary
here.

I mentioned in an earlier comment to rtoy@ that the sample rate passed into the
AudioHandler() constructor is no longer used anywhere and should be removed but
I wasn't sure if I should include that as part of this CL since it touches a
number of files in the webaudio code. I'm happy to take care of it right now
though if that's best!

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:69: virtual
int framesPerBuffer() const = 0;
On 2016/12/02 17:40:12, hongchan wrote:
> What does this mean? Isn't it same with the 'render quantum' size? Or is it
the
> platform-dependent batch process size? Please make a comment.

This is the buffer size in frames used by the underlying audio hardware, we
should see this value changing based on the different latencyHint values. I've
now added a comment but as rtoy@ also asked about this would a different name
work better here? audioBufferSize()?

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.h:105: const
WebAudioLatencyHint&,
On 2016/12/02 17:40:12, hongchan wrote:
> BaseAudioContext neeeds |AudioContextOptions|. |AudioContextOptions| has the
> latency hint in it. This signature does not reflect the structure.

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/BaseAudioContextTest.cpp:29:
WebAudioLatencyHint::CategoryInteractive);
On 2016/12/02 17:40:12, hongchan wrote:
> Can't this be a part of WebAudioLatencyHint?

I have refactored this now to pass an AudioContextOptions rather than a
WebAudioLatencyHint so that there is only one static create() method on the
AudioContext, let me know if it works better like this.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.h
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/DefaultAudioDestinationNode.h:31:
#include "public/platform/WebAudioLatencyHint.h"
On 2016/12/02 17:40:12, hongchan wrote:
> Do we need the actual inclusion here? I think the forward declaration below is
> good enough.

I believe it's required due to the m_latencyHint member on the type here.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h
(right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/OfflineAudioDestinationNode.h:63: int
framesPerBuffer() const override { return 0; }
On 2016/12/02 17:40:12, hongchan wrote:
> On 2016/12/02 12:02:13, o1ka wrote:
> > Is it never expected to be called? DCHECK?
> 
> Or NOTREACHED().

Done.

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/publ...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/publ...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:14:
WebAudioLatencyHint(Category category) : m_category(category) {}
On 2016/12/02 12:02:13, o1ka wrote:
> Not sure about WebKit, but in Chromium single-parameter constructors are
always
> explicit
> (https://google.github.io/styleguide/cppguide.html#Implicit_Conversions)

Done.

[Raymond Toy, 2016-12-05 17:36:43]

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
On 2016/12/05 at 14:12:53, Andrew MacPherson wrote:
> On 2016/12/02 16:57:45, Raymond Toy wrote:
> > On 2016/12/02 12:02:12, o1ka wrote:
> > > can sampleRate() be zero?
> > 
> > Good catch!  Yes, it can be zero here.  If the context is closed, the sample
> > rate will be zero, and it's valid (but meaningless?) to query the
baseLatency
> > value on a closed context.
> > 
> > However, see also
https://bugs.chromium.org/p/chromium/issues/detail?id=670065. 
> > I want to fix this soon, and there won't be a divide-by-zero issue anymore.
> 
> Should that be fixed as part of this CL or should it wait for a subsequent
one? It would be straightforward to just cache the sampleRate in the
BaseAudioContext when the m_destinationNode is created if that makes sense.

We should do something now. My original idea was to cache the value when the
context was closed, not when the context is created.  For the bug that I filed,
we might need to do something more complicated, so you should prevent division
by zero in this CL.

[Andrew MacPherson, 2016-12-06 15:56:19]

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:29:
RendererWebAudioDeviceImpl(media::ChannelLayout layout,
On 2016/12/05 14:12:52, Andrew MacPherson wrote:
> On 2016/12/02 12:02:12, o1ka wrote:
> > Would be great if we add unit tests for RendererWebAudioDeviceImpl as well
> 
> Sounds good! I will look into adding this in the next day or two.

olka@: I took a look into how the unit tests work and it looks like we'd need to
do a bit of refactoring in the RendererWebAudioDeviceImpl since it calls a
number of static methods (WebLocalFrame::frameForCurrentContext(),
AudioDeviceFactory::GetOutputDeviceInfo(),
AudioDeviceFactory::NewAudioRendererSink()) that I believe we'd need to mock.
This is no problem for me to do but I just wanted to double check that this is
what you intended before I start. Thanks!

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
> We should do something now. My original idea was to cache the value when the
> context was closed, not when the context is created.  For the bug that I
filed,
> we might need to do something more complicated, so you should prevent division
> by zero in this CL.

Done, I've updated the sampleRate() method to use the existing
closedContextSampleRate() method if there is no m_destinationNode.

[o1ka, 2016-12-06 16:08:26]

Thank you for working on unit tests!

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:29:
RendererWebAudioDeviceImpl(media::ChannelLayout layout,
On 2016/12/06 15:56:19, Andrew MacPherson wrote:
> On 2016/12/05 14:12:52, Andrew MacPherson wrote:
> > On 2016/12/02 12:02:12, o1ka wrote:
> > > Would be great if we add unit tests for RendererWebAudioDeviceImpl as well
> > 
> > Sounds good! I will look into adding this in the next day or two.
> 
> olka@: I took a look into how the unit tests work and it looks like we'd need
to
> do a bit of refactoring in the RendererWebAudioDeviceImpl since it calls a
> number of static methods (WebLocalFrame::frameForCurrentContext(),
> AudioDeviceFactory::GetOutputDeviceInfo(),
> AudioDeviceFactory::NewAudioRendererSink()) that I believe we'd need to mock.
> This is no problem for me to do but I just wanted to double check that this is
> what you intended before I start. Thanks! 

Right. For AudioDeviceFactory and AudioRendererSink mocking you can take a look
at WebRtcAudioRendererTest
(https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_rend...).
And for frame id you can, for example, pass  FrameIdFromCurrentContext() as
callback into constructor (there are also other tricks to choose from).

[Raymond Toy, 2016-12-06 23:55:40]

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
On 2016/12/06 15:56:19, Andrew MacPherson wrote:
> > We should do something now. My original idea was to cache the value when the
> > context was closed, not when the context is created.  For the bug that I
> filed,
> > we might need to do something more complicated, so you should prevent
division
> > by zero in this CL.
> 
> Done, I've updated the sampleRate() method to use the existing
> closedContextSampleRate() method if there is no m_destinationNode.

Awesome.

It would be great if you could add a layout test for this where you query the
baseLatency after the context is closed.

[Andrew MacPherson, 2016-12-07 10:55:24]

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/80001/content/renderer/media/...
content/renderer/media/renderer_webaudiodevice_impl.h:29:
RendererWebAudioDeviceImpl(media::ChannelLayout layout,
> Right. For AudioDeviceFactory and AudioRendererSink mocking you can take a
look
> at WebRtcAudioRendererTest
>
(https://cs.chromium.org/chromium/src/content/renderer/media/webrtc_audio_rend...).
> And for frame id you can, for example, pass  FrameIdFromCurrentContext() as
> callback into constructor (there are also other tricks to choose from).

Done, I've added a unit test to the WebAudioDeviceImpl that just verifies the
latency hint buffer sizes and does a simple start/stop test. I also added
callbacks as you suggested for the static method calls that were being used in
the constructor. Let me know if anything else is needed there!

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webaudio/AudioContext.cpp (right):

https://codereview.chromium.org/2501863003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webaudio/AudioContext.cpp:246: return
framesPerBuffer() * 2 / static_cast<double>(sampleRate());
On 2016/12/06 23:55:40, Raymond Toy wrote:
> On 2016/12/06 15:56:19, Andrew MacPherson wrote:
> > > We should do something now. My original idea was to cache the value when
the
> > > context was closed, not when the context is created.  For the bug that I
> > filed,
> > > we might need to do something more complicated, so you should prevent
> division
> > > by zero in this CL.
> > 
> > Done, I've updated the sampleRate() method to use the existing
> > closedContextSampleRate() method if there is no m_destinationNode.
> 
> Awesome.
> 
> It would be great if you could add a layout test for this where you query the
> baseLatency after the context is closed.

Done.

[o1ka, 2016-12-07 20:56:02]

Thanks Andrew! Would be nice to see a couple more tests (see comments).

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:54: // Callback to get
output device params (for tests)
nit:  end with "."

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:60: // Callback get render
frame ID for current context (for tests)
nits: empty line before this one, dot in the end

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:38: class
MockRendererWebAudioDeviceImpl : public RendererWebAudioDeviceImpl {
Nit: such class is usually called  FooUnderTest. (this is not a mock, this is
the real class we are testing).

(BTW I realized that an alternative to passing callbacks as constructor
parameters may be wrapping static calls in RendererWebAudioDevice virtual
functions, and overloading them in this "UnderTest" object - since you have it
anyways. This may look a bit neater -  but up to you, current approach is fine.)

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:97:
scoped_refptr<media::MockAudioRendererSink> fake_sink(
Is this local needed?

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:104:
EXPECT_CALL(*mock_sink_.get(), Start());
Do you start WebAudioDeviceImpl in each test? Will TestLatencyHint fail?

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:106: 
We should ensure the sink gets stopped - this is a requirement for sink users.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:133:
EXPECT_GT(balanced_frames, interactive_frames);
Have you considered comparing with media::AudioLatency values?

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:143: 
Could you add testing that sampleRate() and framesPerBuffer() return correct
values before and after WAD is started, and after it is stopped? And that a
start-stop-start-stop sequence works correctly.

https://codereview.chromium.org/2501863003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:98:
callbackBufferSizeHistogram.sample(m_callbackBufferSize);
rtoy@ - callback buffer size will be different for different latencies, won't
this histogram loose its meaning?
Is WebAudio interested in a separate histogram for each latency (keeping the
existing one for Interactive)?

[Andrew MacPherson, 2016-12-08 10:06:12]

Thanks olka@!

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:54: // Callback to get
output device params (for tests)
On 2016/12/07 20:56:01, o1ka wrote:
> nit:  end with "."

Done.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:60: // Callback get render
frame ID for current context (for tests)
On 2016/12/07 20:56:01, o1ka wrote:
> nits: empty line before this one, dot in the end

Done.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:38: class
MockRendererWebAudioDeviceImpl : public RendererWebAudioDeviceImpl {
On 2016/12/07 20:56:01, o1ka wrote:
> Nit: such class is usually called  FooUnderTest. (this is not a mock, this is
> the real class we are testing).
> 
> (BTW I realized that an alternative to passing callbacks as constructor
> parameters may be wrapping static calls in RendererWebAudioDevice virtual
> functions, and overloading them in this "UnderTest" object - since you have it
> anyways. This may look a bit neater -  but up to you, current approach is
fine.)

Done, fixed the class name. Re: virtual methods I had considered that but as the
methods are used in the WebAudioDevice constructor I believe that C++ would not
call the most-derived version of the virtual method. Not 100% sure here though
and if you're ok with the current approach I'll leave it as-is. :)

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:97:
scoped_refptr<media::MockAudioRendererSink> fake_sink(
On 2016/12/07 20:56:01, o1ka wrote:
> Is this local needed?

Done.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:104:
EXPECT_CALL(*mock_sink_.get(), Start());
On 2016/12/07 20:56:01, o1ka wrote:
> Do you start WebAudioDeviceImpl in each test? Will TestLatencyHint fail?

The CreateAudioRendererSink method is only invoked in the
WebAudioDevice::start() method so the LatencyHint tests pass as start() is never
called there.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:106: 
On 2016/12/07 20:56:01, o1ka wrote:
> We should ensure the sink gets stopped - this is a requirement for sink users.

Done, I've moved the EXPECT_CALL for Stop up to where the ones for Start and
Play are. ATM this is never hit because there's a DCHECK(!sink) in the
WebAudioDevice destructor that gets hit first if someone forgets a stop() but
it's there in case this changes.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:133:
EXPECT_GT(balanced_frames, interactive_frames);
On 2016/12/07 20:56:01, o1ka wrote:
> Have you considered comparing with media::AudioLatency values?

I've added a latencyType() method to the WebAudioDevice and a check to ensure
that the AudioLatency::LatencyType values are ordered as expected. I've also
left in the comparisons on framesPerBuffer sizes, let me know if this is what
you intended!

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:143: 
On 2016/12/07 20:56:01, o1ka wrote:
> Could you add testing that sampleRate() and framesPerBuffer() return correct
> values before and after WAD is started, and after it is stopped? And that a
> start-stop-start-stop sequence works correctly.

Done, I just modified the very simple StartStop test to start and stop twice and
to validate the framesPerBuffer and sampleRate after each call.

[Andrew MacPherson, 2016-12-08 10:23:49]

The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 10:24:04]

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, 2016-12-08 10:24:05]

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

[commit-bot: I haz the power, 2016-12-08 10:24:06]

Dry run: No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[o1ka, 2016-12-08 10:57:19]

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:38: class
MockRendererWebAudioDeviceImpl : public RendererWebAudioDeviceImpl {
On 2016/12/08 10:06:12, Andrew MacPherson wrote:
> On 2016/12/07 20:56:01, o1ka wrote:
> > Nit: such class is usually called  FooUnderTest. (this is not a mock, this
is
> > the real class we are testing).
> > 
> > (BTW I realized that an alternative to passing callbacks as constructor
> > parameters may be wrapping static calls in RendererWebAudioDevice virtual
> > functions, and overloading them in this "UnderTest" object - since you have
it
> > anyways. This may look a bit neater -  but up to you, current approach is
> fine.)
> 
> Done, fixed the class name. Re: virtual methods I had considered that but as
the
> methods are used in the WebAudioDevice constructor I believe that C++ would
not
> call the most-derived version of the virtual method. Not 100% sure here though
> and if you're ok with the current approach I'll leave it as-is. :)

Sounds good.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:104:
EXPECT_CALL(*mock_sink_.get(), Start());
On 2016/12/08 10:06:12, Andrew MacPherson wrote:
> On 2016/12/07 20:56:01, o1ka wrote:
> > Do you start WebAudioDeviceImpl in each test? Will TestLatencyHint fail?
> 
> The CreateAudioRendererSink method is only invoked in the
> WebAudioDevice::start() method so the LatencyHint tests pass as start() is
never
> called there.

Acknowledged.

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:133:
EXPECT_GT(balanced_frames, interactive_frames);
On 2016/12/08 10:06:12, Andrew MacPherson wrote:
> On 2016/12/07 20:56:01, o1ka wrote:
> > Have you considered comparing with media::AudioLatency values?
> 
> I've added a latencyType() method to the WebAudioDevice and a check to ensure
> that the AudioLatency::LatencyType values are ordered as expected. I've also
> left in the comparisons on framesPerBuffer sizes, let me know if this is what
> you intended!

Sorry, I was unclear. latencyType() method is not needed. I meant comparing
number of frames with AudioLatency::Get... function results.

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:148:
EXPECT_EQ(webaudio_device_->framesPerBuffer(), kHardwareBufferSize);
Here and below: shouldn't WAD return latency-dependent buffer size?

[Andrew MacPherson, 2016-12-08 12:22:54]

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:133:
EXPECT_GT(balanced_frames, interactive_frames);
On 2016/12/08 10:57:19, o1ka wrote:
> On 2016/12/08 10:06:12, Andrew MacPherson wrote:
> > On 2016/12/07 20:56:01, o1ka wrote:
> > > Have you considered comparing with media::AudioLatency values?
> > 
> > I've added a latencyType() method to the WebAudioDevice and a check to
ensure
> > that the AudioLatency::LatencyType values are ordered as expected. I've also
> > left in the comparisons on framesPerBuffer sizes, let me know if this is
what
> > you intended!
> 
> Sorry, I was unclear. latencyType() method is not needed. I meant comparing
> number of frames with AudioLatency::Get... function results.

Ah, sorry, I think I read too quickly and misunderstood you. I have updated this
now to compare the frame sizes to the ones returned from the AudioLatency calls.

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:148:
EXPECT_EQ(webaudio_device_->framesPerBuffer(), kHardwareBufferSize);
On 2016/12/08 10:57:19, o1ka wrote:
> Here and below: shouldn't WAD return latency-dependent buffer size?

Yes you're right, I was cutting a corner since the interactive size is currently
the same as the hardware buffer size but have updated this correctly now. :)

[o1ka, 2016-12-08 13:10:16]

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/140001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:133:
EXPECT_GT(balanced_frames, interactive_frames);
On 2016/12/08 12:22:54, Andrew MacPherson wrote:
> On 2016/12/08 10:57:19, o1ka wrote:
> > On 2016/12/08 10:06:12, Andrew MacPherson wrote:
> > > On 2016/12/07 20:56:01, o1ka wrote:
> > > > Have you considered comparing with media::AudioLatency values?
> > > 
> > > I've added a latencyType() method to the WebAudioDevice and a check to
> ensure
> > > that the AudioLatency::LatencyType values are ordered as expected. I've
also
> > > left in the comparisons on framesPerBuffer sizes, let me know if this is
> what
> > > you intended!
> > 
> > Sorry, I was unclear. latencyType() method is not needed. I meant comparing
> > number of frames with AudioLatency::Get... function results.
> 
> Ah, sorry, I think I read too quickly and misunderstood you. I have updated
this
> now to compare the frame sizes to the ones returned from the AudioLatency
calls.

Great, thanks!

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:148:
EXPECT_EQ(webaudio_device_->framesPerBuffer(), kHardwareBufferSize);
On 2016/12/08 12:22:54, Andrew MacPherson wrote:
> On 2016/12/08 10:57:19, o1ka wrote:
> > Here and below: shouldn't WAD return latency-dependent buffer size?
> 
> Yes you're right, I was cutting a corner since the interactive size is
currently
> the same as the hardware buffer size but have updated this correctly now. :)

Could you check it for each latency type, so that we are sure that we do not mix
up hardware buffer size and sink buffer size at any stage? It may worth
combining TestLatencyHint and this one

[Andrew MacPherson, 2016-12-08 13:53:53]

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:148:
EXPECT_EQ(webaudio_device_->framesPerBuffer(), kHardwareBufferSize);
On 2016/12/08 13:10:16, o1ka wrote:
> On 2016/12/08 12:22:54, Andrew MacPherson wrote:
> > On 2016/12/08 10:57:19, o1ka wrote:
> > > Here and below: shouldn't WAD return latency-dependent buffer size?
> > 
> > Yes you're right, I was cutting a corner since the interactive size is
> currently
> > the same as the hardware buffer size but have updated this correctly now. :)
> 
> Could you check it for each latency type, so that we are sure that we do not
mix
> up hardware buffer size and sink buffer size at any stage? It may worth
> combining TestLatencyHint and this one

No problem, done. The code for checking each type could be refactored out into a
function that each type calls but I like that the failure message now gives a
line number that points to exactly which type is failing, let me know if this is
not recommended though.

[o1ka, 2016-12-08 14:07:07]

On 2016/12/08 13:53:53, Andrew MacPherson wrote:
>
https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
> File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2501863003/diff/160001/content/renderer/media...
> content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:148:
> EXPECT_EQ(webaudio_device_->framesPerBuffer(), kHardwareBufferSize);
> On 2016/12/08 13:10:16, o1ka wrote:
> > On 2016/12/08 12:22:54, Andrew MacPherson wrote:
> > > On 2016/12/08 10:57:19, o1ka wrote:
> > > > Here and below: shouldn't WAD return latency-dependent buffer size?
> > > 
> > > Yes you're right, I was cutting a corner since the interactive size is
> > currently
> > > the same as the hardware buffer size but have updated this correctly now.
:)
> > 
> > Could you check it for each latency type, so that we are sure that we do not
> mix
> > up hardware buffer size and sink buffer size at any stage? It may worth
> > combining TestLatencyHint and this one
> 
> No problem, done. The code for checking each type could be refactored out into
a
> function that each type calls but I like that the failure message now gives a
> line number that points to exactly which type is failing, let me know if this
is
> not recommended though.

Awesome, thanks!
content/renderer lgtm

[Raymond Toy, 2016-12-08 16:24:50]

https://codereview.chromium.org/2501863003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/audio/AudioDestination.cpp (right):

https://codereview.chromium.org/2501863003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/audio/AudioDestination.cpp:98:
callbackBufferSizeHistogram.sample(m_callbackBufferSize);
On 2016/12/07 20:56:01, o1ka wrote:
> rtoy@ - callback buffer size will be different for different latencies, won't
> this histogram loose its meaning?
> Is WebAudio interested in a separate histogram for each latency (keeping the
> existing one for Interactive)?

Good point. It probably would be a good idea to add two more histograms to keep
track of the categories.  And an additional two more, maybe, to keep track of
the requested float latency and the actual value.

But we don't need to block this CL for that.  We have time to do that later.

[Raymond Toy, 2016-12-08 17:14:38]

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:57: }).throw();
Include the type of error thrown in "throw()".

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:62: }).throw();
Include the type of error thrown.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:9:
LatencyHint latencyHint = "interactive";
Does this introduce LatencyHint into the global namespace?  If so, that's
probably not something we want.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:71: //
hardware.
This isn't the size of the underlying audio hardware is it? Isn't it the buffer
size for the AudioContext, which is related to the audio hardware size, but not
necessary equal to it.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (left):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:117:
initialize();
Ah, nice catch.  You removed this because OfflineAudioContext was calling this
to create the context, which sets m_destinationNode here, but the
OfflineAudioContext would replace it with another right away.

Right?

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:12: class
WebAudioLatencyHint {
I assume this class will also be used to hold the float latency value.

Would it be hard to make this include the necessary information.  I think it
would be ideal if we supported the necessary slots and methods to expose this,
even if we don't support float latency right now.

I'm just trying to avoid a future big change if this class is not structured
correctly for a float latency.

[Andrew MacPherson, 2016-12-09 12:34:14]

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:57: }).throw();
On 2016/12/08 17:14:38, Raymond Toy wrote:
> Include the type of error thrown in "throw()".

Done.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/webaudio/audiocontextoptions.html:62: }).throw();
On 2016/12/08 17:14:38, Raymond Toy wrote:
> Include the type of error thrown.

Done.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:9:
LatencyHint latencyHint = "interactive";
On 2016/12/08 17:14:38, Raymond Toy wrote:
> Does this introduce LatencyHint into the global namespace?  If so, that's
> probably not something we want.

I believe it does. I've renamed it to AudioContextLatencyHint, does that work?
hongchan@ mentioned in a previous comment that having this typedef is the way to
ensure that the AudioContextLatencyCategoryOrDouble type is generated properly.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioDestinationNode.h:71: //
hardware.
On 2016/12/08 17:14:38, Raymond Toy wrote:
> This isn't the size of the underlying audio hardware is it? Isn't it the
buffer
> size for the AudioContext, which is related to the audio hardware size, but
not
> necessary equal to it.

Makes sense, I've updated it now.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (left):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:117:
initialize();
On 2016/12/08 17:14:38, Raymond Toy wrote:
> Ah, nice catch.  You removed this because OfflineAudioContext was calling this
> to create the context, which sets m_destinationNode here, but the
> OfflineAudioContext would replace it with another right away.
> 
> Right?

The OfflineAudioContext actually uses a different constructor so it wasn't being
set here in that case, however now that the DefaultAudioDestinationNode takes a
WebAudioLatencyHint parameter I figured it would make sense to move these lines
into the AudioContext so that the BaseAudioContext doesn't need to know about
the WebAudioLatencyHint. This also made it so that both the OfflineAudioContext
and the AudioContext are now responsible for the m_destinationNode rather than
the BaseAudioContext creating it in one case only but not the other.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:12: class
WebAudioLatencyHint {
On 2016/12/08 17:14:38, Raymond Toy wrote:
> I assume this class will also be used to hold the float latency value.
> 
> Would it be hard to make this include the necessary information.  I think it
> would be ideal if we supported the necessary slots and methods to expose this,
> even if we don't support float latency right now.
> 
> I'm just trying to avoid a future big change if this class is not structured
> correctly for a float latency.

No problem, I've just added a new Category, double m_seconds member, constructor
and getter method. I also had to add this new Category to a switch statement in
content/renderer, hopefully this doesn't affect olka@'s lgtm. Does that work?

[Raymond Toy, 2016-12-13 15:53:11]

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:9:
LatencyHint latencyHint = "interactive";
On 2016/12/09 12:34:14, Andrew MacPherson wrote:
> On 2016/12/08 17:14:38, Raymond Toy wrote:
> > Does this introduce LatencyHint into the global namespace?  If so, that's
> > probably not something we want.
> 
> I believe it does. I've renamed it to AudioContextLatencyHint, does that work?
> hongchan@ mentioned in a previous comment that having this typedef is the way
to
> ensure that the AudioContextLatencyCategoryOrDouble type is generated
properly.

That would be unfortunate, I think.  In my original CL, I don't think I had to
to this.  AudioContextLatencyCategoryOrDouble was defined in the C++ code,
eventually.  I don't 
remember what I had to do to make this happen.  Or maybe I'm just wrong.

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp (left):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/BaseAudioContext.cpp:117:
initialize();
On 2016/12/09 12:34:14, Andrew MacPherson wrote:
> On 2016/12/08 17:14:38, Raymond Toy wrote:
> > Ah, nice catch.  You removed this because OfflineAudioContext was calling
this
> > to create the context, which sets m_destinationNode here, but the
> > OfflineAudioContext would replace it with another right away.
> > 
> > Right?
> 
> The OfflineAudioContext actually uses a different constructor so it wasn't
being
> set here in that case, however now that the DefaultAudioDestinationNode takes
a
> WebAudioLatencyHint parameter I figured it would make sense to move these
lines
> into the AudioContext so that the BaseAudioContext doesn't need to know about
> the WebAudioLatencyHint. This also made it so that both the
OfflineAudioContext
> and the AudioContext are now responsible for the m_destinationNode rather than
> the BaseAudioContext creating it in one case only but not the other.

Thanks for the explanation.  This makes the code much nicer.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:14: enum Category {
Category seems a bit terse.  Maybe AudioLatencyCategory or even
AudioContextLatencyCategory (if this doesn't collide with  anything.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:19: };
https://www.chromium.org/blink/coding-style, item 12 says to use kCamelCase for
enums.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:28: } else {
I think you need a case for category == CategoryExact.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:40: double seconds()
const { return m_seconds; }
Should there be asserts here so that you only ask for seconds() when the
category is CategoryExact?

[Andrew MacPherson, 2016-12-14 09:07:20]

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:9:
LatencyHint latencyHint = "interactive";
On 2016/12/13 15:53:11, Raymond Toy wrote:
> On 2016/12/09 12:34:14, Andrew MacPherson wrote:
> > On 2016/12/08 17:14:38, Raymond Toy wrote:
> > > Does this introduce LatencyHint into the global namespace?  If so, that's
> > > probably not something we want.
> > 
> > I believe it does. I've renamed it to AudioContextLatencyHint, does that
work?
> > hongchan@ mentioned in a previous comment that having this typedef is the
way
> to
> > ensure that the AudioContextLatencyCategoryOrDouble type is generated
> properly.
> 
> That would be unfortunate, I think.  In my original CL, I don't think I had to
> to this.  AudioContextLatencyCategoryOrDouble was defined in the C++ code,
> eventually.  I don't 
> remember what I had to do to make this happen.  Or maybe I'm just wrong.

The original CL doesn't compile but I took another look here and you're right
that the typedef is not needed. This was my mistake, the part I believe I was
missing was that the type's .cpp/.h files needed to be included in
Source/bindings/modules/v8/generated.gni (they are now but weren't when I was
first looking at this). I've removed the typedef now, thanks!

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:14: enum Category {
On 2016/12/13 15:53:11, Raymond Toy wrote:
> Category seems a bit terse.  Maybe AudioLatencyCategory or even
> AudioContextLatencyCategory (if this doesn't collide with  anything.

Done, with AudioContextLatencyCategory.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:19: };
On 2016/12/13 15:53:11, Raymond Toy wrote:
> https://www.chromium.org/blink/coding-style, item 12 says to use kCamelCase
for
> enums.

Done.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:28: } else {
On 2016/12/13 15:53:11, Raymond Toy wrote:
> I think you need a case for category == CategoryExact.

Should it be possible to create a WebAudioLatencyHint with CategoryExact by
passing a String into the constructor? Right now the IDL prevents passing
anything but these three strings.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:40: double seconds()
const { return m_seconds; }
On 2016/12/13 15:53:11, Raymond Toy wrote:
> Should there be asserts here so that you only ask for seconds() when the
> category is CategoryExact?

Done.

[Raymond Toy, 2016-12-14 18:23:33]

lgtm for the webaudio parts.

Thanks for working on this and for your patience during the review.

We will, of course have to still wait for the 3 lgtms on the intent.

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:28: } else {
On 2016/12/14 09:07:20, Andrew MacPherson wrote:
> On 2016/12/13 15:53:11, Raymond Toy wrote:
> > I think you need a case for category == CategoryExact.
> 
> Should it be possible to create a WebAudioLatencyHint with CategoryExact by
> passing a String into the constructor? Right now the IDL prevents passing
> anything but these three strings.

Sorry, you're right. It shouldn't be possible, and you have tests for this
already.

[Andrew MacPherson, 2016-12-14 19:51:00]

Awesome, thanks Raymond and thanks again for all the help here!

Regarding the intent, is there anything I can do to help move the PR you filed
against the Web Audio spec forward or is it just waiting for approval from other
WG members?

[Raymond Toy, 2016-12-14 19:54:53]

On 2016/12/14 19:51:00, Andrew MacPherson wrote:
> Awesome, thanks Raymond and thanks again for all the help here!
> 
> Regarding the intent, is there anything I can do to help move the PR you filed
> against the Web Audio spec forward or is it just waiting for approval from
other
> WG members?

We have a teleconf tomorrow and I'll be sure to bring it up.  I'll let you know
what happens.

[hongchan, 2016-12-20 19:40:23]

Raymond already l-g-t-m this, but I'll leave mine here as well.

lgtm

[Andrew MacPherson, 2017-02-08 16:38:04]

The CQ bit was checked by andrew.macpherson@soundtrap.com

[Andrew MacPherson, 2017-02-08 16:38:04]

The patchset sent to the CQ was uploaded after l-g-t-m from olka@chromium.org,
rtoy@chromium.org, hongchan@chromium.org
Link to the patchset: https://codereview.chromium.org/2501863003/#ps300001
(title: "Rebasing")

[commit-bot: I haz the power, 2017-02-08 16:38:37]

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

[Andrew MacPherson, 2017-02-08 16:43:24]

The CQ bit was unchecked by andrew.macpherson@soundtrap.com

[Raymond Toy, 2017-02-08 16:45:41]

rtoy@chromium.org changed reviewers:
+ tkent@chromium.org

[Raymond Toy, 2017-02-08 16:45:42]

tkent: PTAL at this.

andrew: Can you update the description to include a link to the intent email and
a link to the feature entry on Chromium dashboard features?

[Andrew MacPherson, 2017-02-08 16:51:22]

Description was changed from

==========
Support for AudioContextOptions latencyHint.

This adds an optional AudioContextOptions argument to AudioContext
creation with a single 'latencyHint' member taking values of either
'interactive', 'balanced, or 'playback'. Support for passing a double
for an exact latency amount will be added in a subsequent CL.

BUG=564276
R=rtoy@chromium.org
==========

to

==========
Support for AudioContextOptions latencyHint.

This adds an optional AudioContextOptions argument to AudioContext
creation with a single 'latencyHint' member taking values of either
'interactive', 'balanced, or 'playback'. Support for passing a double
for an exact latency amount will be added in a subsequent CL.

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t9SapYQiUtI/...

Earlier thread:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FyxYC3TE8ck/discus...

Chrome feature dashboard: https://www.chromestatus.com/features/5678699475107840

BUG=564276
R=rtoy@chromium.org
==========

[Andrew MacPherson, 2017-02-08 16:51:23]

andrew.macpherson@soundtrap.com changed reviewers:
- tkent@chromium.org

[Andrew MacPherson, 2017-02-08 16:51:52]

On 2017/02/08 at 16:45:42, rtoy wrote:
> tkent: PTAL at this.
> 
> andrew: Can you update the description to include a link to the intent email
and a link to the feature entry on Chromium dashboard features?

Done, links are added now!

[tkent, 2017-02-08 22:38:36]

Web-exposed change lgtm.

[tkent, 2017-02-08 22:38:45]

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

[tkent, 2017-02-08 22:38:47]

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl (right):

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
2016 -> 2017

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp (right):

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp:1: // Copyright
2016 The Chromium Authors. All rights reserved.
2016 -> 2017

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp:71: void
TearDown() override {}
Is this necessary?

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
2016 -> 2017

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:8: #include
"WebString.h"
WebString.h -> public/platform/WebString.h

https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
third_party/WebKit/public/platform/WebAudioLatencyHint.h:21: explicit
WebAudioLatencyHint(WebString category) {
WebString -> const WebString&

[Andrew MacPherson, 2017-02-09 08:24:03]

On 2017/02/08 at 22:38:47, tkent wrote:
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl
(right):
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webaudio/AudioContextOptions.idl:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
> 2016 -> 2017
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp (right):
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
> 2016 -> 2017
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/Sou...
> third_party/WebKit/Source/modules/webaudio/AudioContextTest.cpp:71: void
TearDown() override {}
> Is this necessary?
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
> File third_party/WebKit/public/platform/WebAudioLatencyHint.h (right):
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebAudioLatencyHint.h:1: // Copyright 2016
The Chromium Authors. All rights reserved.
> 2016 -> 2017
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebAudioLatencyHint.h:8: #include
"WebString.h"
> WebString.h -> public/platform/WebString.h
> 
>
https://codereview.chromium.org/2501863003/diff/300001/third_party/WebKit/pub...
> third_party/WebKit/public/platform/WebAudioLatencyHint.h:21: explicit
WebAudioLatencyHint(WebString category) {
> WebString -> const WebString&

I've pushed a fix resolving these comments, thanks! I'm going to commit the CL
now as it looks like all the required l-g-t-ms are in place. Thanks again!

[Andrew MacPherson, 2017-02-09 08:26:26]

The CQ bit was checked by andrew.macpherson@soundtrap.com

[Andrew MacPherson, 2017-02-09 08:26:27]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
hongchan@chromium.org, olka@chromium.org, rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/2501863003/#ps320001
(title: "Fixes based on reviewer comments.")

[commit-bot: I haz the power, 2017-02-09 08:26:49]

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-09 08:29:37]

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

[commit-bot: I haz the power, 2017-02-09 08:29:39]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Andrew MacPherson, 2017-02-09 12:25:57]

The CQ bit was checked by andrew.macpherson@soundtrap.com

[Andrew MacPherson, 2017-02-09 12:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
hongchan@chromium.org, olka@chromium.org, rtoy@chromium.org
Link to the patchset: https://codereview.chromium.org/2501863003/#ps340001
(title: "Update formatting from presubmit check.")

[commit-bot: I haz the power, 2017-02-09 12:26:15]

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-09 12:33:34]

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

[commit-bot: I haz the power, 2017-02-09 12:33:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Andrew MacPherson, 2017-02-09 15:56:08]

olka@ rtoy@ The Android trybots are failing in the new
RendererWebAudioDeviceImplTest.TestLatencyHintValues test due this check:

EXPECT_GE(balancedBufferSize, interactiveBufferSize);

In AudioLatency::GetInteractiveBufferSize() and GetRtcBufferSize() there is
#ifdef OS_ANDROID code that changes the buffer sizes and results in an
Interactive size of 2048 and an Rtc size (which we use for Balanced) of 882.

Any thoughts on what should be done here and does it make sense that the Rtc
buffer size is lower than the Interactive one on Android? One alternative would
be to add a GetBalancedBufferSize() to AudioLatency which just calls
GetRtcBufferSize() for most OSes and something else on Android.

Thanks again for the help here!

[o1ka, 2017-02-09 16:09:52]

On 2017/02/09 15:56:08, Andrew MacPherson wrote:
> olka@ rtoy@ The Android trybots are failing in the new
> RendererWebAudioDeviceImplTest.TestLatencyHintValues test due this check:
> 
> EXPECT_GE(balancedBufferSize, interactiveBufferSize);
> 
> In AudioLatency::GetInteractiveBufferSize() and GetRtcBufferSize() there is
> #ifdef OS_ANDROID code that changes the buffer sizes and results in an
> Interactive size of 2048 and an Rtc size (which we use for Balanced) of 882.
> 
> Any thoughts on what should be done here and does it make sense that the Rtc
> buffer size is lower than the Interactive one on Android? One alternative
would
> be to add a GetBalancedBufferSize() to AudioLatency which just calls
> GetRtcBufferSize() for most OSes and something else on Android.
> 
> Thanks again for the help here!

Could you remind me why do we have these two comparison checks here? 
Generally there is no guarantee on the buffer size ordering basing on the
category, because buffer sizes are adjusted per platform to address various
hw/platform issues.

[Raymond Toy, 2017-02-09 16:35:23]

On 2017/02/09 16:09:52, o1ka (CET) wrote:
> On 2017/02/09 15:56:08, Andrew MacPherson wrote:
> > olka@ rtoy@ The Android trybots are failing in the new
> > RendererWebAudioDeviceImplTest.TestLatencyHintValues test due this check:
> > 
> > EXPECT_GE(balancedBufferSize, interactiveBufferSize);
> > 
> > In AudioLatency::GetInteractiveBufferSize() and GetRtcBufferSize() there is
> > #ifdef OS_ANDROID code that changes the buffer sizes and results in an
> > Interactive size of 2048 and an Rtc size (which we use for Balanced) of 882.
> > 
> > Any thoughts on what should be done here and does it make sense that the Rtc
> > buffer size is lower than the Interactive one on Android? One alternative
> would
> > be to add a GetBalancedBufferSize() to AudioLatency which just calls
> > GetRtcBufferSize() for most OSes and something else on Android.
> > 
> > Thanks again for the help here!
> 
> Could you remind me why do we have these two comparison checks here? 
> Generally there is no guarantee on the buffer size ordering basing on the
> category, because buffer sizes are adjusted per platform to address various
> hw/platform issues.

I agree with that.  And Android latency really needs to be updated because it's
excessively conservative and really degrades performance on devices that have
good low latency audio.

[o1ka, 2017-02-09 16:39:59]

> Android latency really needs to be updated because it's
> excessively conservative and really degrades performance on devices that have
> good low latency audio.

Do we have a bug for that?

[Raymond Toy, 2017-02-09 16:43:45]

On 2017/02/09 16:39:59, o1ka (CET) wrote:
> > Android latency really needs to be updated because it's
> > excessively conservative and really degrades performance on devices that
have
> > good low latency audio.
> 
> Do we have a bug for that?

I haven't written a bug for that.  But basically it means making
GetInteractiveBufferSize() smarter so that if Android supports the low-latency
audio feature and the HW size is a power of 2, we should just use it instead
clamping it to 1024 or 2048.  Devices are much faster now than when that code
was written.

[Andrew MacPherson, 2017-02-09 16:46:06]

On 2017/02/09 at 16:43:45, rtoy wrote:
> On 2017/02/09 16:39:59, o1ka (CET) wrote:
> > > Android latency really needs to be updated because it's
> > > excessively conservative and really degrades performance on devices that
have
> > > good low latency audio.
> > 
> > Do we have a bug for that?
> 
> I haven't written a bug for that.  But basically it means making
GetInteractiveBufferSize() smarter so that if Android supports the low-latency
audio feature and the HW size is a power of 2, we should just use it instead
clamping it to 1024 or 2048.  Devices are much faster now than when that code
was written.

Ok, that makes sense to address Android later then. Should I remove those checks
from RendererWebAudioDeviceImplTest entirely or just comment them out on
Android?

[Andrew MacPherson, 2017-02-09 17:04:01]

On 2017/02/09 at 16:09:52, olka wrote:
>
> Could you remind me why do we have these two comparison checks here? 
> Generally there is no guarantee on the buffer size ordering basing on the
category, because buffer sizes are adjusted per platform to address various
hw/platform issues.

Sorry, missed answering this question as well. My assumption was based on the
wording in the WebAudio API spec here:
https://webaudio.github.io/web-audio-api/#idl-def-AudioContextLatencyCategory

It seems to imply that Interactive latency is always lowest, then Balanced, then
Playback but maybe that's not correct.

[Raymond Toy, 2017-02-09 17:08:42]

On 2017/02/09 17:04:01, Andrew MacPherson wrote:
> On 2017/02/09 at 16:09:52, olka wrote:
> >
> > Could you remind me why do we have these two comparison checks here? 
> > Generally there is no guarantee on the buffer size ordering basing on the
> category, because buffer sizes are adjusted per platform to address various
> hw/platform issues.
> 
> Sorry, missed answering this question as well. My assumption was based on the
> wording in the WebAudio API spec here:
> https://webaudio.github.io/web-audio-api/#idl-def-AudioContextLatencyCategory
> 
> It seems to imply that Interactive latency is always lowest, then Balanced,
then
> Playback but maybe that's not correct.

I think it would be ok to remove the ordering check for now, at least on
Android.  We'll need to improve Android's low latency support and also see how
balanced works.  If, as you mentioned above that balanced returns 882 and that
actually sounds ok, we could at the least make low latency return the same
value.  But that's a different bug which we need to file.

[Andrew MacPherson, 2017-02-09 17:25:00]

andrew.macpherson@soundtrap.com changed reviewers:
+ pfeldman@chromium.org

[Andrew MacPherson, 2017-02-09 17:25:38]

Hi pfeldman@, would you be able to take a quick look at these files from the CL,
which still need an OWNER's l-g-t-m:

content/public/renderer/content_renderer_client.cc
content/public/renderer/content_renderer_client.h
content/renderer/media/renderer_webaudiodevice_impl.cc
content/renderer/media/renderer_webaudiodevice_impl.h
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc
content/renderer/renderer_blink_platform_impl.cc
content/renderer/renderer_blink_platform_impl.h
content/shell/renderer/layout_test/layout_test_content_renderer_client.cc
content/shell/renderer/layout_test/layout_test_content_renderer_client.h

Thanks a lot for any help!

[Andrew MacPherson, 2017-02-09 17:40:50]

The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 17:41:35]

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-09 19:38:10]

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

[commit-bot: I haz the power, 2017-02-09 19:38:12]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[pfeldman, 2017-02-09 19:57:10]

lgtm

[pfeldman, 2017-02-09 20:48:18]

lgtm

[Andrew MacPherson, 2017-02-10 07:19:02]

On 2017/02/09 at 20:48:18, pfeldman wrote:
> lgtm

Great, thanks for the quick turnaround pfeldman!

There's one remaining Android failure that I'll need to use the trybots to
diagnose but with any luck I'll be able to land this today.

[Andrew MacPherson, 2017-02-10 09:43:46]

The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run

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

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

[Andrew MacPherson, 2017-02-10 09:44:45]

The CQ bit was unchecked by andrew.macpherson@soundtrap.com

[Andrew MacPherson, 2017-02-10 11:07:14]

The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run

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

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

[Andrew MacPherson, 2017-02-10 12:06:05]

olka@ It looks like the Android failure in the new WebAudioDeviceImpl test is
due to a call to RenderThreadImpl::current()->GetMediaThreadTaskRunner() in
RendererWebAudioDeviceImpl::start() on Android only, the RenderThread appears to
be null when running the content_unittests. I've commented out the test entirely
on Android for now but just wanted to check with you that this is ok before I
commit. Thanks again!

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

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

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

Dry run: This issue passed the CQ dry run.

[o1ka, 2017-02-10 13:44:35]

On 2017/02/10 12:06:05, Andrew MacPherson wrote:
> olka@ It looks like the Android failure in the new WebAudioDeviceImpl test is
> due to a call to RenderThreadImpl::current()->GetMediaThreadTaskRunner() in
> RendererWebAudioDeviceImpl::start() on Android only, the RenderThread appears
to
> be null when running the content_unittests. I've commented out the test
entirely
> on Android for now but just wanted to check with you that this is ok before I
> commit. Thanks again!

This is not ok. We need to fix it. RendererWebAudioDeviceImpl can receive a task
runner as a constructor parameter or have "SetMediaTaskRunnerForTesting()"
method (the latter is pretty common and may be easier),

[Andrew MacPherson, 2017-02-10 15:33:36]

On 2017/02/10 at 13:44:35, olka wrote:
> On 2017/02/10 12:06:05, Andrew MacPherson wrote:
> > olka@ It looks like the Android failure in the new WebAudioDeviceImpl test
is
> > due to a call to RenderThreadImpl::current()->GetMediaThreadTaskRunner() in
> > RendererWebAudioDeviceImpl::start() on Android only, the RenderThread
appears to
> > be null when running the content_unittests. I've commented out the test
entirely
> > on Android for now but just wanted to check with you that this is ok before
I
> > commit. Thanks again!
> 
> This is not ok. We need to fix it. RendererWebAudioDeviceImpl can receive a
task runner as a constructor parameter or have "SetMediaTaskRunnerForTesting()"
method (the latter is pretty common and may be easier),

Ok no problem, I will add this first thing next week, verify that it's running
properly on Android (minus the latency ordering check there) and then look into
landing the CL at that point. Thanks again for the help here and have a good
weekend!

[Andrew MacPherson, 2017-02-12 13:47:25]

The CQ bit was checked by andrew.macpherson@soundtrap.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-12 13:47:39]

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-12 15:24:02]

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

[commit-bot: I haz the power, 2017-02-12 15:24:03]

Dry run: This issue passed the CQ dry run.

[Andrew MacPherson, 2017-02-12 19:32:53]

On 2017/02/10 at 13:44:35, olka wrote:
> 
> This is not ok. We need to fix it. RendererWebAudioDeviceImpl can receive a
task runner as a constructor parameter or have "SetMediaTaskRunnerForTesting()"
method (the latter is pretty common and may be easier),

olka@ I've updated the test to use a SetMediaTaskRunnerForTesting() method now
and verified that it's passing on Android. Let me know if that's ok and I'll try
to land the CL. Thanks!

[o1ka, 2017-02-13 08:29:30]

Thanks Andrew!
a question:

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:79: const
scoped_refptr<base::SingleThreadTaskRunner>& GetMediaTaskRunner();
private?

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:114:
base::MessageLoop message_loop_;
Why both |task_runner_| and |message_loop_| are needed?

[Andrew MacPherson, 2017-02-13 08:52:21]

Thanks olka!

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl.h (right):

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl.h:79: const
scoped_refptr<base::SingleThreadTaskRunner>& GetMediaTaskRunner();
On 2017/02/13 at 08:29:30, o1ka (CET) wrote:
> private?

Done.

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):

https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:114:
base::MessageLoop message_loop_;
On 2017/02/13 at 08:29:30, o1ka (CET) wrote:
> Why both |task_runner_| and |message_loop_| are needed?

Without the MessageLoop then in the SilentSinkSuspender constructor for Android
there is a call to base::ThreadTaskRunnerHandle::Get() which was failing as the
ThreadTaskRunnerHandle isn't initialized. I looked at how other unit tests were
initializing it and it seemed to be by just instantiating a MessageLoop, but I
see what you mean now about the need for both the loop and the task_runner. I've
removed the task_runner_ now and am just using message_loop_.task_runner()
instead, does that make more sense?

[o1ka, 2017-02-13 09:07:35]

On 2017/02/13 08:52:21, Andrew MacPherson wrote:
> Thanks olka!
> 
>
https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
> File content/renderer/media/renderer_webaudiodevice_impl.h (right):
> 
>
https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
> content/renderer/media/renderer_webaudiodevice_impl.h:79: const
> scoped_refptr<base::SingleThreadTaskRunner>& GetMediaTaskRunner();
> On 2017/02/13 at 08:29:30, o1ka (CET) wrote:
> > private?
> 
> Done.
> 
>
https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
> File content/renderer/media/renderer_webaudiodevice_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/2501863003/diff/440001/content/renderer/media...
> content/renderer/media/renderer_webaudiodevice_impl_unittest.cc:114:
> base::MessageLoop message_loop_;
> On 2017/02/13 at 08:29:30, o1ka (CET) wrote:
> > Why both |task_runner_| and |message_loop_| are needed?
> 
> Without the MessageLoop then in the SilentSinkSuspender constructor for
Android
> there is a call to base::ThreadTaskRunnerHandle::Get() which was failing as
the
> ThreadTaskRunnerHandle isn't initialized. I looked at how other unit tests
were
> initializing it and it seemed to be by just instantiating a MessageLoop, but I
> see what you mean now about the need for both the loop and the task_runner.
I've
> removed the task_runner_ now and am just using message_loop_.task_runner()
> instead, does that make more sense?

Yes, lgtm - thanks!

[Andrew MacPherson, 2017-02-13 09:12:14]

The CQ bit was checked by andrew.macpherson@soundtrap.com

[Andrew MacPherson, 2017-02-13 09:12:15]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
hongchan@chromium.org, rtoy@chromium.org, pfeldman@chromium.org
Link to the patchset: https://codereview.chromium.org/2501863003/#ps460001
(title: "Fixes to WebAudioDeviceImpl unit test.")

[commit-bot: I haz the power, 2017-02-13 09:12:35]

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-13 10:51:06]

CQ is committing da patch.

Bot data: {"patchset_id": 460001, "attempt_start_ts": 1486977134900530,
"parent_rev": "d674502071b65ada9815fdb81b92beeda4add7dc", "commit_rev":
"4f85155e4b7ae25e1dfb21cf686890fb8634eb6f"}

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

Description was changed from

==========
Support for AudioContextOptions latencyHint.

This adds an optional AudioContextOptions argument to AudioContext
creation with a single 'latencyHint' member taking values of either
'interactive', 'balanced, or 'playback'. Support for passing a double
for an exact latency amount will be added in a subsequent CL.

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t9SapYQiUtI/...

Earlier thread:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FyxYC3TE8ck/discus...

Chrome feature dashboard: https://www.chromestatus.com/features/5678699475107840

BUG=564276
R=rtoy@chromium.org
==========

to

==========
Support for AudioContextOptions latencyHint.

This adds an optional AudioContextOptions argument to AudioContext
creation with a single 'latencyHint' member taking values of either
'interactive', 'balanced, or 'playback'. Support for passing a double
for an exact latency amount will be added in a subsequent CL.

Intent:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/t9SapYQiUtI/...

Earlier thread:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/FyxYC3TE8ck/discus...

Chrome feature dashboard: https://www.chromestatus.com/features/5678699475107840

BUG=564276
R=rtoy@chromium.org

Review-Url: https://codereview.chromium.org/2501863003
Cr-Commit-Position: refs/heads/master@{#449927}
Committed:
https://chromium.googlesource.com/chromium/src/+/4f85155e4b7ae25e1dfb21cf6868...
==========

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

Committed patchset #24 (id:460001) as
https://chromium.googlesource.com/chromium/src/+/4f85155e4b7ae25e1dfb21cf6868...