Issue 8477037: Simplify AudioRendererImpl by using AudioDevice.

Chris Rogers

I've tested this a fair bit on a variety of audio files, and tried many ...

9 years, 1 month ago (2011-11-07 20:48:48 UTC) #1

Chris Rogers

By the way, the changes to audio_renderer_impl.cc are so significant, that it may be difficult ...

9 years, 1 month ago (2011-11-07 20:50:48 UTC) #2

henrika (OOO until Aug 14)

Feels like a good idea to merge; thanks Chris. I will focus on the AudioDevice ...

9 years, 1 month ago (2011-11-08 08:11:56 UTC) #3

Chris Rogers

Henrik, thanks for having a look. It seems like both your and Tommi's patches are ...

9 years, 1 month ago (2011-11-08 18:50:30 UTC) #4

acolwell GONE FROM CHROMIUM

Overall I think this is looking pretty good. You should definitely get enal@ to review ...

9 years, 1 month ago (2011-11-08 21:44:20 UTC) #5

scherkus (not reviewing)

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc#newcode18 content/renderer/media/audio_device.cc:18: : buffer_size_(0), indent by two more http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc#newcode53 content/renderer/media/audio_device.cc:53: CHECK_EQ(0, ...

9 years, 1 month ago (2011-11-09 02:39:04 UTC) #6

henrika (OOO until Aug 14)

Focused on the AudioDevice parts. I see some potential race issues but otherwise it looks ...

9 years, 1 month ago (2011-11-09 17:05:22 UTC) #7

Chris Rogers

Everybody, thanks for your comments. I've tried to respond to them all. http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc ...

9 years, 1 month ago (2011-11-10 02:17:22 UTC) #8

Everybody, thanks for your comments.

I've tried to respond to them all.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.cc (left):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:141: NOTIMPLEMENTED();
These are unrelated changes - just part of general cleanup.  The
NOTIMPLEMENTED() macro causes noisy console output which is unhelpful and
misleading in the sense that it implies we should be implementing something
here.  In fact, the correct implementation is to "do nothing", and not have this
macro here.
On 2011/11/08 21:44:20, acolwell wrote:
> Why? This and the 2 below.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:215: (pending_data >= 0)) {
We need to remove this check, because this case comes up when we "pause" the
stream (with the addition of the Pause() method).  If we leave the check in,
then we exit the while() loop (and the thread) and audio never plays again after
it's been paused.

The thread *does* still properly shut down at the end.

On 2011/11/08 21:44:20, acolwell wrote:
> Why is it safe to remove the >= 0 check?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:18: : buffer_size_(0),
On 2011/11/09 02:39:05, scherkus wrote:
> indent by two more

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:53: CHECK_EQ(0, stream_id_);
On 2011/11/09 02:39:05, scherkus wrote:
> nit: perhaps add logging?
> CHECK_EQ(0, stream_id) << "reason why this is bad";
> 
> also return statement won't get executed as a failed CHECK is a crash

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:64: delete [] audio_data_[i];
It seems that scoped_array<> cannot be used with STL containers, like:
std::vector<scoped_array<float> >

Do you think shared_array should be used here?  

On 2011/11/08 21:44:20, acolwell wrote:
> How about changing audio_data_ to use scoped_array<float>. Then you could
simply
> do audio_data_.clear();

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:86: if (stream_id_)
I believe Henrik is right here.  This check is not needed since the check is
made in InitializedOnIOThread()

Removing

On 2011/11/09 17:05:22, henrika1 wrote:
> Why is the existing check in InitializeOnIOThread() not sufficient? One of the
> goals with the previous AudioDevice CL was to improve thread safety and only
> access stream_id_ from the IO thread.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:98:
base::Bind(&AudioDevice::InitializeOnIOThread, this, params));
That would change the client-API contract, as we currently require the Start()
method to be called.  AudioDevice is currently used by WebRTC and WebAudio, so
we don't want to change the API contract ... unless we have a good reason to

On 2011/11/08 21:44:20, acolwell wrote:
> Any reason we can't just move all this into Initialize() and eliminate
Start()?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:102: if (!stream_id_)
Yes the check that's already in ShutDownOnIOThread() should be sufficient.  I
think Henrik's design for this was that it would be OK to call Stop() multiple
times - without crashing.  It will just be a NOP if called multiple times.

Removing unnecessary check.
On 2011/11/09 17:05:22, henrika1 wrote:
> Similar comment as for Start().

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:139: void AudioDevice::Pause(bool flush)
{
Yes, I think we can make it even simpler and consider a potential race between:

Start(), Pause()

Because OnLowLatencyCreated() might get called back late (after
PauseOnIOThread() )

The good news is that we're effectively dealing with a single-thread (the IO
thread) so there are easy ways to detect this situation and defer the pause
until after OnLowLatencyCreated() is called.  Similarly if there's any arbitrary
sequence of Pause()/Play() calls in this period we can keep track of the "final"
state and do the right thing when OnLowLatencyCreated() is finally called.

I think this can be as simple as a "bool" member variable representing if
OnLowLatencyCreated() has yet been called, and another "bool" for the final play
state for when OnLowLatencyCreated() is finally called.  No need to worry about
locks since this is all single-threaded.

Does this make sense to you?

