1144486 - Prototype state-watching machinery to reduce notification bugs in media code

Status: RESOLVED FIXED

Description

[Bobby Holley (:bholley)]

Another common class of media race condition occurs when a value gets updated, but that update doesn't get processed. For example, we've seen various situation where a piece of state gets updated in the state machine, but we neglect to trigger HTMLMediaElement::UpdateReadyStateForData. This leads to missing events, inconsistent data, etc - and often happens intermittently.

I have a rough strategy to handle this with explicitly-watchable state. To work properly we'll first need the state mirroring bug 1144481, so I'm going to do that first.

Comment 1

[Bobby Holley (:bholley)]

My prototype usage of this for ready state updating is mostly green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=550d7fe11ec9

As usual, there are some intermittent android/b2g failures. Trying to get to the bottom of them:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f62f2e09072
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e92a133a7684

Comment 2

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=af7acc59d947

This one looks finally green!

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Fix Bogus test. v1

cajbir said we should do this in the media meeting.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Implement direct tasks. v1

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement state watching machinery. v1

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 4 - Use state watching machinery for mReadyState updates. v1

Comment 7

[Bobby Holley (:bholley)]

(State mirroring stuff is coming in bug 1144481, just have one more failure to look at).

Comment 8

[Chris Pearce [:cpearce (GMT+13)]]

Comment on attachment 8594403 [details] [diff] [review]
Part 1 - Fix Bogus test. v1

Review of attachment 8594403 [details] [diff] [review]:
-----------------------------------------------------------------

I think I recall cajbir saying, but best to let him confirm.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8594405 [details] [diff] [review]
Part 3 - Implement state watching machinery. v1

Review of attachment 8594405 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/StateWatching.h
@@ +91,5 @@
> + * WatchTarget is a superclass from which all watchable things must inherit.
> + * Unlike AbstractWatcher, it is a fully-implemented Mix-in, and the subclass
> + * needs only to invoke NotifyWatchers when something changes.
> + */
> +class WatchTarget

Unlike MediaPromise, this class is not thread-safe. Need some comments to note the client code which is most likely our media code.

@@ +197,5 @@
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &Watcher::FireCallbacks);
> +    AbstractThread::GetCurrent()->TailDispatcher().AddDirectTask(r.forget());
> +
> +    // Notify dependent watchers.
> +    NotifyWatchers();

I am worried we have 2 inconsistent ways to notify the subscribers of Watcher. One is weak callback and the other is via AbstractWatcher::Notify. For example, if Watcher::Notify is called multiple times in an event cycle (as far as AbstractThread is concerned), the weak callback will fire only once however AbstractWatcher::Notify will be called multiple times. I would prefer a notification model that is consistent with that of MediaPromise where resove/reject will notify it consumers in an async way.

@@ +222,5 @@
> +    }
> +  }
> +
> +private:
> +  class AbstractMethodCall

Doesn't NS_NewNonOwningRunnableMethod serve the same purpose?

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8594405 [details] [diff] [review]
Part 3 - Implement state watching machinery. v1

Review of attachment 8594405 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/StateWatching.h
@@ +197,5 @@
> +    nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(this, &Watcher::FireCallbacks);
> +    AbstractThread::GetCurrent()->TailDispatcher().AddDirectTask(r.forget());
> +
> +    // Notify dependent watchers.
> +    NotifyWatchers();

Oops. AbstractWatcher::Notify won't be called multiple times for there is |mNotifying|. But it is called at a time different from the weak callback and might see different values of the watch target.

Comment 11

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #9)
> > +class WatchTarget
> 
> Unlike MediaPromise, this class is not thread-safe. Need some comments to
> note the client code which is most likely our media code.

Good point. I also noticed that I'd accidentally given AbstractWatcher threadsafe refcounting, which I've fixed.
> I am worried we have 2 inconsistent ways to notify the subscribers of
> Watcher. One is weak callback and the other is via AbstractWatcher::Notify.
> For example, if Watcher::Notify is called multiple times in an event cycle
> (as far as AbstractThread is concerned), the weak callback will fire only
> once however AbstractWatcher::Notify will be called multiple times. I would
> prefer a notification model that is consistent with that of MediaPromise
> where resove/reject will notify it consumers in an async way.


