779721 - Simplify MediaStreamGraph

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is in preparation for bug 779715.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: simplify invariants around when messages are processed

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: don't block for no consumers

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: don't shut down MediaStreams on the MediaStreamGraph thread during forced shutdown

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 4: Fix lock ordering inversion when running control messages during a forced shutdown

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 5: Add a comment explaining more about how MediaStream lifetimes are managed

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 6: Check that Destroy is the last message sent from the main thread for a stream

The IsDestroyed() getter will come in handy later for future patches, too.

Comment 7

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

Comment on attachment 648210 [details] [diff] [review]
Part 1: simplify invariants around when messages are processed

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

::: content/media/MediaStreamGraph.cpp
@@ +895,3 @@
>      LOG(PR_LOG_DEBUG, ("Media graph %p computed blocking for interval %f to %f",
> +                       this, MediaTimeToSeconds(mStateComputedTime),
> +                       MediaTimeToSeconds(end)));                       

trailing whitespace

Comment 8

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

Comment on attachment 648214 [details] [diff] [review]
Part 5: Add a comment explaining more about how MediaStream lifetimes are managed

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

I like the lifetime description!  Everything should have one... :-)

Comment 9

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

Comment on attachment 648597 [details] [diff] [review]
Part 6: Check that Destroy is the last message sent from the main thread for a stream

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

Looks good to me - I debugged through one these quite a bit; this makes it easier to try to flag and trigger on

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51de5c54e500
https://hg.mozilla.org/integration/mozilla-inbound/rev/28860e96d78d
https://hg.mozilla.org/integration/mozilla-inbound/rev/cecd42ed7943
https://hg.mozilla.org/integration/mozilla-inbound/rev/409c0d581dcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdf37b6a641a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e9222b7cfcc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51de5c54e500
https://hg.mozilla.org/mozilla-central/rev/28860e96d78d
https://hg.mozilla.org/mozilla-central/rev/cecd42ed7943
https://hg.mozilla.org/mozilla-central/rev/409c0d581dcd
https://hg.mozilla.org/mozilla-central/rev/cdf37b6a641a
https://hg.mozilla.org/mozilla-central/rev/2e9222b7cfcc