Remove the AudioManager singleton.

Remove the AudioManager singleton.

Unit tests now instantiate their own AudioManager and can choose to
use the default one or provide their own mock implementation without
having to worry about conflicting with the singleton.

The teardown sequence of the AudioManager and its thread has been cleaned
up significantly and I don't think it has been completely tested before as
the audio thread was terminated before all objects that belonged to the
thread had a chance to do cleanup.  The AudioManager unit tests do not use
the actual audio thread, so this part seems to have been left out.

In Chrome, the AudioManager instance is now owned by BrowserProcessImpl
and always constructed on the UI thread.  This instance is then shared
in the same way that several other 'manager' type objects are shared to
'content' code, via content::ResourceContext.  Audio specific classes
do though receive a direct pointer to the AudioManager and are required
to do proper reference counting if they need to hold onto the instance.
I chose to use the ResourceContext rather than direct use of g_browser_process
to avoid requiring another singleton when writing relatively simple tests
that touch the AudioManager.

I added a couple of safeguards to guard against future regressions:
- Not more than one instance of the AudioManager should be created.
- The AudioManager should not be addrefed by its own thread.  This
  can basically become a circular reference and prevent deterministic
  shutdown.

Reviewers:  Of course you're free to review everything,
but here's the breakdown in terms of the bare minimum from
the standpoint of "Owners approval".  I'm asking Henrik to be the
main reviewer of the entire patch (sorry!).

Henrik: Everything minus the below, but it would be great if you could
        take a look at the whole thing, specifically media/audio.

Pawel:  I'd like you to take a generic look at this approach.
        The key areas as far as the singleton itself goes are in
        media/audio/audio_manager[_base].* and
        chrome/browser/browser_process*.*

