Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2929823002:
Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic (Closed)
DescriptionMaking AudioManagerBase::ShutdownOnAudioThread() platform-agnostic
Migrated to https://chromium-review.googlesource.com/c/chromium/src/+/637696
BUG=731124
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Patch Set 1 #
Total comments: 2
Patch Set 2 : moving closing streams to AudioManagerMac #
Total comments: 1
Messages
Total messages: 32 (22 generated)
Description was changed from ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic BUG=731124 ========== to ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic BUG=731124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
olka@chromium.org changed reviewers: + alokp@chromium.org
Hi Alok - PTAL
Description was changed from ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic BUG=731124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic BUG=731124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2929823002/diff/20001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2929823002/diff/20001/media/audio/audio_manag... media/audio/audio_manager_base.cc:353: // Even if tasks to close the streams are enqueued, they would not run nit: Would it be better to run this block of code in AudioManagerMac, so that we do not need to plumb "immediately" everywhere? AudioManagerMac already tracks all input streams, so it should be pretty easy to close them.
https://codereview.chromium.org/2929823002/diff/20001/media/audio/audio_manag... File media/audio/audio_manager_base.cc (right): https://codereview.chromium.org/2929823002/diff/20001/media/audio/audio_manag... media/audio/audio_manager_base.cc:353: // Even if tasks to close the streams are enqueued, they would not run On 2017/06/09 16:59:17, alokp wrote: > nit: Would it be better to run this block of code in AudioManagerMac, so that we > do not need to plumb "immediately" everywhere? > > AudioManagerMac already tracks all input streams, so it should be pretty easy to > close them. Hmm.. Won't open streams be a problem on other platforms if we run AudioManager on the main thread of the audio process? (see bug description in the CL) I'm a bit confused, since crbug.com/608049 this comment refers to is not only for Mac.
> Hmm.. Won't open streams be a problem on other platforms if we run AudioManager > on the main thread of the audio process? (see bug description in the CL) > I'm a bit confused, since crbug.com/608049 this comment refers to is not only > for Mac. I do not think so. This is unique to browser process threading model. It happens because the main message loop quits *before* stopping the IO thread where audio IO streams live. When the IO thread is stopped, we do close all open streams by posting tasks to the audio thread. Since the audio thread on mac is the main thread, whose message loop has already stopped, those tasks do not get a chance to run. I do not think this should be a problem for audio service because the audio service will *own* all physical streams that can be cleaned up during shutdown. The clients only hold InterfacePtr (proxies) to the physical streams. So even if there are outstanding streams, the clients would simply get connection error.
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
Patchset #2 (id:40001) has been deleted
On 2017/06/09 18:04:22, alokp wrote:
> > Hmm.. Won't open streams be a problem on other platforms if we run
> AudioManager
> > on the main thread of the audio process? (see bug description in the CL)
> > I'm a bit confused, since crbug.com/608049 this comment refers to is not
only
> > for Mac.
>
> I do not think so. This is unique to browser process threading model. It
happens
> because the main message loop quits *before* stopping the IO thread where
audio
> IO streams live. When the IO thread is stopped, we do close all open streams
by
> posting tasks to the audio thread. Since the audio thread on mac is the main
> thread, whose message loop has already stopped, those tasks do not get a
chance
> to run.
So in other words: AudioInputController lives on IO thread; AudioInputStream
lives on audio thread.
First BrowserMainLoop::ShutdownThreadsAndCleanUp() is called [1], and then
BrowserMainLoop is destroyed [2].
As a part of [1], first IO thread is destroyed [3], and then AudioManager
(+AudioThread) is shut down [4].
And the problem with closing the streams is caused by two facts:
1) IO (control) thread is not the same as audio thread, so we have to post
AudioInputStream::Close() to audio thread.
2) Main thread is the same as audio thread, so AudioInputStream::Close won't be
processed.
Right?
But why don't we see the same problem with AudioOutputStream instances?
>
> I do not think this should be a problem for audio service because the audio
> service will *own* all physical streams that can be cleaned up during
shutdown.
> The clients only hold InterfacePtr (proxies) to the physical streams. So even
if
> there are outstanding streams, the clients would simply get connection error.
For audio service it's not actually related to clients threading: it's about
whether we separate control ("IO") thread and audio thread in the service, and
which of them is the main one.
(where control operations would be AudioStream control requests, device info
requests, audio stream state change notifications, audio mirroring requests,
etc.)
Because of deadlock issues on Mac we want audio thread to be the main one.
For now I don't see any issues with serializing control operations on the same
audio thread. And in such a case "AudioInputController analog in audio
service"::Close() can synchronously close the audio stream, and we are all set.
But if we need a separate "IO" thread which is not the main one, we are in the
same situation on all the platforms as now here on Mac: events posted from IO
thread to the main one won't be processed on shutdown.
So either we hope that we don't need IO thread in the audio service - then we
can shutdown streams in AudioManagerMac as you suggested;
or we need a way to let each AudioManager implementation know tabout an
immediate shutdown.
Maybe I'm overthinking :)
WDYT?
[1]
https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?g...
[2]
https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?g...
[3]
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?gsn...
[4]
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?gsn...
alokp@ - friendly ping!
On 2017/06/26 12:32:41, o1ka wrote:
> On 2017/06/09 18:04:22, alokp wrote:
> > > Hmm.. Won't open streams be a problem on other platforms if we run
> > AudioManager
> > > on the main thread of the audio process? (see bug description in the CL)
> > > I'm a bit confused, since crbug.com/608049 this comment refers to is not
> only
> > > for Mac.
> >
> > I do not think so. This is unique to browser process threading model. It
> happens
> > because the main message loop quits *before* stopping the IO thread where
> audio
> > IO streams live. When the IO thread is stopped, we do close all open streams
> by
> > posting tasks to the audio thread. Since the audio thread on mac is the main
> > thread, whose message loop has already stopped, those tasks do not get a
> chance
> > to run.
>
> So in other words: AudioInputController lives on IO thread; AudioInputStream
> lives on audio thread.
> First BrowserMainLoop::ShutdownThreadsAndCleanUp() is called [1], and then
> BrowserMainLoop is destroyed [2].
> As a part of [1], first IO thread is destroyed [3], and then AudioManager
> (+AudioThread) is shut down [4].
>
> And the problem with closing the streams is caused by two facts:
> 1) IO (control) thread is not the same as audio thread, so we have to post
> AudioInputStream::Close() to audio thread.
> 2) Main thread is the same as audio thread, so AudioInputStream::Close won't
be
> processed.
>
> Right?
> But why don't we see the same problem with AudioOutputStream instances?
>
> >
> > I do not think this should be a problem for audio service because the audio
> > service will *own* all physical streams that can be cleaned up during
> shutdown.
> > The clients only hold InterfacePtr (proxies) to the physical streams. So
even
> if
> > there are outstanding streams, the clients would simply get connection
error.
>
> For audio service it's not actually related to clients threading: it's about
> whether we separate control ("IO") thread and audio thread in the service, and
> which of them is the main one.
> (where control operations would be AudioStream control requests, device info
> requests, audio stream state change notifications, audio mirroring requests,
> etc.)
> Because of deadlock issues on Mac we want audio thread to be the main one.
> For now I don't see any issues with serializing control operations on the same
> audio thread. And in such a case "AudioInputController analog in audio
> service"::Close() can synchronously close the audio stream, and we are all
set.
> But if we need a separate "IO" thread which is not the main one, we are in the
> same situation on all the platforms as now here on Mac: events posted from IO
> thread to the main one won't be processed on shutdown.
>
> So either we hope that we don't need IO thread in the audio service - then we
> can shutdown streams in AudioManagerMac as you suggested;
> or we need a way to let each AudioManager implementation know tabout an
> immediate shutdown.
>
> Maybe I'm overthinking :)
>
> WDYT?
>
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?g...
> [2]
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_runner.cc?g...
> [3]
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?gsn...
> [4]
>
https://cs.chromium.org/chromium/src/content/browser/browser_main_loop.cc?gsn...
> So in other words: AudioInputController lives on IO thread; AudioInputStream > lives on audio thread. > First BrowserMainLoop::ShutdownThreadsAndCleanUp() is called [1], and then > BrowserMainLoop is destroyed [2]. > As a part of [1], first IO thread is destroyed [3], and then AudioManager > (+AudioThread) is shut down [4]. > > And the problem with closing the streams is caused by two facts: > 1) IO (control) thread is not the same as audio thread, so we have to post > AudioInputStream::Close() to audio thread. > 2) Main thread is the same as audio thread, so AudioInputStream::Close won't be > processed. > > Right? Exactly! > But why don't we see the same problem with AudioOutputStream instances? This is because AudioOutputController uses AudioOutputProxy, which holds a weak reference to AudioOutputDispatcher: https://cs.chromium.org/chromium/src/media/audio/audio_output_proxy.h?type=cs... This allows us to destroy AudioOutputDispatcher instances (which owns physical audio streams) independent of proxy streams. > For audio service it's not actually related to clients threading: it's about > whether we separate control ("IO") thread and audio thread in the service, and > which of them is the main one. > (where control operations would be AudioStream control requests, device info > requests, audio stream state change notifications, audio mirroring requests, > etc.) > Because of deadlock issues on Mac we want audio thread to be the main one. > For now I don't see any issues with serializing control operations on the same > audio thread. And in such a case "AudioInputController analog in audio > service"::Close() can synchronously close the audio stream, and we are all set. > But if we need a separate "IO" thread which is not the main one, we are in the > same situation on all the platforms as now here on Mac: events posted from IO > thread to the main one won't be processed on shutdown. > > So either we hope that we don't need IO thread in the audio service - then we > can shutdown streams in AudioManagerMac as you suggested; > or we need a way to let each AudioManager implementation know tabout an > immediate shutdown. > > Maybe I'm overthinking :) > > WDYT? I think it would be much simpler to use the main thread for both IO and audio. So may be punt the decision until we find out that this does not work for some reason?
On 2017/07/07 17:03:01, alokp wrote: > > So in other words: AudioInputController lives on IO thread; AudioInputStream > > lives on audio thread. > > First BrowserMainLoop::ShutdownThreadsAndCleanUp() is called [1], and then > > BrowserMainLoop is destroyed [2]. > > As a part of [1], first IO thread is destroyed [3], and then AudioManager > > (+AudioThread) is shut down [4]. > > > > And the problem with closing the streams is caused by two facts: > > 1) IO (control) thread is not the same as audio thread, so we have to post > > AudioInputStream::Close() to audio thread. > > 2) Main thread is the same as audio thread, so AudioInputStream::Close won't > be > > processed. > > > > Right? > > Exactly! > > > But why don't we see the same problem with AudioOutputStream instances? > > This is because AudioOutputController uses AudioOutputProxy, which holds a weak > reference to AudioOutputDispatcher: > https://cs.chromium.org/chromium/src/media/audio/audio_output_proxy.h?type=cs... > > This allows us to destroy AudioOutputDispatcher instances (which owns physical > audio streams) independent of proxy streams. > > > For audio service it's not actually related to clients threading: it's about > > whether we separate control ("IO") thread and audio thread in the service, and > > which of them is the main one. > > (where control operations would be AudioStream control requests, device info > > requests, audio stream state change notifications, audio mirroring requests, > > etc.) > > Because of deadlock issues on Mac we want audio thread to be the main one. > > For now I don't see any issues with serializing control operations on the same > > audio thread. And in such a case "AudioInputController analog in audio > > service"::Close() can synchronously close the audio stream, and we are all > set. > > But if we need a separate "IO" thread which is not the main one, we are in the > > same situation on all the platforms as now here on Mac: events posted from IO > > thread to the main one won't be processed on shutdown. > > > > So either we hope that we don't need IO thread in the audio service - then we > > can shutdown streams in AudioManagerMac as you suggested; > > or we need a way to let each AudioManager implementation know tabout an > > immediate shutdown. > > > > Maybe I'm overthinking :) > > > > WDYT? > > I think it would be much simpler to use the main thread for both IO and audio. > So may be punt the decision until we find out that this does not work for some > reason? SGTM. Moved the logic into AudioManagerMac
The CQ bit was checked by olka@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/11 14:27:31, o1ka wrote: > On 2017/07/07 17:03:01, alokp wrote: > > > So in other words: AudioInputController lives on IO thread; AudioInputStream > > > lives on audio thread. > > > First BrowserMainLoop::ShutdownThreadsAndCleanUp() is called [1], and then > > > BrowserMainLoop is destroyed [2]. > > > As a part of [1], first IO thread is destroyed [3], and then AudioManager > > > (+AudioThread) is shut down [4]. > > > > > > And the problem with closing the streams is caused by two facts: > > > 1) IO (control) thread is not the same as audio thread, so we have to post > > > AudioInputStream::Close() to audio thread. > > > 2) Main thread is the same as audio thread, so AudioInputStream::Close won't > > be > > > processed. > > > > > > Right? > > > > Exactly! > > > > > But why don't we see the same problem with AudioOutputStream instances? > > > > This is because AudioOutputController uses AudioOutputProxy, which holds a > weak > > reference to AudioOutputDispatcher: > > > https://cs.chromium.org/chromium/src/media/audio/audio_output_proxy.h?type=cs... > > > > This allows us to destroy AudioOutputDispatcher instances (which owns physical > > audio streams) independent of proxy streams. > > > > > For audio service it's not actually related to clients threading: it's about > > > whether we separate control ("IO") thread and audio thread in the service, > and > > > which of them is the main one. > > > (where control operations would be AudioStream control requests, device info > > > requests, audio stream state change notifications, audio mirroring requests, > > > etc.) > > > Because of deadlock issues on Mac we want audio thread to be the main one. > > > For now I don't see any issues with serializing control operations on the > same > > > audio thread. And in such a case "AudioInputController analog in audio > > > service"::Close() can synchronously close the audio stream, and we are all > > set. > > > But if we need a separate "IO" thread which is not the main one, we are in > the > > > same situation on all the platforms as now here on Mac: events posted from > IO > > > thread to the main one won't be processed on shutdown. > > > > > > So either we hope that we don't need IO thread in the audio service - then > we > > > can shutdown streams in AudioManagerMac as you suggested; > > > or we need a way to let each AudioManager implementation know tabout an > > > immediate shutdown. > > > > > > Maybe I'm overthinking :) > > > > > > WDYT? > > > > I think it would be much simpler to use the main thread for both IO and audio. > > So may be punt the decision until we find out that this does not work for some > > reason? > > SGTM. Moved the logic into AudioManagerMac Ping again :)
lgtm sorry for the delay - I do not look at rietveld anymore :) https://codereview.chromium.org/2929823002/diff/60001/media/audio/mac/audio_m... File media/audio/mac/audio_manager_mac.cc (right): https://codereview.chromium.org/2929823002/diff/60001/media/audio/mac/audio_m... media/audio/mac/audio_manager_mac.cc:532: AudioManagerBase::ShutdownOnAudioThread(); nit: May be release base class resources *after* local resources to be consistent with the ways destructors work?
Description was changed from ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic BUG=731124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Making AudioManagerBase::ShutdownOnAudioThread() platform-agnostic Migrated to https://chromium-review.googlesource.com/c/chromium/src/+/637696 BUG=731124 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
