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

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:
1 year, 9 months ago by xians1
Modified:
1 year, 9 months ago
Reviewers:
jam, Avi, tommi, darin, mflodman
CC:
chromium-reviews_chromium.org, 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) Lint 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 0 errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors 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 ? errors Download
Commit:

Messages

Total messages: 21
xians1
1 year, 9 months ago #1
xians1
Would you please review this CL? Andrew, Magnus: would you please review the general code ...
1 year, 9 months ago #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 ...
1 year, 9 months ago #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 ...
1 year, 9 months ago #4
xians1
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 ...
1 year, 9 months ago #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 ...
1 year, 9 months ago #6
scherkus
one suggestion for the GetMessageLoop() stuff I'm removing myself as a reviewer for now as ...
1 year, 9 months ago #7
xians1
Andrew, done with removing the code which disposes the message loop for unit test. Have ...
1 year, 9 months ago #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 ...
1 year, 9 months ago #9
xians1
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 ...
1 year, 9 months ago #10
xians1
Tommi, please take a quick look. Thanks. BR, /SX
1 year, 9 months ago #11
tommi
I like simplifying the code but I'm not so sure about replacing it with dependency ...
1 year, 9 months ago #12
mflodman
On 2012/07/02 13:36:36, tommi wrote: > I like simplifying the code but I'm not so ...
1 year, 9 months ago #13
xians1
Tommi and John, this version move the MediaStreamManager from resource context to BrowserMainloop. One of ...
1 year, 9 months ago #14
tommi
Excellent work Shijing. The classes are now much more easily testable as well as being ...
1 year, 9 months ago #15
xians1
Many thanks to Tommi's great help. John, how do you think about the changes? Can ...
1 year, 9 months ago #16
xians1
Removed John from the reviewer list since he is on vocation. sky: could you please ...
1 year, 9 months ago #17
xians1
avi or darin, can I have owner stamp from either of you for content/browser? Thanks, ...
1 year, 9 months ago #18
jam
content/browser lgtm (didn't look at the media subdirs)
1 year, 9 months ago #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
1 year, 9 months ago #20
I haz the power (commit-bot)
1 year, 9 months ago #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 1275:d14800f88434