1062293 - Fix MediaStreamGraph shutdown sequence

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Paul Adenot (:padenot)]

Attachment: Add specialized logging to track the lifetime state change of MediaStreamGraphs r=

As noted in the comment, it is very useful to have this interleaved with the ADB
logcat on Android/b2g, because this way we can correlate gecko state with the
state of the Android HAL.

Comment 2

[Paul Adenot (:padenot)]

Attachment: Ensure the graph stays alive when doing driver switches and audio stream shutdown operations. r=

This basically gets a grip on the graph while doing driver switches operations
on another thread (system thread or audio thread), because those can take time.
Because the graph is refcounted, it'll be freed when the last operation
finishes, which is what we want.

This patch also only allows driver switching when the graph is in state
LIFECYCLE_RUNNING, which is what we want anyway.

Comment 3

[Paul Adenot (:padenot)]

Attachment: Fix opensl's cubeb backend draining. r=

This ensures the callback won't be called when draining is done.

I've seen a bunch of cases on slow, mono-core machines where we call the
playhead state handler for the drain right before the next callback after having
set `stm->draining = 1`, so we actually stop draining by resetting
`stm->draining` to 0 and keep calling the user-provided callback.

This caused a bunch of UAF / deadlocks on Android and B2G.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8483484 [details] [diff] [review]
Add specialized logging to track the lifetime state change of MediaStreamGraphs r=

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

::: content/media/GraphDriver.cpp
@@ +18,5 @@
>  #endif
>  
> +// We don't use NSPR log here because we want this interleaved with adb logcat
> +// on Android/B2G
> +#define ENABLE_LIFECYCLE_LOG

are we ever going to disable this?

::: content/media/MediaStreamGraph.cpp
@@ +44,5 @@
>  #else
>  #define STREAM_LOG(type, msg)
>  #endif
>  
> +#define ENABLE_LIFECYCLE_LOG

Are we ever going to disable this?  If so, file a bug.  If we don't want it in release builds, maybe something like
#if defined(DEBUG) || !defined(SOME_RELEASE_BUILD_DEFINE)

@@ +1576,5 @@
> +    "LIFECYCLE_THREAD_NOT_STARTED",
> +    "LIFECYCLE_RUNNING",
> +    "LIFECYCLE_WAITING_FOR_MAIN_THREAD_CLEANUP",
> +    "LIFECYCLE_WAITING_FOR_THREAD_SHUTDOWN",
> +    "LIFECYCLE_WAITING_FOR_STREAM_DESTRUCTION"

Add a note where the states are defined saying if you change it to update this.

Comment 5

[Paul Adenot (:padenot)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/030dfb5fa2af
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dcd375e2750d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c5249f8eec65

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/030dfb5fa2af
https://hg.mozilla.org/mozilla-central/rev/dcd375e2750d
https://hg.mozilla.org/mozilla-central/rev/c5249f8eec65

Comment 7

[Paul Adenot (:padenot)]

We need this on Aurora, as it fixes a bunch of intermittents

Comment 8

[Paul Adenot (:padenot)]

Comment on attachment 8483484 [details] [diff] [review]
Add specialized logging to track the lifetime state change of MediaStreamGraphs r=

Approval Request Comment
[Feature/regressing bug #]: MSG refactoring (bug 848954)
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: this  fixes a lot of intermittents
[Risks and why]: it's been on inbound and central for some days
[String/UUID change made/needed]: none

Comment 9

[Paul Adenot (:padenot)]

Comment on attachment 8483487 [details] [diff] [review]
Ensure the graph stays alive when doing driver switches and audio stream shutdown operations. r=

Approval Request Comment
[Feature/regressing bug #]: MSG refactoring (bug 848954)
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: this  fixes a lot of intermittents
[Risks and why]: it's been on inbound and central for some days
[String/UUID change made/needed]: none

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8483487 [details] [diff] [review]
Ensure the graph stays alive when doing driver switches and audio stream shutdown operations. r=

Approval Request Comment
[Feature/regressing bug #]: MSG refactoring (bug 848954)
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: this  fixes a lot of intermittents
[Risks and why]: it's been on inbound and central for some days
[String/UUID change made/needed]: none

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8483490 [details] [diff] [review]
Fix opensl's cubeb backend draining. r=

Approval Request Comment
[Feature/regressing bug #]: MSG refactoring (bug 848954)
[User impact if declined]: crashes
[Describe test coverage new/current, TBPL]: this  fixes a lot of intermittents
[Risks and why]: it's been on inbound and central for some days
[String/UUID change made/needed]: none

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

padenot put approval requests for the other two patches in here, but didn't set the bits on the patches

Comment 13

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8483484 [details] [diff] [review]
Add specialized logging to track the lifetime state change of MediaStreamGraphs r=

Aurora+

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/731305558f1c
https://hg.mozilla.org/releases/mozilla-aurora/rev/94140d5b6008
https://hg.mozilla.org/releases/mozilla-aurora/rev/2a6352464e21