On 2011/11/09 17:05:22, henrika1 wrote:
> What happens if a user calls Start(), Pause(), Play() in one sequence? To me
it
> feels like a potential race condition.
> 
> Same question on Start(), Play() in one sequence.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:72: class RenderCallback {
This isn't part of my CL - it's the way the class currently exists.  We might
consider further cleanup here possibly as a separate CL.

On 2011/11/08 21:44:20, acolwell wrote:
> Is there a reason we can't use base::Callback here? It would be more
consistent
> with the rest of the media code.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:86: AudioDevice(size_t buffer_size,
Henrik any thoughts here?

My feeling is that in the common case, the audio format (sample-rate, etc.)
*are* actually known at construction time, so it's a bit clunky to not have the
"normal" constructor supplied as its being currently used.  This constructor is
currently being used by both WebRTC and WebAudio.

I'm open to changing it if we can get consensus, but it seems like we're making
things more complicated for the normal use of this class if we remove it.

On 2011/11/08 21:44:20, acolwell wrote:
> How about we remove this constructor so all AudioDevice users call
Initialize()
> explicitly.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:92: void Initialize(size_t buffer_size,
I'm just following the type we are currently using in the constructor - to be
consistent.

On 2011/11/08 21:44:20, acolwell wrote:
> Any reason we can't just make this an int?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:105: void Play();
The intention is to closely follow the existing low-level control messages that
are passed to the browser process:
AudioHostMsg_CreateStream / AudioHostMsg_PlayStream <-> Start()
AudioHostMsg_PlayStream <-> Play()
AudioHostMsg_PauseStream / AudioHostMsg_FlushStream<-> Pause()
AudioHostMsg_CloseStream <-> Stop()

Because AudioRendererImpl was using these *exact* control messages, I wanted to
replicate as closely as possible the same logic and behavior in the new
AudioRendererImpl.  Because the AudioRendererImpl code is delicate, I want to be
very careful that the <audio> and <video> tags continue to work.  I've done my
best to stress test these changes to make sure everything still works correctly.

I agree that in the long-term, it's probably not ideal to introduce the concepts
of Play() / Pause().  And I think we can continue to do additional re-factorings
here (and in AudioRendererImpl) to simplify this to Start() / Stop().  But, I
think we should be very careful and first make sure that this first major
AudioRendererImpl re-factoring works correctly by following the existing logic
fairly closely, before attempting to simplify.  Any simplification will need to
take into account that the Stop() method is a synchronous call, and since
AudioRendererImpl makes pretty liberal use of the Play() method, there are some
pretty serious performance issues (and raciness with Start()) to consider there.
 Also, the existence of AudioHostMsg_PlayStream and AudioHostMsg_PauseStream,
means we're following the IPC message protocol pretty closely.

I can add a FIXME comment here, describing the reasons for Play() / Pause() and
the desire to simplify this with further re-factoring.

On 2011/11/08 21:44:20, acolwell wrote:
> Why do we need Start()/Stop() AND Play()/Pause()? This is confusing to me. If
> Play() is only called after Pause() would Resume() be a more fitting name?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:18: const size_t kBufferSize =
2048;
Added GetBufferSizeForSampleRate() method and added comments there.

On 2011/11/08 21:44:20, acolwell wrote:
> Add a comment about the reasoning for this magic number value.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:24: // The AudioDevice is
completely initialized when we first know
Added what I hope is a better comment here.

On 2011/11/09 17:05:22, henrika1 wrote:
> Is it possible to make this comment more clear? Two 'when' in one sentence is
> too much for me ;-)

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:61: // does not currently support
all the sample-rates that we require.
Yes, good to track this.  I've created bug:
http://code.google.com/p/chromium/issues/detail?id=103627

and added this link in the comment itself.

We could consider this a sort of "meta-bug" or technical note, where other more
specific bugs can be written to address this issue.

On 2011/11/09 02:39:05, scherkus wrote:
> do we have a bug tracking this work?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:164: if (audio_device_.get())
On 2011/11/08 21:44:20, acolwell wrote:
> Shouldn't this be a DCHECK?

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:169: if (audio_device_.get())
On 2011/11/08 21:44:20, acolwell wrote:
> DCHECK

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:176: if (audio_device_.get()) {
On 2011/11/08 21:44:20, acolwell wrote:
> DCHECK

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:188: // base::AutoLock
auto_lock(lock_);
Good point.  I've removed this lock and the commented out line.  The details of
the IO thread safety issues are now being handled in AudioDevice.  The only last
question I have for you is the thread safety of FillBuffer().  I think we're OK
because AudioDevice::Stop() is synchronous, but can you sanity check this for
me?
On 2011/11/08 21:44:20, acolwell wrote:
> Why isn't this needed anymore? If it isn't needed delete the line.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:200: audio_device_->Stop();
Done.

On 2011/11/08 21:44:20, acolwell wrote:
> add audio_device_ = NULL so Stop() can't accidentally get called twice.

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:209: memset(audio_data[i], 0,
sizeof(float) * number_of_frames);
On 2011/11/09 02:39:05, scherkus wrote:
> de-indent by 2

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:243: for (int channelIndex = 0;
channelIndex < channels; ++channelIndex) {
FIXED - I'm spending too much time in WebKit ;)

On 2011/11/09 02:39:05, scherkus wrote:
> channel_index

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:244:
media::DeinterleaveAudioChannel(buf.get(),
the AudioDevice class takes care of the proper conversion so we're good.

On 2011/11/09 02:39:05, scherkus wrote:
> We're decoding to PCM data but audio_data is float -- anything to be concerned
> about?

henrika (OOO until Aug 14)

Answered questions from Chris. http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc#newcode139 content/renderer/media/audio_device.cc:139: void AudioDevice::Pause(bool flush) { Fine ...

9 years, 1 month ago (2011-11-10 11:45:07 UTC) #9

henrika (OOO until Aug 14)

Added one more nit. I could give OK here but assume that you will add ...

9 years, 1 month ago (2011-11-10 11:50:24 UTC) #10

acolwell GONE FROM CHROMIUM

Looking good. Just a few more questions/comments. http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (left): http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_device.cc#oldcode215 content/renderer/media/audio_device.cc:215: (pending_data >= ...

9 years, 1 month ago (2011-11-10 21:58:15 UTC) #11

Looking good. Just a few more questions/comments.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.cc (left):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:215: (pending_data >= 0)) {
Won't allow pending_data < 0 break the audio_delay_milliseconds_ update below? I
don't think we want this value to go negative.

On 2011/11/10 02:17:22, Chris Rogers wrote:
> We need to remove this check, because this case comes up when we "pause" the
> stream (with the addition of the Pause() method).  If we leave the check in,
> then we exit the while() loop (and the thread) and audio never plays again
after
> it's been paused.
> 
> The thread *does* still properly shut down at the end.
> 
> On 2011/11/08 21:44:20, acolwell wrote:
> > Why is it safe to remove the >= 0 check?
>

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:86: AudioDevice(size_t buffer_size,
I guess I'm looking for consistency between use cases here. Does it make sense
to defer AudioDevice creation until we actually have the required information?
If so then we can just remove the new constructor & Initialize() method.

On 2011/11/10 02:17:22, Chris Rogers wrote:
> Henrik any thoughts here?
> 
> My feeling is that in the common case, the audio format (sample-rate, etc.)
> *are* actually known at construction time, so it's a bit clunky to not have
the
> "normal" constructor supplied as its being currently used.  This constructor
is
> currently being used by both WebRTC and WebAudio.
> 
> I'm open to changing it if we can get consensus, but it seems like we're
making
> things more complicated for the normal use of this class if we remove it.
> 
> On 2011/11/08 21:44:20, acolwell wrote:
> > How about we remove this constructor so all AudioDevice users call
> Initialize()
> > explicitly.
>

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:197:
DISALLOW_IMPLICIT_CONSTRUCTORS(AudioDevice);
Reinstate if you decide to remove noarg constructor. Use
DISALLOW_COPY_AND_ASSIGN(AudioDevice) if the constructor stays.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:244:
media::DeinterleaveAudioChannel(buf.get(),
Have you quantified the performance hit of deinterleaving & then reinterleaving?
It seems unfortunate that this code and WebRTC has to do this just because the
WebAudio code uses deinterleaved channels. Maybe not in this CL, but in a
follow-up one we should remedy this so deinterleaving only happens when the data
goes to WebAudio.
On 2011/11/10 02:17:22, Chris Rogers wrote:
> the AudioDevice class takes care of the proper conversion so we're good.
> 
> On 2011/11/09 02:39:05, scherkus wrote:
> > We're decoding to PCM data but audio_data is float -- anything to be
concerned
> > about?
>

Chris Rogers

Henrik and Aaron, thanks again for your comments. I think I've answered them all. media ...

9 years, 1 month ago (2011-11-15 22:48:29 UTC) #12

Henrik and Aaron, thanks again for your comments.  I think I've answered them
all.

media layout tests and unit test now passes.

I added a lock for the socket and audio thread lifetime.  See 
AudioDevice::ShutDownAudioThread() and the use of |lock_| for details.  Strictly
speaking, it may not be necessary given the calling sequences and nature of most
logic being handled on the IO thread.  But still, I felt a bit nervous that the
thread is created on the IO thread, but joined on a different thread.

Henrik, I'm happy to remove it if you feel it superfluous, but I just wanted it
there for extra protection and resilience against further re-factoring in this
class.  For what it's worth, the lock itself should add effectively no overhead
since it should never be contended...

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.cc (left):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:215: (pending_data >= 0)) {
Yes, good point -- fixed.

On 2011/11/10 21:58:15, acolwell wrote:
> Won't allow pending_data < 0 break the audio_delay_milliseconds_ update below?
I
> don't think we want this value to go negative.
> 
> On 2011/11/10 02:17:22, Chris Rogers wrote:
> > We need to remove this check, because this case comes up when we "pause" the
> > stream (with the addition of the Pause() method).  If we leave the check in,
> > then we exit the while() loop (and the thread) and audio never plays again
> after
> > it's been paused.
> > 
> > The thread *does* still properly shut down at the end.
> > 
> > On 2011/11/08 21:44:20, acolwell wrote:
> > > Why is it safe to remove the >= 0 check?
> > 
>

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.cc:139: void AudioDevice::Pause(bool flush)
{
I think I've addressed this problem with |play_on_start_| and |is_started_|

On 2011/11/10 11:45:07, henrika1 wrote:
> Fine by me. In general I prefer simple and readable solutions where it is
> obvious what we are trying to prevent (race in this case). Even if the
solution
> is not perfect, it will at least be of help if more complicated calling
> sequences are required. And yes, no locks.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:26: // Start -> InitializeOnIOThread
------> AudioHostMsg_CreateStream -------->
Good idea - DONE

On 2011/11/09 17:05:22, henrika1 wrote:
> Care to add similar sequence for Play/Resume to clearly illustrate the
> difference?

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:40: // 2. IO thread (internal
implementation detail - not exposed to public API)
On 2011/11/08 21:44:20, acolwell wrote:
> Change the second 2. to a 3. and update the 3. below.

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:50: // - Start() is
asynchronous/non-blocking.
On 2011/11/09 17:05:22, henrika1 wrote:
> Merge with comments about Play/Pause?

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:84: explicit AudioDevice(RenderCallback*
callback);
On 2011/11/08 21:44:20, acolwell wrote:
> Why not pass this callback in Initialize()? Is it needed before then?

Done.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:86: AudioDevice(size_t buffer_size,
I definitely agree that it would be nice to defer AudioDevice construction and
just avoid the Initialize() altogether, but unfortunately we can't because the
constructors *must* be called on the main renderer thread because of the
initialization of the "filter_" member.

There's a comment at the top of this header file (thread 1.) about this and also
a comment in AudioRendererImpl::AudioRendererImpl() describing why we construct
at this point.

On 2011/11/10 21:58:15, acolwell wrote:
> I guess I'm looking for consistency between use cases here. Does it make sense
> to defer AudioDevice creation until we actually have the required information?
> If so then we can just remove the new constructor & Initialize() method.
> 
> On 2011/11/10 02:17:22, Chris Rogers wrote:
> > Henrik any thoughts here?
> > 
> > My feeling is that in the common case, the audio format (sample-rate, etc.)
> > *are* actually known at construction time, so it's a bit clunky to not have
> the
> > "normal" constructor supplied as its being currently used.  This constructor
> is
> > currently being used by both WebRTC and WebAudio.
> > 
> > I'm open to changing it if we can get consensus, but it seems like we're
> making
> > things more complicated for the normal use of this class if we remove it.
> > 
> > On 2011/11/08 21:44:20, acolwell wrote:
> > > How about we remove this constructor so all AudioDevice users call
> > Initialize()
> > > explicitly.
> > 
>

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:105: void Play();
I've added a TODO(crogers) for Play()/Pause() describing how we can get rid of
Play()/Pause() upon further re-factoring of AudioRendererImpl

Keeping for now because of the delicate logic (and to make sure we don't change
too many things all at once).

On 2011/11/10 11:45:07, henrika1 wrote:
> I am fine with trying to mimic the existing AudioRendererImpl  as much as
> possible. As you say, the code *is* delicate and larger changes might have to
> wait. A TODO/FIXME would not hurt though.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de...
content/renderer/media/audio_device.h:197:
DISALLOW_IMPLICIT_CONSTRUCTORS(AudioDevice);
Added DISALLOW_COPY_AND_ASSIGN(AudioDevice)

On 2011/11/10 21:58:15, acolwell wrote:
> Reinstate if you decide to remove noarg constructor. Use
> DISALLOW_COPY_AND_ASSIGN(AudioDevice) if the constructor stays.

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_re...
content/renderer/media/audio_renderer_impl.cc:244:
media::DeinterleaveAudioChannel(buf.get(),
Having spent a few years optimizing code and profiling at the assembly level
using tools like "Shark", I can pretty confidently say that the overhead  here
is very very far down in the noise, especially considering the cost of audio
decoding, and the IPC involved.

The AudioDevice API was designed to abstract away audio format details, and use
a canonical representation (nominal -1 -> +1 non-interleaved floating-point). 
It'll be more and more important to have this abstraction as we do more
renderer-side mixing, sample-rate conversion, and other processing.  We want to
be able to pass higher precision sample data to the browser where the audio
hardware may often have DACS > 16bit.  What if we're playing back a 24bit audio
file?

That said, I totally agree that it seems odd to convert to float here, then back
to int16 in AudioDevice.  So I've added a TODO in AudioDevice about passing the
floating-point data all the way to the browser.

On 2011/11/10 21:58:15, acolwell wrote:
> Have you quantified the performance hit of deinterleaving & then
reinterleaving?
> It seems unfortunate that this code and WebRTC has to do this just because the
> WebAudio code uses deinterleaved channels. Maybe not in this CL, but in a
> follow-up one we should remedy this so deinterleaving only happens when the
data
> goes to WebAudio.
> On 2011/11/10 02:17:22, Chris Rogers wrote:
> > the AudioDevice class takes care of the proper conversion so we're good.
> > 
> > On 2011/11/09 02:39:05, scherkus wrote:
> > > We're decoding to PCM data but audio_data is float -- anything to be
> concerned
> > > about?
> > 
>

acolwell GONE FROM CHROMIUM

Looks good Chris just 2 final comments and I think it'll be ready for checkin. ...

9 years, 1 month ago (2011-11-16 06:41:04 UTC) #13

henrika (OOO until Aug 14)

Chris, I have added some minor comments. The lock seems OK. Feels like "nice to ...

9 years, 1 month ago (2011-11-16 20:48:02 UTC) #14

Chris Rogers

Latest patch addresses all comments. Henrik, I ran the WebRTC content test on Mac OS ...

9 years, 1 month ago (2011-11-17 02:37:29 UTC) #15

Latest patch addresses all comments.

Henrik, I ran the WebRTC content test on Mac OS X, but a few of the tests were
unable to run properly since it seems like they still need to be ported to Mac
OS X.  I'd really appreciate your help in trying out the patch and verifying
these tests to your satisfaction.  And of course I'll fix any problems which
come up.

Aaron, thanks for your last comments.  With your suggestions, AudioRendererImpl
is now even simpler.  We still should discuss the last details of
WillDestroyCurrentMessageLoop() --- is it needed?  Also, I agree we should run
the TSAN tests.  I assume you mean use a "try" job on the TSAN bot?

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_device.cc:50: Initialize(buffer_size,
On 2011/11/16 20:48:03, henrika1 wrote:
> You assign values to members twice here. Both in the ctor init list and in
> Init().

Done.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_device.cc:83: return audio_data_.size() > 0;
On 2011/11/16 20:48:03, henrika1 wrote:
> I would add an explicit state instead. These types of conditions may slip over
> time.

Done.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_device.cc:108: if (MessageLoop::current() ==
FIXED

I've removed this added code in the latest patch.  The reason I added it before
is because AudioRendererImpl::WillDestroyCurrentMessageLoop() was wanting to
call Stop() on the IO thread.  But I've now removed
AudioRendererImpl::WillDestroyCurrentMessageLoop() in addressing Aaron's
comments.

On 2011/11/16 20:48:03, henrika1 wrote:
> Why is this added for Stop() only. Comments still say that all public APIs
must
> be called from the same thread.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
File content/renderer/media/audio_device.h (right):

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_device.h:45: //    Must be called on the same
thread.
On 2011/11/16 20:48:03, henrika1 wrote:
> must

Done.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_device.h:92: // Minimal constructor where
Initialize() must later be called.
On 2011/11/16 20:48:03, henrika1 wrote:
> You are the native American but I would say "must be called later" ;-)

Done.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
File content/renderer/media/audio_renderer_impl.cc (right):

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:121: if (stopped_)
Good point - in looking closer at this it's best to simply not overload this
method.

On 2011/11/16 06:41:04, acolwell wrote:
> Doesn't this get called from the audio decoder thread? I think that might have
> been why the lock was here. Everywhere else it looks like stopped_ is accessed
> from the pipeline thread except here. Does it cause problems if we just don't
> overload this function? The AudioRendererBase has locking to protect itself.

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audi...
content/renderer/media/audio_renderer_impl.cc:212: if (stopped_)
I've removed WillDestroyCurrentMessageLoop() since I agree that, if it belongs
anywhere at all, it should be in AudioDevice.  For now, I've simply removed it
altogether.  We should discuss why this was here in the first place and if we
need to have it at all.

This code may have been buggy in the first place, since it appears that
WillDestroyCurrentMessageLoop()  potentially could have been called after the
AudioRendererImpl object has been deleted.

On 2011/11/16 06:41:04, acolwell wrote:
> So this is another access to stopped_ on a thread other than the pipeline
> thread. Are we sure this is safe?
> 
> Has this been tested with ThreadSanitizer? It might be a good idea to do so
just
> to make sure it doesn't have a problem with this access & modification from
> multiple threads. 
> 
> I'm also wondering if this code still makes sense to be here. This is the only
> remaining reference to the io_message_loop() in this file. Maybe this should
be
> in AudioDevice somewhere instead?

henrika (OOO until Aug 14)

LGTM ;-) "Henrik, I ran the WebRTC content test on Mac OS X, but a ...

9 years, 1 month ago (2011-11-17 12:15:31 UTC) #16

henrika (OOO until Aug 14)

FYI - I did not review the unit test for AudioRenderImpl since I don't know ...

9 years, 1 month ago (2011-11-17 12:16:49 UTC) #17

acolwell GONE FROM CHROMIUM

http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audio_renderer_impl.cc File content/renderer/media/audio_renderer_impl.cc (right): http://codereview.chromium.org/8477037/diff/16003/content/renderer/media/audio_renderer_impl.cc#newcode212 content/renderer/media/audio_renderer_impl.cc:212: if (stopped_) On 2011/11/17 02:37:30, Chris Rogers wrote: > ...

9 years, 1 month ago (2011-11-17 19:16:12 UTC) #18

vrk (LEFT CHROMIUM)

Taking over crogers' CL since he's on vacation! Looks like the MessageLoop DestructionObserver was introduced ...

9 years, 1 month ago (2011-11-22 01:17:39 UTC) #19

no longer working on chromium

Great work. Some little concerns on AudioDevice. BR, /SX

9 years ago (2011-11-28 15:17:17 UTC) #20

no longer working on chromium

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audio_device.cc File content/renderer/media/audio_device.cc (right): http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audio_device.cc#newcode117 content/renderer/media/audio_device.cc:117: base::Bind(&AudioDevice::ShutDownOnIOThread, this, &completion)); We use a WitableEvent in ShutDownOnIOThread() ...

9 years ago (2011-11-28 15:17:41 UTC) #21

vrk (LEFT CHROMIUM)

D'oh, unfortunately one cannot add patches to a CL one does not own... Please take ...

9 years ago (2011-12-02 22:54:45 UTC) #22

D'oh, unfortunately one cannot add patches to a CL one does not own...

Please take a look at my changes in this CL:
http://codereview.chromium.org/8785008

Main diffs:
- Added DCHECKs for the methods that claim to be on IO thread
- Deleted the no longer relevant DestroyedMessageLoop tests from
AudioRendererImplTest
- Instead of posting PlayOnIOThread in OnLowLatencyCreated(), calls
PlayOnIOThread() directly

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
File content/renderer/media/audio_device.cc (right):

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:117:
base::Bind(&AudioDevice::ShutDownOnIOThread, this, &completion));
On 2011/11/28 15:17:41, xians1 wrote:
> We use a WitableEvent in ShutDownOnIOThread() to sync with IO thread, so after
> the event is signaled, stream_id_ is set to 0, we have no chance to access
> socket_ and audio_thread_ in OnLowLatencyCreated().
> So no lock is needed in for socket_ and audio_thread_ for the shutdown.

Hmm, I think you're right, but since this isn't my CL and I'm not that familiar
with the code, I'm not comfortable making that change before landing this. :/

Write a follow-up CL to delete the lock after this CL lands? I don't think it's
critical, so it seems like something to ask crogers about when he comes back
from vacation.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:172: void AudioDevice::PlayOnIOThread() {
On 2011/11/28 15:17:41, xians1 wrote:
> What happens if the clients call Play() or Pause() for multiple times? How do
> you think adding a flag on IO thread to protect them?

This is a good question, but I think it may be something to examine as part of
another CL. This CL preserves the same behavior as the original
AudioRendererImpl in this situation, so I don't think it's necessary to change.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:179: void
AudioDevice::PauseOnIOThread(bool flush) {
On 2011/11/28 15:17:41, xians1 wrote:
> Same question as PlayOnIOThread()

Ditto above.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:191: void
AudioDevice::ShutDownOnIOThread(base::WaitableEvent* completion) {
On 2011/11/28 15:17:41, xians1 wrote:
> We should clean up all the states like is_started, play_on_start_.

Yeah, these states are probably an indication that the API needs some tweaking
yet. But let's leave that to another CL.  It seems like a reasonable solution
for now for the problem that was discussed here:
http://codereview.chromium.org/8477037/diff/1/content/renderer/media/audio_de....

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:202: Send(new
AudioHostMsg_CloseStream(stream_id_));
On 2011/11/22 01:17:39, Victoria Kirst wrote:
> Doesn't AudioHostMsg_CloseStream need to be synchronous, i.e.
> IPC_SYNC_MESSAGE_ROUTED?
> 
> Otherwise I believe this will only wait for the AudioHostMsg_CloseStream
message
> to be sent, not for the receiver to handle the message.

Since this potential problem was not introduced by the CL, I'll make a note on
my own to examine it in a follow-up CL.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:277:
base::Bind(&AudioDevice::PlayOnIOThread, this));
On 2011/11/28 15:17:41, xians1 wrote:
> One potential racing condition here, for example, the following calls can
happen
> in sequence on IO thread:
> PlayOnIOThread()
> OnLowLatencyCreated()
> PauseOnIOThread()
> 
> Since OnLowLatencyCreated() will post a PlayOnIOThread(), so 
> PauseOnIOThread() will send a pause PauseStream message to the brwoser before
> the stream is played.
> 
> Maybe we should just call PlayOnIOThread() directly, instead of posting a
task.

Yes, you're right; good catch! I think calling PlayOnIOThread() directly makes
the most sense, but I'm not sure if there was a reason this task was essentially
delayed in the past... 

Looking at history, this was introduced in
http://codereview.chromium.org/7157001/. Doesn't look like it was intentionally
delayed, so I changed this to call PlayOnIOThread() directly.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:329: void
AudioDevice::ShutDownAudioThread() {
On 2011/11/28 15:17:41, xians1 wrote:
> Please see comment in Stop().

Done.

http://codereview.chromium.org/8477037/diff/29001/content/renderer/media/audi...
content/renderer/media/audio_device.cc:355: // Uses cached value if possible.
On 2011/11/17 19:16:12, acolwell wrote:
> FYI. You might need to coordinate w/ tommi on this. He's moved this to a
helper
> function in http://codereview.chromium.org/8588030/

Thanks Aaron! These methods weren't being used in this class, so rebasing to ToT
was smooth sailing.

http://codereview.chromium.org/8477037/diff/29001/media/filters/audio_rendere...
File media/filters/audio_renderer_base.cc (right):

http://codereview.chromium.org/8477037/diff/29001/media/filters/audio_rendere...
media/filters/audio_renderer_base.cc:152: if (!pending_reads_)
On 2011/11/17 19:16:12, acolwell wrote:
> nit: Consider just moving the DCHECK_GT() and --pending_reads_ below the
(state_
> == kStopped) check below. I think that will produce the desired result and
still
> allow us to detect cases where we got unexpected calls while not stopped.

Since this is a nit and I'm not that familiar with the code, I will leave the CL
as-is if you don't mind!

(Also, scherkus has a CL out that will soon change this anyway.
http://codereview.chromium.org/8763010/)

vrk (LEFT CHROMIUM)

Two other comments: 1. Updated the linked CL so you can just look at the ...

9 years ago (2011-12-02 23:07:23 UTC) #23

enal1

On 2011/12/02 23:07:23, Victoria Kirst wrote: > Two other comments: > > 1. Updated the ...

9 years ago (2011-12-13 18:49:25 UTC) #24

acolwell GONE FROM CHROMIUM

On 2011/12/13 18:49:25, enal1 wrote: > On 2011/12/02 23:07:23, Victoria Kirst wrote: > > Two ...

9 years ago (2011-12-13 20:18:49 UTC) #25

no longer working on chromium

On Tue, Dec 13, 2011 at 9:18 PM, <acolwell@chromium.org> wrote: > >> >> I propose ...

9 years ago (2011-12-13 22:06:30 UTC) #26

On Tue, Dec 13, 2011 at 9:18 PM, <acolwell@chromium.org> wrote:

>
>>
>> I propose we do the following.
> 1. Rename Chris' new AudioRendererImpl to AudioRenderererImpl2 temporarily.
> 2. Restore AudioRendererImpl to its original state but leave the
> AudioDevice &
> AudioRendererBase changes in.
> 3. Update the unit test so it can work w/ both implementations.
> 4. Create new tests for the issues enal mentioned so that we can avoid
> this from
> happening again.
>
>
+ 1 on renaming AudioRendererImpl to a temporary file. It is easier to
debug the problems and improve the AudioDevice.


On 2011/12/13 18:49:25, enal1 wrote:

> On 2011/12/02 23:07:23, Victoria Kirst wrote:
> > Two other comments:
> >
> > 1. Updated the linked CL so you can just look at the patchset diff ("2")
> >
http://codereview.chromium.**org/8785008<http://codereview.chromium.org/8785008>
> >
> > 2. Because of riskiness involved in this CL, I will make sure to land
> the it
> > after m17 branch cut.
>

There are at least 4 issues with that CL:
> * Race condition on startup, AudioOutputController::DoPlay(**) asking for
> data
> while AudioDevice::Run() already processing first package.

* Bad handling of last buffer, we are signalling "end of data" the moment
> algorithm_->IsQueueEmpty() returns true to AudioRendererBase::FillBuffer(*
> *),
> without taking into consideration amount of data already in the OS buffers.
> (Nobody calls AudioRendererImpl::**UpdateEarliestEndTime())
>

* Nobody is setting length of data in buffer, as a result when polling on
>
stream

> startup we never exit early and always wait the max allowed time.
>
* Even when I band-aid all those issues,
>
http://www.corp.google.com/%**7Etommi/average.html<http://www.corp.google.com...
still
> sounds terrible, and
>
pauses

> between clicks are much longer than they should be. Not sure what causes
> the
> problem.
>

I'd strongly prefer to revert the change.


If needed, I can help fixing the problems above, and please let me know if
someone has already looked into it.

BR,
/SX

Let me look what we can do easily.

On Tue, Dec 13, 2011 at 2:06 PM, Shijing Xian <xians@chromium.org> wrote:
>
> On Tue, Dec 13, 2011 at 9:18 PM, <acolwell@chromium.org> wrote:
>>>
>>>
>>>
>> I propose we do the following.
>> 1. Rename Chris' new AudioRendererImpl to AudioRenderererImpl2
>> temporarily.
>> 2. Restore AudioRendererImpl to its original state but leave the
>> AudioDevice &
>> AudioRendererBase changes in.
>> 3. Update the unit test so it can work w/ both implementations.
>> 4. Create new tests for the issues enal mentioned so that we can avoid
>> this from
>> happening again.
>>
>
> + 1 on renaming AudioRendererImpl to a temporary file. It is easier to debug
> the problems and improve the AudioDevice.
>
>
> On 2011/12/13 18:49:25, enal1 wrote:
>>
>> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>> > Two other comments:
>> >
>> > 1. Updated the linked CL so you can just look at the patchset diff ("2")
>> > http://codereview.chromium.org/8785008
>> >
>> > 2. Because of riskiness involved in this CL, I will make sure to land
>> > the it
>> > after m17 branch cut.
>
>
>> There are at least 4 issues with that CL:
>> * Race condition on startup, AudioOutputController::DoPlay() asking for
>> data
>> while AudioDevice::Run() already processing first package.
>>
>> * Bad handling of last buffer, we are signalling "end of data" the moment
>> algorithm_->IsQueueEmpty() returns true to
>> AudioRendererBase::FillBuffer(),
>> without taking into consideration amount of data already in the OS
>> buffers.
>> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
>
>
>> * Nobody is setting length of data in buffer, as a result when polling on
>
> stream
>>
>> startup we never exit early and always wait the max allowed time.
>>
>> * Even when I band-aid all those issues,
>> http://www.corp.google.com/%7Etommi/average.html still sounds terrible,
>> and
>
> pauses
>>
>> between clicks are much longer than they should be. Not sure what causes
>> the
>> problem.
>
>
>> I'd strongly prefer to revert the change.
>
>
> If needed, I can help fixing the problems above, and please let me know if
> someone has already looked into it.
>
> BR,
> /SX
>

Chris Rogers

Aaron's proposal about temporarily renaming AudioRendererImpl to AudioRendererImpl2 seems reasonable at this stage. But I ...

9 years ago (2011-12-13 22:49:12 UTC) #28

Aaron's proposal about temporarily renaming AudioRendererImpl
to AudioRendererImpl2 seems reasonable at this stage.
But I would like an agreement to not make any further changes
to AudioRendererImpl and instead focus on fixing AudioRendererImpl2 and
moving it back.

Sorry about the problems.  I had hoped Eugene would have been able to look
at the CL during code review.

Chris

On Tue, Dec 13, 2011 at 12:18 PM, <acolwell@chromium.org> wrote:

> On 2011/12/13 18:49:25, enal1 wrote:
>
>> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>> > Two other comments:
>> >
>> > 1. Updated the linked CL so you can just look at the patchset diff ("2")
>> >
http://codereview.chromium.**org/8785008<http://codereview.chromium.org/8785008>
>> >
>> > 2. Because of riskiness involved in this CL, I will make sure to land
>> the it
>> > after m17 branch cut.
>>
>
>  There are at least 4 issues with that CL:
>> * Race condition on startup, AudioOutputController::DoPlay(**) asking
>> for data
>> while AudioDevice::Run() already processing first package.
>> * Bad handling of last buffer, we are signalling "end of data" the moment
>> algorithm_->IsQueueEmpty() returns true to AudioRendererBase::FillBuffer(
>> **),
>> without taking into consideration amount of data already in the OS
>> buffers.
>> (Nobody calls AudioRendererImpl::**UpdateEarliestEndTime())
>> * Nobody is setting length of data in buffer, as a result when polling on
>>
> stream
>
>> startup we never exit early and always wait the max allowed time.
>> * Even when I band-aid all those issues,
>>
http://www.corp.google.com/%**7Etommi/average.html<http://www.corp.google.com...
sounds terrible, and
>>
> pauses
>
>> between clicks are much longer than they should be. Not sure what causes
>> the
>> problem.
>>
>
>  I'd strongly prefer to revert the change.
>>
>
> I propose we do the following.
> 1. Rename Chris' new AudioRendererImpl to AudioRenderererImpl2 temporarily.
> 2. Restore AudioRendererImpl to its original state but leave the
> AudioDevice &
> AudioRendererBase changes in.
> 3. Update the unit test so it can work w/ both implementations.
> 4. Create new tests for the issues enal mentioned so that we can avoid
> this from
> happening again.
>
>
http://codereview.chromium.**org/8477037/<http://codereview.chromium.org/8477...
>

no longer working on chromium

On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote: > On 2011/12/02 23:07:23, Victoria ...

9 years ago (2011-12-14 13:19:26 UTC) #29

On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:

> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>
>> Two other comments:
>>
>
>  1. Updated the linked CL so you can just look at the patchset diff ("2")
>>
http://codereview.chromium.**org/8785008<http://codereview.chromium.org/8785008>
>>
>
>  2. Because of riskiness involved in this CL, I will make sure to land the
>> it
>> after m17 branch cut.
>>
>
> There are at least 4 issues with that CL:
> * Race condition on startup, AudioOutputController::DoPlay(**) asking for
> data
> while AudioDevice::Run() already processing first package.
>

This can be fixed easily by changing the relevant code in AudioDevice or
AudioOutputController.
I can make a CL for it.

* Bad handling of last buffer, we are signalling "end of data" the moment
> algorithm_->IsQueueEmpty() returns true to AudioRendererBase::FillBuffer(*
> *),
> without taking into consideration amount of data already in the OS buffers.
> (Nobody calls AudioRendererImpl::**UpdateEarliestEndTime())
> * Even when I band-aid all those issues,
>
http://www.corp.google.com/~**tommi/average.html<http://www.corp.google.com/~...
sounds terrible, and pauses
> between clicks are much longer than they should be. Not sure what causes
> the
> problem.
>
> I think they are actually one problem, or at least closely related.
In the AudioRendererImpl, we need to update the |earliest_end_time_| to be
able to notify the
client when we shall stop the playing, which was done by AudioRendererImpl::
**UpdateEarliestEndTime(), as Enal pointed out.
And this UpdateEarliestEndTime() needs to know the number of pending data
reported by the browser.
In the non-low-latency mode, we get the number of pending data using:
uint32 size = buffer_.Read(dest, max_size);
buffers_state.pending_bytes += size + buffer_.forward_bytes();

And in the low-latency mode
uint32 size =  sync_reader_->Read(dest, max_size);
sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);

The value from the low-latency mode can be completely wrong, one of the
problems is that sync_reader always
returns a value of buffer size even though no data on the shared_memory_
(it just writes 0). Which means the client
can't get the right |earliest_end_time_| when it comes to the end of stream.
While non-low-latency mode is using the seakable buffer, which truly
reports the number of data it writes to the soundcard.

I can see that we need to do quite some change in the
AudioOutputController, besides AudioRendererImpl.

BR,
/SX


* Nobody is setting length of data in buffer, as a result when polling on
> stream
> startup we never exit early and always wait the max allowed time.
>
http://codereview.chromium.**org/8477037/<http://codereview.chromium.org/8477...
>

enal1

Guys, I believe I fixed all timing-related issues. Now http://www.corp.google.com/~tommi/average.html again plays regularly and in ...

9 years ago (2011-12-14 17:55:29 UTC) #30

Guys,

I believe I fixed all timing-related issues. Now
http://www.corp.google.com/~tommi/average.html again plays regularly
and in reasonable time.

Unfortunately I think it *sounds* slightly differently than before, at
least on Windows. Can you please apply

http://codereview.chromium.org/8909006

and listen? I compared it to 17.0.938.0...

Can the difference be caused by us now calling
media::DeinterleaveAudioChannel() / media::InterleaveFloatToInt16()?

Thanks,
Eugene

PS. CL does not include tests changes yet.

On Wed, Dec 14, 2011 at 5:19 AM, Shijing Xian <xians@chromium.org> wrote:
> On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:
>>
>> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>>>
>>> Two other comments:
>>
>>
>>> 1. Updated the linked CL so you can just look at the patchset diff ("2")
>>> http://codereview.chromium.org/8785008
>>
>>
>>> 2. Because of riskiness involved in this CL, I will make sure to land the
>>> it
>>> after m17 branch cut.
>>
>>
>> There are at least 4 issues with that CL:
>> * Race condition on startup, AudioOutputController::DoPlay() asking for
>> data
>> while AudioDevice::Run() already processing first package.
>
>
> This can be fixed easily by changing the relevant code in AudioDevice or
> AudioOutputController.
> I can make a CL for it.
>
>> * Bad handling of last buffer, we are signalling "end of data" the moment
>> algorithm_->IsQueueEmpty() returns true to
>> AudioRendererBase::FillBuffer(),
>> without taking into consideration amount of data already in the OS
>> buffers.
>> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
>> * Even when I band-aid all those issues,
>> http://www.corp.google.com/~tommi/average.html still sounds terrible, and
>> pauses
>> between clicks are much longer than they should be. Not sure what causes
>> the
>> problem.
>>
> I think they are actually one problem, or at least closely related.
> In the AudioRendererImpl, we need to update the |earliest_end_time_| to be
> able to notify the
> client when we shall stop the playing, which was done
> by AudioRendererImpl::UpdateEarliestEndTime(), as Enal pointed out.
> And this UpdateEarliestEndTime() needs to know the number of pending data
> reported by the browser.
> In the non-low-latency mode, we get the number of pending data using:
> uint32 size = buffer_.Read(dest, max_size);
> buffers_state.pending_bytes += size + buffer_.forward_bytes();
>
> And in the low-latency mode
> uint32 size =  sync_reader_->Read(dest, max_size);
> sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);
>
> The value from the low-latency mode can be completely wrong, one of the
> problems is that sync_reader always
> returns a value of buffer size even though no data on the shared_memory_ (it
> just writes 0). Which means the client
> can't get the right |earliest_end_time_| when it comes to the end of stream.
> While non-low-latency mode is using the seakable buffer, which truly reports
> the number of data it writes to the soundcard.
>
> I can see that we need to do quite some change in the AudioOutputController,
> besides AudioRendererImpl.
>
> BR,
> /SX
>
>
>> * Nobody is setting length of data in buffer, as a result when polling on
>> stream
>> startup we never exit early and always wait the max allowed time.
>> http://codereview.chromium.org/8477037/
>
>

no longer working on chromium

Eugene, good work, we should push the CL out ASAP, I am working on the ...

9 years ago (2011-12-14 19:00:52 UTC) #31

Eugene, good work, we should push the CL out ASAP, I am working on the same
thing in the local machine, but I am obviously a bit slow :)

removing the following code int he audio_renderer_impl.cc can get back how
it sounds:
    if (filled_frames < number_of_frames) {
      int frames_to_zero = number_of_frames - filled_frames;
      memset(audio_data[channel_index], 0, sizeof(float) * frames_to_zero);
    }

Though I am not 100% sure if it is correct thing to do.

BR,
/SX




On Wed, Dec 14, 2011 at 6:55 PM, Eugene Nalimov <enal@google.com> wrote:

> Guys,
>
> I believe I fixed all timing-related issues. Now
> http://www.corp.google.com/~tommi/average.html again plays regularly
> and in reasonable time.
>
> Unfortunately I think it *sounds* slightly differently than before, at
> least on Windows. Can you please apply
>
> http://codereview.chromium.org/8909006
>
> and listen? I compared it to 17.0.938.0...
>
> Can the difference be caused by us now calling
> media::DeinterleaveAudioChannel() / media::InterleaveFloatToInt16()?
>
> Thanks,
> Eugene
>
> PS. CL does not include tests changes yet.
>
> On Wed, Dec 14, 2011 at 5:19 AM, Shijing Xian <xians@chromium.org> wrote:
> > On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:
> >>
> >> On 2011/12/02 23:07:23, Victoria Kirst wrote:
> >>>
> >>> Two other comments:
> >>
> >>
> >>> 1. Updated the linked CL so you can just look at the patchset diff
> ("2")
> >>> http://codereview.chromium.org/8785008
> >>
> >>
> >>> 2. Because of riskiness involved in this CL, I will make sure to land
> the
> >>> it
> >>> after m17 branch cut.
> >>
> >>
> >> There are at least 4 issues with that CL:
> >> * Race condition on startup, AudioOutputController::DoPlay() asking for
> >> data
> >> while AudioDevice::Run() already processing first package.
> >
> >
> > This can be fixed easily by changing the relevant code in AudioDevice or
> > AudioOutputController.
> > I can make a CL for it.
> >
> >> * Bad handling of last buffer, we are signalling "end of data" the
> moment
> >> algorithm_->IsQueueEmpty() returns true to
> >> AudioRendererBase::FillBuffer(),
> >> without taking into consideration amount of data already in the OS
> >> buffers.
> >> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
> >> * Even when I band-aid all those issues,
> >> http://www.corp.google.com/~tommi/average.html still sounds terrible,
> and
> >> pauses
> >> between clicks are much longer than they should be. Not sure what causes
> >> the
> >> problem.
> >>
> > I think they are actually one problem, or at least closely related.
> > In the AudioRendererImpl, we need to update the |earliest_end_time_| to
> be
> > able to notify the
> > client when we shall stop the playing, which was done
> > by AudioRendererImpl::UpdateEarliestEndTime(), as Enal pointed out.
> > And this UpdateEarliestEndTime() needs to know the number of pending data
> > reported by the browser.
> > In the non-low-latency mode, we get the number of pending data using:
> > uint32 size = buffer_.Read(dest, max_size);
> > buffers_state.pending_bytes += size + buffer_.forward_bytes();
> >
> > And in the low-latency mode
> > uint32 size =  sync_reader_->Read(dest, max_size);
> > sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);
> >
> > The value from the low-latency mode can be completely wrong, one of the
> > problems is that sync_reader always
> > returns a value of buffer size even though no data on the shared_memory_
> (it
> > just writes 0). Which means the client
> > can't get the right |earliest_end_time_| when it comes to the end of
> stream.
> > While non-low-latency mode is using the seakable buffer, which truly
> reports
> > the number of data it writes to the soundcard.
> >
> > I can see that we need to do quite some change in the
> AudioOutputController,
> > besides AudioRendererImpl.
> >
> > BR,
> > /SX
> >
> >
> >> * Nobody is setting length of data in buffer, as a result when polling
> on
> >> stream
> >> startup we never exit early and always wait the max allowed time.
> >> http://codereview.chromium.org/8477037/
> >
> >
>

enal1

Thank you for pointing. Code was zeroing out not the reminder but the entire buffer. ...

9 years ago (2011-12-14 19:10:14 UTC) #32

Thank you for pointing. Code was zeroing out not the reminder but the
entire buffer.

Will work on tests now.

On Wed, Dec 14, 2011 at 11:00 AM, Shijing Xian <xians@chromium.org> wrote:
>
> Eugene, good work, we should push the CL out ASAP, I am working on the same
> thing in the local machine, but I am obviously a bit slow :)
>
> removing the following code int he audio_renderer_impl.cc can get back how
> it sounds:
>     if (filled_frames < number_of_frames) {
>       int frames_to_zero = number_of_frames - filled_frames;
>       memset(audio_data[channel_index], 0, sizeof(float) * frames_to_zero);
>     }
>
> Though I am not 100% sure if it is correct thing to do.
>
> BR,
> /SX
>
>
>
>
> On Wed, Dec 14, 2011 at 6:55 PM, Eugene Nalimov <enal@google.com> wrote:
>>
>> Guys,
>>
>> I believe I fixed all timing-related issues. Now
>> http://www.corp.google.com/~tommi/average.html again plays regularly
>> and in reasonable time.
>>
>> Unfortunately I think it *sounds* slightly differently than before, at
>> least on Windows. Can you please apply
>>
>> http://codereview.chromium.org/8909006
>>
>> and listen? I compared it to 17.0.938.0...
>>
>> Can the difference be caused by us now calling
>> media::DeinterleaveAudioChannel() / media::InterleaveFloatToInt16()?
>>
>> Thanks,
>> Eugene
>>
>> PS. CL does not include tests changes yet.
>>
>> On Wed, Dec 14, 2011 at 5:19 AM, Shijing Xian <xians@chromium.org> wrote:
>> > On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:
>> >>
>> >> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>> >>>
>> >>> Two other comments:
>> >>
>> >>
>> >>> 1. Updated the linked CL so you can just look at the patchset diff
>> >>> ("2")
>> >>> http://codereview.chromium.org/8785008
>> >>
>> >>
>> >>> 2. Because of riskiness involved in this CL, I will make sure to land
>> >>> the
>> >>> it
>> >>> after m17 branch cut.
>> >>
>> >>
>> >> There are at least 4 issues with that CL:
>> >> * Race condition on startup, AudioOutputController::DoPlay() asking for
>> >> data
>> >> while AudioDevice::Run() already processing first package.
>> >
>> >
>> > This can be fixed easily by changing the relevant code in AudioDevice or
>> > AudioOutputController.
>> > I can make a CL for it.
>> >
>> >> * Bad handling of last buffer, we are signalling "end of data" the
>> >> moment
>> >> algorithm_->IsQueueEmpty() returns true to
>> >> AudioRendererBase::FillBuffer(),
>> >> without taking into consideration amount of data already in the OS
>> >> buffers.
>> >> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
>> >> * Even when I band-aid all those issues,
>> >> http://www.corp.google.com/~tommi/average.html still sounds terrible,
>> >> and
>> >> pauses
>> >> between clicks are much longer than they should be. Not sure what
>> >> causes
>> >> the
>> >> problem.
>> >>
>> > I think they are actually one problem, or at least closely related.
>> > In the AudioRendererImpl, we need to update the |earliest_end_time_| to
>> > be
>> > able to notify the
>> > client when we shall stop the playing, which was done
>> > by AudioRendererImpl::UpdateEarliestEndTime(), as Enal pointed out.
>> > And this UpdateEarliestEndTime() needs to know the number of pending
>> > data
>> > reported by the browser.
>> > In the non-low-latency mode, we get the number of pending data using:
>> > uint32 size = buffer_.Read(dest, max_size);
>> > buffers_state.pending_bytes += size + buffer_.forward_bytes();
>> >
>> > And in the low-latency mode
>> > uint32 size =  sync_reader_->Read(dest, max_size);
>> > sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);
>> >
>> > The value from the low-latency mode can be completely wrong, one of the
>> > problems is that sync_reader always
>> > returns a value of buffer size even though no data on the shared_memory_
>> > (it
>> > just writes 0). Which means the client
>> > can't get the right |earliest_end_time_| when it comes to the end of
>> > stream.
>> > While non-low-latency mode is using the seakable buffer, which truly
>> > reports
>> > the number of data it writes to the soundcard.
>> >
>> > I can see that we need to do quite some change in the
>> > AudioOutputController,
>> > besides AudioRendererImpl.
>> >
>> > BR,
>> > /SX
>> >
>> >
>> >> * Nobody is setting length of data in buffer, as a result when polling
>> >> on
>> >> stream
>> >> startup we never exit early and always wait the max allowed time.
>> >> http://codereview.chromium.org/8477037/
>> >
>> >
>
>

no longer working on chromium

> > >> > There are at least 4 issues with that CL: > * ...

9 years ago (2011-12-14 19:11:54 UTC) #33

no longer working on chromium

Great, please write down what kind of manual tests you need to pass on the ...

9 years ago (2011-12-14 19:16:29 UTC) #34

Great, please write down what kind of manual tests you need to pass on the
description of the CL.
So that next time we know what tests we need to run through before
committing the CLs.

BR,
/SX

On Wed, Dec 14, 2011 at 8:10 PM, Eugene Nalimov <enal@google.com> wrote:

> Thank you for pointing. Code was zeroing out not the reminder but the
> entire buffer.
>
> Will work on tests now.
>
> On Wed, Dec 14, 2011 at 11:00 AM, Shijing Xian <xians@chromium.org> wrote:
> >
> > Eugene, good work, we should push the CL out ASAP, I am working on the
> same
> > thing in the local machine, but I am obviously a bit slow :)
> >
> > removing the following code int he audio_renderer_impl.cc can get back
> how
> > it sounds:
> >     if (filled_frames < number_of_frames) {
> >       int frames_to_zero = number_of_frames - filled_frames;
> >       memset(audio_data[channel_index], 0, sizeof(float) *
> frames_to_zero);
> >     }
> >
> > Though I am not 100% sure if it is correct thing to do.
> >
> > BR,
> > /SX
> >
> >
> >
> >
> > On Wed, Dec 14, 2011 at 6:55 PM, Eugene Nalimov <enal@google.com> wrote:
> >>
> >> Guys,
> >>
> >> I believe I fixed all timing-related issues. Now
> >> http://www.corp.google.com/~tommi/average.html again plays regularly
> >> and in reasonable time.
> >>
> >> Unfortunately I think it *sounds* slightly differently than before, at
> >> least on Windows. Can you please apply
> >>
> >> http://codereview.chromium.org/8909006
> >>
> >> and listen? I compared it to 17.0.938.0...
> >>
> >> Can the difference be caused by us now calling
> >> media::DeinterleaveAudioChannel() / media::InterleaveFloatToInt16()?
> >>
> >> Thanks,
> >> Eugene
> >>
> >> PS. CL does not include tests changes yet.
> >>
> >> On Wed, Dec 14, 2011 at 5:19 AM, Shijing Xian <xians@chromium.org>
> wrote:
> >> > On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:
> >> >>
> >> >> On 2011/12/02 23:07:23, Victoria Kirst wrote:
> >> >>>
> >> >>> Two other comments:
> >> >>
> >> >>
> >> >>> 1. Updated the linked CL so you can just look at the patchset diff
> >> >>> ("2")
> >> >>> http://codereview.chromium.org/8785008
> >> >>
> >> >>
> >> >>> 2. Because of riskiness involved in this CL, I will make sure to
> land
> >> >>> the
> >> >>> it
> >> >>> after m17 branch cut.
> >> >>
> >> >>
> >> >> There are at least 4 issues with that CL:
> >> >> * Race condition on startup, AudioOutputController::DoPlay() asking
> for
> >> >> data
> >> >> while AudioDevice::Run() already processing first package.
> >> >
> >> >
> >> > This can be fixed easily by changing the relevant code in AudioDevice
> or
> >> > AudioOutputController.
> >> > I can make a CL for it.
> >> >
> >> >> * Bad handling of last buffer, we are signalling "end of data" the
> >> >> moment
> >> >> algorithm_->IsQueueEmpty() returns true to
> >> >> AudioRendererBase::FillBuffer(),
> >> >> without taking into consideration amount of data already in the OS
> >> >> buffers.
> >> >> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
> >> >> * Even when I band-aid all those issues,
> >> >> http://www.corp.google.com/~tommi/average.html still sounds
> terrible,
> >> >> and
> >> >> pauses
> >> >> between clicks are much longer than they should be. Not sure what
> >> >> causes
> >> >> the
> >> >> problem.
> >> >>
> >> > I think they are actually one problem, or at least closely related.
> >> > In the AudioRendererImpl, we need to update the |earliest_end_time_|
> to
> >> > be
> >> > able to notify the
> >> > client when we shall stop the playing, which was done
> >> > by AudioRendererImpl::UpdateEarliestEndTime(), as Enal pointed out.
> >> > And this UpdateEarliestEndTime() needs to know the number of pending
> >> > data
> >> > reported by the browser.
> >> > In the non-low-latency mode, we get the number of pending data using:
> >> > uint32 size = buffer_.Read(dest, max_size);
> >> > buffers_state.pending_bytes += size + buffer_.forward_bytes();
> >> >
> >> > And in the low-latency mode
> >> > uint32 size =  sync_reader_->Read(dest, max_size);
> >> > sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);
> >> >
> >> > The value from the low-latency mode can be completely wrong, one of
> the
> >> > problems is that sync_reader always
> >> > returns a value of buffer size even though no data on the
> shared_memory_
> >> > (it
> >> > just writes 0). Which means the client
> >> > can't get the right |earliest_end_time_| when it comes to the end of
> >> > stream.
> >> > While non-low-latency mode is using the seakable buffer, which truly
> >> > reports
> >> > the number of data it writes to the soundcard.
> >> >
> >> > I can see that we need to do quite some change in the
> >> > AudioOutputController,
> >> > besides AudioRendererImpl.
> >> >
> >> > BR,
> >> > /SX
> >> >
> >> >
> >> >> * Nobody is setting length of data in buffer, as a result when
> polling
> >> >> on
> >> >> stream
> >> >> startup we never exit early and always wait the max allowed time.
> >> >> http://codereview.chromium.org/8477037/
> >> >
> >> >
> >
> >
>

enal1

Will do. I am also planning to add automatic test. It cannot verify that it ...

9 years ago (2011-12-14 19:31:35 UTC) #35

Will do.

I am also planning to add automatic test. It cannot verify that it is
getting the right sound, but at least it can verify timing -- if we
are playing 146ms file 100 times, total time cannot be less than 14.6
seconds or more than (say) 30 seconds. Such test would immediately
fail facing regression we hit this time. We discussed that with Tommi
last week when I was debugging race condition.

On Wed, Dec 14, 2011 at 11:14 AM, Shijing Xian <xians@chromium.org> wrote:
>
> Great, please write down what kind of manual tests you need to pass on the
> description of the CL.
> So that next time we know what tests we need to run through before
> committing the CLs.
>
> BR,
> /SX
>
> On Wed, Dec 14, 2011 at 8:10 PM, Eugene Nalimov <enal@google.com> wrote:
>>
>> Thank you for pointing. Code was zeroing out not the reminder but the
>> entire buffer.
>>
>> Will work on tests now.
>>
>> On Wed, Dec 14, 2011 at 11:00 AM, Shijing Xian <xians@chromium.org> wrote:
>> >
>> > Eugene, good work, we should push the CL out ASAP, I am working on the
>> > same
>> > thing in the local machine, but I am obviously a bit slow :)
>> >
>> > removing the following code int he audio_renderer_impl.cc can get back
>> > how
>> > it sounds:
>> >     if (filled_frames < number_of_frames) {
>> >       int frames_to_zero = number_of_frames - filled_frames;
>> >       memset(audio_data[channel_index], 0, sizeof(float) *
>> > frames_to_zero);
>> >     }
>> >
>> > Though I am not 100% sure if it is correct thing to do.
>> >
>> > BR,
>> > /SX
>> >
>> >
>> >
>> >
>> > On Wed, Dec 14, 2011 at 6:55 PM, Eugene Nalimov <enal@google.com> wrote:
>> >>
>> >> Guys,
>> >>
>> >> I believe I fixed all timing-related issues. Now
>> >> http://www.corp.google.com/~tommi/average.html again plays regularly
>> >> and in reasonable time.
>> >>
>> >> Unfortunately I think it *sounds* slightly differently than before, at
>> >> least on Windows. Can you please apply
>> >>
>> >> http://codereview.chromium.org/8909006
>> >>
>> >> and listen? I compared it to 17.0.938.0...
>> >>
>> >> Can the difference be caused by us now calling
>> >> media::DeinterleaveAudioChannel() / media::InterleaveFloatToInt16()?
>> >>
>> >> Thanks,
>> >> Eugene
>> >>
>> >> PS. CL does not include tests changes yet.
>> >>
>> >> On Wed, Dec 14, 2011 at 5:19 AM, Shijing Xian <xians@chromium.org>
>> >> wrote:
>> >> > On Tue, Dec 13, 2011 at 7:49 PM, <enal@google.com> wrote:
>> >> >>
>> >> >> On 2011/12/02 23:07:23, Victoria Kirst wrote:
>> >> >>>
>> >> >>> Two other comments:
>> >> >>
>> >> >>
>> >> >>> 1. Updated the linked CL so you can just look at the patchset diff
>> >> >>> ("2")
>> >> >>> http://codereview.chromium.org/8785008
>> >> >>
>> >> >>
>> >> >>> 2. Because of riskiness involved in this CL, I will make sure to
>> >> >>> land
>> >> >>> the
>> >> >>> it
>> >> >>> after m17 branch cut.
>> >> >>
>> >> >>
>> >> >> There are at least 4 issues with that CL:
>> >> >> * Race condition on startup, AudioOutputController::DoPlay() asking
>> >> >> for
>> >> >> data
>> >> >> while AudioDevice::Run() already processing first package.
>> >> >
>> >> >
>> >> > This can be fixed easily by changing the relevant code in AudioDevice
>> >> > or
>> >> > AudioOutputController.
>> >> > I can make a CL for it.
>> >> >
>> >> >> * Bad handling of last buffer, we are signalling "end of data" the
>> >> >> moment
>> >> >> algorithm_->IsQueueEmpty() returns true to
>> >> >> AudioRendererBase::FillBuffer(),
>> >> >> without taking into consideration amount of data already in the OS
>> >> >> buffers.
>> >> >> (Nobody calls AudioRendererImpl::UpdateEarliestEndTime())
>> >> >> * Even when I band-aid all those issues,
>> >> >> http://www.corp.google.com/~tommi/average.html still sounds
>> >> >> terrible,
>> >> >> and
>> >> >> pauses
>> >> >> between clicks are much longer than they should be. Not sure what
>> >> >> causes
>> >> >> the
>> >> >> problem.
>> >> >>
>> >> > I think they are actually one problem, or at least closely related.
>> >> > In the AudioRendererImpl, we need to update the |earliest_end_time_|
>> >> > to
>> >> > be
>> >> > able to notify the
>> >> > client when we shall stop the playing, which was done
>> >> > by AudioRendererImpl::UpdateEarliestEndTime(), as Enal pointed out.
>> >> > And this UpdateEarliestEndTime() needs to know the number of pending
>> >> > data
>> >> > reported by the browser.
>> >> > In the non-low-latency mode, we get the number of pending data using:
>> >> > uint32 size = buffer_.Read(dest, max_size);
>> >> > buffers_state.pending_bytes += size + buffer_.forward_bytes();
>> >> >
>> >> > And in the low-latency mode
>> >> > uint32 size =  sync_reader_->Read(dest, max_size);
>> >> > sync_reader_->UpdatePendingBytes(buffers_state.total_bytes() + size);
>> >> >
>> >> > The value from the low-latency mode can be completely wrong, one of
>> >> > the
>> >> > problems is that sync_reader always
>> >> > returns a value of buffer size even though no data on the
>> >> > shared_memory_
>> >> > (it
>> >> > just writes 0). Which means the client
>> >> > can't get the right |earliest_end_time_| when it comes to the end of
>> >> > stream.
>> >> > While non-low-latency mode is using the seakable buffer, which truly
>> >> > reports
>> >> > the number of data it writes to the soundcard.
>> >> >
>> >> > I can see that we need to do quite some change in the
>> >> > AudioOutputController,
>> >> > besides AudioRendererImpl.
>> >> >
>> >> > BR,
>> >> > /SX
>> >> >
>> >> >
>> >> >> * Nobody is setting length of data in buffer, as a result when
>> >> >> polling
>> >> >> on
>> >> >> stream
>> >> >> startup we never exit early and always wait the max allowed time.
>> >> >> http://codereview.chromium.org/8477037/
>> >> >
>> >> >
>> >
>> >
>
>

enal1

Audio device calls RenderCallback() on startup to pre-populate the buffer, and then it calls in ...

9 years ago (2011-12-14 19:45:00 UTC) #36

no longer working on chromium

OK, then the concern is actually not on the race, but more on the per-buffering ...

9 years ago (2011-12-14 20:47:01 UTC) #37

OK, then the concern is actually not on the race, but more on the
per-buffering and the first packet.

It recalls me some odd code in the AudioOutputControll::DoPlay()

if (LowLatencyMode()) {
    state_ = kStarting;

    // Ask for first packet.
    sync_reader_->UpdatePendingBytes(0);

    // Cannot start stream immediately, should give renderer some time
    // to deliver data.
    number_polling_attempts_left_ = kPollNumAttempts;
    message_loop_->PostDelayedTask(
        FROM_HERE,
        base::Bind(&AudioOutputController::PollAndStartIfDataReady,
        weak_this_.GetWeakPtr()),
        kPollPauseInMilliseconds);
  } else {
    StartStream();
  }

It sends a 0 through the socket and does some kind of pre-buffer, but as I
understand,
this will destroy the pre-buffer in the AudioDevice, which leads to the
loss of the first packet.
I think that is the reason why you need to make AudioRendererImpl to ignore
the first call.

Webrtc does not really need pre-buffer, but I think WebAudio needs it. And
the pre-buffer in
AudioDevice should not hurt anything but makes the start of audio smooth.
So I think we should keep how it is in AudioDevice but fix the problem
in AudioOutputControll::DoPlay().
Basically the idea is to check if DataReady(), if it is, start the stream
immediately, otherwise call
PollAndStartIfDataReady(), then you should be able to get rid of the hack
to which makes
AudioRendererImpl to ignore the first call.
How do you think? I can make a separate CL to do it if you think it is good.

BR,
-SX



On Wed, Dec 14, 2011 at 8:44 PM, Eugene Nalimov <enal@google.com> wrote:

> Audio device calls RenderCallback() on startup to pre-populate the
> buffer, and then it calls in the loop when getting request for data.
> That is probably Ok for other clients but not for html5 -- we are
> creating player object, and it can call Play() lot of times, with
> seeks to new position in between. Every time, when starting new audio
> stream, we would ask for new data, sending request
>
> As a result, depending on the exact timing host can get the first
> buffer filled by pre-populating renderer call, by 2nd renderer call
> (which for short file would just say 'no data' and fill buffer with
> silence, as end-of-file was reached on the 1st call), or something in
> between. That is exactly what happened when I fixed problem at the end
> of the stream but not in the beginning. Situation is different from
> what it were earlier, when there were no pre-populating call -- we
> could get full buffer of silence if renderer started very slowly, but
> position in the audio file would not move, as there were no Render()
> call, and 2nd request will bring in first chunk of real data; playback
> would be delayed, but you will hear the entire file.
>
> We cannot get rid of first request for data in Play(), because
> AudioDevice would pre-populate the buffer only once, and we can have
> several Play() calls. I did not risk to remove pre-populating call
> from AudioDevice, fearing it'll break something for other clients. So
> I just made AudioRendererImpl to ignore very first call to it.
>
> On Wed, Dec 14, 2011 at 11:09 AM, Shijing Xian <xians@chromium.org> wrote:
> >>>
> >>
> >> There are at least 4 issues with that CL:
> >> * Race condition on startup, AudioOutputController::DoPlay() asking for
> >> data
> >> while AudioDevice::Run() already processing first package.
> >
> >
> > Eugene, could you please be more specific on this race condition?
> > I dont see any thing obviously wrong in the code, do we have any test to
> > address the problem?
> >
> > BR,
> > /SX
>

Chris Rogers

On Wed, Dec 14, 2011 at 12:43 PM, Shijing Xian <xians@chromium.org> wrote: > > OK, ...

9 years ago (2011-12-14 20:51:28 UTC) #38

enal1

DoPlay() can be called lot of times on the same AudioDevice, and the buffer may ...

9 years ago (2011-12-14 20:53:53 UTC) #39

DoPlay() can be called lot of times on the same AudioDevice, and the
buffer may keep its state from the previous playback. So we are asking
for fresh data here.

If you want to work on some CL, can you please get rid of
double-closing the socket handle? It is very annoying to debug now...

On Wed, Dec 14, 2011 at 12:43 PM, Shijing Xian <xians@chromium.org> wrote:
>
> OK, then the concern is actually not on the race, but more on the
> per-buffering and the first packet.
>
> It recalls me some odd code in the AudioOutputControll::DoPlay()
>
> if (LowLatencyMode()) {
>     state_ = kStarting;
>
>     // Ask for first packet.
>     sync_reader_->UpdatePendingBytes(0);
>
>     // Cannot start stream immediately, should give renderer some time
>     // to deliver data.
>     number_polling_attempts_left_ = kPollNumAttempts;
>     message_loop_->PostDelayedTask(
>         FROM_HERE,
>         base::Bind(&AudioOutputController::PollAndStartIfDataReady,
>         weak_this_.GetWeakPtr()),
>         kPollPauseInMilliseconds);
>   } else {
>     StartStream();
>   }
>
> It sends a 0 through the socket and does some kind of pre-buffer, but as I
> understand,
> this will destroy the pre-buffer in the AudioDevice, which leads to the loss
> of the first packet.
> I think that is the reason why you need to make AudioRendererImpl to ignore
> the first call.
>
> Webrtc does not really need pre-buffer, but I think WebAudio needs it. And
> the pre-buffer in
> AudioDevice should not hurt anything but makes the start of audio smooth.
> So I think we should keep how it is in AudioDevice but fix the problem
> in AudioOutputControll::DoPlay().
> Basically the idea is to check if DataReady(), if it is, start the stream
> immediately, otherwise call
> PollAndStartIfDataReady(), then you should be able to get rid of the hack to
> which makes
> AudioRendererImpl to ignore the first call.
> How do you think? I can make a separate CL to do it if you think it is good.
>
> BR,
> -SX
>
>
>
> On Wed, Dec 14, 2011 at 8:44 PM, Eugene Nalimov <enal@google.com> wrote:
>>
>> Audio device calls RenderCallback() on startup to pre-populate the
>> buffer, and then it calls in the loop when getting request for data.
>> That is probably Ok for other clients but not for html5 -- we are
>> creating player object, and it can call Play() lot of times, with
>> seeks to new position in between. Every time, when starting new audio
>> stream, we would ask for new data, sending request
>>
>> As a result, depending on the exact timing host can get the first
>> buffer filled by pre-populating renderer call, by 2nd renderer call
>> (which for short file would just say 'no data' and fill buffer with
>> silence, as end-of-file was reached on the 1st call), or something in
>> between. That is exactly what happened when I fixed problem at the end
>> of the stream but not in the beginning. Situation is different from
>> what it were earlier, when there were no pre-populating call -- we
>> could get full buffer of silence if renderer started very slowly, but
>> position in the audio file would not move, as there were no Render()
>> call, and 2nd request will bring in first chunk of real data; playback
>> would be delayed, but you will hear the entire file.
>>
>> We cannot get rid of first request for data in Play(), because
>> AudioDevice would pre-populate the buffer only once, and we can have
>> several Play() calls. I did not risk to remove pre-populating call
>> from AudioDevice, fearing it'll break something for other clients. So
>> I just made AudioRendererImpl to ignore very first call to it.
>>
>> On Wed, Dec 14, 2011 at 11:09 AM, Shijing Xian <xians@chromium.org> wrote:
>> >>>
>> >>
>> >> There are at least 4 issues with that CL:
>> >> * Race condition on startup, AudioOutputController::DoPlay() asking for
>> >> data
>> >> while AudioDevice::Run() already processing first package.
>> >
>> >
>> > Eugene, could you please be more specific on this race condition?
>> > I dont see any thing obviously wrong in the code, do we have any test to
>> > address the problem?
>> >
>> > BR,
>> > /SX
>
>

enal1

So I'll get rid of pre-population and send modified CL. On Wed, Dec 14, 2011 ...

9 years ago (2011-12-14 21:05:26 UTC) #40

no longer working on chromium

Thanks Chris for the clarification. Then the only reason for this pre-buffer is likely because ...

9 years ago (2011-12-14 21:08:23 UTC) #41

Chris Rogers

On Wed, Dec 14, 2011 at 1:08 PM, Shijing Xian <xians@chromium.org> wrote: > > Thanks ...

9 years ago (2011-12-15 00:41:42 UTC) #42

vrk (LEFT CHROMIUM)

I was out of office a few days and missed this discussion! Could I get ...

9 years ago (2011-12-15 01:55:15 UTC) #43

enal1

Yes, Victoria, you correctly summarized current situation. On Wed, Dec 14, 2011 at 5:55 PM, ...

9 years ago (2011-12-15 02:18:57 UTC) #44

enal1

9 years ago (2011-12-15 02:31:10 UTC) #45

We have all platforms yield on startup, it works on "normal" thread,
not in time-critical callback. Actually, there are 2 different places
in the code -- for 1st buffer I could schedule delayed callback if
data is not ready yet, so we are not burning extra cycles or blocking
message loop. For the 2nd/3rd buffers I had to use sleep(), it is
almost impossible to use message loop there. Those sleep() calls are
not in the callbacks either.

We have Windows-only yield() inside a callback for all buffers, but on
Windows callback is called by one of the worker threads in the process
thread pool, not by anything time-critical. That helps on a very slow
single-core systems running XP (I am using my 9-years old notebook for
testing).

On Wed, Dec 14, 2011 at 4:41 PM, Chris Rogers <crogers@google.com> wrote:
>
>
> On Wed, Dec 14, 2011 at 1:08 PM, Shijing Xian <xians@chromium.org> wrote:
>>
>>
>> Thanks Chris for the clarification. Then the only reason for this
>> pre-buffer is likely because
>> OnMoreData() reads the share_memory before sending a signal to
>> sync_socket, so it needs
>> to be some data in the share_memory, otherwise sync_reader::Read() will
>> yield.
>
>
> I think that was the intention - as long as we only yield on the very first
> time.  I remember we had problems where Yield() was being called *every*
> time OnMoreData() was called in certain cases.  We have to be super careful
> that doesn't ever happen.  I'm not even a big fan of the approach of using
> Yield() on the first time, but...
>
>>
>>
>>
>> On Wed, Dec 14, 2011 at 9:53 PM, Eugene Nalimov <enal@google.com> wrote:
>>>
>>> DoPlay() can be called lot of times on the same AudioDevice, and the
>>> buffer may keep its state from the previous playback. So we are asking
>>> for fresh data here.
>>>
>>> If you want to work on some CL, can you please get rid of
>>> double-closing the socket handle? It is very annoying to debug now...
>>>
>>
>> It has been ongoing, using shutdown(socket_handle_) solves all the
>> problems for linux and mac.
>> And Tommi is taking a look at windows. What I heard last time was that he
>> had some problem
>> on XP. But I believe he will soon have the CL out.
>>
>> BR,
>> /SX
>>
>

Issue 8477037: Simplify AudioRendererImpl by using AudioDevice. (Closed)

Description

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Messages