1188269 - Create AudioShutdownManager to manage the lifetime of different AudioSinks.

Status: RESOLVED INVALID

(Core :: Audio/Video: Playback, defect, P3)

Description

[Kilik Kuo [:kikuo] (inactive)]

Per Bug 1146795 Comment 30, Bug 1146795 Comment 32,
Create an AudioShutdownManager, and enforce new instantiated AudioSink to register to it. So that AudioSink Shutdown API could be asynchronized inside itself, and once the process accomplished, AudioShutdownManager could do AudioSink destruction.

Comment 1

[Kilik Kuo [:kikuo] (inactive)]

Attachment: Add_MediaSinkShutdownManager_255041

Hi JW,

I created a MediaSinkShutdownManager to make AudioSink::Shutdown() asynchronized inside AudioSink.
This pattern could be useful once other MediaSinks take more time for shutdown themselves. But right now, it's not high priority.

Several thoughts about the machinery, 
First, I was thinking shutting down this manager by listening to this topic "NS_XPCOM_SHUTDOWN_OBSERVER_ID", but in this case, MediaSinkShutdownManager will be idle most of the time.  Also registering MediaSinkShutdownManager to observerservice needs to be done in main thread. But we only need to create this manager once there's a mediasink needs to be destroyed and it needn't to happen in main thread.    So I discarded this practice.

Second,
To reduce the complexity, I'm not trying to provide API to get the static instance pointer, that's why I directly using "static" on DispatchXXX() & Shutdown().

Third,
I've thought about utilizing thread in MedaiThreadPool to reduce the overhead of creating/terminating thread.  Any thoughts ?

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8642922 [details] [diff] [review]
Add_MediaSinkShutdownManager_255041

Review of attachment 8642922 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaSinkShutdownManager.cpp
@@ +15,5 @@
> +MediaSinkShutdownManager::MediaSinkShutdownManager()
> +  : mRemainingTasks(0)
> +{
> +  MOZ_COUNT_CTOR(MediaSinkShutdownManager);
> +  nsresult rv = NS_NewNamedThread("MediaSinkMgr",

Why not using TaskQueue? SharedThreadPool::SpinUntilEmpty will consume all pending tasks during shutdown. So you don't need MediaSinkShutdownManager::Shutdown()

@@ +58,5 @@
> +  nsCOMPtr<nsIRunnable> r =
> +    new MediaSinkShutdownManager::MediaSinkShutdownTask(Move(event));
> +  DebugOnly<nsresult> rv =
> +    sInstance->mThread->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

You don't have to deal with the chore when using TaskQueue.

::: dom/media/MediaSinkShutdownManager.h
@@ +6,5 @@
> +
> +#if !defined(MediaSinkShutdownManager_h_)
> +#define MediaSinkShutdownManager_h_
> +
> +#include "mozilla/RefPtr.h"

Prefer nsRefPtr over RefPtr.

@@ +17,5 @@
> +class MediaSinkShutdownManager
> +{
> +public:
> +  static void Shutdown();
> +  static void DispatchShutdown(already_AddRefed<nsIRunnable>&& event);

Use a singleton instead of static methods. See MediaShutdownManager.

Comment 3

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Mass change P2 -> P3

Comment 4

[Alastor Wu [:alwu]]

This bug tries modify MediaSinkShutdownManager which is no longer existing in our codebase, so close this bug.