Chromium Code Reviews
Help | Chromium Project | Sign in
(64)

Issue 10662049: Move the device enumerate/open/close work to device thread from IO thread (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 8 months ago by no longer working on chromium
Modified:
2 years, 7 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, jochen+watch-content_chromium.org, feature-media-reviews_chromium.org
Visibility:
Public.

Description

Pepper needs to repeatedly call enumeration API in order to get a synchronous up-to-date device list. This heavily loads the IO thread since the enumeration is done on IO thread. This patch moves the device thread from VideoCaptureManager to MediaStreamManager, so that audio and video can share one device thread, and also allow running audio device API on this shared device thread. BUG=132701, 130113 TEST=content_unittests Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=145584

Patch Set 1 #

Patch Set 2 : ready for review. #

Total comments: 21

Patch Set 3 : addressed Andrew and John's comments. #

Total comments: 1

Patch Set 4 : addressed Andrew's comment and do not dispose the message_loop in MediaStreamManager #

Patch Set 5 : change to a STA thread on windows. #

Patch Set 6 : small fixed on windows. #

Patch Set 7 : minor fix for the problem detected by the trybots #

Total comments: 15

Patch Set 8 : addressed Magnus' comments. #

Total comments: 26

Patch Set 9 : moved MediaStream to BrowserMainloop and addressed all the comments from Tommi and Magnus. #

Total comments: 18

Patch Set 10 : addressed Tommi's comments. #

Patch Set 11 : small changes to fix the trybots' failure #

Unified diffs Side-by-side diffs Delta from patch set Stats (+365 lines, -390 lines) Patch
M content/browser/browser_main_loop.h View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -0 lines 0 comments Download
M content/browser/browser_main_loop.cc View 1 2 3 4 5 6 7 8 9 4 chunks +18 lines, -0 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_device_manager.h View 1 2 3 4 5 6 7 8 9 3 chunks +17 lines, -4 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_device_manager.cc View 1 2 3 4 5 6 7 8 9 5 chunks +46 lines, -16 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_device_manager_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +7 lines, -11 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_renderer_host.h View 1 2 3 4 5 6 7 8 3 chunks +11 lines, -8 lines 0 comments Download
M content/browser/renderer_host/media/audio_input_renderer_host.cc View 1 2 3 4 5 6 7 8 3 chunks +6 lines, -14 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_device_settings.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +0 lines, -1 line 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host.h View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -11 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host.cc View 1 2 3 4 5 6 7 8 8 chunks +13 lines, -17 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_dispatcher_host_unittest.cc View 1 2 3 4 5 6 7 8 8 chunks +32 lines, -54 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.h View 1 2 3 4 5 6 7 8 9 5 chunks +31 lines, -18 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_manager.cc View 1 2 3 4 5 6 7 8 3 chunks +26 lines, -31 lines 0 comments Download
M content/browser/renderer_host/media/media_stream_provider.h View 1 2 3 4 5 6 7 8 9 2 chunks +7 lines, -2 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_controller_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -3 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host.h View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -16 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host.cc View 1 2 3 4 5 6 7 8 3 chunks +4 lines, -7 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_host_unittest.cc View 1 2 3 4 5 6 7 8 9 chunks +30 lines, -49 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager.h View 1 2 3 4 5 6 7 8 9 8 chunks +11 lines, -21 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager.cc View 1 2 3 4 5 6 7 8 9 23 chunks +31 lines, -39 lines 0 comments Download
M content/browser/renderer_host/media/video_capture_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 13 chunks +21 lines, -52 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -5 lines 0 comments Download
M content/test/webrtc_audio_device_test.cc View 1 2 3 4 5 6 7 8 5 chunks +17 lines, -6 lines 0 comments Download
M media/video/capture/win/video_capture_device_win.h View 1 2 3 4 5 6 7 8 9 3 chunks +4 lines, -4 lines 0 comments Download
M media/video/capture/win/video_capture_device_win.cc View 1 2 3 4 5 6 7 8 9 10 11 chunks +11 lines, -1 line 0 comments Download
Commit:

Messages

Total messages: 21 (0 generated)
no longer working on chromium
2 years, 8 months ago (2012-06-26 16:55:10 UTC) #1
no longer working on chromium
Would you please review this CL? Andrew, Magnus: would you please review the general code ...
2 years, 8 months ago (2012-06-26 19:46:01 UTC) #2
jam
http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/media_stream_manager.cc#newcode120 content/browser/renderer_host/media/media_stream_manager.cc:120: DCHECK(thread); nit: unnecessary, if it's NULL the next line ...
2 years, 8 months ago (2012-06-26 22:32:01 UTC) #3
scherkus
https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): https://chromiumcodereview.appspot.com/10662049/diff/2001/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode111 content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:111: device_thread_.reset(new base::Thread("AudioInputDeviceManagerTestThread")); given that you can inject message loops ...
2 years, 8 months ago (2012-06-27 00:52:31 UTC) #4
no longer working on chromium
http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc File content/browser/renderer_host/media/audio_input_device_manager_unittest.cc (right): http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc#newcode111 content/browser/renderer_host/media/audio_input_device_manager_unittest.cc:111: device_thread_.reset(new base::Thread("AudioInputDeviceManagerTestThread")); On 2012/06/27 00:52:31, scherkus wrote: > given ...
2 years, 8 months ago (2012-06-27 14:07:16 UTC) #5
jam
http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/media_stream_manager.cc File content/browser/renderer_host/media/media_stream_manager.cc (right): http://codereview.chromium.org/10662049/diff/2001/content/browser/renderer_host/media/media_stream_manager.cc#newcode152 content/browser/renderer_host/media/media_stream_manager.cc:152: BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, On 2012/06/27 14:07:16, xians1 wrote: > On ...
2 years, 8 months ago (2012-06-27 16:13:14 UTC) #6
scherkus
one suggestion for the GetMessageLoop() stuff I'm removing myself as a reviewer for now as ...
2 years, 8 months ago (2012-06-28 01:24:20 UTC) #7
no longer working on chromium
Andrew, done with removing the code which disposes the message loop for unit test. Have ...
2 years, 8 months ago (2012-06-28 16:53:22 UTC) #8
mflodman
Nice CL! Some comments. http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_host/media/audio_input_device_manager.h File content/browser/renderer_host/media/audio_input_device_manager.h (right): http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_host/media/audio_input_device_manager.h#newcode40 content/browser/renderer_host/media/audio_input_device_manager.h:40: AudioInputDeviceManager(scoped_refptr<base::MessageLoopProxy> message_loop); Add explicit. http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_host/media/audio_input_device_manager_unittest.cc ...
2 years, 8 months ago (2012-06-29 14:46:04 UTC) #9
no longer working on chromium
http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_host/media/audio_input_device_manager.h File content/browser/renderer_host/media/audio_input_device_manager.h (right): http://codereview.chromium.org/10662049/diff/28004/content/browser/renderer_host/media/audio_input_device_manager.h#newcode40 content/browser/renderer_host/media/audio_input_device_manager.h:40: AudioInputDeviceManager(scoped_refptr<base::MessageLoopProxy> message_loop); On 2012/06/29 14:46:04, mflodman wrote: > Add ...
2 years, 8 months ago (2012-07-02 08:40:49 UTC) #10
no longer working on chromium
Tommi, please take a quick look. Thanks. BR, /SX
2 years, 8 months ago (2012-07-02 09:49:20 UTC) #11
tommi
I like simplifying the code but I'm not so sure about replacing it with dependency ...
2 years, 8 months ago (2012-07-02 13:36:36 UTC) #12
mflodman
On 2012/07/02 13:36:36, tommi wrote: > I like simplifying the code but I'm not so ...
2 years, 8 months ago (2012-07-02 18:56:30 UTC) #13
no longer working on chromium
Tommi and John, this version move the MediaStreamManager from resource context to BrowserMainloop. One of ...
2 years, 8 months ago (2012-07-04 12:35:25 UTC) #14
tommi
Excellent work Shijing. The classes are now much more easily testable as well as being ...
2 years, 8 months ago (2012-07-04 13:46:48 UTC) #15
no longer working on chromium
Many thanks to Tommi's great help. John, how do you think about the changes? Can ...
2 years, 8 months ago (2012-07-04 14:31:39 UTC) #16
no longer working on chromium
Removed John from the reviewer list since he is on vocation. sky: could you please ...
2 years, 8 months ago (2012-07-05 09:10:32 UTC) #17
no longer working on chromium
avi or darin, can I have owner stamp from either of you for content/browser? Thanks, ...
2 years, 8 months ago (2012-07-05 09:20:16 UTC) #18
jam
content/browser lgtm (didn't look at the media subdirs)
2 years, 8 months ago (2012-07-05 17:49:59 UTC) #19
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10662049/18013
2 years, 7 months ago (2012-07-06 10:38:17 UTC) #20
I haz the power (commit-bot)
2 years, 7 months ago (2012-07-06 12:03:07 UTC) #21
Change committed as 145584
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87e6a26