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

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:
3 years ago by no longer working on chromium
Modified:
3 years 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: CQ not working?

Messages

Total messages: 21 (0 generated)
no longer working on chromium
3 years 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 ...
3 years 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 ...
3 years ago (2012-06-26 22:32:01 UTC) #3
scherkus (not reviewing)
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 ...
3 years 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 ...
3 years 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 ...
3 years ago (2012-06-27 16:13:14 UTC) #6
scherkus (not reviewing)
one suggestion for the GetMessageLoop() stuff I'm removing myself as a reviewer for now as ...
3 years 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 ...
3 years ago (2012-06-28 16:53:22 UTC) #8
mflodman_chromium_OOO
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 ...
3 years 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 ...
3 years ago (2012-07-02 08:40:49 UTC) #10
no longer working on chromium
Tommi, please take a quick look. Thanks. BR, /SX
3 years 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 ...
3 years ago (2012-07-02 13:36:36 UTC) #12
mflodman_chromium_OOO
On 2012/07/02 13:36:36, tommi wrote: > I like simplifying the code but I'm not so ...
3 years 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 ...
3 years 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 ...
3 years 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 ...
3 years 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 ...
3 years 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, ...
3 years ago (2012-07-05 09:20:16 UTC) #18
jam
content/browser lgtm (didn't look at the media subdirs)
3 years ago (2012-07-05 17:49:59 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/xians@chromium.org/10662049/18013
3 years ago (2012-07-06 10:38:17 UTC) #20
commit-bot: I haz the power
3 years 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 2cec24f