Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(643)

Issue 9104043: Monitor the IO message loop in the AudioDevice classes. (Closed)

Created:
8 years, 10 months ago by tommi (sloooow) - chröme
Modified:
8 years, 10 months ago
CC:
chromium-reviews, hclam+watch_chromium.org, ddorwin+watch_chromium.org, fischman+watch_chromium.org, jam, acolwell+watch_chromium.org, annacc+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, vrk (LEFT CHROMIUM), scherkus (not reviewing), ihf+watch_chromium.org
Visibility:
Public.

Description

Monitor the IO message loop in the AudioDevice classes. We need to do this since the IO thread can go out of scope before the pipeline thread gets around to closing all the audio devices. BUG=112101 TEST=Run content_unittests. Also run Chrome and play with opening pages with lots of audio elements and close the pages while still playing audio. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=120749

Patch Set 1 #

Total comments: 6

Patch Set 2 : Fix errors and rebase #

Patch Set 3 : Address review comments #

Patch Set 4 : doh! #

Total comments: 12

Patch Set 5 : Move the observer class to separate files and un-templatize it #

Patch Set 6 : Address review comments #

Total comments: 2

Patch Set 7 : Fix indent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+179 lines, -75 lines) Patch
M content/content_renderer.gypi View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M content/renderer/media/audio_device.h View 1 2 3 4 2 chunks +9 lines, -5 lines 0 comments Download
M content/renderer/media/audio_device.cc View 1 2 3 4 14 chunks +30 lines, -25 lines 0 comments Download
M content/renderer/media/audio_input_device.h View 1 2 3 4 3 chunks +6 lines, -0 lines 0 comments Download
M content/renderer/media/audio_input_device.cc View 1 2 3 4 10 chunks +44 lines, -44 lines 0 comments Download
A content/renderer/media/scoped_loop_observer.h View 1 2 3 4 5 6 1 chunk +45 lines, -0 lines 0 comments Download
A content/renderer/media/scoped_loop_observer.cc View 1 2 3 4 1 chunk +42 lines, -0 lines 0 comments Download
M media/audio/audio_manager_base.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 16 (0 generated)
tommi (sloooow) - chröme
8 years, 10 months ago (2012-01-31 16:56:15 UTC) #1
enal1
https://chromiumcodereview.appspot.com/9104043/diff/1/content/renderer/media/audio_device.h File content/renderer/media/audio_device.h (right): https://chromiumcodereview.appspot.com/9104043/diff/1/content/renderer/media/audio_device.h#newcode118 content/renderer/media/audio_device.h:118: enable ? loop->AddDestructionObserver(this) : Why not regular if statement? ...
8 years, 10 months ago (2012-01-31 17:23:54 UTC) #2
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9104043/diff/1/content/renderer/media/audio_device.h File content/renderer/media/audio_device.h (right): https://chromiumcodereview.appspot.com/9104043/diff/1/content/renderer/media/audio_device.h#newcode118 content/renderer/media/audio_device.h:118: enable ? loop->AddDestructionObserver(this) : On 2012/01/31 17:23:54, enal1 wrote: ...
8 years, 10 months ago (2012-02-01 14:50:49 UTC) #3
tommi (sloooow) - chröme
ping?
8 years, 10 months ago (2012-02-02 14:01:14 UTC) #4
scherkus (not reviewing)
is it guaranteed that the render thread terminates before the IO thread? could this alternatively ...
8 years, 10 months ago (2012-02-02 18:28:18 UTC) #5
tommi (sloooow) - chröme
On Thu, Feb 2, 2012 at 7:28 PM, <scherkus@chromium.org> wrote: > is it guaranteed that ...
8 years, 10 months ago (2012-02-03 00:35:13 UTC) #6
tommi (sloooow) - chröme
hehe, sorry, let me answer that first question again :) On 2012/02/03 00:35:13, tommi wrote: ...
8 years, 10 months ago (2012-02-03 00:48:00 UTC) #7
scherkus (not reviewing)
thanks for the explanation! I've got a few nits https://chromiumcodereview.appspot.com/9104043/diff/3003/content/renderer/media/audio_device.h File content/renderer/media/audio_device.h (right): https://chromiumcodereview.appspot.com/9104043/diff/3003/content/renderer/media/audio_device.h#newcode80 content/renderer/media/audio_device.h:80: ...
8 years, 10 months ago (2012-02-04 00:27:46 UTC) #8
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9104043/diff/3003/content/renderer/media/audio_device.h File content/renderer/media/audio_device.h (right): https://chromiumcodereview.appspot.com/9104043/diff/3003/content/renderer/media/audio_device.h#newcode80 content/renderer/media/audio_device.h:80: // A common base class for AudioDevice and AudioInputDevice ...
8 years, 10 months ago (2012-02-06 13:38:55 UTC) #9
enal1
On 2012/02/06 13:38:55, tommi wrote: lgtm (Code made a great progress while I was listening ...
8 years, 10 months ago (2012-02-06 16:14:25 UTC) #10
scherkus (not reviewing)
LGTM w/ one indent fix https://chromiumcodereview.appspot.com/9104043/diff/3006/content/renderer/media/scoped_loop_observer.h File content/renderer/media/scoped_loop_observer.h (right): https://chromiumcodereview.appspot.com/9104043/diff/3006/content/renderer/media/scoped_loop_observer.h#newcode35 content/renderer/media/scoped_loop_observer.h:35: // the message loop. ...
8 years, 10 months ago (2012-02-06 17:14:30 UTC) #11
tommi (sloooow) - chröme
https://chromiumcodereview.appspot.com/9104043/diff/3006/content/renderer/media/scoped_loop_observer.h File content/renderer/media/scoped_loop_observer.h (right): https://chromiumcodereview.appspot.com/9104043/diff/3006/content/renderer/media/scoped_loop_observer.h#newcode35 content/renderer/media/scoped_loop_observer.h:35: // the message loop. On 2012/02/06 17:14:30, scherkus wrote: ...
8 years, 10 months ago (2012-02-06 20:13:41 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/9104043/15001
8 years, 10 months ago (2012-02-06 20:16:46 UTC) #13
commit-bot: I haz the power
Try job failure for 9104043-15001 (retry) (previous was lost) (previous was lost) on mac_rel for ...
8 years, 10 months ago (2012-02-06 23:20:51 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommi@chromium.org/9104043/15001
8 years, 10 months ago (2012-02-07 06:50:08 UTC) #15
commit-bot: I haz the power
8 years, 10 months ago (2012-02-07 09:24:26 UTC) #16
Try job failure for 9104043-15001 (previous was lost) (retry) on mac_rel for
step "ui_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Powered by Google App Engine
This is Rietveld 408576698