Closed Bug 1135682 Opened 9 years ago Closed 9 years ago

Properly implement stream order invalidation in the MediaStreamGraph

Categories

(Core :: Web Audio, defect)

32 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: padenot, Assigned: baku)

Details

Attachments

(1 file)

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).
Attached patch patchSplinter Review
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: nobody → amarchesini
OS: Linux → All
Hardware: x86_64 → All
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+
https://hg.mozilla.org/mozilla-central/rev/c26d8b2d5edf
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.