Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Description • 9 years ago

From bug 1235968 comment 14: rework the shutdown code for MediaStreamGraph -- much like for MediaManager (using AsyncShutdown).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 1 • 9 years ago

Review commit: https://reviewboard.mozilla.org/r/31889/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31889/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 2 • 9 years ago

Note that I couldn’t block past the final LIFECYCLE_WAITING_FOR_STREAM_DESTRUCTION stage, since completion of that stage seems to require shutdown to proceed.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 3 • 9 years ago

Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31889/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 4 • 9 years ago

Rebase. try seems to have crapped up?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 5 • 9 years ago

Try is *almost* perma-orange. More to do.

	Randell Jesup [:jesup] (needinfo me)
	Comment 6 • 9 years ago

Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

https://reviewboard.mozilla.org/r/31889/#review28685

Overall looks good; a few questions and one real (I think) issue

::: dom/media/MediaStreamGraph.cpp:1356
(Diff revision 2)
> +      gMediaStreamGraphShutdownBlocker = nullptr;

So I'm wondering... I think MediaStreamGraphShutDownRunnables exist for *each* extant graph, so don't we want to remove the blocker only when all of the ones we marked for mForceShutdown have actually shut down, not just one of them?

::: dom/media/MediaStreamGraph.cpp:1359
(Diff revision 2)
> +      // stage, since completion of that stage requires shutdown to proceed.

Elaborate on this (I presume other things not yet shut down are holding streams still)

::: dom/media/MediaStreamGraph.cpp:2728
(Diff revision 2)
> +        {

Should we assert MainThread here?  Or is it well-guaranteed for ShutdownBlocker?  (Likely we don't need to assert it.)

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 7 • 9 years ago

https://reviewboard.mozilla.org/r/31889/#review28703

::: dom/media/MediaStreamGraph.cpp:1356
(Diff revision 2)
> +      gMediaStreamGraphShutdownBlocker = nullptr;

Yes, and I also need to handle when there are zero open graphs at shutdown (i.e. remove blocker), which seems to be what's biting me on try.

Tip: you can drag-highlight several lines in mozReview. ;)

::: dom/media/MediaStreamGraph.cpp:1359
(Diff revision 2)
> +      // stage, since completion of that stage requires shutdown to proceed.

I presume so as well. Here's what I observed in an earlier version where I held the blocker all the way till MediaStreamGraph::Destroy():

> 2016-01-22 03:34:14.810041 UTC - [0x116873070]: D/MediaStreamGraph MediaStreamGraph 1731e6500 ForceShutdown
> 2016-01-22 03:34:14.810212 UTC - [0x17526fb30]: V/MediaStreamGraph MediaStream 172f44d00 bufferStartTime=0.043537 blockedTime=0.031927
> 2016-01-22 03:34:14.810240 UTC - [0x17526fb30]: D/MediaStreamGraph MediaStreamGraph 1731e6500 waiting for main thread cleanup
> 2016-01-22 03:34:14.843263 UTC - [0x116873070]: D/MediaStreamGraph Stopping threads for MediaStreamGraph 172de11c0
> 2016-01-22 03:34:22.345408 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.345432 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.345444 UTC - [0x116873070]: D/MediaStreamGraph Removing media stream 0 from the graph
> 2016-01-22 03:34:22.347174 UTC - [0x116873070]: D/MediaStreamGraph MediaStreamGraph 1731e6500 destroyed

I believe the ~8 idle seconds after "Stopping threads for MediaStreamGraph" is the AsyncShutdown blocker timing out after 10 seconds. Streams seem held up by other stuff.

::: dom/media/MediaStreamGraph.cpp:2728
(Diff revision 2)
> +        {

Seems well-guaranteed by API.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 8 • 9 years ago

Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31889/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 9 • 9 years ago

Would ideally like another pass to clean this up a bit, but seems green on try, so I'm putting it up.

	Randell Jesup [:jesup] (needinfo me)
	Comment 10 • 9 years ago

Comment on attachment 8710865 [details]
MozReview Request: Bug 1239873 - Use AsyncShutdown API to shut down MediaStreamGraph thread.

https://reviewboard.mozilla.org/r/31889/#review28799

::: dom/media/MediaStreamGraph.cpp:1455
(Diff revisions 2 - 3)
> -    }
> +    // which requires shutdown to proceed.

Should we re-block at a later stage?  This might be tricky, since some streams might continue to exist until the final CC pass, perhaps - which is after xpcom-shutdown-threads.  We have shut down the driver threads, however, so this should be ok.

::: dom/media/MediaStreamGraph.cpp:2834
(Diff revisions 2 - 3)
> -            MediaStreamGraphImpl* graph = iter.UserData();
> +            iter.UserData()->ForceShutDown(ticket);

This looks good!

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 11 • 9 years ago

(In reply to Randell Jesup [:jesup] from comment #10)
> Should we re-block at a later stage?

That's definitely something we could look at as a followup, though I suspect you're right that we'd need to wait until final GC, which doesn't seem doable.

Try looks green and almost complete, I also have a green try from yesterday.

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f31612f8f3bb

	Phil Ringnalda (:philor)
	Comment 13 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f31612f8f3bb

	Randell Jesup [:jesup] (needinfo me)
	Comment 16 • 8 years ago

MozReview-Commit-ID: J39kgU76VwL

	Randell Jesup [:jesup] (needinfo me)
	Comment 17 • 8 years ago

Comment on attachment 8755520 [details] [diff] [review]
Backout f31612f8f3bb () from beta 47

wrong bug...

	Paul Adenot (:padenot)
	Comment 18 • 8 years ago

We've made progress on this (two patches, fixing a lot of intermittents and
probably shutdown crashes in the wild as well), but we're not quite ready yet,
best to back it out again.