Closed Bug 1993981 Opened 10 months ago Closed 14 days ago

Make MediaTrackGraph a tail-dispatchable(-only) nsISerialEventTarget

Categories

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

task

Tracking

()

RESOLVED FIXED
156 Branch
Tracking Status
firefox156 --- wontfix
firefox157 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

(Blocks 2 open bugs, Regressed 2 open bugs)

Details

Attachments

(12 files, 1 obsolete file)

48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review
48 bytes, text/x-phabricator-request
Details | Review

The idea is to scrap the current front-/back-buffers of messages used for dispatch to main thread stable state, and instead use (mainly) common code (see TaskDispatcher.h, nsIDirectTaskDispatcher, nsITailDispatcher (bug 1645785) or AbstractThread). This enables higher-order stuff like MozPromise, MediaEvents, StateMirroring for use with the graph.

See Also: → 1785619

This patch makes MediaTrackGraphImpl implement AbstractThread and its base
interface nsISerialEventTarget.

This makes the backing store for ControlMessages change to nsIRunnable.
To facilitate runnables that can optionally run during shutdown, a new
convention is introduced: An nsICancelableRunnable that ignores cancellation
will run during shutdown. The dispatch of an nsIRunnable that does not implement
nsICancelableRunnable will result in the adding of a wrapper that
releases the nsIRunnable on cancellation.

MessageBlocks are removed, as the graph no longer will manage the batching of
tasks dispatched from main thread. The main thread's tail dispatcher will manage
batching instead, and dispatched per-thread group runnables will enter
mBackMessageQueue regularly. The same mechanism now works from any thread. Only
main thread dispatches affect the graph's lifecycle, however.

For a non-main thread to dispatch tasks to the graph, it will have to be sure to
keep the graph alive. The only guarantee for this is still to hold onto a live
track.

Any thread that is used to dispatch a task to the graph must support tail
dispatch. While not strictly required for the function of the graph, this
enforces the old mechanism of running messages from main thread atomically, and
spreads that behavior to any other thread. It also reduces contention on the
graph's Monitor.

Dispatching from the graph to any other thread should also use tail dispatch for
performance reasons -- e.g dispatching to the busy main thread may suffer from
lock contention which is not ideal on the real-time audio thread. However,
enforcing tail dispatch when dispatching from the graph thread is not possible
without going through a proxy function. This is by convention for now.

This patch does two things to support AudioWorklet:

  • Supports target shutdown tasks.
  • Exempts the IPC I/O Child thread from the tail dispatch requirement.

The IPC I/O Child thread does not support tail dispatch, and there are no
ordering requirements between those messages and any from other threads.

Prior to this patch, these messages went through GraphRunner::mThread. It was
given to the MessageChannel opened in the AudioWorklet through
GetCurrentSerialEventTarget().

For MediaTrackGraph to be able to switch to its manual batching of control
messages to tail dispatch, the message atomicity guarantee it provides needs to
be retained. This patch introduces an atomicity mode for tail dispatch to
support the graph.

This test is racy. The contestants are:

  • The canvas capturing the first frame and adding it to the graph track on the
    main thread.
  • VideoOutput handling the enabled state change and inventing a black frame for
    rendering on the graph thread.

With this patch series, dispatches from main to the graph thread now use tail
dispatch, which is slightly different from the previous behavior of queueing up
runnables for dispatch during stable state. This means the timing
characteristics of when such dispatches happen has changed, which affects the
timing of when the runnables run in the graph.

Exact expected behavior in this case is still being discussed on the spec,
see e.g. https://github.com/w3c/mediacapture-main/issues/1011.

The expected outcome of this test has changed

Attachment #9534200 - Attachment description: Bug 1993981 - Allow canceling cancelable tail-dispatched runnables. r?#xpcom-reviewers → Bug 1993981 - Allow canceling cancelable and discardable tail-dispatched runnables. r?#xpcom-reviewers
Attachment #9534207 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Pushed by pehrsons@gmail.com: https://github.com/mozilla-firefox/firefox/commit/68a2b11b5fc8 https://hg.mozilla.org/integration/autoland/rev/c1c655a55b5c Store ControlMessages as runnables. r=karlt https://github.com/mozilla-firefox/firefox/commit/05da49140e79 https://hg.mozilla.org/integration/autoland/rev/dd1d0a9ea81f Implement nsIDirectTaskDispatcher for MediaTrackGraphImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/5c0ae53343b5 https://hg.mozilla.org/integration/autoland/rev/b7413f2349f7 Allow canceling cancelable and discardable tail-dispatched runnables. r=xpcom-reviewers,nika https://github.com/mozilla-firefox/firefox/commit/186f4c8a3c90 https://hg.mozilla.org/integration/autoland/rev/bb4b18c692e6 Rename MediaTrackGraphImpl::Dispatch to DispatchToMainThread to avoid naming conflicts with nsISerialEventTarget. r=karlt https://github.com/mozilla-firefox/firefox/commit/7cdca42e6da4 https://hg.mozilla.org/integration/autoland/rev/36a2e5538e1b Implement AbstractThread for MediaTrackGraphImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/7365f2d5bb28 https://hg.mozilla.org/integration/autoland/rev/f3fce358e2ba In MediaTrackGraphImpl natively support AudioWorklet's message passing. r=karlt https://github.com/mozilla-firefox/firefox/commit/165e177d7aeb https://hg.mozilla.org/integration/autoland/rev/6707668d3fac To MockProcessedMediaTrack add mocks for {Add|Remove}ListenerImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/864197925faf https://hg.mozilla.org/integration/autoland/rev/694f5770d973 Unit test event ordering between the graph and main thread. r=karlt https://github.com/mozilla-firefox/firefox/commit/edde7ff9c219 https://hg.mozilla.org/integration/autoland/rev/0a7814a68ff3 Unit test message atomicity. r=karlt https://github.com/mozilla-firefox/firefox/commit/3eba0a27139e https://hg.mozilla.org/integration/autoland/rev/48574c987980 Introduce an atomicity policy for tail dispatch. r=karlt,xpcom-reviewers,nika,webrtc-reviewers,bwc,media-playback-reviewers https://github.com/mozilla-firefox/firefox/commit/1a7e168f54f6 https://hg.mozilla.org/integration/autoland/rev/d7ba75a4c1ef Unit test graph shutdown message ordering. r=karlt https://github.com/mozilla-firefox/firefox/commit/86b579e21cdd https://hg.mozilla.org/integration/autoland/rev/6d2e13f1a339 Rename MediaTrackGraphImpl::LifecycleStateRef() -> LifecycleState() and return state by value. r=karlt
Pushed by nfay@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/78d889f1dc36 https://hg.mozilla.org/integration/autoland/rev/2d0da34c9aba Revert "Bug 1993981 - Rename MediaTrackGraphImpl::LifecycleStateRef() -> LifecycleState() and return state by value. r=karlt" for causing bustage @ AbstractThread.h and Gtest failures @ StateWatching.h

