Audio software mixer.

Re-land software audio mixer.
Code goes through old or new paths depending on the AudioParameters or command-line flag.
Added unit tests.
Changed suppression for http://code.google.com/p/chromium/issues/detail?id=123112
because call stack changed (class was split into 2).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133010

[enal1, 2012-03-26 20:22:49]

Sending for official review...

[scherkus (not reviewing), 2012-03-27 14:34:19]

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
media/audio/audio_parameters.h:36: bool use_browser_mixer,
Is the only reason to have programmatic control over using the mixer is to let
certain clients opt out of using the mixer?

Given we're developing this behind a flag I don't see a reason to include this
until we've proven that the mixer isn't a good fit for those clients. In other
words, reverse the logic and have the flag *enable* the mixer for *all* clients.

That way it stays off by default, the media team can test with it on, fix bugs +
analyze performance, then turn it on by default. If everything works as intended
then there'll be no need to for use_browser_mixer at all.

[enal1, 2012-03-27 15:12:01]

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
media/audio/audio_parameters.h:36: bool use_browser_mixer,
yes, not having that flag is very lukrative suggestion. Among other things it
will drastically reduce amount of changed files.

Unfortunately, there are strong arguments for that flag as well. Main argument
is that it will allow per-client switch to the mixer, as developers for some
clients feel their clients don't need mixer.

Chris was very happy with that flag...

On 2012/03/27 14:34:19, scherkus wrote:
> Is the only reason to have programmatic control over using the mixer is to let
> certain clients opt out of using the mixer?
> 
> Given we're developing this behind a flag I don't see a reason to include this
> until we've proven that the mixer isn't a good fit for those clients. In other
> words, reverse the logic and have the flag *enable* the mixer for *all*
clients.
> 
> That way it stays off by default, the media team can test with it on, fix bugs
+
> analyze performance, then turn it on by default. If everything works as
intended
> then there'll be no need to for use_browser_mixer at all.

[scherkus (not reviewing), 2012-03-27 15:17:05]

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
media/audio/audio_parameters.h:36: bool use_browser_mixer,
On 2012/03/27 15:12:01, enal1 wrote:
> yes, not having that flag is very lukrative suggestion. Among other things it
> will drastically reduce amount of changed files.
> 
> Unfortunately, there are strong arguments for that flag as well. Main argument
> is that it will allow per-client switch to the mixer, as developers for some
> clients feel their clients don't need mixer.
> 
> Chris was very happy with that flag...

They are arguments that aren't based on any real world data or analysis.

> On 2012/03/27 14:34:19, scherkus wrote:
> > Is the only reason to have programmatic control over using the mixer is to
let
> > certain clients opt out of using the mixer?
> > 
> > Given we're developing this behind a flag I don't see a reason to include
this
> > until we've proven that the mixer isn't a good fit for those clients. In
other
> > words, reverse the logic and have the flag *enable* the mixer for *all*
> clients.
> > 
> > That way it stays off by default, the media team can test with it on, fix
bugs
> +
> > analyze performance, then turn it on by default. If everything works as
> intended
> > then there'll be no need to for use_browser_mixer at all.
>

[enal1, 2012-03-27 15:34:08]

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/18001/media/audio/audio_p...
media/audio/audio_parameters.h:36: bool use_browser_mixer,
Arguments are "my code right now works reasonable, I have lot of my own things
to do, so please don't create me new issues. You need something -- please do it,
and when it works reasonable you can switch my code to it, but don't create any
problems for me right now".

I'd say that is reasonable attitude, and peace between groups may be worth extra
(temporary) code complexity.

On 2012/03/27 15:17:06, scherkus wrote:
> On 2012/03/27 15:12:01, enal1 wrote:
> > yes, not having that flag is very lukrative suggestion. Among other things
it
> > will drastically reduce amount of changed files.
> > 
> > Unfortunately, there are strong arguments for that flag as well. Main
argument
> > is that it will allow per-client switch to the mixer, as developers for some
> > clients feel their clients don't need mixer.
> > 
> > Chris was very happy with that flag...
> 
> They are arguments that aren't based on any real world data or analysis.
> 
> > On 2012/03/27 14:34:19, scherkus wrote:
> > > Is the only reason to have programmatic control over using the mixer is to
> let
> > > certain clients opt out of using the mixer?
> > > 
> > > Given we're developing this behind a flag I don't see a reason to include
> this
> > > until we've proven that the mixer isn't a good fit for those clients. In
> other
> > > words, reverse the logic and have the flag *enable* the mixer for *all*
> > clients.
> > > 
> > > That way it stays off by default, the media team can test with it on, fix
> bugs
> > +
> > > analyze performance, then turn it on by default. If everything works as
> > intended
> > > then there'll be no need to for use_browser_mixer at all.
> > 
>

[enal1, 2012-03-28 18:46:59]

Removed flag in AudioParameters, made command-flag "enabling" one, i.e. mixer is
turned off by default.

[scherkus (not reviewing), 2012-03-29 22:46:39]

thanks for making the audioparameters change enal!

I'll defer to tommi/vrk for the bulk of the review -- just had a nit about
switch under content code

https://chromiumcodereview.appspot.com/9691001/diff/26001/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/26001/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:697:
switches::kEnableAudioMixer,
I don't believe this is needed anymore as no code under content/renderer/
requires knowledge that this switch has been set