Well, the assumption in the code is that dependent watchers are all themselves Watchers, which means that they'll do the same AddDirectTask dance to invoke any actual callbacks that get run. But I guess it might be useful someday to implement another kind of AbstractWatcher implementation, at which point this distinction becomes more relevant. I'll move the NotifyWatchers call into the tail-dispatched method.

> Doesn't NS_NewNonOwningRunnableMethod serve the same purpose?

Very good point!

Comment 12

[Bobby Holley (:bholley)]

Attachment: Part 3 - Implement state watching machinery. v2

Updated to address jww's feedback.

Comment 13

[Bobby Holley (:bholley)]

Comment on attachment 8594404 [details] [diff] [review]
Part 2 - Implement direct tasks. v1

Chris says he's happy to delegate this to jww.

Comment 14

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8594404 [details] [diff] [review]
Part 2 - Implement direct tasks. v1

Review of attachment 8594404 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaStreamGraph.cpp
@@ +1823,5 @@
> +    // "Direct" tail dispatcher are supposed to run immediately following the
> +    // execution of the current task. So the meta-tasking that we do here in
> +    // RunInStableState() breaks that abstraction a bit unless we handle it here.
> +    //
> +    // This is particularly important because we can end up with a "stream

I don't quite understand this. Can you elaborate it?

@@ +1828,5 @@
> +    // ended" notification immediately following a "stream available" notification,
> +    // and we need to make sure that the watcher responding to "stream available"
> +    // has a chance to run before the second notification starts tearing things
> +    // down.
> +    tailDispatcher.DrainDirectTasks();

Isn't this supposed to be called outside the loop?

::: dom/media/TaskDispatcher.h
@@ +196,5 @@
>      return nullptr;
>    }
>  
> +  // Direct tasks.
> +  std::queue<nsCOMPtr<nsIRunnable>> mDirectTasks;

Why not nsTArray?

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8594745 [details] [diff] [review]
Part 3 - Implement state watching machinery. v2

