1581074 - Remove MediaStream blocking

Status: NEW

(Core :: Audio/Video: MediaStreamGraph, task, P3)

Description

[Andreas Pehrson [:pehrsons]]

With bug 1454998 a MediaStream (name might change) does not contain multiple tracks -- it is in fact just a single track itself.

Blocking is a remnant from a time when a MediaStream contained multiple tracks, to make sure those tracks stayed in sync.

We have since then moved A/V sync control to the producer of the data rather than letting it be a core functionality of the graph. Only MediaDecoder capture is using the functionality as all other sources are purely realtime.

This means that MediaStream blocking as a concept is no longer needed, and cleaning it up will simplify the MediaStreamGraph somewhat.

Comment 1

[Dan Minor [:dminor]]

Taking this one over.

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1581074 - Remove InputInterval; r=pehrsons!

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1581074 - Remove GraphTimeToTrackTimeWithBlocking; r=pehrsons!

Depends on D55025

Comment 4

[Dan Minor [:dminor]]

Attachment: Bug 1581074 - Make MoveToSegment guarantee that all tracks contain data up to the current time; r=pehrsons!

This also requires tracking time for which null data was appended, as this
should not be part of NotifyOutput.

Comment 5

[Dan Minor [:dminor]]

Attachment: Bug 1581074 - Remove mStartBlocking; r=pehrsons!

This removes most of the uses of mStartBlocking. It is still used to keep
track of time while suspended so that the clock does not advance on
suspended tracks. With these changes, mStartBlocking is renamed to
mStartSuspended;

Depends on D55027

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1581074 - Remove aFlags from ProcessInput; r=karlt!

Depends on D55451

Comment 7

[Dan Minor [:dminor]]

When I did a full try run [1] I ran into mochitest and web-platform test failures on some platforms. The mochitest failures appear to be caused by changes to ForwardedInputTrack and I have a patch which fixes the problem on one of the affected platforms. In the interest of saving CI jobs I wasn't going to test it more widely until I had a fix for the web-platform test.

The web-platform test (/worklets/audio-worklet-referrer.https.html) is pretty opaque to me. Bisecting the commits here, the culprit is [2], but I guess it could just be causing a bug in an earlier commit to manifest itself. It doesn't reproduce on any my systems here. I've spent a bit of time with some extra logging on a local system and that test doesn't seem to hit much of the code that changed with this series. Karl, if you can spare some time, I was wondering if you could have a quick look and see if you have any ideas of areas where I should be looking?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5507b7b90f11180abf1c6d0d48140f3cfba32cd3
[2] https://phabricator.services.mozilla.com/D55451

Comment 8

[Paul Adenot (:padenot)]

The WPT is using an OfflineAudioContext that is never started.

All the rest have little to do with the graph blocking or not blocking, but it needs an AudioWorklet with postMessage working. The graph thread needs to run somehow to execute the postMessage on the other end. Depending on the commit you're basing your patch series on, and the exact properties required to make this test pass this might or might not be working, this is very much in flux.

Also it looks like a bunch of other worklet tests (not related to audio) are failing in the same run, not sure if we're looking at a manifest issue (i.e. this would be excpected), here is a run from central: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=280191337&repo=mozilla-central&lineNumber=10871, where we see this test failing.

Comment 9

[Karl Tomlinson (:karlt)]

The test log has a JS stack.

openWindow/</window.onmessage@https://web-platform.test:8443/worklets/resources/referrer-tests.js:6:22
EventHandlerNonNull*openWindow/<@https://web-platform.test:8443/worklets/resources/referrer-tests.js:5:7
openWindow@https://web-platform.test:8443/worklets/resources/referrer-tests.js:2:10
runReferrerTest@https://web-platform.test:8443/worklets/resources/referrer-tests.js:25:10
runReferrerTests/<@https://web-platform.test:8443/worklets/resources/referrer-tests.js:176:12

The onmessage handler should not be calling an assert_ function without using test.step_function(). The simpler way, without an explicit step_function() is to resolve({win: win, e: e}) from the onmessage handler, and then assert_equals() in the onFulfilled parameter to then().

Doing that would identify which subtests are failing the assert. I'd then edit runReferrerTests() so that only a single test runs, to reduce noise.

Tests that import a script from a "page" are expected to pass. The tests that import a script from a "script" are expected to fail with the message "The operation was aborted."

I don't see where the Object is coming from. Perhaps some console.log()s sprinkled around the postMessage() calls in that file and in referrer-tests.js might provide some clues.

The test doesn't use AudioWorkletNode.port.postMessage() but window.postMessage(). The addModule() call requires a control message to successfully run on the MTG.

Is https://phabricator.services.mozilla.com/D55451 intended to merely remove an unused mStartBlocking variable? If its previous functionality is being replaced by something else in the same patch, then can you describe in the commit message, together with any other intended logic changes, please?

Comment 10

[Andreas Pehrson [:pehrsons]]

FWIW I've seen this test fail inte past in the same way, with expected (string) "LOADED" but got (object) object "[object Object]". That time the js object was a structured message from the harness saying the test had timed out. I had done something that made an offline graph not wake up as intended...

Comment 11

[Dan Minor [:dminor]]

Thank you for your suggestions everyone, I have a better idea of where to poke at things now. Try run off a rebase from yesterday morning here: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=280309209&revision=6d2b6dd4e8a836d9b2c5754099c2b1145d01008b shows the mochitest problem is fixed but the web-platform failure remains.

The previous functionality retained in https://phabricator.services.mozilla.com/D55451 is to handle updating mStartTime for suspended tracks without using the mStartBlocking member. I'll add a comment. The rest should just be a removal.

Comment 12

[Dan Minor [:dminor]]

As an update, this doesn't reproduce if I enable MediaTrackGraph logging. If I try to bisect by commenting out half of the testcases in the audio_worklet_referrer test, either half will pass by itself. So this seems like a timing problem to me. The only idea I have at the moment is to try adding printfs to the affected code to see if I can pinpoint a difference in failing runs without changing things so much that it no longer reproduces.

Comment 13

[Andreas Pehrson [:pehrsons]]

My only thought is whether there's an EnsureNextIteration missing for some case, that you've managed to expose. Around https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/dom/media/MediaTrackGraphImpl.h#614

Also note that I'm changing (moving, mostly) this logic a bit in bug 1586370 so you could take those patches and see whether they improve things. The ones currently on phabricator are probably not green enough, but if you reach out to me we can find better ones.

Comment 14

[Dan Minor [:dminor]]

I tried my stack on top of bug 1586370 and I'm still seeing the same problem. Sounds like the next step might be just adding some EnsureNextIterations here and there and seeing if that gets things moving for me.

Rather than rebase again, I'll plan to land this after bug 1586370 lands.

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:dminor, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 16

[Dan Minor [:dminor]]

I've rebased this a few times but haven't had time to spend on this lately [1]. The failure in /worklets/audio-worklet-referrer.https.html continues to be a problem.

[1] https://en.wikipedia.org/wiki/Sunk_cost#Concorde_effect

Comment 17

[Dan Minor [:dminor]]

I don't think I'll have time to look at this again any time soon.

Comment 18

[Dan Minor [:dminor]]

I'll try fighting the bot one last time.