Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11413078:
Tab Audio Capture: Browser-side connect/disconnect functionality. (Closed)
|
Created:
8 years, 1 month ago by miu Modified:
7 years, 11 months ago Reviewers:
ncarter (slow), tommi (sloooow) - chröme, Alpha Left Google, Cris Neckar, Chris Rogers, DaleCurtis, darin (slow to review) CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, feature-media-reviews_chromium.org, justinlin Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTab Audio Mirroring/Capture: Browser-side connect/disconnect functionality:
1. Added new AudioMirroringManager to dynamically match/route "divertable" audio streams with mirroring destinations.
2. Modified AudioOutputController to provide "divert audio data" functionality.
3. Modified AudioRendererHost to notify AudioMirroringManager of all audio streams.
The intention is, in a later change, to introduce a "WebContentsAudioInputStream" which will implement the AudioMirroringManager::MirroringDestination interface introduced in this change. WCAIS will represent the lifetime of a tab audio mirroring session, calling AudioMirroringManager::Start/StopMirroring() as appropriate.
Testing:
1. Rewrote most of unit testing for AudioOutputController, addressing bug 112500. Also added testing for the new Divert functionality.
2. Added extensive unit testing for the new Start/StopMirroring functionality in AudioMirroringManager.
3. Minor testing clean-ups/additions elsewhere.
BUG=153392, 112500
TEST=Run media_unittests and content_unittests.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=176295
Patch Set 1 #
Total comments: 25
Patch Set 2 : Fixed WCAIS threading issue (methods called on audio thread). Addressed hclam's review comments. #
Total comments: 22
Patch Set 3 : Cleaned up threading and ref-counting issues, per Alpha's comments. Finished 'divert' impl in Audi… #Patch Set 4 : Minor tweaks and added some useful comments. #
Total comments: 12
Patch Set 5 : Minor tweaks to threading, per Alpha's comments. #Patch Set 6 : Rewrote tests for AOC (and bug 112500). Add tests for Start/StopMirroring in ARH. #Patch Set 7 : Remove WCAudioInputStream changes (split into another change). #
Total comments: 34
Patch Set 8 : Address Dale's comments. Non-blocking pointer swapping in AOC. #Patch Set 9 : Add done closure to MirroringDestination::RemoveInput(). #
Total comments: 10
Patch Set 10 : Moved Glue to cc file, and renamed to SourceSharingCallback. #
Total comments: 2
Patch Set 11 : Simplify! #
Total comments: 20
Patch Set 12 : Broke mirroring out of AudioRendererHost into separate AudioMirroringManager class. #
Total comments: 18
Patch Set 13 : Addressed tommi's comments (dependency injection for testing AudioMirroringManager). #
Total comments: 20
Patch Set 14 : Move Diverter interface into audio_io.h. Replace use of singleton with BrowserMainLoop. #
Total comments: 8
Patch Set 15 : AudioSourceDiverter now in its own file. Other tweaks, per Dale's comments. #
Total comments: 4
Patch Set 16 : media_observer_ can be NULL, sez every trybot. #Patch Set 17 : Addressed Darin's comments. #Patch Set 18 : Added guard against reentrancy in AudioMirroringManager methods. #
Total comments: 8
Patch Set 19 : Fix interface destructor link issue for win_aura. #Patch Set 20 : Comment tweaks, per crogers@. #Patch Set 21 : Disable mirroring on ios platform, since much of content is not compiled in. #Messages
Total messages: 43 (0 generated)
I don't see how WCAIS is created, can you elaborate on that? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:47: scoped_ptr<media::DivertedAudioOutputStream> diverted_stream; Should this be owned by AudioOutputController instead? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:64: base::Lock lock; I don't think a lock is necessary, rather ASSERT for threads. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:87: GlobalLookup& lookup = g_lookup.Get(); Assert for threads. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:108: DeregisterAudioRendererHost(render_process_id_); How does each |destination| know when to call StopMirroring() when this is being destroyed. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:275: StopMirroring(render_view_id, session_it->second); Should this just be an error? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:394: MirrorSessionMap::const_iterator prev_session_it = Comments to explain what this is doing. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:405: MirrorSessionMap::const_iterator next_session_it = Would be helpful to explain what this is supposed to do. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.h:92: void StopMirroring(int render_view_id, Does a destination associate with multiple render view? If not can this just take |destination|? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:49: class WebContentsAudioInputStream::AudioRendererHostTracker I suggest we split this class out to a separate file. This really feels like can be shared between video and audio. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:59: base::AutoLock guard(lock_); There's no need to use a lock if you post tasks back to IO thread. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:115: callback_.Run(render_process_id, render_view_id); WeakPtr doesn't work cross-threads. It has a DCHECK to prevent that. Post a task to IO thread to do this? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:128: base::Lock lock_; No need to use lock. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:192: target_host_->StopMirroring(target_render_view_id_, this); Does this need |target_render_view_id_|? It seems |this| is enough. https://codereview.chromium.org/11413078/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.h:169: DivertedAudioOutputStream* Divert(); I think this class should be DAOS.
Hey Alpha, Thanks for the review comments. I believe I've addressed all of them. Some issues may be easier to talk about in-person tomorrow. Note: Justin pointed out a threading issue: WCAIS's methods are being invoked on the AudioManager thread, not the IO thread as I originally assumed. Thankfully, a small set of changes fixed that problem, but I had to make WCAIS ref-counted. Before reviewing further, I want to thoroughly review and understand Justin's work (https://codereview.chromium.org/11298006/). We're going to land his change first, so I want to make sure I've stubbed things out in the right way in my change here. I'll ping you again once I'm ready to continue the review process on this change. Thanks, Yuri https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:64: base::Lock lock; On 2012/11/20 21:49:43, Alpha wrote: > I don't think a lock is necessary, rather ASSERT for threads. Unfortunately, the lock will be needed since an entry is added/removed from the map via the UI BrowserThread (whenever a RenderProcessHost is created/destroyed), but the map is read via the IO thread. Do you know of a better way to look-up the AudioRendererHost instance by render_process_id? https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:108: DeregisterAudioRendererHost(render_process_id_); On 2012/11/20 21:49:43, Alpha wrote: > How does each |destination| know when to call StopMirroring() when this is being > destroyed. > AudioRendererHost is ref-counted. Each |destination| holds a ref to AudioRendererHost until StopMirroring() is called, so it's guaranteed not to go away. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:275: StopMirroring(render_view_id, session_it->second); On 2012/11/20 21:49:43, Alpha wrote: > Should this just be an error? Justin and I talked about this. He believe the extension API should enforce the "one mirroring of a tab at a time" constraint. I've put this here as a precaution for now and it really doesn't hurt anything to leave it in (an extra protection). https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:394: MirrorSessionMap::const_iterator prev_session_it = On 2012/11/20 21:49:43, Alpha wrote: > Comments to explain what this is doing. Done. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.cc:405: MirrorSessionMap::const_iterator next_session_it = On 2012/11/20 21:49:43, Alpha wrote: > Would be helpful to explain what this is supposed to do. Done. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/audio_renderer_host.h:92: void StopMirroring(int render_view_id, On 2012/11/20 21:49:43, Alpha wrote: > Does a destination associate with multiple render view? If not can this just > take |destination|? I thought about this, and I don't think I needed the 2nd argument. However, after resolving the WCAIS threading issue, I do need to pass destination in a scoped_refptr to make sure a reference is held on |destination| until the IO thread removes all the streams from it. (This all might be easier to discuss in-person tomorrow.) https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:49: class WebContentsAudioInputStream::AudioRendererHostTracker On 2012/11/20 21:49:43, Alpha wrote: > I suggest we split this class out to a separate file. This really feels like can > be shared between video and audio. Absolutely agree! :-) We think alike. I had already noticed this and asked Justin to create such a file (see: https://codereview.chromium.org/11298006/). I've moved this code over to there. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:59: base::AutoLock guard(lock_); On 2012/11/20 21:49:43, Alpha wrote: > There's no need to use a lock if you post tasks back to IO thread. The lock is needed for two reasons: 1. callback_.operator=(), callback_.Reset(), and callback_.Run() are not thread-safe with respect to each other. 2. The locking ensures any outstanding tasks (posted to the UI or IO threads) don't invoke the callback once the Stop() method returns. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:115: callback_.Run(render_process_id, render_view_id); On 2012/11/20 21:49:43, Alpha wrote: > WeakPtr doesn't work cross-threads. It has a DCHECK to prevent that. Post a task > to IO thread to do this? callback_ is a base::Callback<...>, not a weak pointer. Also, note that I'm using the media::BindToLoop() utility to bounce the callback to another thread. https://codereview.chromium.org/11413078/diff/1/content/browser/renderer_host... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:192: target_host_->StopMirroring(target_render_view_id_, this); On 2012/11/20 21:49:43, Alpha wrote: > Does this need |target_render_view_id_|? It seems |this| is enough. Answered in one of my comment responses in audio_renderer_host.h. We can discuss further tomorrow to clarify things. https://codereview.chromium.org/11413078/diff/1/media/audio/audio_output_cont... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/1/media/audio/audio_output_cont... media/audio/audio_output_controller.h:169: DivertedAudioOutputStream* Divert(); On 2012/11/20 21:49:43, Alpha wrote: > I think this class should be DAOS. Yep. The more I think about it, the more we should have a Revert() method to un-divert the stream. I'll make that change.
https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:71: GlobalLookup& lookup = g_lookup.Get(); Make sure RegisterARG and DeregisterARH are called IO thread and you don't need this lock. See below how to do this. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:102: RegisterAudioRendererHost(render_process_id_, this); Call register in OnChannelConnected() which is called on IO thread. See the interface of MessageFilter. ARH is only useful when it is attached to a IPC channel. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:108: DeregisterAudioRendererHost(render_process_id_); Do it in OnChannelClosing(). https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:114: // Since the IPC channel is gone, close all requested audio streams. You can call StopMirroring() here, this method is always called on the IO thread. This is called when a IPC channel is gone and there will be no more IPC activity. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.h:90: void StartMirroring( What if StartMirroring and StopMirroring are static methods and access the global map? This way WCAIS doesn't need to own AudioRendererHost which is scary. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:70: target_host_->StartMirroring(target_render_view_id_, this); Does it work if WCAIS doesn't own or reference ARH? What if this becomes AudioRendererHost::StartMirroring(target_render_view_id, this)? https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:82: target_host_->StopMirroring(target_render_view_id_, this); What if this becomes AudioRendererHost::StopMirroring(target_render_view_id)? https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:148: base::AutoLock guard(streams_lock_); Is a lock necessary here? I see these two methods only called on IO thread. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_audio_input_stream.h (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.h:80: scoped_refptr<AudioRendererHost> target_host_; Owning AudioRendererHost scares me.. AudioRendererHost should only be owned by the IPC channel. This adds uncertainty as to when ARH is destroyed. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:53: { I still think this can be done without a lock: Start(), Stop() and callback is accessed only on the IO thread. During Stop(), callback is reset() on IO thread. When Lookup is completed it post a task back to IO thread, it never touches callback on UI thread. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:59: base::Bind(&RenderViewTracker::LookUpAndObserveWebContents, this, Use BindToLoop here such that LookUpAndObserveWebContents is called on IO thread. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:86: callback_.Run(render_process_id, render_view_id); If you access |callback| only on IO thread you don't need this lock. In fact because of BindToLoop() OnTargetChanged is called on IO thread anyway.
Addressed all your comments. Things are much cleaner now. Updated CL description to define the scope of this change, and what will be left for future CLs. PTAL. Thanks, Yuri https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:71: GlobalLookup& lookup = g_lookup.Get(); On 2012/11/26 22:59:59, Alpha wrote: > Make sure RegisterARG and DeregisterARH are called IO thread and you don't need > this lock. See below how to do this. Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:102: RegisterAudioRendererHost(render_process_id_, this); On 2012/11/26 22:59:59, Alpha wrote: > Call register in OnChannelConnected() which is called on IO thread. See the > interface of MessageFilter. ARH is only useful when it is attached to a IPC > channel. Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:108: DeregisterAudioRendererHost(render_process_id_); On 2012/11/26 22:59:59, Alpha wrote: > Do it in OnChannelClosing(). Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.cc:114: // Since the IPC channel is gone, close all requested audio streams. On 2012/11/26 22:59:59, Alpha wrote: > You can call StopMirroring() here, this method is always called on the IO > thread. This is called when a IPC channel is gone and there will be no more IPC > activity. Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/audio_renderer_host.h:90: void StartMirroring( On 2012/11/26 22:59:59, Alpha wrote: > What if StartMirroring and StopMirroring are static methods and access the > global map? This way WCAIS doesn't need to own AudioRendererHost which is scary. Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:70: target_host_->StartMirroring(target_render_view_id_, this); On 2012/11/26 22:59:59, Alpha wrote: > Does it work if WCAIS doesn't own or reference ARH? > > What if this becomes AudioRendererHost::StartMirroring(target_render_view_id, > this)? Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:82: target_host_->StopMirroring(target_render_view_id_, this); On 2012/11/26 22:59:59, Alpha wrote: > What if this becomes AudioRendererHost::StopMirroring(target_render_view_id)? Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:148: base::AutoLock guard(streams_lock_); On 2012/11/26 22:59:59, Alpha wrote: > Is a lock necessary here? I see these two methods only called on IO thread. Removed the locking for now. I agree that it'll probably turn out not to be necessary when I replace the stubs in later CLs. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_audio_input_stream.h (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_audio_input_stream.h:80: scoped_refptr<AudioRendererHost> target_host_; On 2012/11/26 22:59:59, Alpha wrote: > Owning AudioRendererHost scares me.. AudioRendererHost should only be owned by > the IPC channel. This adds uncertainty as to when ARH is destroyed. Done. https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11413078/diff/7001/content/browser/renderer_h... content/browser/renderer_host/media/web_contents_capture_util.cc:53: { On 2012/11/26 22:59:59, Alpha wrote: > I still think this can be done without a lock: > > Start(), Stop() and callback is accessed only on the IO thread. During Stop(), > callback is reset() on IO thread. When Lookup is completed it post a task back > to IO thread, it never touches callback on UI thread. Actually, that's not guaranteed. The reason is because I intend to re-use this class for WebContentsVideoCaptureDevice (common functionality). WCVCD will not be able to instantiate the RenderViewTracker from the IO thread. In addition, it will want to be called back on the UI thread (for simplicity). This is what motivated the use of the lock_ around the callback_. Also, I'm making it the caller's responsibility to use media::BindToLoop() to make sure the "change events" are posted back to the correct thread.
Some questions and comments on threading and ownership. I haven't looked at the implementation of divert() yet, will look after lunch. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:260: int render_process_id, int render_view_id, This is called on the IO thread, why the extra thread jumping? https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:93: // references are held to both the AudioRendererHost and Can we not hold reference to AudioRendererHost? https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:96: static void StartMirroring( It seems to me these two methods are called on the IO thread, so if you don't do thread jumping you don't need to hold the reference right? https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:115: typedef std::map<int, scoped_refptr<AudioRendererHost> > ActiveHostMap; Don't use scoped_refptr, just a * is enough. I suggest you assert that when AudioRendererHost it is not attached to ActiveHostMap. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:113: AudioRendererHost::StopMirroring( This is on IO thread right? https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:42: { I would like to keep things simple for this patch. This looks like is for the video device. I think we should make that change later as there might be other solution when we visit the problem. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:53: { As suggested I think we don't need to use lock for this change, let's not worry about video in this CL.
Addressed your comments. PTAL. Thanks, Yuri https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:260: int render_process_id, int render_view_id, On 2012/11/28 20:02:25, Alpha wrote: > This is called on the IO thread, why the extra thread jumping? It's called from the audio thread. To make this clearer, I just changed code in WCAIS to explicitly grab a ref to the audio thread's message loop from AudioManager so that the DCHECKs are making it clear where code is running on the audio thread. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:93: // references are held to both the AudioRendererHost and On 2012/11/28 20:02:25, Alpha wrote: > Can we not hold reference to AudioRendererHost? Done. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:115: typedef std::map<int, scoped_refptr<AudioRendererHost> > ActiveHostMap; On 2012/11/28 20:02:25, Alpha wrote: > Don't use scoped_refptr, just a * is enough. I suggest you assert that when > AudioRendererHost it is not attached to ActiveHostMap. Done. Not sure what you meant about the asserts. Are you talking about checking that AudioRendererHost::FromRenderProcessID() doesn't ever return NULL when called from StartMirroring() or StopMirroring()? If so, I don't think asserts would be appropriate there. Any races during shutdown are actually completely clean: The absence of an AudioRendererHost in the ActiveHostMap indicates that there are no active mirroring sessions on it, which is a reasonabl state to be in. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_audio_input_stream.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/web_contents_audio_input_stream.cc:113: AudioRendererHost::StopMirroring( On 2012/11/28 20:02:25, Alpha wrote: > This is on IO thread right? Nope. Audio thread. See my other comments for explanation. https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... File content/browser/renderer_host/media/web_contents_capture_util.cc (right): https://codereview.chromium.org/11413078/diff/11007/content/browser/renderer_... content/browser/renderer_host/media/web_contents_capture_util.cc:53: { On 2012/11/28 20:02:25, Alpha wrote: > As suggested I think we don't need to use lock for this change, let's not worry > about video in this CL. Done.
lgtm. Dale should have a close look at AOC.
Dale/Chris, Would you PTAL also? Thanks, Yuri
I don't understand this change in relation to Justin's. He's already attaching virtual streams via the existing NotifyDeviceChange() interface. Why is this necessary as well? Also, this patch adds a lot of code and no unit tests. Please add some for your new classes.
On 2012/11/28 23:55:48, DaleCurtis wrote: > I don't understand this change in relation to Justin's. He's already attaching > virtual streams via the existing NotifyDeviceChange() interface. Why is this > necessary as well? Discussed offline: Justin is working to provide browser-wide mirroring (by M25). My goal is to provide tab-level mirroring. What you see in this change is the logic around dynamically connecting and disconnecting audio output streams to a WebContents-level (essentially a tab, pop-up, or other app window) mirroring session. This builds upon the other recent change I had you look at (https://codereview.chromium.org/11359196/), which plumbed-in the association between render_view_id and each audio stream. There's one piece of the puzzle left, to be introduced in a later change: Integration. I'll be taking the reusable components from Justin's work and massaging/refactoring them as needed to pump the audio outputs from one tab through converters/mixers to produce a single AudioInputStream that represents the mirroring of one tab's audio. > Also, this patch adds a lot of code and no unit tests. Please add some for your > new classes. Agreed. Will do.
Hi all, I've addressed Alpha's and Dale's comments, and now believe this is change is ready for your official review. BTW--As you can see from the most recent patchset, I've split this change so that the focus is on AudioRendererHost and AudioOutputController and their extensive unit test additions. For your reference, my next change after this would be https://codereview.chromium.org/11416350, which is not yet ready for review. Thanks, Yuri
Nice tests! Looks mostly good. The one global concern I have is that you're making your code surface too large by trying to "if check" every possible usage instead of using CHECK() or DCHECK() to ensure callers are doing the right thing. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:258: return it == host_map.end() ? NULL : it->second; Should this just CHECK() fail on missing entry? https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:279: if (!host) { CHECK() instead? https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:305: // Divert any existing audio outputs to |destination|. If streams were This seems strange. Are you saying callers can change the diverter target w/o calling StopMirroring() ? Do you expect a use case for this? If not it seems simpler to force callers to always call StopMirroring() before the next StartMirroring(). https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:342: if (!host) { Same CHECK() comment. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:348: if (session_it == host->mirror_sessions_.end() || Ditto. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:73: : public base::RefCountedThreadSafe<MirroringDestination> { Do we really need this to be ref counted? https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:97: virtual void OnChannelConnected(int32 peer_pid) OVERRIDE; What is a peer_id? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.cc (left): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:369: // We should always have a stream by this point. Is this not true anymore? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:140: audio_manager_->AddOutputDeviceChangeListener(this); I don't see where you're removing the device change listener? Won't this add a new listener everytime? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:214: if (stream_) stream_ should always be valid here. why not check() instead. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:396: DCHECK(!message_loop_->BelongsToCurrentThread()); Is this helpful? It doesn't really matter what thread it's called from right? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:408: blocker.Wait(); Hmmm, seems risky. You're blocking the IO thread with this. Would a callback based solution be better? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:419: DCHECK_EQ(this, asc); Again seems odd. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.h:165: AudioOutputStream::AudioSourceCallback* Divert(); Hmm, this is odd since AudioOutputController is generally the target callback. Why not have Divert(AudioSourceCallback* callback) and Revert() instead? Why make the caller manage the original callback? https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller_unittest.cc:84: if (ShouldSkipTest()) { Shouldn't be necessary anymore since we have FakeAudioOutputStream.
Dale, I've explained, in comment responses below, why so much of the "if checking" was necessary in AudioRendererHost. Also, after our hallway conversation the other day, I came up with a cleaner scheme for switching audio data consumers in AudioOutputController. Again, comments (and code) explain further. PTAL. Thanks, Yuri https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:258: return it == host_map.end() ? NULL : it->second; On 2012/12/05 23:35:14, DaleCurtis wrote: > Should this just CHECK() fail on missing entry? Short answer: No, because it's possible for look-ups to fail. Chrome doesn't synchronize across render processes (that I'm aware of). Long answer: I added comment in the code to explain this part of the plumbing and why it's normal for look-ups here to fail. Please see that for the details. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:279: if (!host) { On 2012/12/05 23:35:14, DaleCurtis wrote: > CHECK() instead? Explained above. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:305: // Divert any existing audio outputs to |destination|. If streams were On 2012/12/05 23:35:14, DaleCurtis wrote: > This seems strange. Are you saying callers can change the diverter target w/o > calling StopMirroring() ? Do you expect a use case for this? If not it seems > simpler to force callers to always call StopMirroring() before the next > StartMirroring(). Yes. Two ways to answer this: 1. Architecture: Let's say two different web pages (or extensions) call into the tab capture JS API to start a mirroring session. They want to mirror the same tab, yet are not aware of each other's existence. So, there's no way for a second entity to ask the first to stop mirroring, wait for it to do so, and then go ahead. Even if there was, it's not clear we would want to burden web developers with these details. 2. User intention: Mirroring is a user-initiated action. Therefore, we interpret the later action as the pertinent one. In other words, it's the user's intention to start mirroring to a new destination rather than maintain the old mirroring session (which they may have forgotten about). https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:342: if (!host) { On 2012/12/05 23:35:14, DaleCurtis wrote: > Same CHECK() comment. Explained above. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:348: if (session_it == host->mirror_sessions_.end() || On 2012/12/05 23:35:14, DaleCurtis wrote: > Ditto. This "if" check is necessary because StartMirroring() may switch mirroring over to a new destination. In that case, we want the new destination to gain control over stopping mirroring. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:73: : public base::RefCountedThreadSafe<MirroringDestination> { On 2012/12/05 23:35:14, DaleCurtis wrote: > Do we really need this to be ref counted? Yes. AudioRendererHost operates mainly on the IO thread while MirroringDestination (to implemented by WebContentsAudioInputStream) will operate mainly on the audio thread. Without the ref-counting to guarantee the object lifetime of MirroringDestination, there are several places where we'd have to block one thread, waiting for a task to run on the other. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:97: virtual void OnChannelConnected(int32 peer_pid) OVERRIDE; On 2012/12/05 23:35:14, DaleCurtis wrote: > What is a peer_id? The process ID (operating system PID?) of the process at the other end of the IPC channel. It doesn't actually matter for our purposes here. We're just overriding this method to get notified when the IPC channel is up-and-running. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.cc (left): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:369: // We should always have a stream by this point. On 2012/12/05 23:35:14, DaleCurtis wrote: > Is this not true anymore? Done. (Reverted back to original behavior.) https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:140: audio_manager_->AddOutputDeviceChangeListener(this); On 2012/12/05 23:35:14, DaleCurtis wrote: > I don't see where you're removing the device change listener? Won't this add a > new listener everytime? The call to DoStopCloseAndClearStream() above called RemoveOutputDeviceChangeListener(). I've clarified the comment on the line above. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:214: if (stream_) On 2012/12/05 23:35:14, DaleCurtis wrote: > stream_ should always be valid here. why not check() instead. I had changed the semantics so that stream_ is NULL whenever audio data is not destined for the AudioManager-owned AudioOutputStream. Having simplified my approach, I reverted to the original semantics: stream_ is always non-NULL and until closed. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:396: DCHECK(!message_loop_->BelongsToCurrentThread()); On 2012/12/05 23:35:14, DaleCurtis wrote: > Is this helpful? It doesn't really matter what thread it's called from right? Done. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:408: blocker.Wait(); On 2012/12/05 23:35:14, DaleCurtis wrote: > Hmmm, seems risky. You're blocking the IO thread with this. Would a callback > based solution be better? I changed my approach, and came up with something cleaner that doesn't block any threads: 1. AudioOutputController no longer extends AudioSourceCallback. Created a "Glue" helper class that implements AudioSourceCallback instead. 2. When diverting/reverting audio, Glue::PassSoon() will ensure a non-blocking "pointer swap." Hopefully the code is clear enough that you can easily see how this is accomplished. 3. For simplicity, I don't mess with stream_ at all to do a Divert/Revert. stream_ simply gets zeros while mirroring is active (i.e., muted audio). Look how simple AOC::Divert() and AOC::Revert() look now! :) https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.cc:419: DCHECK_EQ(this, asc); On 2012/12/05 23:35:14, DaleCurtis wrote: > Again seems odd. Done. https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller.h:165: AudioOutputStream::AudioSourceCallback* Divert(); On 2012/12/05 23:35:14, DaleCurtis wrote: > Why not have Divert(AudioSourceCallback* callback) and Revert() instead? It's a pull model, not a push model (the AudioSourceCallback interface). In other words, some outside entity needs AudioOutputController to provide an interface whereby it can invoke OnMoreData() to pull audio data out. > Hmm, this is odd since AudioOutputController is generally the target callback. I took a look, and I think this is actually a weirdness in the existing code organization: AudioOutputController implements the AudioSourceCallback interface. However, you might ask, "What objects call those methods?" As it turns out, only one object: AOC::stream_, which is private/internal to AOC. Therefore, I removed ASC from AOC's inheritance hierarchy, and added a private helper class (AOC::Glue) that implements ASC. Doing this also solved another major problem: AOC::Divert() is virtually non-blocking across these timing-sensitive threads. Two birds with one stone! :-) https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... File media/audio/audio_output_controller_unittest.cc (right): https://codereview.chromium.org/11413078/diff/11008/media/audio/audio_output_... media/audio/audio_output_controller_unittest.cc:84: if (ShouldSkipTest()) { On 2012/12/05 23:35:14, DaleCurtis wrote: > Shouldn't be necessary anymore since we have FakeAudioOutputStream. Done.
https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.cc:306: void AudioOutputController::Glue::PassSoon(const scoped_refptr<Glue>& to) { This is all really complicated and I don't quite follow why we need it. You're still using OnDeviceChange() right? Why not just pass the diverter callback ptr in with stream start, then when the streams are disconnected pass in the AudioSourceCallback. If that's not an option, you could just have a task runner that is only pumped on OnMoreData() callback until idle. Divert/Revert would post messages to the task runner to switch the stream. https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:180: // An AudioSourceCallback that allows passing around the underlying SyncReader Move to CC file? https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:185: class Glue A better name would be nice. https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:187: public base::RefCountedThreadSafe<Glue> { Why does this need refcounting?
Dale, Good design questions/concerns, which I've tried to explain further (below). If you're still not convinced, we should make sure and meet tomorrow in-person. Changes: Moved code to CC file, renamed helper class. https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.cc:306: void AudioOutputController::Glue::PassSoon(const scoped_refptr<Glue>& to) { On 2012/12/12 01:29:15, DaleCurtis wrote: > I don't quite follow why we need it. Main reason: Per our hallway conversation, "Though shalt not block audio threads." The hardware audio thread may take a significant amount of time doing the read/write from SyncReader, so it's not acceptable to use a simpler locking scheme that could potentially block the audio manager thread. Other reasons: Please see my comments/responses from yesterday. > You're still using OnDeviceChange() right? No. That's one reason I'm advocating this new approach: We don't need to complexify AudioOutputController. In patch set 7, we were closing the stream but pretending it's still open. Remember how stream_ could be NULL while in the kPlayback state? In fact, with this new approach, the rest of AOC can be implemented in *total ignorance* of the divert/revert mechanism! How much simpler can it get? :) > ... you could just have a task runner that is only pumped > on OnMoreData() callback until idle. Divert/Revert would post messages to the > task runner to switch the stream. That exactly what I'm doing here, unless I'm misunderstanding you. The PassSoon() method will switch the pointer if OnMoreData() is idle. When not idle, the pointer will swapped by the other thread, once OnMoreData() is about to return. https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:180: // An AudioSourceCallback that allows passing around the underlying SyncReader On 2012/12/12 01:29:15, DaleCurtis wrote: > Move to CC file? Done. https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:185: class Glue On 2012/12/12 01:29:15, DaleCurtis wrote: > A better name would be nice. Done. s/Glue/SourceSharingCallback/ https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.h:187: public base::RefCountedThreadSafe<Glue> { On 2012/12/12 01:29:15, DaleCurtis wrote: > Why does this need refcounting? An instance has to live until the "pass" completes, either on the current thread or the hardware audio thread. In other words, we need to be able to orphan an instance. I tried coding up a way to have the instance self-delete after a pass, but this began to get complex, and I was pretty much reinventing this code: base::Bind(&DoDelayedPass, this, other)
https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.cc:306: void AudioOutputController::Glue::PassSoon(const scoped_refptr<Glue>& to) { On 2012/12/12 03:13:39, Yuri wrote: > On 2012/12/12 01:29:15, DaleCurtis wrote: > > I don't quite follow why we need it. > > Main reason: Per our hallway conversation, "Though shalt not block audio > threads." The hardware audio thread may take a significant amount of time doing > the read/write from SyncReader, so it's not acceptable to use a simpler locking > scheme that could potentially block the audio manager thread. > > Other reasons: Please see my comments/responses from yesterday. > > > You're still using OnDeviceChange() right? > > No. That's one reason I'm advocating this new approach: We don't need to > complexify AudioOutputController. In patch set 7, we were closing the stream > but pretending it's still open. Remember how stream_ could be NULL while in the > kPlayback state? > > In fact, with this new approach, the rest of AOC can be implemented in *total > ignorance* of the divert/revert mechanism! How much simpler can it get? :) > > > ... you could just have a task runner that is only pumped > > on OnMoreData() callback until idle. Divert/Revert would post messages to the > > task runner to switch the stream. > > That exactly what I'm doing here, unless I'm misunderstanding you. The > PassSoon() method will switch the pointer if OnMoreData() is idle. When not > idle, the pointer will swapped by the other thread, once OnMoreData() is about > to return. How come you don't do something similar to OnDeviceChange does and just Stop the real audio stream, and Start() a fake one provided by the MirroringDestination? Then restart the original once mirroring is complete? Lets discuss this more. https://codereview.chromium.org/11413078/diff/29002/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/29002/media/audio/audio_output_... media/audio/audio_output_controller.h:180: class SourceSharingCallback; Don't nest, allows you to avoid huge method prefix. Just forward declare outside of class definition.
Dale, Things got a ton simpler. Is this what you had in mind? -Yuri https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/24001/media/audio/audio_output_... media/audio/audio_output_controller.cc:306: void AudioOutputController::Glue::PassSoon(const scoped_refptr<Glue>& to) { On 2012/12/12 20:38:38, DaleCurtis wrote: > How come you don't do something similar to OnDeviceChange does and just Stop the > real audio stream, and Start() a fake one provided by the MirroringDestination? > Then restart the original once mirroring is complete? Lets discuss this more. Done. Good call, Dale! :-) Per hallway conversation, I'm allowed to assume no thread is in OnMoreData() after AudioOutputController::Stop() returns. I was able to use the OnDeviceChange() mechanism and this drastically simplified everything (both in AOC and AudioRendererHost)! https://codereview.chromium.org/11413078/diff/29002/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/29002/media/audio/audio_output_... media/audio/audio_output_controller.h:180: class SourceSharingCallback; On 2012/12/12 20:38:38, DaleCurtis wrote: > Don't nest, allows you to avoid huge method prefix. Just forward declare outside > of class definition. I removed this.
https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:110: static void StartMirroring( I worry somewhat about what seems to be the bolting on of Mirroring functionality via static methods here. Would it make more sense for AudioRendererHost to access a lazy-instance TabMirroring() object that holds all this functionality? Having all these static methods tells me this functionality could probably live in a separate helper class for clarity. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:377: state_ = kRecreating; Is this state necessary anymore? Should DoStopCloseAndClearStream just set state_ = kEmpty? https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:399: if (!message_loop_->BelongsToCurrentThread()) { Which threads will be calling this? https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:407: diverting_to_stream_ = to_stream; I suspect this might have issues if it gets fired as shutdown is starting. You might consider explicitly checking state and bailing if it's not okay. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:413: if (!message_loop_->BelongsToCurrentThread()) { Ditto. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.h:169: void RevertDiversion(); Naming is a bit odd; but my suggestions sound wonky too. The best I can come up with is StartDiverting(<stream>) and StopDiverting().
https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:97: virtual void OnChannelConnected(int32 peer_pid) OVERRIDE; is int32 needed or is int enough? https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:133: typedef std::map<int, AudioRendererHost*> ActiveHostMap; can you add a note about AudioRendererHost ownership? https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:73: AudioRendererHost::g_host_map_ = LAZY_INSTANCE_INITIALIZER; thinking out loud here... this map is only accessed on the IO thread... instead of making it global, can we somehow tie it to the IO thread and only make it accessible from there? https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:286: if (!host) { no {} https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:343: if (!host) { no {} https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:363: if (entry->render_view_id == render_view_id && !entry->pending_close) { same here (no {}) and throughout for single line if's (where the condition is a single line and the body is a single line as well) https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:73: : public base::RefCountedThreadSafe<MirroringDestination> { Can we avoid reference counting here? Instead use scoped_ptr and explicit ownership passing where needed? https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:110: static void StartMirroring( On 2012/12/17 20:00:47, DaleCurtis wrote: > I worry somewhat about what seems to be the bolting on of Mirroring > functionality via static methods here. Would it make more sense for > AudioRendererHost to access a lazy-instance TabMirroring() object that holds all > this functionality? > > Having all these static methods tells me this functionality could probably live > in a separate helper class for clarity. +1. Maybe a Mirror object of sorts per render view with Start and Stop methods. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.h:169: void RevertDiversion(); On 2012/12/17 20:00:47, DaleCurtis wrote: > Naming is a bit odd; but my suggestions sound wonky too. The best I can come up > with is StartDiverting(<stream>) and StopDiverting(). RevertDivert? yeah, hard to come up with a good one :)
Dale/Tommi, PTAL. No rush, as I'll be out-of-office until Thu Dec 27th. Thanks, Yuri https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:97: virtual void OnChannelConnected(int32 peer_pid) OVERRIDE; On 2012/12/18 13:41:25, tommi wrote: > is int32 needed or is int enough? The method signature should probably match what's in BrowserMessageFilter on all platforms, so int32. It no longer matters, however, as I don't need this override anymore. https://codereview.chromium.org/11413078/diff/11008/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:133: typedef std::map<int, AudioRendererHost*> ActiveHostMap; On 2012/12/18 13:41:25, tommi wrote: > can you add a note about AudioRendererHost ownership? Done. (in audio_mirroring_manager.h) https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:73: : public base::RefCountedThreadSafe<MirroringDestination> { On 2012/12/18 13:41:26, tommi wrote: > Can we avoid reference counting here? Instead use scoped_ptr and explicit > ownership passing where needed? With the breaking-out of the mirroring functionality, I was able to remove the ref-counting on this interface. :-) https://codereview.chromium.org/11413078/diff/35002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:110: static void StartMirroring( On 2012/12/17 20:00:47, DaleCurtis wrote: > I worry somewhat about what seems to be the bolting on of Mirroring > functionality via static methods here. Would it make more sense for > AudioRendererHost to access a lazy-instance TabMirroring() object that holds all > this functionality? > > Having all these static methods tells me this functionality could probably live > in a separate helper class for clarity. Yeah, now that you guys have pointed it out, I agree too. So, I broke the functionality out into a separate AudioMirroringManager. AudioRendererHost will tell AudioMirroringManager about all the streams as they are created and closed, and provide a simple object that implements the AudioMirroringManager::Diverter interface. Then, WebContentsAudioInputStream (in a later CL) will call StartMirroring() and StopMirroring(). The unit tests (in audio_mirroring_manager_unittest.cc) should make everything obvious. PTAL. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:377: state_ = kRecreating; On 2012/12/17 20:00:47, DaleCurtis wrote: > Is this state necessary anymore? Should DoStopCloseAndClearStream just set > state_ = kEmpty? Yes, it's still necessary. The code on line 155 uses the kRecreating state to prevent an extra call to handler_->OnCreated(this). https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:399: if (!message_loop_->BelongsToCurrentThread()) { On 2012/12/17 20:00:47, DaleCurtis wrote: > Which threads will be calling this? The IO thread (from AudioRendererHost). Added ThreadChecker to guarantee single-thread use of these methods. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:407: diverting_to_stream_ = to_stream; On 2012/12/17 20:00:47, DaleCurtis wrote: > I suspect this might have issues if it gets fired as shutdown is starting. You > might consider explicitly checking state and bailing if it's not okay. See DCHECK one line down. ;-) https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.cc:413: if (!message_loop_->BelongsToCurrentThread()) { On 2012/12/17 20:00:47, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... File media/audio/audio_output_controller.h (right): https://codereview.chromium.org/11413078/diff/35002/media/audio/audio_output_... media/audio/audio_output_controller.h:169: void RevertDiversion(); On 2012/12/17 20:00:47, DaleCurtis wrote: > Naming is a bit odd; but my suggestions sound wonky too. The best I can come up > with is StartDiverting(<stream>) and StopDiverting(). Done.
Haven't taken another look yet, but don't forget to add someone from the security team for the IPC message implementations as requested by cdn.
https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.cc:132: if (session_it == sessions_.end() || destination != session_it->second) { nit: either always use {} in cases like this or never. It's up to you in this file but I think in general chromium code doesn't use {} for single line conditionals. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:15: // 2. At some point, AudioRendererHost receives an "associate with render view" 80+ https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:94: // |destination|. |destination| must live until after StopMirroring() is called. 80+ https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:108: static void SetInstanceForTesting(AudioMirroringManager* test_instance); is there no better way to support testing? This anti pattern can cause flakiness and hide bugs. There's no guarantee that a test will properly cleanup after itself and the value of the pointer from one test can affect the results of the next one etc. I'd really like to avoid this approach if at all possible. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:113: typedef std::pair<int, int> Target; why not just define a struct? Remembering first and second is prone to errors. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:64: AudioMirroringManager::GetInstance()-> instead of using GetInstance() suggest instead to create a specific manager object per test and inject a pointer to it into this class. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:319: entry->diverter.SetController(entry->controller); nit: keep an empty line after end of scope https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:48: AudioMirroringManager::SetInstanceForTesting(this); Sorry but I'm really not a fan of this approach, see comments elsewhere. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:174: revert
https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.cc:132: if (session_it == sessions_.end() || destination != session_it->second) { On 2012/12/19 12:27:54, tommi wrote: > nit: either always use {} in cases like this or never. It's up to you in this > file but I think in general chromium code doesn't use {} for single line > conditionals. Done. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:15: // 2. At some point, AudioRendererHost receives an "associate with render view" On 2012/12/19 12:27:54, tommi wrote: > 80+ Done. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:94: // |destination|. |destination| must live until after StopMirroring() is called. On 2012/12/19 12:27:54, tommi wrote: > 80+ Done. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:108: static void SetInstanceForTesting(AudioMirroringManager* test_instance); On 2012/12/19 12:27:54, tommi wrote: > is there no better way to support testing? Done. Used dependency injection via the AudioRendererHost ctor so test code could pass in the mock AudioMirroringManager instance (in audio_renderer_host_unittest.cc). https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:113: typedef std::pair<int, int> Target; On 2012/12/19 12:27:54, tommi wrote: > why not just define a struct? Remembering first and second is prone to errors. Two reasons: 1. It's only used as a key in the maps. There are no references to "first" and "second" in audio_mirroring_mangaer.cc. 2. We get operator<() with the desired semantics for free. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:64: AudioMirroringManager::GetInstance()-> On 2012/12/19 12:27:54, tommi wrote: > instead of using GetInstance() suggest instead to create a specific manager > object per test and inject a pointer to it into this class. Done. Made it a member of AudioMirroringManagerTest, which is constructed fresh for each test. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:319: entry->diverter.SetController(entry->controller); On 2012/12/19 12:27:54, tommi wrote: > nit: keep an empty line after end of scope Done. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:48: AudioMirroringManager::SetInstanceForTesting(this); On 2012/12/19 12:27:54, tommi wrote: > Sorry but I'm really not a fan of this approach, see comments elsewhere. Done. https://codereview.chromium.org/11413078/diff/41001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:174: On 2012/12/19 12:27:54, tommi wrote: > revert Done.
On 2012/12/19 02:55:06, DaleCurtis wrote: > Haven't taken another look yet, but don't forget to add someone from the > security team for the IPC message implementations as requested by cdn. Yep. I've been waiting for the magnitude of code changes between patch sets to settle down first.
Still going over the AudioMirroringManager. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:81: static AudioMirroringManager* GetInstance(); Instead of being a static-memory / singleton pattern can this be owned by BrowserMainLoop a la AudioManager is today? https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:28: class ControllerDiverter : public AudioMirroringManager::Diverter { ISTM that you could just have AudioOutputController implement this interface and forgo this wrapper class. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:39: }; Disallow copy and assign? Why not make provide controller_ in the constructor instead of having a SetController method? Just make the AudioEntry version a scoped_ptr. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:42: controller_ = ctlr; DCHECK(ctlr) for haystack hunts. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:350: if (mirroring_manager_) { Mirroring manager should always be present since it's provided during the constructor? Just DCHECK() in constructor? https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:450: if (mirroring_manager_) { Ditto. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:402: if (!message_loop_->BelongsToCurrentThread()) { This is still odd and inconsistent with the rest of the file. The thread checker doesn't add anything either since you're always posting the task. Instead you should break this up into StartDiverting() and DoStartDiverting() like the rest of the methods which do the same thing. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:412: DCHECK_NE(kClosed, state_); Instead of a DHCECK_NE, shouldn't this be an if (state == kClosed) return before diverting_to_stream_ is set? https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:416: void AudioOutputController::StopDiverting() { Ditto. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:425: OnDeviceChange(); This needs a comment since it's subtle that OnDeviceChange will automatically stop diverting when DoStopCloseAndClearStream() occurs w/ divert_to_stream_ == stream_.
Rebased, and moved a few things around per suggestions. PTAL. Thanks, Yuri https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:81: static AudioMirroringManager* GetInstance(); On 2013/01/02 21:56:44, DaleCurtis wrote: > Instead of being a static-memory / singleton pattern can this be owned by > BrowserMainLoop a la AudioManager is today? Done. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:28: class ControllerDiverter : public AudioMirroringManager::Diverter { On 2013/01/02 21:56:44, DaleCurtis wrote: > ISTM that you could just have AudioOutputController implement this interface and > forgo this wrapper class. Done. I had thought about this, but I quickly dismissed the idea because of component boundaries (the AMM::Diverter interface is in content, whereas AOC is in media). However, I just tried moving the interface definition into media (i.e., AudioMirroringManager::Diverter is now AudioOutputStream::AudioSourceDiverter), and it seems to work well w.r.t. code organization. Do you like it better this way? I could see arguments for doing things either way. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:39: }; On 2013/01/02 21:56:44, DaleCurtis wrote: > Disallow copy and assign? Why not make provide controller_ in the constructor > instead of having a SetController method? Just make the AudioEntry version a > scoped_ptr. No longer applicable. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:42: controller_ = ctlr; On 2013/01/02 21:56:44, DaleCurtis wrote: > DCHECK(ctlr) for haystack hunts. No longer applicable. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:350: if (mirroring_manager_) { On 2013/01/02 21:56:44, DaleCurtis wrote: > Mirroring manager should always be present since it's provided during the > constructor? Just DCHECK() in constructor? Done. And fixed things so MediaObserver must never be NULL either. https://codereview.chromium.org/11413078/diff/46003/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:450: if (mirroring_manager_) { On 2013/01/02 21:56:44, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:402: if (!message_loop_->BelongsToCurrentThread()) { On 2013/01/02 21:56:44, DaleCurtis wrote: > This is still odd and inconsistent with the rest of the file. The thread checker > doesn't add anything either since you're always posting the task. > > Instead you should break this up into StartDiverting() and DoStartDiverting() > like the rest of the methods which do the same thing. Done. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:412: DCHECK_NE(kClosed, state_); On 2013/01/02 21:56:44, DaleCurtis wrote: > Instead of a DHCECK_NE, shouldn't this be an if (state == kClosed) return before > diverting_to_stream_ is set? Done. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:416: void AudioOutputController::StopDiverting() { On 2013/01/02 21:56:44, DaleCurtis wrote: > Ditto. Done. https://codereview.chromium.org/11413078/diff/46003/media/audio/audio_output_... media/audio/audio_output_controller.cc:425: OnDeviceChange(); On 2013/01/02 21:56:44, DaleCurtis wrote: > This needs a comment since it's subtle that OnDeviceChange will automatically > stop diverting when DoStopCloseAndClearStream() occurs w/ divert_to_stream_ == > stream_. Done.
lgtm % nits. Nice work Yuri! https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:28: class MockDiverter : public AudioMirroringManager::Diverter { These mocks make me a bit sad since there aren't any end to end test cases for this feature. What's the difficulty in using a real diverter and destination (if only for one or two tests)? https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:173: if (!IsRunningHeadless()) Since we have fake audio streams you should always be able to use a real device. https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_io.h#ne... media/audio/audio_io.h:93: class MEDIA_EXPORT AudioSourceDiverter { Does this need to be nested with AudioOutputStream? https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_output_... media/audio/audio_output_controller.cc:308: { Can you submit the removal CL for this that we discussed previously? Since stream_->Stop() ensures that outstanding calls have completed, this is unnecessary.
1. Addressed Dale's comments. 2. Need security review (added cdn@) for AudioRendererHost::OnAssociateStreamWithProducer(). 3. Need OWNER approval for a few files outside media's realm (added darin@). Thanks, Yuri https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:28: class MockDiverter : public AudioMirroringManager::Diverter { On 2013/01/08 19:23:33, DaleCurtis wrote: > These mocks make me a bit sad since there aren't any end to end test cases for > this feature. What's the difficulty in using a real diverter and destination > (if only for one or two tests)? There isn't a "real destination" yet. That's the next CL. I do have plans to put together an end-to-end integration test (was discussing this with the team this morning, actually). Once the last missing piece is in-place (next CL), I'll definitely cook one up. https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host_unittest.cc (right): https://codereview.chromium.org/11413078/diff/55001/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host_unittest.cc:173: if (!IsRunningHeadless()) On 2013/01/08 19:23:33, DaleCurtis wrote: > Since we have fake audio streams you should always be able to use a real device. Done. https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_io.h File media/audio/audio_io.h (right): https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_io.h#ne... media/audio/audio_io.h:93: class MEDIA_EXPORT AudioSourceDiverter { On 2013/01/08 19:23:33, DaleCurtis wrote: > Does this need to be nested with AudioOutputStream? Done. Moved to: base/audio/audio_source_diverter.h https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_output_... File media/audio/audio_output_controller.cc (right): https://codereview.chromium.org/11413078/diff/55001/media/audio/audio_output_... media/audio/audio_output_controller.cc:308: { On 2013/01/08 19:23:33, DaleCurtis wrote: > Can you submit the removal CL for this that we discussed previously? Since > stream_->Stop() ensures that outstanding calls have completed, this is > unnecessary. Done. Thanks for reminding me. Link: http://crbug.com/168902
https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.cc (right): https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.cc:100: diverter->StartDiverting( What happens if a Diverter calls Add/RemoveDiverter inside this loop? DiverterMap is probably not safe to mutate during iteration, right? For example, maybe a Diverter would call RemoveDiverter during StopDiverting? https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:28: class MockDiverter : public AudioMirroringManager::Diverter { nit: shouldn't these classes also be in the anonymous namespace?
https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.cc (right): https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.cc:100: diverter->StartDiverting( On 2013/01/09 05:06:53, darin wrote: > What happens if a Diverter calls Add/RemoveDiverter inside this loop? > DiverterMap is probably not safe to mutate during iteration, right? For > example, maybe a Diverter would call RemoveDiverter during StopDiverting? Added a comment in the header file to warn about the re-entrancy issue. I'm reluctant to take further steps since this can't happen the way the code is currently written, and I don't anticipate anyone would want/need to do such a thing in future coding. https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc (right): https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc:28: class MockDiverter : public AudioMirroringManager::Diverter { On 2013/01/09 05:06:53, darin wrote: > nit: shouldn't these classes also be in the anonymous namespace? Done. Good call, since all these tests get linked together in one binary.
On Tue, Jan 8, 2013 at 11:21 PM, <miu@chromium.org> wrote: > > https://codereview.chromium.**org/11413078/diff/71001/** > content/browser/renderer_host/**media/audio_mirroring_manager.**cc<https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_host/media/audio_mirroring_manager.cc> > File content/browser/renderer_host/**media/audio_mirroring_manager.**cc > (right): > > https://codereview.chromium.**org/11413078/diff/71001/** > content/browser/renderer_host/**media/audio_mirroring_manager.** > cc#newcode100<https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_host/media/audio_mirroring_manager.cc#newcode100> > content/browser/renderer_host/**media/audio_mirroring_manager.**cc:100: > diverter->StartDiverting( > On 2013/01/09 05:06:53, darin wrote: > >> What happens if a Diverter calls Add/RemoveDiverter inside this loop? >> DiverterMap is probably not safe to mutate during iteration, right? >> > For > >> example, maybe a Diverter would call RemoveDiverter during >> > StopDiverting? > > Added a comment in the header file to warn about the re-entrancy issue. > I'm reluctant to take further steps since this can't happen the way the > code is currently written, and I don't anticipate anyone would want/need > to do such a thing in future coding. Famous Last Words. You should at least add a DCHECK. Seriously. Large programs frequently encounter bugs like this... eventually. Calling out to foreign code for each element of a STL collection is a classic source of bugs. (For reference, tools like base/observer_list.h and base/id_map.h provide mutation-safe element enumeration.) -Darin > > > https://codereview.chromium.**org/11413078/diff/71001/** > content/browser/renderer_host/**media/audio_mirroring_manager_** > unittest.cc<https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc> > File > content/browser/renderer_host/**media/audio_mirroring_manager_** > unittest.cc > (right): > > https://codereview.chromium.**org/11413078/diff/71001/** > content/browser/renderer_host/**media/audio_mirroring_manager_** > unittest.cc#newcode28<https://codereview.chromium.org/11413078/diff/71001/content/browser/renderer_host/media/audio_mirroring_manager_unittest.cc#newcode28> > content/browser/renderer_host/**media/audio_mirroring_manager_** > unittest.cc:28: > class MockDiverter : public AudioMirroringManager::**Diverter { > On 2013/01/09 05:06:53, darin wrote: > >> nit: shouldn't these classes also be in the anonymous namespace? >> > > Done. Good call, since all these tests get linked together in one > binary. > > https://codereview.chromium.**org/11413078/<https://codereview.chromium.org/1... >
On 2013/01/09 09:16:38, darin wrote: > You should at least add a DCHECK. Seriously. Large programs frequently > encounter bugs like this... eventually. Calling out to foreign code for > each element of a STL collection is a classic source of bugs. Done. Added checks in audio_mirroring_manager.cc.
Thanks, LGTM.
Yuri, this looks ok from a high-level. I've done some moderate review, but otherwise am deferring to other reviewers. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:91: typedef std::pair<int, int> Target; nit: not a big deal, but I'm not a fan of std:pair. It makes code harder to read than using a simple struct. I know that tommi has commented in other CLs about this. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:309: return; // No change. comment is obvious - adds little value https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:315: render_process_id_, entry->render_view_id, entry->controller); We certainly don't have to do this now, but struct AudioRendererHost::AudioEntry has been begging to be made into a full-fledged class to help relieve the burden on implementation details now handled in AudioRendererHost. It could handle some of the nitty-gritty work relating to setting up the shared-memory and SyncSocket. It could also handle the logic you're adding in lines 308:315 maybe worth thinking about for future cleanup, a TODO, etc. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:173: // Dependencies. comment is vague and adds little value - I'd remove or make a more specific comment or comments
IPC security audit LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11413078/74023
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_mirroring_manager.h (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_mirroring_manager.h:91: typedef std::pair<int, int> Target; On 2013/01/09 21:18:44, Chris Rogers wrote: > nit: not a big deal, but I'm not a fan of std:pair. It makes code harder to > read than using a simple struct. I know that tommi has commented in other CLs > about this. Normally, I'd agree with you (and Tommi raised the same concern). The reason I've not changed it is because the Target objects are only used as keys into the maps. The individual .first and .second members are never directly accessed/manipulated. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.cc (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:309: return; // No change. On 2013/01/09 21:18:44, Chris Rogers wrote: > comment is obvious - adds little value Done. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.cc:315: render_process_id_, entry->render_view_id, entry->controller); On 2013/01/09 21:18:44, Chris Rogers wrote: > We certainly don't have to do this now, but struct AudioRendererHost::AudioEntry > has been begging to be made into a full-fledged class to help relieve the burden Agreed. There's also other clean-up and even bug fixes I've noticed are needed in this area. For example, the complex "pending_close" scheme should be unnecessary. This CL's big enough right now, so this is definitely future cleanup territory. https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... File content/browser/renderer_host/media/audio_renderer_host.h (right): https://codereview.chromium.org/11413078/diff/81002/content/browser/renderer_... content/browser/renderer_host/media/audio_renderer_host.h:173: // Dependencies. On 2013/01/09 21:18:44, Chris Rogers wrote: > comment is vague and adds little value - I'd remove or make a more specific > comment or comments Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11413078/72027
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11413078/78015
Retried try job too often on win_rel for step(s) nacl_integration
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/miu@chromium.org/11413078/78015
Message was sent while issue was closed.
Change committed as 176295 |
