Closed
Bug 1135682
Opened 9 years ago
Closed 9 years ago
Properly implement stream order invalidation in the MediaStreamGraph
Categories
(Core :: Web Audio, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: padenot, Assigned: baku)
Details
Attachments
(1 file)
2.98 KB,
patch
|
padenot
:
review+
|
Details | Diff | Splinter Review |
Currently, we recompute the order in which MediaStreams in the MediaStreamGraph are processed at each graph iteration. This involves a (sort-of) topological sort (MediaStreamGraphImpl::UpdateStreamOrder), is expensive, and useless to compute when the actual topology of the graph is not changing: no new streams (= vertices), no new connections (= edges), etc. There was a attempt a while ago at keeping a "needs reordering" flag on the MSG, called mStreamOrderDirty (set to false right after an ordering, set to true when something modifies the graph, and guarding the MediaStreamGraphImpl::UpdateStreamOrder method so it's called only when needed). I apparently failed to land the chunk of the patch that turns the value back to false (right after ordering the streams), so the patch was useless. Of course, since then, try is not green anymore when I re-add this chunk. This bug is about re-adding the `mStreamsOrderDirty = false;` chunk at the bottom of UpdateStreamOrder, running the tests, and adding more `SetStreamOrderDirty()` calls at the locations where the topology of the graph is modified (basically, creation/destruction/connection/disconnection of MediaStreams).
Assignee | ||
Comment 1•9 years ago
|
||
Actually, it's more a feedback request than a review. This works. I pushed it to try to be 100% sure but it's fully green on my laptop. Probably we can optimize this: if (!audioTrackPresent && CurrentDriver()->AsAudioCallbackDriver()) { in order to avoid to loop between all the streams.
Attachment #8568085 -
Flags: review?(padenot)
Assignee | ||
Comment 2•9 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ad6243a4faa8
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → amarchesini
OS: Linux → All
Hardware: x86_64 → All
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8568085 [details] [diff] [review] patch Review of attachment 8568085 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. I'm paranoid, so I ran a try push with more platforms, but it's green. I remember the issues I had with this patch were showing up only on some platforms. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b691ca5d921a ::: dom/media/MediaStreamGraph.cpp @@ +545,5 @@ > > if (!audioTrackPresent && > CurrentDriver()->AsAudioCallbackDriver()) { > + MonitorAutoLock mon(mMonitor); > + if (CurrentDriver()->AsAudioCallbackDriver()->IsStarted()) { I think I had a reason to do it this weird way, but I can't remember what it was.
Attachment #8568085 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 4•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c26d8b2d5edf
https://hg.mozilla.org/mozilla-central/rev/c26d8b2d5edf
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•