Review of attachment 8594745 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/StateWatching.h
@@ +122,5 @@
> +protected:
> +  void NotifyWatchers()
> +  {
> +    WATCH_LOG("%s[%p] notifying watchers\n", mName, this);
> +    PruneWatchers();

I think we can be as lazy as only pruning watchers in NotifyWatchers() without doing that in {Add,Remove}Watcher.

@@ +138,5 @@
> +  {
> +    for (int i = 0; i < (int) mWatchers.Length(); ++i) {
> +      if (mWatchers[i]->IsDestroyed()) {
> +        mWatchers.RemoveElementAt(i);
> +        --i;

You can do reverse iteration to remove the elements with this outstanding decrement.

Comment 16

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #14)
 
> I don't quite understand this. Can you elaborate it?

See below.
 
> Isn't this supposed to be called outside the loop?

No. The issue is that the stream graph code has a meta runnable that executes a series of callbacks (such as "a stream was created" and "a stream has finished"). The current architecture requires that we respond to each update before processing the next one. On trunk, HTMLMediaElement::NotifyMediaStreamTracksAvailable invokes UpdateReadyStateForData, which pushes us past HAVE_METADATA and gets the ball rolling.

When we hoist update processing into direct tasks on the tail dispatcher, we run into trouble, because both "stream available" and "stream ended" run before we get around to invoking UpdateReadyStateForData. This causes us to get stuck in HAVE_METADATA (because HasVideo is false again). The existing src stream architecture is a bit shallow, but I just want to keep the invariants as the code expects.

> > +  // Direct tasks.
> > +  std::queue<nsCOMPtr<nsIRunnable>> mDirectTasks;
> 
> Why not nsTArray?

Because nsTArray has pessimal performance when popping from the front. This is the same reason that MediaTaskQueue uses std::queue.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Bobby Holley (:bholley) from comment #16)
> No. The issue is that the stream graph code has a meta runnable that
> executes a series of callbacks (such as "a stream was created" and "a stream
> has finished"). The current architecture requires that we respond to each
> update before processing the next one. On trunk,
> HTMLMediaElement::NotifyMediaStreamTracksAvailable invokes
> UpdateReadyStateForData, which pushes us past HAVE_METADATA and gets the
> ball rolling.
> 
> When we hoist update processing into direct tasks on the tail dispatcher, we
> run into trouble, because both "stream available" and "stream ended" run
> before we get around to invoking UpdateReadyStateForData. This causes us to
> get stuck in HAVE_METADATA (because HasVideo is false again). The existing
> src stream architecture is a bit shallow, but I just want to keep the
> invariants as the code expects.

I see. But it is still suspicious to drain all direct tasks in the 1st iteration of the loop. The direct listeners waiting for "stream ended" will miss it since they are all consumed in the 1st iteration right after "stream available" is processed, right?

Comment 18

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #17)
> (In reply to Bobby Holley (:bholley) from comment #16)
> > No. The issue is that the stream graph code has a meta runnable that
> > executes a series of callbacks (such as "a stream was created" and "a stream
> > has finished"). The current architecture requires that we respond to each
> > update before processing the next one. On trunk,
> > HTMLMediaElement::NotifyMediaStreamTracksAvailable invokes
> > UpdateReadyStateForData, which pushes us past HAVE_METADATA and gets the
> > ball rolling.
> > 
> > When we hoist update processing into direct tasks on the tail dispatcher, we
> > run into trouble, because both "stream available" and "stream ended" run
> > before we get around to invoking UpdateReadyStateForData. This causes us to
> > get stuck in HAVE_METADATA (because HasVideo is false again). The existing
> > src stream architecture is a bit shallow, but I just want to keep the
> > invariants as the code expects.
> 
> I see. But it is still suspicious to drain all direct tasks in the 1st
> iteration of the loop.

It's not the first iteration, but rather _every_ iteration. Each iteration/sub-task has the potential to add direct tasks to the tail dispatcher. The idea here is to scope the execution of those direct tasks more tightly to the code that generated them.

> The direct listeners waiting for "stream ended"

I'm not sure what you mean by "direct listeners".

> will miss it

Miss what?

> since they are all consumed in the 1st iteration right after "stream
> available" is processed, right?

What is consumed? Direct tasks?

Basically, the point of direct tasks is that they're supposed to be run at the point we fuzzily define as "when the callstack unwinds". When HTMLMediaElement::NotifyMediaStreamTracksAvailable changes a watched variable, we want to recompute our ready state immediately after NotifyMediaStreamTracksAvailable returns, not after an arbitrary number of other tasks from MediaStreamGraphImpl::RunInStableState run, since those other tasks may change things out from under us.

Comment 19

[JW Wang [:jwwang] [:jw_wang]]

I see. I didn't consider direct tasks could be generated during each iteration. For example, "stream available" will generate a direct task to recompute ready state as well as "stream ended".

Comment 20

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8594404 [details] [diff] [review]
Part 2 - Implement direct tasks. v1

Review of attachment 8594404 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation.

Comment 21

[Bobby Holley (:bholley)]

Attachment: Part 2 Adendum - Drain direct tasks in TaskGroupRunnable.

Thanks for the reviews! Our discussion on the above made me realize that this
is probably a good idea too.

Comment 22

[Bobby Holley (:bholley)]

Attachment: Part 0 - Don't spin the event loop by synchronously dispatching when we're already on the main thread. v1

This was causing us to re-enter RunSyncSectionsInternal and therefore FireTailDispatcher.

Comment 23

[Bobby Holley (:bholley)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbd6305e508

Comment 24

[Bobby Holley (:bholley)]

Comment on attachment 8596246 [details] [diff] [review]
Part 0 - Don't spin the event loop by synchronously dispatching when we're already on the main thread. v1

I guess it makes more sense to do this as part of bug 1157488, since it's necessary to make those assertions green.

Comment 25

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #23)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbd6305e508

Looks green. Double-checking with a full try push, then we can get this landed!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0e2e841a1e0

Comment 26

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5226e698dd5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/323d1ad78755
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/667eb19c63ad
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7ec784680bdd

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5226e698dd5f
https://hg.mozilla.org/mozilla-central/rev/323d1ad78755
https://hg.mozilla.org/mozilla-central/rev/667eb19c63ad
https://hg.mozilla.org/mozilla-central/rev/7ec784680bdd