Backed out for causing bustage @ AbstractThread.h and Gtest failures @ StateWatching.h

Backout link

Push with failures

Failure log - build
Failure log - Gtest

Flags: needinfo?(apehrson)
Flags: needinfo?(apehrson)
Pushed by pehrsons@gmail.com: https://github.com/mozilla-firefox/firefox/commit/001889747710 https://hg.mozilla.org/integration/autoland/rev/0074d8c37b03 Store ControlMessages as runnables. r=karlt https://github.com/mozilla-firefox/firefox/commit/73bbfc2af3db https://hg.mozilla.org/integration/autoland/rev/07992a96a786 Implement nsIDirectTaskDispatcher for MediaTrackGraphImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/b6c1a23845f4 https://hg.mozilla.org/integration/autoland/rev/057cda252e8b Allow canceling cancelable and discardable tail-dispatched runnables. r=xpcom-reviewers,nika https://github.com/mozilla-firefox/firefox/commit/b104d50d22f0 https://hg.mozilla.org/integration/autoland/rev/7668d22f06ad Rename MediaTrackGraphImpl::Dispatch to DispatchToMainThread to avoid naming conflicts with nsISerialEventTarget. r=karlt https://github.com/mozilla-firefox/firefox/commit/4cdd700b6db0 https://hg.mozilla.org/integration/autoland/rev/e7ad1bf72931 Implement AbstractThread for MediaTrackGraphImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/e85a4933447c https://hg.mozilla.org/integration/autoland/rev/1b7fbc53f0e0 In MediaTrackGraphImpl natively support AudioWorklet's message passing. r=karlt https://github.com/mozilla-firefox/firefox/commit/926ffa5b4954 https://hg.mozilla.org/integration/autoland/rev/af7418e64820 To MockProcessedMediaTrack add mocks for {Add|Remove}ListenerImpl. r=karlt https://github.com/mozilla-firefox/firefox/commit/534803c0da0f https://hg.mozilla.org/integration/autoland/rev/7aaca6bbf089 Unit test event ordering between the graph and main thread. r=karlt https://github.com/mozilla-firefox/firefox/commit/d8fe367d2f2f https://hg.mozilla.org/integration/autoland/rev/16047aca2a0b Unit test message atomicity. r=karlt https://github.com/mozilla-firefox/firefox/commit/4bfa0a6de22b https://hg.mozilla.org/integration/autoland/rev/b6637fb55859 Introduce an atomicity policy for tail dispatch. r=karlt,xpcom-reviewers,nika,webrtc-reviewers,bwc,media-playback-reviewers https://github.com/mozilla-firefox/firefox/commit/a9e6367bff12 https://hg.mozilla.org/integration/autoland/rev/6b7d59a2dc94 Unit test graph shutdown message ordering. r=karlt https://github.com/mozilla-firefox/firefox/commit/62b91c19bd49 https://hg.mozilla.org/integration/autoland/rev/81f70526e4dc Rename MediaTrackGraphImpl::LifecycleStateRef() -> LifecycleState() and return state by value. r=karlt
Regressions: 2064436
No longer depends on: 1645785
Regressions: 2065072
Regressions: 2065075
Blocks: 2066381
Pushed by dmeehan@mozilla.com: https://github.com/mozilla-firefox/firefox/commit/c4528230e80e https://hg.mozilla.org/releases/mozilla-beta/rev/2d3ab4109347 Revert "Bug 1993981 - Rename MediaTrackGraphImpl::LifecycleStateRef() -> LifecycleState() and return state by value. r=karlt" from beta due to multiple regressions
QA Whiteboard: [qa-triage-done-c157/b156]
You need to log in before you can comment on or make changes to this bug.