Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1195633003:
Add a silent audio sink to consume WebAudio data on silence detection. (Closed)
|
Created:
5 years, 6 months ago by qinmin Modified:
5 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mcasas+watch_chromium.org, mkwst+moarreviews-renderer_chromium.org, mlamouri (slow - plz ping), posciak+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd a silent audio sink to consume WebAudio data on silence detection.
The current android implementation of WebAudio is not power friendly.
It holds the AudioMix wakelock, and causes a lot of battery consumption even if tab is backgrounded.
When an AudioContext is created, WebAudio will start enqueueing data to the output device.
This happens even when no data is decoded or no audio buffer is appended.
This CL adds a SilentAudioSink to consume data when consecutive empty audio buffers are received.
In the idle mode, the player will no longer enqueue data to the output device.
It regularly check the received data and exits the idle mode if non-empty data are encountered.
The intervals to check the data is calculated by the consumption time of the last received buffer.
BUG=470153
Committed: https://crrev.com/bda540eb1cbfd36dda3eec0206913331ca1ab822
Cr-Commit-Position: refs/heads/master@{#335799}
Patch Set 1 #Patch Set 2 : add missing silent_audio_sink.cc #Patch Set 3 : simplify the CL to use NullAudioSink #
Total comments: 15
Patch Set 4 : addressing comments #
Total comments: 31
Patch Set 5 : remove weak_ptr #
Total comments: 6
Patch Set 6 : nits #Patch Set 7 : ifdef android only constants #
Messages
Total messages: 31 (8 generated)
qinmin@chromium.org changed reviewers: + dalecurtis@chromium.org, rtoy@chromium.org
This CL moves the silence detection logic into WebAudio layer, PTAL.
Looks like you didn't upload SilentAudioSink, but it looks like all the logic
for silence detection is in the WebAudio impl, so why not just use
NullAudioSink?
Then inside of Render:
if (fake && !have_silence) {
// suspend fake callbacks.
// restart real callbacks.
// return real_data once started.
} else if (have_silence) {
// start fake callbacks.
}
No need for the RenderInternal or OnSuspend/OnResume methods.
silent_audio_sink.h(cc) are attached.
The problem to put everything inside render() is threading. Render() was called
on the AudioDeviceThread, and that is causing a lot of troubles:
1. To suspend real callbacks, we need to call pause() on the output device. Here
is a comment from renderer_webaudiodevice_impl.h, "to avoid the need for
locking, ensure the control methods of the
blink::WebAudioDevice implementation are called on the same thread". So we need
to post a task to suspend real callbacks.
2. AudioDeviceThread is a special thread with no message loops or task runners.
So the audio sink cannot be created on the it as there is no way for it to post
tasks later on. And AudioDeviceThread will hang for listening to the socket once
pause() is called.
So we need to create the audio sink on the main render thread, and IMHO we
better split things into separate functions so we it is much clear for us to
know functions are executed on which threads.
On 2015/06/18 22:41:18, DaleCurtis wrote:
> Looks like you didn't upload SilentAudioSink, but it looks like all the logic
> for silence detection is in the WebAudio impl, so why not just use
> NullAudioSink?
>
> Then inside of Render:
> if (fake && !have_silence) {
> // suspend fake callbacks.
> // restart real callbacks.
> // return real_data once started.
> } else if (have_silence) {
> // start fake callbacks.
> }
>
> No need for the RenderInternal or OnSuspend/OnResume methods.
I don't follow, it shouldn't matter if the device thread has a message loop.
This code should be fine:
Render() {
// acquire data
if (fake && have_data) {
// okay to call play here it will just trampoline to IO.
audio_device_->Play();
} else (!have_data && deadline_exceeded) {
audio_device_->Pause();
fake = true;
}
if (fake)
task_runner_->PostTask(Render, delay);
}
No need for the timestamp helper either I think, just use base::TimeTicks.
On 2015/06/19 01:08:20, DaleCurtis wrote:
> I don't follow, it shouldn't matter if the device thread has a message loop.
> This code should be fine:
>
> Render() {
> // acquire data
>
> if (fake && have_data) {
> // okay to call play here it will just trampoline to IO.
> audio_device_->Play();
> } else (!have_data && deadline_exceeded) {
> audio_device_->Pause();
> fake = true;
> }
>
> if (fake)
> task_runner_->PostTask(Render, delay);
> }
>
> No need for the timestamp helper either I think, just use base::TimeTicks.
You need a lock in this case since audio_device_ is accessed on both audio
device thread and the render main thread, posting a task to call suspend() and
resume() avoids the usage of a lock.
audio timestamp helper just facilitates the calculation of the next callback
delay, and to guard against drifting (so we don't accumulate small delay errors
over time)
On 2015/06/19 03:03:20, qinmin wrote:
> On 2015/06/19 01:08:20, DaleCurtis wrote:
> > I don't follow, it shouldn't matter if the device thread has a message loop.
> > This code should be fine:
> >
> > Render() {
> > // acquire data
> >
> > if (fake && have_data) {
> > // okay to call play here it will just trampoline to IO.
> > audio_device_->Play();
> > } else (!have_data && deadline_exceeded) {
> > audio_device_->Pause();
> > fake = true;
> > }
> >
> > if (fake)
> > task_runner_->PostTask(Render, delay);
> > }
> >
> > No need for the timestamp helper either I think, just use base::TimeTicks.
>
> You need a lock in this case since audio_device_ is accessed on both audio
> device thread and the render main thread, posting a task to call suspend() and
> resume() avoids the usage of a lock.
> audio timestamp helper just facilitates the calculation of the next callback
> delay, and to guard against drifting (so we don't accumulate small delay
errors
> over time)
Just posting a task acquires a lock, so the commentary about avoiding locks
isn't relevant anymore. Adding a lock would be fine.
I think you shouldn't need a lock since if we're a fake we're on the right
thread, and if we're not a fake, Render() is in progress, so either no
Pause/Stop is outstanding or one is and is blocked on the completion of
Render(). In which case we'll just get a Pause() after Stop() on the IO thread,
which is harmless -- no Start() could be queued while our render thread is
blocked.
On 2015/06/19 17:41:56, DaleCurtis wrote:
> On 2015/06/19 03:03:20, qinmin wrote:
> > On 2015/06/19 01:08:20, DaleCurtis wrote:
> > > I don't follow, it shouldn't matter if the device thread has a message
loop.
> > > This code should be fine:
> > >
> > > Render() {
> > > // acquire data
> > >
> > > if (fake && have_data) {
> > > // okay to call play here it will just trampoline to IO.
> > > audio_device_->Play();
> > > } else (!have_data && deadline_exceeded) {
> > > audio_device_->Pause();
> > > fake = true;
> > > }
> > >
> > > if (fake)
> > > task_runner_->PostTask(Render, delay);
> > > }
> > >
> > > No need for the timestamp helper either I think, just use base::TimeTicks.
> >
> > You need a lock in this case since audio_device_ is accessed on both audio
> > device thread and the render main thread, posting a task to call suspend()
and
> > resume() avoids the usage of a lock.
> > audio timestamp helper just facilitates the calculation of the next callback
> > delay, and to guard against drifting (so we don't accumulate small delay
> errors
> > over time)
>
> Just posting a task acquires a lock, so the commentary about avoiding locks
> isn't relevant anymore. Adding a lock would be fine.
>
> I think you shouldn't need a lock since if we're a fake we're on the right
> thread, and if we're not a fake, Render() is in progress, so either no
> Pause/Stop is outstanding or one is and is blocked on the completion of
> Render(). In which case we'll just get a Pause() after Stop() on the IO
thread,
> which is harmless -- no Start() could be queued while our render thread is
> blocked.
I see. If we are sure that Stop() won't be called on main thread while Render()
is executing on the AudioDeviceThread, i will change the CL according to your
suggestions.
Do you have any numbers on how long it takes to resume when non-zero audio samples get played? I think this is pretty important to know. As cwilso@ mentioned, any delay above a few ms would be annoying.
I don't think the timestamp helper is necessary and actually won't work because of delays in posttask processing. Take a look at how FakeAudioConsumer calculates the delay while accounting for current time. NullAudioSink will do this for you, but can only be used from the render thread, so you'll need to coordinate post tasks in that case.
Stop may be called on the main thread, but the Render() lock will be held: https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_... So the call would block until we finish whatever we do in Render().
On 2015/06/19 17:52:19, DaleCurtis wrote: > I don't think the timestamp helper is necessary and actually won't work because > of delays in posttask processing. Take a look at how FakeAudioConsumer > calculates the delay while accounting for current time. > > NullAudioSink will do this for you, but can only be used from the render thread, > so you'll need to coordinate post tasks in that case. The delay calculation in SilentAudioSink is about the same as that of the FakeAudioWorker. However, since we fill a fixed number of frames into the AudioBus, the timestamp helper is not very helpful as we can add a constant each time when calculating the audio time.
PTAL again. Simplified the CL to use NullAudioSink and the assertion that Render() won't be called while other functions are being executed. After audio goes to the idle mode, the first audio playback takes about 5ms to start, which seems not bad at all.
https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:83: StopNullAudioSink(); Stop can be called multiple times, so I'd just call it directly. https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:108: if (!dest->AreFramesZero() && is_using_null_audio_sink_) { We lose this buffer in this case, do we want to save it for the next Render() call or just drop it? https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:118: audio_timestamp_helper_->AddFrames(dest->frames()); Instead of using a timestamp helper: On, l.108: const bool is_zero = dest->AreFramesZero(); if (!is_zero) first_silence_time_ = base::TimeTicks(); Within l.117: const base::TimeTicks now = base::TimeTicks::Now(); if (first_silence_time_.is_null()) first_silence_time_ = now; if (now - first_silence_time_ > kAllowedSilence) { ... } To use this in unittests you'll want to use a base::TickClock and base::SimpleTestTickClock, see WallClockTimeSourceTest for an example. https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:142: if (null_audio_sink_) Shouldn't this never be null? Additionally since AudioRendererSink objects are ref-counted you can just bind directly to the null sink instead of using a weak_this_ and helper methods. https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc... media/base/audio_bus.cc:222: if (frames_ <= 0) Seems unnecessary, loop will exit quickly anyway. https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc... media/base/audio_bus.cc:227: if (*(channel_data_[i] + j) != 0.0f) Just use "if (channel_data_[i][j]) return false;"
https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:83: StopNullAudioSink(); On 2015/06/22 19:14:40, DaleCurtis wrote: > Stop can be called multiple times, so I'd just call it directly. Done. https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:108: if (!dest->AreFramesZero() && is_using_null_audio_sink_) { On 2015/06/22 19:14:40, DaleCurtis wrote: > We lose this buffer in this case, do we want to save it for the next Render() > call or just drop it? I had the same question here. Changed the code to have an extra AudioBus to store the data. https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:118: audio_timestamp_helper_->AddFrames(dest->frames()); On 2015/06/22 19:14:40, DaleCurtis wrote: > Instead of using a timestamp helper: > > On, l.108: > const bool is_zero = dest->AreFramesZero(); > if (!is_zero) > first_silence_time_ = base::TimeTicks(); > > Within l.117: > const base::TimeTicks now = base::TimeTicks::Now(); > if (first_silence_time_.is_null()) > first_silence_time_ = now; > > if (now - first_silence_time_ > kAllowedSilence) { > ... > } > > To use this in unittests you'll want to use a base::TickClock and > base::SimpleTestTickClock, see WallClockTimeSourceTest for an example. If I remember clearly, TimtTicks::Now() are very expensive on Android. A timestamp helper just use the math of addition and multiplication, so it is much cheaper. And I use AudioTimestampHelper on l.130 to calculate when NullAudioSink should be started. Anyway, modified the code according to your suggestion, but it looks to me the code looks much heavier. https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:142: if (null_audio_sink_) On 2015/06/22 19:14:41, DaleCurtis wrote: > Shouldn't this never be null? Additionally since AudioRendererSink objects are > ref-counted you can just bind directly to the null sink instead of using a > weak_this_ and helper methods. Done. https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc... media/base/audio_bus.cc:222: if (frames_ <= 0) On 2015/06/22 19:14:41, DaleCurtis wrote: > Seems unnecessary, loop will exit quickly anyway. Done. https://codereview.chromium.org/1195633003/diff/40001/media/base/audio_bus.cc... media/base/audio_bus.cc:227: if (*(channel_data_[i] + j) != 0.0f) On 2015/06/22 19:14:41, DaleCurtis wrote: > Just use "if (channel_data_[i][j]) return false;" Done.
https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:118: audio_timestamp_helper_->AddFrames(dest->frames()); On 2015/06/22 22:23:16, qinmin wrote: > On 2015/06/22 19:14:40, DaleCurtis wrote: > > Instead of using a timestamp helper: > > > > On, l.108: > > const bool is_zero = dest->AreFramesZero(); > > if (!is_zero) > > first_silence_time_ = base::TimeTicks(); > > > > Within l.117: > > const base::TimeTicks now = base::TimeTicks::Now(); > > if (first_silence_time_.is_null()) > > first_silence_time_ = now; > > > > if (now - first_silence_time_ > kAllowedSilence) { > > ... > > } > > > > To use this in unittests you'll want to use a base::TickClock and > > base::SimpleTestTickClock, see WallClockTimeSourceTest for an example. > > If I remember clearly, TimtTicks::Now() are very expensive on Android. A > timestamp helper just use the math of addition and multiplication, so it is much > cheaper. > And I use AudioTimestampHelper on l.130 to calculate when NullAudioSink should > be started. > Anyway, modified the code according to your suggestion, but it looks to me the > code looks much heavier. Do you have a source for TimeTicks::Now being expensive on android? I don't think that's true anymore. It's just a call to clock_gettime() which a brief search doesn't reveal slowness on android for. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:16: #include "media/base/audio_timestamp_helper.h" Remove? https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:28: static const int kSilenceInSecondsToEnterIdleMode = 30.0; Remove .0 https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:40: weak_factory_(this) { Remove. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:43: weak_this_ = weak_factory_.GetWeakPtr(); Remove. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:70: // Note: Default behavior is to auto-play on start. Technically you should call NullAudioSink::Start() here, but it doesn't care since it's a null sink. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:122: first_buffer_after_silence_ = media::AudioBus::Create(params_); If we ever lower the silence threshold it might be better to only create once and use a bool for checking. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:133: if ((now - first_silence_time_).InSecondsF() Instead of doing this, compare against base::TimeDelta::FromSeconds(kSilenceInSecondsToEnterIdleMode) https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:137: // If Stop() is called right after the task is posted, This isn't true, only the Stop() above is safe, you need to cancel this task if Stop() is called before it runs. Otherwise the ref will keep the null sink alive, but be called after Stop() (which may even be after the destruction of this class), so playback could start post-destruction. I'd just use a base::CancelableClosure to do this, bind it during construction and vend cancelable_closure_.callback() here. Then during Stop() just call cancelable_closure_.Cancel(). https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:139: double delay = Just use params_.GetBufferDuration() https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:144: base::Bind(&RendererWebAudioDeviceImpl::StartNullAudioSink, Bind directly to Play() here. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:157: void RendererWebAudioDeviceImpl::StartNullAudioSink() { Delete? https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.h (right): https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:9: #include "base/memory/weak_ptr.h" Remove? https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:22: class AudioTimestampHelper; Remove? https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:48: // Helper method to start and stop the |null_audio_sink_|. Remove? https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:85: // Weak pointer for posting callbacks. Remove these? https://codereview.chromium.org/1195633003/diff/60001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1195633003/diff/60001/media/base/audio_bus.cc... media/base/audio_bus.cc:223: for (int j = 0; j < frames_; j++) { ++j
https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:118: audio_timestamp_helper_->AddFrames(dest->frames()); On 2015/06/22 22:49:42, DaleCurtis wrote: > On 2015/06/22 22:23:16, qinmin wrote: > > On 2015/06/22 19:14:40, DaleCurtis wrote: > > > Instead of using a timestamp helper: > > > > > > On, l.108: > > > const bool is_zero = dest->AreFramesZero(); > > > if (!is_zero) > > > first_silence_time_ = base::TimeTicks(); > > > > > > Within l.117: > > > const base::TimeTicks now = base::TimeTicks::Now(); > > > if (first_silence_time_.is_null()) > > > first_silence_time_ = now; > > > > > > if (now - first_silence_time_ > kAllowedSilence) { > > > ... > > > } > > > > > > To use this in unittests you'll want to use a base::TickClock and > > > base::SimpleTestTickClock, see WallClockTimeSourceTest for an example. > > > > If I remember clearly, TimtTicks::Now() are very expensive on Android. A > > timestamp helper just use the math of addition and multiplication, so it is > much > > cheaper. > > And I use AudioTimestampHelper on l.130 to calculate when NullAudioSink should > > be started. > > Anyway, modified the code according to your suggestion, but it looks to me the > > code looks much heavier. > > Do you have a source for TimeTicks::Now being expensive on android? I don't > think that's true anymore. It's just a call to clock_gettime() which a brief > search doesn't reveal slowness on android for. see http://gamasutra.com/view/feature/171774/getting_high_precision_timing_on_.ph.... This involves a syscall and it can be expensive on many platforms https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:16: #include "media/base/audio_timestamp_helper.h" On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:28: static const int kSilenceInSecondsToEnterIdleMode = 30.0; On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove .0 Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:40: weak_factory_(this) { On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove. Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:43: weak_this_ = weak_factory_.GetWeakPtr(); On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove. Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:122: first_buffer_after_silence_ = media::AudioBus::Create(params_); On 2015/06/22 22:49:42, DaleCurtis wrote: > If we ever lower the silence threshold it might be better to only create once > and use a bool for checking. Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:133: if ((now - first_silence_time_).InSecondsF() On 2015/06/22 22:49:42, DaleCurtis wrote: > Instead of doing this, compare against > base::TimeDelta::FromSeconds(kSilenceInSecondsToEnterIdleMode) Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:137: // If Stop() is called right after the task is posted, On 2015/06/22 22:49:42, DaleCurtis wrote: > This isn't true, only the Stop() above is safe, you need to cancel this task if > Stop() is called before it runs. Otherwise the ref will keep the null sink > alive, but be called after Stop() (which may even be after the destruction of > this class), so playback could start post-destruction. > > I'd just use a base::CancelableClosure to do this, bind it during construction > and vend cancelable_closure_.callback() here. Then during Stop() just call > cancelable_closure_.Cancel(). That's why I kept the weak_ptr_ here. If StartNullAudioSink() is posted and Stop() is immediately called, Stop() should set is_using_null_audio_sink_ to false and stop the null_audio_sink_ from playing. And if RendererWebAudioDeviceImpl is destructed right after the task is posted, the weak_ptr will keep StartNullAudioSink() from running. Anyway, since weak_ptr_ is gone, switch to cancelableClosure. The problem with cancelableClosure is that it has to be created and canceled on the same thread, so we have to reconstruct the cancelableClosure right after we cancel it. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:139: double delay = On 2015/06/22 22:49:42, DaleCurtis wrote: > Just use params_.GetBufferDuration() Thanks, didn't notice that method. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:144: base::Bind(&RendererWebAudioDeviceImpl::StartNullAudioSink, On 2015/06/22 22:49:42, DaleCurtis wrote: > Bind directly to Play() here. Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:157: void RendererWebAudioDeviceImpl::StartNullAudioSink() { On 2015/06/22 22:49:42, DaleCurtis wrote: > Delete? Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.h (right): https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:9: #include "base/memory/weak_ptr.h" On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:22: class AudioTimestampHelper; On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:48: // Helper method to start and stop the |null_audio_sink_|. On 2015/06/22 22:49:43, DaleCurtis wrote: > Remove? Done. https://codereview.chromium.org/1195633003/diff/60001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.h:85: // Weak pointer for posting callbacks. On 2015/06/22 22:49:42, DaleCurtis wrote: > Remove these? Done. https://codereview.chromium.org/1195633003/diff/60001/media/base/audio_bus.cc File media/base/audio_bus.cc (right): https://codereview.chromium.org/1195633003/diff/60001/media/base/audio_bus.cc... media/base/audio_bus.cc:223: for (int j = 0; j < frames_; j++) { On 2015/06/22 22:49:43, DaleCurtis wrote: > ++j Done.
lgtm % some minor fixes. Thanks for doing this Qinmin! https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/40001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:118: audio_timestamp_helper_->AddFrames(dest->frames()); On 2015/06/23 00:01:53, qinmin wrote: > On 2015/06/22 22:49:42, DaleCurtis wrote: > > On 2015/06/22 22:23:16, qinmin wrote: > > > On 2015/06/22 19:14:40, DaleCurtis wrote: > > > > Instead of using a timestamp helper: > > > > > > > > On, l.108: > > > > const bool is_zero = dest->AreFramesZero(); > > > > if (!is_zero) > > > > first_silence_time_ = base::TimeTicks(); > > > > > > > > Within l.117: > > > > const base::TimeTicks now = base::TimeTicks::Now(); > > > > if (first_silence_time_.is_null()) > > > > first_silence_time_ = now; > > > > > > > > if (now - first_silence_time_ > kAllowedSilence) { > > > > ... > > > > } > > > > > > > > To use this in unittests you'll want to use a base::TickClock and > > > > base::SimpleTestTickClock, see WallClockTimeSourceTest for an example. > > > > > > If I remember clearly, TimtTicks::Now() are very expensive on Android. A > > > timestamp helper just use the math of addition and multiplication, so it is > > much > > > cheaper. > > > And I use AudioTimestampHelper on l.130 to calculate when NullAudioSink > should > > > be started. > > > Anyway, modified the code according to your suggestion, but it looks to me > the > > > code looks much heavier. > > > > Do you have a source for TimeTicks::Now being expensive on android? I don't > > think that's true anymore. It's just a call to clock_gettime() which a brief > > search doesn't reveal slowness on android for. > > see > http://gamasutra.com/view/feature/171774/getting_high_precision_timing_on_.ph.... > This involves a syscall and it can be expensive on many platforms Despite those early statements, the benchmarks show the Java method nanoTime(), based on clock_gettime(), completing 100k runs in 892ns, I can only imagine the native call is even faster, so I think we're okay :) https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:81: is_using_null_audio_sink_ = false; Move this below the Stop() since Render() may be in progress up until this point. https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:85: start_null_audio_sink_callback_.Reset( Just call Reset() during ::start() and only call Cancel() here (you can remove the constructor entry then). https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:95: if (client_callback_) { Looks like this can never be null, can you remove the conditional? Otherwise the zero check seems like it could pick up uninitialized data.
https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... File content/renderer/media/renderer_webaudiodevice_impl.cc (right): https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:81: is_using_null_audio_sink_ = false; On 2015/06/23 16:38:14, DaleCurtis wrote: > Move this below the Stop() since Render() may be in progress up until this > point. Done. https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:85: start_null_audio_sink_callback_.Reset( On 2015/06/23 16:38:14, DaleCurtis wrote: > Just call Reset() during ::start() and only call Cancel() here (you can remove > the constructor entry then). Done. https://codereview.chromium.org/1195633003/diff/80001/content/renderer/media/... content/renderer/media/renderer_webaudiodevice_impl.cc:95: if (client_callback_) { On 2015/06/23 16:38:14, DaleCurtis wrote: > Looks like this can never be null, can you remove the conditional? Otherwise the > zero check seems like it could pick up uninitialized data. Done.
The CQ bit was checked by qinmin@chromium.org
The CQ bit was unchecked by qinmin@chromium.org
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1195633003/#ps100001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195633003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/1195633003/#ps120001 (title: "ifdef android only constants")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1195633003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/bda540eb1cbfd36dda3eec0206913331ca1ab822 Cr-Commit-Position: refs/heads/master@{#335799} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