[enal1, 2012-03-30 16:45:49]

Sync'ed sources, addressed Andrew's comment.

[vrk (LEFT CHROMIUM), 2012-03-30 22:25:58]

Hey enal, sorry for the delay in reviewing! I wasn't very familiar with this
neck of the audio woods so it took me a lot of prior code reading to understand
what was going on...

Things are looking pretty good, but before I finish reviewing, I wanted to bring
up a concern that I have. 

AudioOutputDispatcher has now turned into a shared interface between
AudioOutputDispatcherImpl (the existing logic) and AudioOutputMixer. 

The AudioOutputDispatcher interface has a couple of big flaws:

 - StreamStarted() returns the physical stream for AudioOutputDispatcherImpl,
but it returns the stream_proxy (meaningless) for AudioOutputMixer.

 - For both StreamClosed() and StreamStopped(), the stream_proxy parameter is
meaningless for AudioOutputDispatcherImpl, and the physical_stream parameter is
meaningless for AudioOutputMixer

 - Creating/opening streams between AudioOutputDispatcherImpl and
AudioOutputMixer have very similar logic.

 - Both AudioOutputDispatcherImpl and AudioOutputMixer have a timer that has
very similar logic.

What this means is there needs to be a better shared interface between the
AudioOutputDispatcherImpl and AudioOutputMixer. I also think it should be
possible to share more code with regard to the physical streams -- the timer,
creation, and closing should all be pretty similar, I would think, only that the
AudioOutputMixer has only one stream and the AudioOutputDispatcherImpl may have
more than one.

It will be easier and faster to iterate on the AudioOutputDispatcher interface
design if you added it to your design doc instead of coding it out. You could
write out the methods of the AudioOutputDispatcher, then write a brief
description of how AudioOutputDispatcherImpl and AudioOutputMixer would
implement each.

Would you please update the design doc, then ping me/others for comments?
Thanks!

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_m...
File media/audio/audio_manager_base.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_m...
media/audio/audio_manager_base.h:9: #include <string>
remove unneeded include

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_o...
File media/audio/audio_output_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:22: #include <list>
no longer needed

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/26001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:48: physical_stream_=
dispatcher_->StreamStarted(callback, this);
nit: space before "="

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:139: base::AutoLock lock(lock_);
Hmm, I'm a little worried about lock contention. This is a potentially
time-intensive method call, and if I understand correctly, many different
threads may be calling StreamStarted().