Satish: content/browser/speech/*
        media/audio/audio_manager_base.* (new reference counting code)

Andrew: content/browser/renderer_host/media/*
        content/renderer/media/webrtc_audio_device_unittest.cc (Owner)

Avi:    content/browser/renderer_host/render_process_host_impl.cc
        content/browser/resource_context.*

William: chrome/browser/profiles/profile_io_data.cc
	 chrome/browser/browser_process*.*

Robert:  This is basically a heads up. I hope that I didn't break the OpenBSD
         implementation, but unfortunately I have no way of knowing for sure.

Shijing: Please take a look at AudioManagerLinux.  I replaced the set of
         active streams with a simple counter.

BUG=105249
TEST=content_unittests, media_unittests, browser_tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114084

[Robert Nagy, 2011-12-06 14:25:45]

On 2011/12/06 14:20:38, tommi wrote:

Don't worry about the OpenBSD build, I will fix it if needed after this gets
commited.

[Avi (use Gerrit), 2011-12-06 15:22:59]

I'm happy with the content/browser/renderer_host/render_process_host_impl.cc
changes with the style issues noted below.

I don't know about content/browser/resource_context.* Talk to willchan.

http://codereview.chromium.org/8818012/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8818012/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_process_host_impl.cc:478:
channel_->AddFilter(new AudioInputRendererHost(
unwrap this and subsequent lines where wrapping is no longer needed

[tommi (sloooow) - chröme, 2011-12-06 15:48:20]

Thanks Avi - sorry, the 'content/browser/resource_context.*' bit was indeed
meant for willchan.

http://codereview.chromium.org/8818012/diff/1/content/browser/renderer_host/r...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8818012/diff/1/content/browser/renderer_host/r...
content/browser/renderer_host/render_process_host_impl.cc:478:
channel_->AddFilter(new AudioInputRendererHost(
On 2011/12/06 15:22:59, Avi wrote:
> unwrap this and subsequent lines where wrapping is no longer needed

Done.

[Avi (use Gerrit), 2011-12-06 16:09:08]

NP.

Once you have everyone's OK, I'm ready to rubberstamp as needed for content.

[henrika (OOO until Aug 14), 2011-12-06 16:53:28]

Impressive stuff Tommi. It will take some time to digest this CL but I have one
initial question (see comment).

http://codereview.chromium.org/8818012/diff/1/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/8818012/diff/1/media/audio/audio_manager.cc#ne...
media/audio/audio_manager.cc:11: // Used only to make sure we never create more
than one instance.
I have read the comment but still don't understand why this global handle is
needed? What type of situation are you protecting us from here?

[willchan no longer on Chromium, 2011-12-06 17:02:12]

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
content/browser/resource_context.cc:184: // The AudioManager isn't necessarily
initialized with other
Can you explain this comment? When is AudioManager initialized then? And why
isn't there a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))?

[tommi (sloooow) - chröme, 2011-12-06 20:28:48]

http://codereview.chromium.org/8818012/diff/1/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/8818012/diff/1/media/audio/audio_manager.cc#ne...
media/audio/audio_manager.cc:11: // Used only to make sure we never create more
than one instance.
On 2011/12/06 16:53:28, henrika1 wrote:
> I have read the comment but still don't understand why this global handle is
> needed? What type of situation are you protecting us from here?

This is a safeguard to make sure that there exists never more than one instance
of the AudioManager.  This variable is not exposed or used for anything other
than to check if there already exists an instance and catch the programmer error
early.
The checks are in the constructor and destructor.  The check in
AudioManager::Create isn't really necessary, but I put it there in case anyone
would ever return an "AudioManager" that does not derive from the AudioManager
class.

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
content/browser/resource_context.cc:184: // The AudioManager isn't necessarily
initialized with other
On 2011/12/06 17:02:12, willchan wrote:
> Can you explain this comment? When is AudioManager initialized then? And why
> isn't there a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))?

Sorry, this comment isn't correct. I must have forgotten to update it since an
early version of the cl.  I'll fix it.

When Chrome runs, the audio manager instance is created and owned by the
BrowserProcessImpl class.  This always happens on the IO thread and (as you no
doubt know) there is a check inside EnsureInitialized() for that.
What's different here though, and the reason why there isn't a dcheck here like
with the other accessors, is that the audio_manager accessor can be called on
other threads than the IO thread.  I suspect that the reason for this is
historical since the AudioManager was a singleton with an undefined threading
model and anyone could call the instance method at any time and on any thread.

[Paweł Hajdan Jr., 2011-12-07 09:18:10]

I took a brief look, seems fine. I don't know most of the code in this CL so
I've removed myself from reviewers.

Thank you for working on this!

[henrika (OOO until Aug 14), 2011-12-07 10:08:34]

Focused on the manager as such and how it is used in the media/audio unit tests.

Must say that it looks great! It is so much nicer without the singleton.
Readability and code design is highly improved.

Will check the more complex tests in content renderer next and also take a
"AudioManager usage in Chrome" round.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h#...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
nit, indent two more?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h#...
media/audio/audio_manager.h:39: // TODO(cpu): Define threading requirements for
interacting with AudioManager.
Still a valid comment?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:25: #ifndef NDEBUG
Please add some comments here to explain the purpose of your checks. Thanks.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:40: CHECK(audio_thread_.Start());
Any point in checking Initialized() here to ensure that the thread is running?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:77:
base::AtomicRefCountInc(&num_active_input_streams_);
Nice!

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:100: // Stop() will wait for any posted
messages to be processed first.
Can you add some more information here please. You post a task on the audio
thread and then closes the same thread directly after. Even if your design
works; is it a good design? To me it feels a bit advanced. Can't it be
simplified or can you use any alternative method? If not, why?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
media/audio/audio_output_controller.h:247: // Used to post delayed tasks to
ourselves that we can cancel.
Can you add some more details on why you had to introduce this pattern. It is
not obvious why it is required. Perhaps add some extended comments in this area
in the actual header?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
media/audio/audio_output_controller_unittest.cc:104:
AudioOutputController::Create(audio_manager.get(), &event_handler, params,
So much nicer to inject the manager here. All magic is now gone ;-) Thanks!

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
Not sure why this is needed.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.h:101: // Used to post delayed tasks to
ourselves that we can cancel.
Nit, empty line above comment.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_pro...
File media/audio/audio_output_proxy.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_pro...
media/audio/audio_output_proxy.cc:92: // Delete the object now like is done in
the Close() implementation of
Have to trust you on this part. Don't know the details well enough to comment
;-)

[henrika (OOO until Aug 14), 2011-12-07 10:26:43]

Checked WebRTC unit test and files in browser/render_host as well.

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_renderer_host.cc:249:
resource_context_->audio_manager(),
Do we need the full resource context here or can we inject only the manager
instead?

http://codereview.chromium.org/8818012/diff/7002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:352: 2000);
You will get a comment from Andrew on this one ;-)

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
File content/test/webrtc_audio_device_test.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
content/test/webrtc_audio_device_test.cc:131: // Kick of the cleanup process by
closing channel. This queues up
the

[henrika (OOO until Aug 14), 2011-12-07 10:27:55]

should be: content/browser/render_host/media

[tommi (sloooow) - chröme, 2011-12-07 12:26:43]

All comments addressed and uploaded.

Henrik - in addition to your comments, you sent a reply which is missing
context: "should be: content/browser/render_host/media".
Can you point out where I need to fix?

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_renderer_host.cc:249:
resource_context_->audio_manager(),
On 2011/12/07 10:26:43, henrika1 wrote:
> Do we need the full resource context here or can we inject only the manager
> instead?

Only the manager is being injected... am I misunderstanding?

http://codereview.chromium.org/8818012/diff/7002/content/renderer/media/webrt...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/renderer/media/webrt...
content/renderer/media/webrtc_audio_device_unittest.cc:352: 2000);
On 2011/12/07 10:26:43, henrika1 wrote:
> You will get a comment from Andrew on this one ;-)

Ah, yes.  So, I might as well preemptively answer :)

Previously, the timeout was by default 10 seconds.  This timeout is an 'action
timeout' and it was me that originally put it there.  However, I don't think
that applying that timeout to an audio stream of a fixed length, and not an
action, is the right use of that timer.  We should rather use a fixed timeout.
So, the question becomes, how long should the timeout be?
I picked 2 seconds as random.  Perhaps I should rather pick 500ms as is done in
the WinAudioTest tests, which also play audio (see e.g.
PCMWaveStreamPlay200HzTone44Kss)?

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
File content/test/webrtc_audio_device_test.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
content/test/webrtc_audio_device_test.cc:131: // Kick of the cleanup process by
closing channel. This queues up
On 2011/12/07 10:26:43, henrika1 wrote:
> the

Can you clarify?

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h#...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
On 2011/12/07 10:08:34, henrika1 wrote:
> nit, indent two more?

Done.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager.h#...
media/audio/audio_manager.h:39: // TODO(cpu): Define threading requirements for
interacting with AudioManager.
On 2011/12/07 10:08:34, henrika1 wrote:
> Still a valid comment?
Nope. Removed.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
File media/audio/audio_manager_base.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:25: #ifndef NDEBUG
On 2011/12/07 10:08:34, henrika1 wrote:
> Please add some comments here to explain the purpose of your checks. Thanks.
There are comments in the header.  Please let me know if they're not clear
enough.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:40: CHECK(audio_thread_.Start());
On 2011/12/07 10:08:34, henrika1 wrote:
> Any point in checking Initialized() here to ensure that the thread is running?
audio_thread_.Start() returns true iff the thread is running.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:77:
base::AtomicRefCountInc(&num_active_input_streams_);
On 2011/12/07 10:08:34, henrika1 wrote:
> Nice!
Thanks! :)

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_manager_ba...
media/audio/audio_manager_base.cc:100: // Stop() will wait for any posted
messages to be processed first.
On 2011/12/07 10:08:34, henrika1 wrote:
> Can you add some more information here please. You post a task on the audio
> thread and then closes the same thread directly after. Even if your design
> works; is it a good design? To me it feels a bit advanced. Can't it be
> simplified or can you use any alternative method? If not, why?

Well, this is how the Thread class works, fine or not :) You'll see this pattern
all over chrome where the thread class is used - including inside the
implementation of Thread::Stop().  As the comment explains, Stop() will not
return until all pending messages have been processed, including the one we just
posted.

Regarding an alternative method, I'm not sure if you mean 1) an alternative
method of waiting for ShutdownOnAudioThread to complete or if 2) we can somehow
differently uninitialize the dispatchers. So, I'll answer both.

1) There are other ways such as by using a WaitableEvent, but this is a simpler
way that doesn't require any additional synchronization objects to the thread
itself. Also, it's really just two lines of code (well, two semicolons), so it's
hard to beat with regards to simplicity. ;)

2) The dispatchers are single threaded objects that perform most of their work
on the audio thread (and correctly have a lot of DCHECKs to make sure it stays
that way).  As such, they really must be torn down from that same thread to keep
things robust.  Additionally, the map that holds onto the dispatchers does not
have any lock to guard against multithreaded access, so it too must only be
accessed from the audio thread, so the

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_con...
media/audio/audio_output_controller.h:247: // Used to post delayed tasks to
ourselves that we can cancel.
On 2011/12/07 10:08:34, henrika1 wrote:
> Can you add some more details on why you had to introduce this pattern. It is
> not obvious why it is required. Perhaps add some extended comments in this
area
> in the actual header?

Improved comment.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
On 2011/12/07 10:08:34, henrika1 wrote:
> Not sure why this is needed.

It allows |this| to be used in an initializer list without a warning when you
know it is safe to do so.  For more details:
http://msdn.microsoft.com/en-us/library/3c594ae3(VS.80).aspx

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.h (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.h:101: // Used to post delayed tasks to
ourselves that we can cancel.
On 2011/12/07 10:08:34, henrika1 wrote:
> Nit, empty line above comment.

Done.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_pro...
File media/audio/audio_output_proxy.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_pro...
media/audio/audio_output_proxy.cc:92: // Delete the object now like is done in
the Close() implementation of
On 2011/12/07 10:08:34, henrika1 wrote:
> Have to trust you on this part. Don't know the details well enough to comment
> ;-)

The Mac, Wave and WASAPI implementations delete themselves by calling
manager_->ReleaseOutputStream(this) and most (or even all?) input stream
implementations use some form of "self_deleter" (see e.g. the Alsa
implementation).

So, the post condition for Close() is that the caller must not touch the object
again. This is also true in the implementation as it was, but instead of
deleting the object straight away, it delayed it by posting a task to the
message queue.  Doing so is however not necessary and has two downsides: It
leaves the door open to flakiness, it can delay shutdown since the message queue
has to keep running in order to delete the task.

[henrika (OOO until Aug 14), 2011-12-07 12:48:00]

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
File content/browser/renderer_host/media/audio_input_renderer_host.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/renderer_hos...
content/browser/renderer_host/media/audio_input_renderer_host.cc:249:
resource_context_->audio_manager(),
Sorry, I had this question in my head from another file. Please ignore.

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
File content/test/webrtc_audio_device_test.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
content/test/webrtc_audio_device_test.cc:131: // Kick of the cleanup process by
closing channel. This queues up
"..by closing *the* channel."

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
I meant the weak pointer.

[no longer working on chromium, 2011-12-07 12:52:18]

Very impressive! Great work, Tommi.
LGTM to the AudioManagerLinux.

BR,
/SX

[tommi (sloooow) - chröme, 2011-12-07 14:46:23]

Latest changes uploaded.

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
File content/test/webrtc_audio_device_test.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/test/webrtc_audio_de...
content/test/webrtc_audio_device_test.cc:131: // Kick of the cleanup process by
closing channel. This queues up
On 2011/12/07 12:48:01, henrika1 wrote:
> "..by closing *the* channel."
ah. sorry, refrigerator blindness. Done.

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
File media/audio/audio_output_dispatcher.cc (right):

http://codereview.chromium.org/8818012/diff/7002/media/audio/audio_output_dis...
media/audio/audio_output_dispatcher.cc:22:
ALLOW_THIS_IN_INITIALIZER_LIST(weak_this_(this)),
On 2011/12/07 12:48:01, henrika1 wrote:
> I meant the weak pointer.

Ah.  Here's the comment from the header file:

  // Used to post delayed tasks to ourselves that we can cancel.
  base::WeakPtrFactory<AudioOutputDispatcher> weak_this_;

This is important as if we do not do this, the message queue can hold onto the
the object and cause the stream objects (and held resources) to be not be
deleted at all if MessageLoop::DeletePendingTasks is called and/or cause the
object to be deleted at a point in time that's not guaranteed to always be the
same, which doesn't have any benefits but is harder to test and debug.

As a side note, the AudioOutputDispatcher class doesn't really need to be
reference counted. Things would be simpler if it wasn't.  Perhaps I can do that
refactoring job on a later cl.

[willchan no longer on Chromium, 2011-12-07 15:29:14]

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
File content/browser/resource_context.cc (right):

http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
content/browser/resource_context.cc:184: // The AudioManager isn't necessarily
initialized with other
On 2011/12/06 20:28:48, tommi wrote:
> On 2011/12/06 17:02:12, willchan wrote:
> > Can you explain this comment? When is AudioManager initialized then? And why
> > isn't there a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))?
> 
> Sorry, this comment isn't correct. I must have forgotten to update it since an
> early version of the cl.  I'll fix it.
> 
> When Chrome runs, the audio manager instance is created and owned by the
> BrowserProcessImpl class.  This always happens on the IO thread and (as you no
> doubt know) there is a check inside EnsureInitialized() for that.
> What's different here though, and the reason why there isn't a dcheck here
like
> with the other accessors, is that the audio_manager accessor can be called on
> other threads than the IO thread.  I suspect that the reason for this is
> historical since the AudioManager was a singleton with an undefined threading
> model and anyone could call the instance method at any time and on any thread.

I see. While it's not strictly a problem, it's different from the existing
threading model and we should be careful here. ResourceContext is created and
owned by ProfileIOData, which will be deleted on the IO thread and thus
ResourceContext will get deleted on the IO thread. We need to think carefully
about the shutdown case here. Note the shutdown order indicated by
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/public/browse....
If ResourceContext is used on any thread which outlasts the IO thread, then we
have a problem, because ResourceContext may be deleted, and we'll get a use
after free.

This is the reason we have always just forced the accessors to be non-threadsafe
and only accessible on the IO thread.

[tommi (sloooow) - chröme, 2011-12-07 15:38:09]

On 2011/12/07 15:29:14, willchan wrote:
>
http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
> File content/browser/resource_context.cc (right):
> 
>
http://codereview.chromium.org/8818012/diff/7002/content/browser/resource_con...
> content/browser/resource_context.cc:184: // The AudioManager isn't necessarily
> initialized with other
> On 2011/12/06 20:28:48, tommi wrote:
> > On 2011/12/06 17:02:12, willchan wrote:
> > > Can you explain this comment? When is AudioManager initialized then? And
why
> > > isn't there a DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO))?
> > 
> > Sorry, this comment isn't correct. I must have forgotten to update it since
an
> > early version of the cl.  I'll fix it.
> > 
> > When Chrome runs, the audio manager instance is created and owned by the
> > BrowserProcessImpl class.  This always happens on the IO thread and (as you
no
> > doubt know) there is a check inside EnsureInitialized() for that.
> > What's different here though, and the reason why there isn't a dcheck here
> like
> > with the other accessors, is that the audio_manager accessor can be called
on
> > other threads than the IO thread.  I suspect that the reason for this is
> > historical since the AudioManager was a singleton with an undefined
threading
> > model and anyone could call the instance method at any time and on any
thread.
> 
> I see. While it's not strictly a problem, it's different from the existing
> threading model and we should be careful here. ResourceContext is created and
> owned by ProfileIOData, which will be deleted on the IO thread and thus
> ResourceContext will get deleted on the IO thread. We need to think carefully
> about the shutdown case here. Note the shutdown order indicated by
>
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/content/public/browse....
> If ResourceContext is used on any thread which outlasts the IO thread, then we
> have a problem, because ResourceContext may be deleted, and we'll get a use
> after free.
> 
> This is the reason we have always just forced the accessors to be
non-threadsafe
> and only accessible on the IO thread.

Agreed, perhaps this is a slippery slope.  I'm going to go back and take a look
at those places where audio_manager() is accessed from the FILE or UI thread. 
If it's only from unit tests, then I think that might be easy to fix and this
accessor can abide the same rules as all the others.  I'll report back.

[tommi (sloooow) - chröme, 2011-12-08 16:52:38]

OK, I made the audio_manager() accessor more strict and it was actually "only"
the speech code that I had to tweak.

Satish - This means one feature change that might or might not be important. 
Can you take a look at the latest change in speech_input_bubble_gtk.cc?
Basically, there used to be a check for CanShowAudioInputSettings before adding
a link to the UI, but now the link is always added.  I don't see a similar check
for other platforms, so maybe it's OK and arguably more consistent to simply not
have that check at all.  Your call though :)

[henrika (OOO until Aug 14), 2011-12-09 08:57:02]

LGTM.

[Satish, 2011-12-09 11:46:18]

LGTM. Thanks for doing this Tommi. One reply below, feel free to address it as
you see fit either here or in a subsequent CL.

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_bubble_gtk.cc (right):

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_bubble_gtk.cc:141: // TODO(tommi): The
audio_manager property can only be accessed from the
We do this check only for linux because the code to bring up audio settings
panel relies on the applications available only in gnome or kde3/kde4
(media/audio/linux/audio_manager_inux.cc). We could either do something fancy
like show the link here but start off an IO thread operation to check and hide
the link if we can't show the settings UI.. or we can always show this and in
the speech input code where we invoke the settings UI show an error dialog if
settings UI can't be shown when link was clicked.

[tommi (sloooow) - chröme, 2011-12-09 13:24:07]

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/speech/speec...
File chrome/browser/speech/speech_input_bubble_gtk.cc (right):

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/speech/speec...
chrome/browser/speech/speech_input_bubble_gtk.cc:141: // TODO(tommi): The
audio_manager property can only be accessed from the
On 2011/12/09 11:46:19, Satish wrote:
> We do this check only for linux because the code to bring up audio settings
> panel relies on the applications available only in gnome or kde3/kde4
> (media/audio/linux/audio_manager_inux.cc). We could either do something fancy
> like show the link here but start off an IO thread operation to check and hide
> the link if we can't show the settings UI.. or we can always show this and in
> the speech input code where we invoke the settings UI show an error dialog if
> settings UI can't be shown when link was clicked.

OK, thanks for the background.
I'll take care of it in a followup cl.

[scherkus (not reviewing), 2011-12-09 22:47:30]

awesome!

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:721: if (!audio_manager_.get())
rm .get()

http://codereview.chromium.org/8818012/diff/19001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8818012/diff/19001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:11: #include
<objbase.h>  // For CoInitialize/CoUninitialize.
any reason why this got moved up?

http://codereview.chromium.org/8818012/diff/19001/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/8818012/diff/19001/content/browser/resource_co...
content/browser/resource_context.h:107: AudioManager* audio_manager_;
no ref?

http://codereview.chromium.org/8818012/diff/19001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:352: 2000);
was action_timeout_ms not enough or too much?

would tiny_timeout_ms_ work? (100ms)

perhaps action_timeout_ms() divided by some #?

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:8: #include <string>
nit: blank lines between <system includes.h> and "regular chrome includes.h"

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
we really shouldn't require refcounting but let's save that for another day

there's some detangling of object lifetimes and cleanup of stream
creation/deletion that has to happen first before we can tackling that beast

mind filing a bug + referencing it in this comment?

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:133: static AudioManager* CreateAudioManager();
could we move this to the .cc or do implementors require access to private
methods inside AudioManager?

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager_b...
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:54: void Shutdown();
docs?

also I looked through the patch but couldn't find any callers of this method so
OOC when we would we manually call Shutdown() as opposed to deleting the object
via refcounts?

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
media/audio/audio_output_controller.h:259: // shutdown  and force it to wait for
the most delayed task.
remove extra space

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
media/audio/audio_output_controller_unittest.cc:104:
AudioOutputController::Create(audio_manager.get(), &event_handler, params,
nit: .get() not needed for refptr

here + below

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:97: return *manager_.get();
nit: get not required

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_input.h
File media/audio/linux/alsa_input.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_inpu...
media/audio/linux/alsa_input.h:59: AudioManagerLinux* audio_manager_;
document that this is not reffed?

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_outp...
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_outp...
media/audio/linux/alsa_output_unittest.cc:113: mock_manager_.get(),
.get() here and below

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:85: ++active_output_stream_count_;
we should probably re-think this active stream count business at some point

care to TODOify/file a bug for this stuff?

things I'm curious about:
  a) Is it even useful / used?
  b) If so, are all OSes implementing it?
  c) Can we converge on a scheme (i.e., input streams call
Increase/DecreaseActiveInputStreamCount() while output streams increment on
creation then call ReleaseOutputStream() later on)
  d) Can we make it less error prone (i.e, can the manager handle all of this
transparently, perhaps by requiring Stop() to be called on the manager instead
of the stream itself)

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:118:
AudioManagerLinux::AudioManagerLinux() : active_output_stream_count_(0U) {
nit: collapse into {}

http://codereview.chromium.org/8818012/diff/19001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac_unittest.cc:90: ~MacAudioInputTest()
{}
virtual

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
File media/audio/win/audio_low_latency_input_win_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win_unittest.cc:92: if (NULL ==
audio_man)
if (!audio_man)

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win_unittest.cc:176: if
(!CanRunAudioTests(audio_manager.get()))
.get() etc

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_outpu...
File media/audio/win/audio_output_win_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_outpu...
media/audio/win/audio_output_win_unittest.cc:561: EXPECT_TRUE(opened =
oas->Open());
ASSERT_TRUE(oas->Open())?

would do away w/ opened bool

[scherkus (not reviewing), 2011-12-09 22:48:40]

also global nit on using .get() for scoped_refptr<>

we have operator T*() defined -- might as well use it :)

[tommi (sloooow) - chröme, 2011-12-10 00:11:13]

Thanks Andrew. Changes uploaded.

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/8818012/diff/19001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.cc:721: if (!audio_manager_.get())
On 2011/12/09 22:47:30, scherkus wrote:
> rm .get()

Done.

http://codereview.chromium.org/8818012/diff/19001/content/browser/renderer_ho...
File content/browser/renderer_host/render_process_host_impl.cc (right):

http://codereview.chromium.org/8818012/diff/19001/content/browser/renderer_ho...
content/browser/renderer_host/render_process_host_impl.cc:11: #include
<objbase.h>  // For CoInitialize/CoUninitialize.
On 2011/12/09 22:47:30, scherkus wrote:
> any reason why this got moved up?

lint. (os header)
Maybe we should be using ScopedCOMInitializer here instead.

http://codereview.chromium.org/8818012/diff/19001/content/browser/resource_co...
File content/browser/resource_context.h (right):

http://codereview.chromium.org/8818012/diff/19001/content/browser/resource_co...
content/browser/resource_context.h:107: AudioManager* audio_manager_;
On 2011/12/09 22:47:30, scherkus wrote:
> no ref?

I went with the flow on this one.  There are already refcounted objects here
without a ref, but it's OK for Chrome's implementation, but users have to be
aware.
One benefit (well, could actually be a con, see below) of the current approach
is that you don't have to #include the full class definition in the cc file and
this isn't done for these pointers.  Could it also have something to do with
avoiding breaking include rules?  (if that's the case though, then arguably this
could be a loophole)

http://codereview.chromium.org/8818012/diff/19001/content/renderer/media/webr...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/content/renderer/media/webr...
content/renderer/media/webrtc_audio_device_unittest.cc:352: 2000);
On 2011/12/09 22:47:30, scherkus wrote:
> was action_timeout_ms not enough or too much?
> 
> would tiny_timeout_ms_ work? (100ms)
> 
> perhaps action_timeout_ms() divided by some #?

action_timeout is 10seconds, which is too long when running all the tests
routinely.  I wanted to go with a fixed value since this is an audio file and
it's long enough for you to hear a word or two.  A second or even less would do
fine too.  I initially used action_timeout_ms() here since tiny was too short
and this was the next timeout value in the line.  However, since what we're
doing here isn't really an "action" I figured having a constant would make more
sense.
I can change it to e.g. action_timeout_ms() / 10 or 5 if you want.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:8: #include <string>
On 2011/12/09 22:47:30, scherkus wrote:
> nit: blank lines between <system includes.h> and "regular chrome includes.h"

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
On 2011/12/09 22:47:30, scherkus wrote:
> we really shouldn't require refcounting but let's save that for another day
> 
> there's some detangling of object lifetimes and cleanup of stream
> creation/deletion that has to happen first before we can tackling that beast
> 
> mind filing a bug + referencing it in this comment?

Will do. crbug seems to be down at the moment so I'll file one tomorrow.

I started out without reference counting (hence all the audio_manager.get()'s
that were still in there) but as the cl grew, I saw that lifetime, thread
management, shutdown etc were undefined and using scoped_ptr<> + non-ref
pointers, all over the place would be risky.  However, the threading model,
usage and teardown is a lot more defined now, so switching from refcounted
shouldn't be too hard moving forward.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:133: static AudioManager* CreateAudioManager();
On 2011/12/09 22:47:30, scherkus wrote:
> could we move this to the .cc or do implementors require access to private
> methods inside AudioManager?

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager_b...
File media/audio/audio_manager_base.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager_b...
media/audio/audio_manager_base.h:54: void Shutdown();
On 2011/12/09 22:47:30, scherkus wrote:
> docs?

Added.

> 
> also I looked through the patch but couldn't find any callers of this method
so

AudioManagerLinux calls it although after my changes there, there is less need
to do so.

> OOC when we would we manually call Shutdown() as opposed to deleting the
object
> via refcounts?

Only in cases where we want to check that we're not leaking something after
shutdown or e.g. from tests that verify that the map is empty, thread is not
running etc.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
File media/audio/audio_output_controller.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
media/audio/audio_output_controller.h:259: // shutdown  and force it to wait for
the most delayed task.
On 2011/12/09 22:47:30, scherkus wrote:
> remove extra space

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
File media/audio/audio_output_controller_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_co...
media/audio/audio_output_controller_unittest.cc:104:
AudioOutputController::Create(audio_manager.get(), &event_handler, params,
On 2011/12/09 22:47:30, scherkus wrote:
> nit: .get() not needed for refptr
> 
> here + below

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_pr...
File media/audio/audio_output_proxy_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_output_pr...
media/audio/audio_output_proxy_unittest.cc:97: return *manager_.get();
On 2011/12/09 22:47:30, scherkus wrote:
> nit: get not required

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_input.h
File media/audio/linux/alsa_input.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_inpu...
media/audio/linux/alsa_input.h:59: AudioManagerLinux* audio_manager_;
On 2011/12/09 22:47:30, scherkus wrote:
> document that this is not reffed?

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_outp...
File media/audio/linux/alsa_output_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/alsa_outp...
media/audio/linux/alsa_output_unittest.cc:113: mock_manager_.get(),
On 2011/12/09 22:47:30, scherkus wrote:
> .get() here and below

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:85: ++active_output_stream_count_;
On 2011/12/09 22:47:30, scherkus wrote:
> we should probably re-think this active stream count business at some point
> 
> care to TODOify/file a bug for this stuff?
> 
> things I'm curious about:
>   a) Is it even useful / used?

It's checked against kMaxOutputStreams above.  Other than that, it looks like a
debugging aid.

>   b) If so, are all OSes implementing it?

This is the only place I've seen it.

>   c) Can we converge on a scheme (i.e., input streams call
> Increase/DecreaseActiveInputStreamCount() while output streams increment on
> creation then call ReleaseOutputStream() later on)

I think that's a good idea, provided that it is useful :)  I'll ask leandro and
satish about usefulness.

>   d) Can we make it less error prone (i.e, can the manager handle all of this
> transparently, perhaps by requiring Stop() to be called on the manager instead
> of the stream itself)

+1 on that.  I'd like to do that on a separate cl if that's alright and with
input from the other guys on usefulness etc.

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:118:
AudioManagerLinux::AudioManagerLinux() : active_output_stream_count_(0U) {
On 2011/12/09 22:47:30, scherkus wrote:
> nit: collapse into {}

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/mac/audio_low_l...
File media/audio/mac/audio_low_latency_input_mac_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/mac/audio_low_l...
media/audio/mac/audio_low_latency_input_mac_unittest.cc:90: ~MacAudioInputTest()
{}
On 2011/12/09 22:47:30, scherkus wrote:
> virtual

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
File media/audio/win/audio_low_latency_input_win_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win_unittest.cc:92: if (NULL ==
audio_man)
On 2011/12/09 22:47:30, scherkus wrote:
> if (!audio_man)

Removed check.  AudioManager:Create is always called and is basically just a
call to new().

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_low_l...
media/audio/win/audio_low_latency_input_win_unittest.cc:176: if
(!CanRunAudioTests(audio_manager.get()))
On 2011/12/09 22:47:30, scherkus wrote:
> .get() etc

Done.

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_outpu...
File media/audio/win/audio_output_win_unittest.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/win/audio_outpu...
media/audio/win/audio_output_win_unittest.cc:561: EXPECT_TRUE(opened =
oas->Open());
On 2011/12/09 22:47:30, scherkus wrote:
> ASSERT_TRUE(oas->Open())?
> 
> would do away w/ opened bool

We still need to call Close(), but I've fixed this already on another cl, so
I'll revert this bit.

[scherkus (not reviewing), 2011-12-10 00:16:51]

LGTM w/ a nit about a blank line that I think snuck in by mistake

thanks for tackling this!

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
On 2011/12/10 00:11:14, tommi wrote:
> On 2011/12/09 22:47:30, scherkus wrote:
> > we really shouldn't require refcounting but let's save that for another day
> > 
> > there's some detangling of object lifetimes and cleanup of stream
> > creation/deletion that has to happen first before we can tackling that beast
> > 
> > mind filing a bug + referencing it in this comment?
> 
> Will do. crbug seems to be down at the moment so I'll file one tomorrow.
> 
> I started out without reference counting (hence all the audio_manager.get()'s
> that were still in there) but as the cl grew, I saw that lifetime, thread
> management, shutdown etc were undefined and using scoped_ptr<> + non-ref
> pointers, all over the place would be risky.  However, the threading model,
> usage and teardown is a lot more defined now, so switching from refcounted
> shouldn't be too hard moving forward.

agree 100%

http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.c...
media/audio/audio_manager.cc:1: 
!!!

[scherkus (not reviewing), 2011-12-10 00:17:51]

final note: in case major stuff has shifted around it'd probably be a good idea
to make sure the CL description still matches the end result of the patch :)

[willchan no longer on Chromium, 2011-12-10 00:27:33]

ResourceContext is fine by me

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/browser_proc...
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/browser_proc...
chrome/browser/browser_process_impl.h:290: scoped_refptr<AudioManager>
audio_manager_;
Why is this refcounted? And if this is used on the IO thread, perhaps you should
make sure the destruction order is such that it gets destroyed before the IO
thread gets joined.

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:500: new
media_stream::MediaStreamManager(g_browser_process->audio_manager()));
Can you make audio_manager get passed in on the UI thread? There's actually a
small race since g_browser_process is a UI thread object and we're accessing it
on the IO thread. If the process shutdown immediately, it's actually
theoretically possible for this to be a use after free.

Also, I like to push use of globals as close to main() as possible, and pass
them into code.

[tommi (sloooow) - chröme, 2011-12-10 09:59:57]

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
On 2011/12/10 00:16:52, scherkus wrote:
> On 2011/12/10 00:11:14, tommi wrote:
> > On 2011/12/09 22:47:30, scherkus wrote:
> > > we really shouldn't require refcounting but let's save that for another
day
> > > 
> > > there's some detangling of object lifetimes and cleanup of stream
> > > creation/deletion that has to happen first before we can tackling that
beast
> > > 
> > > mind filing a bug + referencing it in this comment?
> > 
> > Will do. crbug seems to be down at the moment so I'll file one tomorrow.
> > 
> > I started out without reference counting (hence all the
audio_manager.get()'s
> > that were still in there) but as the cl grew, I saw that lifetime, thread
> > management, shutdown etc were undefined and using scoped_ptr<> + non-ref
> > pointers, all over the place would be risky.  However, the threading model,
> > usage and teardown is a lot more defined now, so switching from refcounted
> > shouldn't be too hard moving forward.
> 
> agree 100%

bug filed and comment added: crbug.com/107087

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
File media/audio/linux/audio_manager_linux.cc (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_man...
media/audio/linux/audio_manager_linux.cc:85: ++active_output_stream_count_;
bug filed: crbug.com/107088

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:500: new
media_stream::MediaStreamManager(g_browser_process->audio_manager()));
On 2011/12/10 00:27:33, willchan wrote:
> Can you make audio_manager get passed in on the UI thread? There's actually a
> small race since g_browser_process is a UI thread object and we're accessing
it
> on the IO thread. If the process shutdown immediately, it's actually
> theoretically possible for this to be a use after free.

Just so that I understand - do you mean that g_browser_process might possibly be
invalid or that the audio manager might be used after BrowserProcessImpl
releases its reference?

What I'll do then is to fetch the audio_manager inside InitializeOnUIThread and
pass it to here via profile_params_.  Sounds reasonable?

> 
> Also, I like to push use of globals as close to main() as possible, and pass
> them into code.

Is there perhaps a better way to share an object across more than one
ResourceContext instances than to keep it with BrowserProcessImpl?  If there is,
I can move the audio manager's home when I make the change to not have the audio
manager ref counted.

http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.cc
File media/audio/audio_manager.cc (right):

http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.c...
media/audio/audio_manager.cc:1: 
On 2011/12/10 00:16:52, scherkus wrote:
> !!!

Done. thanks!!!

[tommi (sloooow) - chröme, 2011-12-12 11:03:13]

Heads up - I need to rebase today as there have been several changes in
these files.
I'll force gcl to upload all files in case some of them haven't changed so
that you can review the diff after the rebase if further changes are needed.

On Sat, Dec 10, 2011 at 10:59 AM, <tommi@chromium.org> wrote:

>
> http://codereview.chromium.**org/8818012/diff/19001/media/**
>
audio/audio_manager.h<http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h>
> File media/audio/audio_manager.h (right):
>
> http://codereview.chromium.**org/8818012/diff/19001/media/**
>
audio/audio_manager.h#**newcode24<http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h#newcode24>
> media/audio/audio_manager.h:**24: : public
> base::RefCountedThreadSafe<**AudioManager> {
> On 2011/12/10 00:16:52, scherkus wrote:
>
>> On 2011/12/10 00:11:14, tommi wrote:
>> > On 2011/12/09 22:47:30, scherkus wrote:
>> > > we really shouldn't require refcounting but let's save that for
>>
> another day
>
>> > >
>> > > there's some detangling of object lifetimes and cleanup of stream
>> > > creation/deletion that has to happen first before we can tackling
>>
> that beast
>
>> > >
>> > > mind filing a bug + referencing it in this comment?
>> >
>> > Will do. crbug seems to be down at the moment so I'll file one
>>
> tomorrow.
>
>> >
>> > I started out without reference counting (hence all the
>>
> audio_manager.get()'s
>
>> > that were still in there) but as the cl grew, I saw that lifetime,
>>
> thread
>
>> > management, shutdown etc were undefined and using scoped_ptr<> +
>>
> non-ref
>
>> > pointers, all over the place would be risky.  However, the threading
>>
> model,
>
>> > usage and teardown is a lot more defined now, so switching from
>>
> refcounted
>
>> > shouldn't be too hard moving forward.
>>
>
>  agree 100%
>>
>
> bug filed and comment added: crbug.com/107087
>
>
> http://codereview.chromium.**org/8818012/diff/19001/media/**
>
audio/linux/audio_manager_**linux.cc<http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_manager_linux.cc>
> File media/audio/linux/audio_**manager_linux.cc (right):
>
> http://codereview.chromium.**org/8818012/diff/19001/media/**
>
audio/linux/audio_manager_**linux.cc#newcode85<http://codereview.chromium.org/8818012/diff/19001/media/audio/linux/audio_manager_linux.cc#newcode85>
> media/audio/linux/audio_**manager_linux.cc:85:
> ++active_output_stream_count_;
> bug filed: crbug.com/107088
>
>
> http://codereview.chromium.**org/8818012/diff/29001/chrome/**
>
browser/profiles/profile_io_**data.cc<http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/profile_io_data.cc>
> File chrome/browser/profiles/**profile_io_data.cc (right):
>
> http://codereview.chromium.**org/8818012/diff/29001/chrome/**
>
browser/profiles/profile_io_**data.cc#newcode500<http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/profile_io_data.cc#newcode500>
> chrome/browser/profiles/**profile_io_data.cc:500: new
> media_stream::**MediaStreamManager(g_browser_**process->audio_manager()));
> On 2011/12/10 00:27:33, willchan wrote:
>
>> Can you make audio_manager get passed in on the UI thread? There's
>>
> actually a
>
>> small race since g_browser_process is a UI thread object and we're
>>
> accessing it
>
>> on the IO thread. If the process shutdown immediately, it's actually
>> theoretically possible for this to be a use after free.
>>
>
> Just so that I understand - do you mean that g_browser_process might
> possibly be invalid or that the audio manager might be used after
> BrowserProcessImpl releases its reference?
>
> What I'll do then is to fetch the audio_manager inside
> InitializeOnUIThread and pass it to here via profile_params_.  Sounds
> reasonable?
>
>
>
>  Also, I like to push use of globals as close to main() as possible,
>>
> and pass
>
>> them into code.
>>
>
> Is there perhaps a better way to share an object across more than one
> ResourceContext instances than to keep it with BrowserProcessImpl?  If
> there is, I can move the audio manager's home when I make the change to
> not have the audio manager ref counted.
>
>
> http://codereview.chromium.**org/8818012/diff/29001/media/**
>
audio/audio_manager.cc<http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.cc>
> File media/audio/audio_manager.cc (right):
>
> http://codereview.chromium.**org/8818012/diff/29001/media/**
>
audio/audio_manager.cc#**newcode1<http://codereview.chromium.org/8818012/diff/29001/media/audio/audio_manager.cc#newcode1>
> media/audio/audio_manager.cc:**1:
> On 2011/12/10 00:16:52, scherkus wrote:
>
>> !!!
>>
>
> Done. thanks!!!
>
>
http://codereview.chromium.**org/8818012/<http://codereview.chromium.org/8818...
>

[tommi (sloooow) - chröme, 2011-12-12 16:07:48]

On 2011/12/10 00:27:33, willchan wrote:
> ResourceContext is fine by me
> 
>
http://codereview.chromium.org/8818012/diff/29001/chrome/browser/browser_proc...
> File chrome/browser/browser_process_impl.h (right):
> 
>
http://codereview.chromium.org/8818012/diff/29001/chrome/browser/browser_proc...
> chrome/browser/browser_process_impl.h:290: scoped_refptr<AudioManager>
> audio_manager_;
> Why is this refcounted? And if this is used on the IO thread, perhaps you
should
> make sure the destruction order is such that it gets destroyed before the IO
> thread gets joined.
> 
>
http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
> File chrome/browser/profiles/profile_io_data.cc (right):
> 
>
http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
> chrome/browser/profiles/profile_io_data.cc:500: new
> media_stream::MediaStreamManager(g_browser_process->audio_manager()));
> Can you make audio_manager get passed in on the UI thread? There's actually a
> small race since g_browser_process is a UI thread object and we're accessing
it
> on the IO thread. If the process shutdown immediately, it's actually
> theoretically possible for this to be a use after free.
> 
> Also, I like to push use of globals as close to main() as possible, and pass
> them into code.

Hey Will - I've updated ProfileIOData to access the audio_manager() only on the
UI thread.  See the changes in diff '8' (should be only 3 files).

[willchan no longer on Chromium, 2011-12-12 16:44:04]

LGTM!

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h
File media/audio/audio_manager.h (right):

http://codereview.chromium.org/8818012/diff/19001/media/audio/audio_manager.h...
media/audio/audio_manager.h:24: : public
base::RefCountedThreadSafe<AudioManager> {
On 2011/12/10 09:59:57, tommi wrote:
> On 2011/12/10 00:16:52, scherkus wrote:
> > On 2011/12/10 00:11:14, tommi wrote:
> > > On 2011/12/09 22:47:30, scherkus wrote:
> > > > we really shouldn't require refcounting but let's save that for another
> day
> > > > 
> > > > there's some detangling of object lifetimes and cleanup of stream
> > > > creation/deletion that has to happen first before we can tackling that
> beast
> > > > 
> > > > mind filing a bug + referencing it in this comment?
> > > 
> > > Will do. crbug seems to be down at the moment so I'll file one tomorrow.
> > > 
> > > I started out without reference counting (hence all the
> audio_manager.get()'s
> > > that were still in there) but as the cl grew, I saw that lifetime, thread
> > > management, shutdown etc were undefined and using scoped_ptr<> + non-ref
> > > pointers, all over the place would be risky.  However, the threading
model,
> > > usage and teardown is a lot more defined now, so switching from refcounted
> > > shouldn't be too hard moving forward.
> > 
> > agree 100%
> 
> bug filed and comment added: crbug.com/107087

This plan sounds great to me! It looks like as long as the BrowserProcessImpl
destroys it after the other threads are destroyed, then we shouldn't have any
dangling pointers.

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_io_data.cc (right):

http://codereview.chromium.org/8818012/diff/29001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_io_data.cc:500: new
media_stream::MediaStreamManager(g_browser_process->audio_manager()));
On 2011/12/10 09:59:57, tommi wrote:
> On 2011/12/10 00:27:33, willchan wrote:
> > Can you make audio_manager get passed in on the UI thread? There's actually
a
> > small race since g_browser_process is a UI thread object and we're accessing
> it
> > on the IO thread. If the process shutdown immediately, it's actually
> > theoretically possible for this to be a use after free.
> 
> Just so that I understand - do you mean that g_browser_process might possibly
be
> invalid or that the audio manager might be used after BrowserProcessImpl
> releases its reference?
> 
> What I'll do then is to fetch the audio_manager inside InitializeOnUIThread
and
> pass it to here via profile_params_.  Sounds reasonable?

Sounds great and your change looks great to me.

> 
> > 
> > Also, I like to push use of globals as close to main() as possible, and pass
> > them into code.
> 
> Is there perhaps a better way to share an object across more than one
> ResourceContext instances than to keep it with BrowserProcessImpl?  If there
is,
> I can move the audio manager's home when I make the change to not have the
audio
> manager ref counted.

[Avi (use Gerrit), 2011-12-12 17:28:34]

owner rubberstamp lgtm

[commit-bot: I haz the power, 2011-12-12 18:24:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/8818012/38094

[commit-bot: I haz the power, 2011-12-12 18:25:10]

Can't process patch for file media/audio/audio_output_proxy.h.
File's status is None, patchset upload is incomplete.