Last Comment Bug 779721 - Simplify MediaStreamGraph
: Simplify MediaStreamGraph
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla17
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
Depends on: 790854
Blocks: 779715
  Show dependency treegraph
 
Reported: 2012-08-01 21:02 PDT by Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
Modified: 2012-11-02 00:45 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: simplify invariants around when messages are processed (29.96 KB, patch)
2012-08-01 21:08 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 2: don't block for no consumers (1.19 KB, patch)
2012-08-01 21:10 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 3: don't shut down MediaStreams on the MediaStreamGraph thread during forced shutdown (2.96 KB, patch)
2012-08-01 21:14 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 4: Fix lock ordering inversion when running control messages during a forced shutdown (3.03 KB, patch)
2012-08-01 21:17 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 5: Add a comment explaining more about how MediaStream lifetimes are managed (2.08 KB, patch)
2012-08-01 21:19 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review
Part 6: Check that Destroy is the last message sent from the main thread for a stream (5.33 KB, patch)
2012-08-02 20:07 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
rjesup: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:02:21 PDT
This is in preparation for bug 779715.
Comment 1 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:08:24 PDT
Created attachment 648210 [details] [diff] [review]
Part 1: simplify invariants around when messages are processed
Comment 2 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:10:48 PDT
Created attachment 648211 [details] [diff] [review]
Part 2: don't block for no consumers
Comment 3 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:14:17 PDT
Created attachment 648212 [details] [diff] [review]
Part 3: don't shut down MediaStreams on the MediaStreamGraph thread during forced shutdown
Comment 4 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:17:33 PDT
Created attachment 648213 [details] [diff] [review]
Part 4: Fix lock ordering inversion when running control messages during a forced shutdown
Comment 5 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-01 21:19:04 PDT
Created attachment 648214 [details] [diff] [review]
Part 5: Add a comment explaining more about how MediaStream lifetimes are managed
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-08-02 20:07:20 PDT
Created attachment 648597 [details] [diff] [review]
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] 2012-08-08 12:54:06 PDT
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] 2012-08-08 13:02:59 PDT
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] 2012-08-08 13:04:54 PDT
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

Note You need to log in before you can comment on or make changes to this bug.