Since this is the first implementation, I think it's right to do the safe thing
by locking and not worry about hypothetical problems. But please add a TODO to
revisit this and see if it causes problems in practice.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:154: int pending_bytes =
std::min(stream_proxy->pending_bytes(),
|pending_bytes| isn't actually used for anything useful -- did you mean to use
it in line 162?

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:158: uint32 actual_size =
stream_proxy->audio_source_callback()->OnMoreData(
Exposing audio_source_callback() is leaking implementation details. Executing
|stream_proxy|'s audio_source_callback should be abstracted away having
AudioOutputProxy implement the AudioSourceCallback interface that will call its
callback under the covers.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:162:
AudioBuffersState(buffers_state.pending_bytes, 0));
You're right that there's no way to get the exact amount of
hardware_delay_bytes, but I'm worried about using "0" here. AV sync relies on
hardware_delay_bytes being accurate, or at least relatively accurate.

I think this is what you were trying to solve with the |pending_bytes|
calculation? Can you explain more what you were doing with |pending_bytes|?

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:185: if (first_stream) {
Instead of the "first_stream" bool, please handle the first stream case before
the for-loop, and begin the for loop on the 2nd element of the list.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:213:
(*it)->audio_source_callback()->OnError(stream, code);
In the same vein as line 158, do not expose audio_source_callback() and instead
have AudioOutputProxy implement the AudioSourceCallback interface.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:220:
(*it)->audio_source_callback()->WaitTillDataReady();
In the same vein as line 158, do not expose audio_source_callback() and instead
have AudioOutputProxy implement the AudioSourceCallback interface.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.h:66: typedef std::vector<AudioOutputProxy*>
ProxyList;
Instead of a vector of AudioOutputProxies, please change this to a map of
AudioSourceCallbacks -> struct { double volume; int pending_bytes; }. (You will
need to make AudioOutputProxy extend AudioSourceCallback, as described in the
mixer.cc file.)

You should make this change because:
- You really only want to call the AudioSourceCallback methods, and have no need
for any other AudioOutputProxy method
- AudioOutputProxy never needs pending_bytes and volume itself; it gets set by
AudioOutputMixer then used by AudioOutputMixer.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:48: physical_stream_=
dispatcher_->StreamStarted(callback, this);
Yikes, this is a big hack. If |dispatcher_| is an AudioOutputDispatcherImpl, you
return the physical stream, but if |dispatcher_| is an AudioOutputMixer, you
return the AudioOutputProxy, since the physical stream is kept in the
AudioOutputMixer instead of the proxy.

The physical stream(s) need to be kept in either the AudioOutputProxy *or*
derived classes of the dispatcher, but not both. I elaborate more feedback on
this in the overall comments.

[tommi (sloooow) - chröme, 2012-04-03 13:47:51]

Sorry for the huge delay Eugene.  I'm jumping between the design doc and the cl
and still groking the change so my comments for now are very shallow! :)
I'll be quicker the next time around.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:42: virtual bool StreamOpened() = 0;
Can you change this method name to OpenStream.  As is, the name+comments are
confusing.  Then update the comments to something like:

// Called by AudioOutputProxy when the stream needs to be opened.
virtual bool OpenStream() = 0;

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:47:
AudioOutputStream::AudioSourceCallback* callback,
add documentation for the new parameters

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:52: virtual void
StreamStopped(AudioOutputStream* physical_stream,
documentation for the parameters

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:42: if (idle_streams_.empty() &&
!CreateAndOpenStream()) {
nit: no {}

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:55: if (idle_streams_.empty() &&
!CreateAndOpenStream()) {
nit: no {}

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:85: paused_proxies_++;
nit: ++paused_proxies_;

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:100: if (physical_stream)
missing DCHECK_EQ(MessageLoop::current(), message_loop_); first?

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:155: AudioOutputStream* stream =
audio_manager_->MakeAudioOutputStream(params_);
first DCHECK_EQ(MessageLoop::current(), message_loop_);

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:168: // Make sure that we have at
least one stream allocated if there
DCHECK_EQ(MessageLoop::current(), message_loop_);

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.h:37: base::TimeDelta close_delay);
const &

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.h:64: ~AudioOutputDispatcherImpl();
virtual?

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:187:
media::AdjustVolume(static_cast<void*>(actual_dest),
cast not necessary

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:199:
media::MixStreams(static_cast<void*>(dest),
casts not necessary and static_cast to void* seems a bit odd

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.h:55: ~AudioOutputMixer();
virtual

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:44: DCHECK_EQ(state_, kOpened);
DCHECK(!audio_source_callback_)?

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:46: audio_source_callback_ = callback;
nit: just assign to this variable where you set state_ to kPlaying.

[enal1, 2012-04-04 18:46:55]

I believe I addressed everything.

It is not easy to move timer from derived to base class -- of course that was
first thing I tried. Timer accepts base class method, so you end up with base
class method that just calls virtual method. There were also problems with "weak
this" -- timer in base class expects base "weak this", but to post a task
derived class needs derived "weak this", everything was painful, required lot of
code, and was much more complex than just having each derived class to declare
it own members.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:42: virtual bool StreamOpened() = 0;
On 2012/04/03 13:47:51, tommi wrote:
> Can you change this method name to OpenStream.  As is, the name+comments are
> confusing.  Then update the comments to something like:
> 
> // Called by AudioOutputProxy when the stream needs to be opened.
> virtual bool OpenStream() = 0;

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:47:
AudioOutputStream::AudioSourceCallback* callback,
On 2012/04/03 13:47:51, tommi wrote:
> add documentation for the new parameters

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher.h:52: virtual void
StreamStopped(AudioOutputStream* physical_stream,
On 2012/04/03 13:47:51, tommi wrote:
> documentation for the parameters

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:42: if (idle_streams_.empty() &&
!CreateAndOpenStream()) {
On 2012/04/03 13:47:51, tommi wrote:
> nit: no {}

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:55: if (idle_streams_.empty() &&
!CreateAndOpenStream()) {
On 2012/04/03 13:47:51, tommi wrote:
> nit: no {}

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:85: paused_proxies_++;
On 2012/04/03 13:47:51, tommi wrote:
> nit: ++paused_proxies_;

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:100: if (physical_stream)
On 2012/04/03 13:47:51, tommi wrote:
> missing DCHECK_EQ(MessageLoop::current(), message_loop_); first?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:155: AudioOutputStream* stream =
audio_manager_->MakeAudioOutputStream(params_);
On 2012/04/03 13:47:51, tommi wrote:
> first DCHECK_EQ(MessageLoop::current(), message_loop_);

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:168: // Make sure that we have at
least one stream allocated if there
On 2012/04/03 13:47:51, tommi wrote:
> DCHECK_EQ(MessageLoop::current(), message_loop_);

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.h:37: base::TimeDelta close_delay);
On 2012/04/03 13:47:51, tommi wrote:
> const &

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.h:64: ~AudioOutputDispatcherImpl();
On 2012/04/03 13:47:51, tommi wrote:
> virtual?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:139: base::AutoLock lock(lock_);
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> Hmm, I'm a little worried about lock contention. This is a potentially
> time-intensive method call, and if I understand correctly, many different
> threads may be calling StreamStarted().
> 
> Since this is the first implementation, I think it's right to do the safe
thing
> by locking and not worry about hypothetical problems. But please add a TODO to
> revisit this and see if it causes problems in practice.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:154: int pending_bytes =
std::min(stream_proxy->pending_bytes(),
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> |pending_bytes| isn't actually used for anything useful -- did you mean to use
> it in line 162?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:158: uint32 actual_size =
stream_proxy->audio_source_callback()->OnMoreData(
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> Exposing audio_source_callback() is leaking implementation details. Executing
> |stream_proxy|'s audio_source_callback should be abstracted away having
> AudioOutputProxy implement the AudioSourceCallback interface that will call
its
> callback under the covers.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:162:
AudioBuffersState(buffers_state.pending_bytes, 0));
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> You're right that there's no way to get the exact amount of
> hardware_delay_bytes, but I'm worried about using "0" here. AV sync relies on
> hardware_delay_bytes being accurate, or at least relatively accurate.
> 
> I think this is what you were trying to solve with the |pending_bytes|
> calculation? Can you explain more what you were doing with |pending_bytes|?

There are 2 different problems here:
(1) We don't know |pending _bytes| for individual stream, and we are giving that
value to stream's OnMoreData(), and it can be used there -- I believe it is used
to evaluate exact position of data it should return. Current code "sorta works"
if everything works perfectly, but would have problems if some of the buffers
are only partially filled -- we don't know how how much data was in the buffer
OS returned to us, so we cannot correctly calculate new value. If we know number
of buffers we can solve the problem by storing not one value but vector of them.
(2) There is no way to get |hardware_delay_bytes| after we switched to the low
latency mode. We should either change the interface, passing not one but 2 words
via sockets (I tried that, but at the moment it was very hard to change native
client; now they are in better shape, so maybe it can be done), or get rid of it
completely -- I changed AudioRendererBase to depend on timing instead of
|hardware_delay_bytes|.

Added comment

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:185: if (first_stream) {
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> Instead of the "first_stream" bool, please handle the first stream case before
> the for-loop, and begin the for loop on the 2nd element of the list.

I thought about it, but decided against it -- (1) there is some common code, and
(2) there is extra "continue" in case volume is zero, and having separate loop
to search for first non-muted stream looks awkward.

Of course we can assume that muting case is rare and accept any first stream,
even if it is muted, but I believe current code is clean enough.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:187:
media::AdjustVolume(static_cast<void*>(actual_dest),
On 2012/04/03 13:47:51, tommi wrote:
> cast not necessary

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:199:
media::MixStreams(static_cast<void*>(dest),
On 2012/04/03 13:47:51, tommi wrote:
> casts not necessary and static_cast to void* seems a bit odd

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:213:
(*it)->audio_source_callback()->OnError(stream, code);
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> In the same vein as line 158, do not expose audio_source_callback() and
instead
> have AudioOutputProxy implement the AudioSourceCallback interface.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:220:
(*it)->audio_source_callback()->WaitTillDataReady();
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> In the same vein as line 158, do not expose audio_source_callback() and
instead
> have AudioOutputProxy implement the AudioSourceCallback interface.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_mixer.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.h:55: ~AudioOutputMixer();
On 2012/04/03 13:47:51, tommi wrote:
> virtual

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_mixer.h:66: typedef std::vector<AudioOutputProxy*>
ProxyList;
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> Instead of a vector of AudioOutputProxies, please change this to a map of
> AudioSourceCallbacks -> struct { double volume; int pending_bytes; }. (You
will
> need to make AudioOutputProxy extend AudioSourceCallback, as described in the
> mixer.cc file.)
> 
> You should make this change because:
> - You really only want to call the AudioSourceCallback methods, and have no
need
> for any other AudioOutputProxy method
> - AudioOutputProxy never needs pending_bytes and volume itself; it gets set by
> AudioOutputMixer then used by AudioOutputMixer.

AudioOutputProxy needs to store volume, as it is part of the AudioOutputStream
interface.

Moved pending_bytes to |proxies_|.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:44: DCHECK_EQ(state_, kOpened);
On 2012/04/03 13:47:51, tommi wrote:
> DCHECK(!audio_source_callback_)?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:46: audio_source_callback_ = callback;
On 2012/04/03 13:47:51, tommi wrote:
> nit: just assign to this variable where you set state_ to kPlaying.

audio_source_callback_->OnMoreData() is called by dispatcher when starting the
stream.

https://chromiumcodereview.appspot.com/9691001/diff/33001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:48: physical_stream_=
dispatcher_->StreamStarted(callback, this);
On 2012/03/30 22:25:58, Victoria Kirst wrote:
> Yikes, this is a big hack. If |dispatcher_| is an AudioOutputDispatcherImpl,
you
> return the physical stream, but if |dispatcher_| is an AudioOutputMixer, you
> return the AudioOutputProxy, since the physical stream is kept in the
> AudioOutputMixer instead of the proxy.
> 
> The physical stream(s) need to be kept in either the AudioOutputProxy *or*
> derived classes of the dispatcher, but not both. I elaborate more feedback on
> this in the overall comments.

Done.

[vrk (LEFT CHROMIUM), 2012-04-06 23:37:04]

The AudioOutputDispatcher interface is looking better, but AudioOutputProxy's
threading story has gotten trickier. Neither of the derived classes *need* to
touch AudioOutputProxy, and I think it's better if neither of them do so as to
keep AudioOutputProxy on one thread. See comments for suggested changes.

Again I'm going to suggest that you update your design doc so that this is
easier to discuss, or at least so there's documentation. The "pending bytes"
issue in particular is tricky and important, considering how pending bytes is
used to calibrate the pipeline clock. Also, it looks like AudioOutputMixer has
some subtle threading gotchas. This should definitely be documented and
discussed.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_m...
File media/audio/audio_manager_base.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_m...
media/audio/audio_manager_base.h:9: #include <string>
You still need to remove this include.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_dispatcher.cc (left):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher.cc:34: DCHECK_EQ(MessageLoop::current(),
message_loop_);
Why is this DCHECK deleted?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:59:
DCHECK_NE(static_cast<AudioOutputStream*>(NULL), physical_stream);
DCHECK(physical_stream)?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:87:
DCHECK_NE(static_cast<AudioOutputStream*>(NULL), physical_stream);
Remove DCHECK: doing a null DCHECK before usage isn't generally very helpful

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:159: pausing_streams_.clear();
Do you need to clear the proxy_to_physical_map_ so that the destructor doesn't
DCHECK?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:66: // We cannot start physical stream under
the lock,
I don't understand this problem. Start() should not make a blocking call to
OnMoreData(), right? Isn't OnMoreData() called on a different thread, meaning
Start() would have to explicitly block for this deadlock to occur?

Can you update the audio_output_mixer.h with an explanation of the threading
story in this class?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:78: // Because of possible deadlock we cannot
stop physical stream
Again, what is the deadlock you're worried about here?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:113: StopStream(proxies_.begin()->first);
nit: I don't like this because it assumes StopStream() is going to delete the
given stream from the list. Instead, make a copy of the list and stop() on all
the copies, then clear |proxies_|?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:129: uint32
AudioOutputMixer::OnMoreData(AudioOutputStream* stream,
What is the first parameter of OnMoreData() used for? Can you get rid of it?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:155: // TODO(enal): We don't know |pending
_bytes| for individual stream, and we
The pending bytes problem is a tricky one indeed... I'm worried about how the
"ended" event will be fired for each of these streams. Have you checked how the
ended event is fired for several short streams mixed into a large stream?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:158: // Current code "sorta works" if
everything works perfectly, but would have
(sorry in advance for this huge comment, but I can't think of a better way to
explain!)

I know you're saying it only "sorta works" but I think we can get a better
estimate. This is my understanding of how the current code would work:

stream 1: 57 bytes long
stream 2: 200 bytes long
max_size: 50 bytes

ITERATION ONE: 
OnMoreData: buffers_state.pending_bytes = 0
stream 1:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50
stream 2:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50

ITERATION TWO: 
OnMoreData: buffers_state.pending_bytes = 30
stream 1:
  pending_bytes = min(50, 30) = 30
  actual_size = 7
  proxy_data->pending_bytes = 30 + 7 = 37
stream 2:
  pending_bytes = min(50, 30) = 30
  actual_size = 50
  proxy_data->pending_bytes = 30 + 50 = 80

ITERATION THREE: 
OnMoreData: buffers_state.pending_bytes = 60
stream 1:
  pending_bytes = min(37, 60) = 30
  actual_size = 0
  proxy_data->pending_bytes = max(30 - 50, 0) = 0   ******
stream 2:
  pending_bytes = min(80, 60) = 60
  actual_size = 50
  proxy_data->pending_bytes = 60 + 50 = 110

This isn't correct. In iteration 2, stream 1 was mixed into stream 2. This means
there are 80 bytes at the device at the end of iteration 2. At the beginning of
iteration 3, 80 has gone down to 60, meaning 20 bytes were consumed at the
device. That means stream 1 should have 17 bytes to go, not 0 (see ****** line).

Wouldn't it make more sense to do the computation using the difference between
the previous iteration's |actual_total_size| and the
buffers_state.pending_bytes?

For example:

End of ITERATION TWO:
previous_bytes_sent_to_device =
    actual_total_size + buffers_state.pending_bytes = 80

ITERATION THREE:
bytes_consumed = previous_bytes_sent_to_device - buffers_state.pending_bytes =
20

if (actual_size == 0) {
  pending_bytes -= bytes_consumed
  etc
}

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:174: // If stream ended, pending_bytes goes
down by max_size.
After a stream is ended, when does it get removed from |proxies_|?

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_mixer.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.h:69: typedef std::map<AudioOutputProxy*,
ProxyData> ProxyMap;
This is closer to what I was thinking, but now that I see the changes to
AudioOutputProxy, I have a bit of a different suggestion:

Instead of referencing AudioOutputProxy at all, you should create a struct or
class with the data that you need. I'm thinking something like

struct AudioSource {
  int stream_id
  AudioSourceCallback* audio_source_cb_;
  int pending_bytes;
  double volume;
}

Then you can have a map of ids -> AudioSource.

The ID can come from the stream_id_ used in AudioDevice and AudioRendererHost.
It will take some wiring through... again, I would recommend adding this to a
design doc and getting consensus on it instead of coding it first!

The main benefit of this is AudioOutputProxy can continue to live on one thread
instead of living on multiple threads. The audio threading story is complicated
enough, and I think it's important to avoid adding complexity.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:66: base::AutoLock lock(lock_);
See suggestion in audio_output_mixer.h, and remove this lock.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:94: uint32
AudioOutputProxy::OnMoreData(AudioOutputStream* stream,
See suggestion in audio_output_mixer.h, and remove these methods. (Sorry, I know
I originally suggested these methods! But I didn't consider the threading
scenario in this class.)

[enal1, 2012-04-16 22:01:35]

Addressed everything. Thanks, Victoria, code looks better now, even though I had
to revert some of the changes asked for during round 1.

I also updated design doc, though did not go very deep. If design doc contains
too many details it quickly becomes obsolete, it is hard to keep even single
copy of comments in sync with code...

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_m...
File media/audio/audio_manager_base.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_m...
media/audio/audio_manager_base.h:9: #include <string>
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> You still need to remove this include.

Line 50:

  virtual AudioInputStream* MakeAudioInputStream(
      const AudioParameters& params, const std::string& device_id) OVERRIDE;

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_dispatcher.cc (left):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher.cc:34: DCHECK_EQ(MessageLoop::current(),
message_loop_);
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> Why is this DCHECK deleted?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_dispatcher_impl.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:59:
DCHECK_NE(static_cast<AudioOutputStream*>(NULL), physical_stream);
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> DCHECK(physical_stream)?

Done.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:87:
DCHECK_NE(static_cast<AudioOutputStream*>(NULL), physical_stream);
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> Remove DCHECK: doing a null DCHECK before usage isn't generally very helpful

Done.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_dispatcher_impl.cc:159: pausing_streams_.clear();
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> Do you need to clear the proxy_to_physical_map_ so that the destructor doesn't
> DCHECK?

StopStream() removes physical stream from the map. If we are deleting dispatcher
while some stream is still playing that is definitely a bug, and it's good that
destructor catches it.

Added more checks to the destructor.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:66: // We cannot start physical stream under
the lock,
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> I don't understand this problem. Start() should not make a blocking call to
> OnMoreData(), right?

Not right. Starting stream will call OnMoreData() to fill initial buffers on the
thread Start() is called. Exact number of buffers depends on the operating
system and type of the stream (normal/low latency).

> Isn't OnMoreData() called on a different thread, meaning
> Start() would have to explicitly block for this deadlock to occur?
> 
> Can you update the audio_output_mixer.h with an explanation of the threading
> story in this class?

There are very good descriptions for both interfaces it implements,
AudioOutputDispatcher and AudioSourceCallback, in corresponding files. I prefer
not to duplicate the comments, it's hard to keep even single instance of
comments in sync with code...

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:78: // Because of possible deadlock we cannot
stop physical stream
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> Again, what is the deadlock you're worried about here?

Updated comment.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:113: StopStream(proxies_.begin()->first);
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> nit: I don't like this because it assumes StopStream() is going to delete the
> given stream from the list. Instead, make a copy of the list and stop() on all
> the copies, then clear |proxies_|?

Updated comment in the header file, now it says that |proxies_| contains only
currently playing streams.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:129: uint32
AudioOutputMixer::OnMoreData(AudioOutputStream* stream,
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> What is the first parameter of OnMoreData() used for? Can you get rid of it?

It is not used, and I am planning to get rid of it, but it requires changes in
two dozen files or so, so it should be separate CL.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:155: // TODO(enal): We don't know |pending
_bytes| for individual stream, and we
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> The pending bytes problem is a tricky one indeed... I'm worried about how the
> "ended" event will be fired for each of these streams. Have you checked how
the
> ended event is fired for several short streams mixed into a large stream?

Yes, I checked that.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:158: // Current code "sorta works" if
everything works perfectly, but would have
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> (sorry in advance for this huge comment, but I can't think of a better way to
> explain!)
> 
> I know you're saying it only "sorta works" but I think we can get a better
> estimate. This is my understanding of how the current code would work:
> 
> stream 1: 57 bytes long
> stream 2: 200 bytes long
> max_size: 50 bytes
> 
> ITERATION ONE: 
> OnMoreData: buffers_state.pending_bytes = 0
> stream 1:
>   pending_bytes = min(0, 0) = 0
>   actual_size = 50
>   proxy_data->pending_bytes = 0 + 50 = 50
> stream 2:
>   pending_bytes = min(0, 0) = 0
>   actual_size = 50
>   proxy_data->pending_bytes = 0 + 50 = 50
> 
> ITERATION TWO: 
> OnMoreData: buffers_state.pending_bytes = 30
> stream 1:
>   pending_bytes = min(50, 30) = 30
>   actual_size = 7
>   proxy_data->pending_bytes = 30 + 7 = 37
> stream 2:
>   pending_bytes = min(50, 30) = 30
>   actual_size = 50
>   proxy_data->pending_bytes = 30 + 50 = 80
> 
> ITERATION THREE: 
> OnMoreData: buffers_state.pending_bytes = 60
> stream 1:
>   pending_bytes = min(37, 60) = 30
>   actual_size = 0
>   proxy_data->pending_bytes = max(30 - 50, 0) = 0   ******
> stream 2:
>   pending_bytes = min(80, 60) = 60
>   actual_size = 50
>   proxy_data->pending_bytes = 60 + 50 = 110
> 
> This isn't correct. In iteration 2, stream 1 was mixed into stream 2. This
means
> there are 80 bytes at the device at the end of iteration 2. At the beginning
of
> iteration 3, 80 has gone down to 60, meaning 20 bytes were consumed at the
> device. That means stream 1 should have 17 bytes to go, not 0 (see ******
line).
> 
> Wouldn't it make more sense to do the computation using the difference between
> the previous iteration's |actual_total_size| and the
> buffers_state.pending_bytes?
> 
> For example:
> 
> End of ITERATION TWO:
> previous_bytes_sent_to_device =
>     actual_total_size + buffers_state.pending_bytes = 80
> 
> ITERATION THREE:
> bytes_consumed = previous_bytes_sent_to_device - buffers_state.pending_bytes =
> 20
> 
> if (actual_size == 0) {
>   pending_bytes -= bytes_consumed
>   etc
> }

We always feed entire buffer to the OS, so numbers would be

ITERATION ONE: 
OnMoreData: buffers_state.pending_bytes = 0
stream 1:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50
stream 2:
  pending_bytes = min(0, 0) = 0
  actual_size = 50
  proxy_data->pending_bytes = 0 + 50 = 50

ITERATION TWO: 
OnMoreData: buffers_state.pending_bytes = 50
stream 1:
  pending_bytes = min(50, 50) = 50
  actual_size = 7
  proxy_data->pending_bytes = 50 + 7 = 57
stream 2:
  pending_bytes = min(50, 50) = 50
  actual_size = 50
  proxy_data->pending_bytes = 50 + 50 = 100

ITERATION THREE: 
OnMoreData: buffers_state.pending_bytes = 100
stream 1:
  pending_bytes = min(57, 100) = 57
  actual_size = 0, and we hit special case in line 176.
  proxy_data->pending_bytes = max(57 - 50, 0) = 7
stream 2:
  pending_bytes = min(80, 60) = 60
  actual_size = 50
  proxy_data->pending_bytes = 60 + 50 = 110

In any case code is very tricky/fragile (see comment) and I'd definitely like to
improve it. We should either pass # of buffers to OnMoreData(), or split
OnMoreData() into 2 calls, adding ReleaseBuffer(). Either way we can keep track
of per-stream pending_bytes.

That requires changes to all (Mac, Linux, 2xWin) AudioOutputStreams, so I want
to make it separate CL, preferably combine it with getting rid of |stream|
OnMoreData() parameter.

I'd prefer to do it immediately after initial CL, maybe even before asking QA
and other groups to start testing the mixer.

Another possible improvement would be to keep physical stream opened for several
seconds after stopping playback, thus speeding up start of new stream. Not sure
how important is that, though, and it may be bad on laptop.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:174: // If stream ended, pending_bytes goes
down by max_size.
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> After a stream is ended, when does it get removed from |proxies_|?


StopStream() does that.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_mixer.h (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.h:69: typedef std::map<AudioOutputProxy*,
ProxyData> ProxyMap;
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> This is closer to what I was thinking, but now that I see the changes to
> AudioOutputProxy, I have a bit of a different suggestion:
> 
> Instead of referencing AudioOutputProxy at all, you should create a struct or
> class with the data that you need. I'm thinking something like
> 
> struct AudioSource {
>   int stream_id
>   AudioSourceCallback* audio_source_cb_;
>   int pending_bytes;
>   double volume;
> }
> 
> Then you can have a map of ids -> AudioSource.
> 
> The ID can come from the stream_id_ used in AudioDevice and AudioRendererHost.
> It will take some wiring through... again, I would recommend adding this to a
> design doc and getting consensus on it instead of coding it first!
> 
> The main benefit of this is AudioOutputProxy can continue to live on one
thread
> instead of living on multiple threads. The audio threading story is
complicated
> enough, and I think it's important to avoid adding complexity.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_proxy.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:66: base::AutoLock lock(lock_);
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> See suggestion in audio_output_mixer.h, and remove this lock.

Done.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_proxy.cc:94: uint32
AudioOutputProxy::OnMoreData(AudioOutputStream* stream,
On 2012/04/06 23:37:05, Victoria Kirst wrote:
> See suggestion in audio_output_mixer.h, and remove these methods. (Sorry, I
know
> I originally suggested these methods! But I didn't consider the threading
> scenario in this class.)

Done.

[vrk (LEFT CHROMIUM), 2012-04-18 14:57:36]

LGTM, great work, enal!!

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
File media/audio/audio_output_mixer.cc (right):

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:66: // We cannot start physical stream under
the lock,
On 2012/04/16 22:01:35, enal1 wrote:
> On 2012/04/06 23:37:05, Victoria Kirst wrote:
> > I don't understand this problem. Start() should not make a blocking call to
> > OnMoreData(), right?
> 
> Not right. Starting stream will call OnMoreData() to fill initial buffers on
the
> thread Start() is called. Exact number of buffers depends on the operating
> system and type of the stream (normal/low latency).

Ah, my mistake! I was only looking at the linux implementation; I see now that
the windows implementation does exactly what you say.

> > Isn't OnMoreData() called on a different thread, meaning
> > Start() would have to explicitly block for this deadlock to occur?
> > 
> > Can you update the audio_output_mixer.h with an explanation of the threading
> > story in this class?
> 
> There are very good descriptions for both interfaces it implements,
> AudioOutputDispatcher and AudioSourceCallback, in corresponding files. I
prefer
> not to duplicate the comments, it's hard to keep even single instance of
> comments in sync with code...

SGTM.

https://chromiumcodereview.appspot.com/9691001/diff/41001/media/audio/audio_o...
media/audio/audio_output_mixer.cc:158: // Current code "sorta works" if
everything works perfectly, but would have
On 2012/04/16 22:01:35, enal1 wrote:
> On 2012/04/06 23:37:05, Victoria Kirst wrote:
> > (sorry in advance for this huge comment, but I can't think of a better way
to
> > explain!)
> > 
> > I know you're saying it only "sorta works" but I think we can get a better
> > estimate. This is my understanding of how the current code would work:
> > 
> > stream 1: 57 bytes long
> > stream 2: 200 bytes long
> > max_size: 50 bytes
> > 
> > ITERATION ONE: 
> > OnMoreData: buffers_state.pending_bytes = 0
> > stream 1:
> >   pending_bytes = min(0, 0) = 0
> >   actual_size = 50
> >   proxy_data->pending_bytes = 0 + 50 = 50
> > stream 2:
> >   pending_bytes = min(0, 0) = 0
> >   actual_size = 50
> >   proxy_data->pending_bytes = 0 + 50 = 50
> > 
> > ITERATION TWO: 
> > OnMoreData: buffers_state.pending_bytes = 30
> > stream 1:
> >   pending_bytes = min(50, 30) = 30
> >   actual_size = 7
> >   proxy_data->pending_bytes = 30 + 7 = 37
> > stream 2:
> >   pending_bytes = min(50, 30) = 30
> >   actual_size = 50
> >   proxy_data->pending_bytes = 30 + 50 = 80
> > 
> > ITERATION THREE: 
> > OnMoreData: buffers_state.pending_bytes = 60
> > stream 1:
> >   pending_bytes = min(37, 60) = 30
> >   actual_size = 0
> >   proxy_data->pending_bytes = max(30 - 50, 0) = 0   ******
> > stream 2:
> >   pending_bytes = min(80, 60) = 60
> >   actual_size = 50
> >   proxy_data->pending_bytes = 60 + 50 = 110
> > 
> > This isn't correct. In iteration 2, stream 1 was mixed into stream 2. This
> means
> > there are 80 bytes at the device at the end of iteration 2. At the beginning
> of
> > iteration 3, 80 has gone down to 60, meaning 20 bytes were consumed at the
> > device. That means stream 1 should have 17 bytes to go, not 0 (see ******
> line).
> > 
> > Wouldn't it make more sense to do the computation using the difference
between
> > the previous iteration's |actual_total_size| and the
> > buffers_state.pending_bytes?
> > 
> > For example:
> > 
> > End of ITERATION TWO:
> > previous_bytes_sent_to_device =
> >     actual_total_size + buffers_state.pending_bytes = 80
> > 
> > ITERATION THREE:
> > bytes_consumed = previous_bytes_sent_to_device - buffers_state.pending_bytes
=
> > 20
> > 
> > if (actual_size == 0) {
> >   pending_bytes -= bytes_consumed
> >   etc
> > }
> 
> We always feed entire buffer to the OS, so numbers would be
> 
> ITERATION ONE: 
> OnMoreData: buffers_state.pending_bytes = 0
> stream 1:
>   pending_bytes = min(0, 0) = 0
>   actual_size = 50
>   proxy_data->pending_bytes = 0 + 50 = 50
> stream 2:
>   pending_bytes = min(0, 0) = 0
>   actual_size = 50
>   proxy_data->pending_bytes = 0 + 50 = 50
> 
> ITERATION TWO: 
> OnMoreData: buffers_state.pending_bytes = 50
> stream 1:
>   pending_bytes = min(50, 50) = 50
>   actual_size = 7
>   proxy_data->pending_bytes = 50 + 7 = 57
> stream 2:
>   pending_bytes = min(50, 50) = 50
>   actual_size = 50
>   proxy_data->pending_bytes = 50 + 50 = 100
> 
> ITERATION THREE: 
> OnMoreData: buffers_state.pending_bytes = 100
> stream 1:
>   pending_bytes = min(57, 100) = 57
>   actual_size = 0, and we hit special case in line 176.
>   proxy_data->pending_bytes = max(57 - 50, 0) = 7
> stream 2:
>   pending_bytes = min(80, 60) = 60
>   actual_size = 50
>   proxy_data->pending_bytes = 60 + 50 = 110
> 
> In any case code is very tricky/fragile (see comment) and I'd definitely like
to
> improve it. We should either pass # of buffers to OnMoreData(), or split
> OnMoreData() into 2 calls, adding ReleaseBuffer(). Either way we can keep
track
> of per-stream pending_bytes.
> 
> That requires changes to all (Mac, Linux, 2xWin) AudioOutputStreams, so I want
> to make it separate CL, preferably combine it with getting rid of |stream|
> OnMoreData() parameter.
> 
> I'd prefer to do it immediately after initial CL, maybe even before asking QA
> and other groups to start testing the mixer.
> 
> Another possible improvement would be to keep physical stream opened for
several
> seconds after stopping playback, thus speeding up start of new stream. Not
sure
> how important is that, though, and it may be bad on laptop.

Thanks for the explanation! Yeah, sounds like something that can be done in a
future CL.

[commit-bot: I haz the power, 2012-04-18 20:45:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/9691001/54005

[commit-bot: I haz the power, 2012-04-18 23:14:18]

Change committed as 132891

[commit-bot: I haz the power, 2012-04-19 15:51:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/enal@chromium.org/9691001/64001

[commit-bot: I haz the power, 2012-04-19 17:44:13]

Change committed as 133010