1157797 - Remove unnecessary calls to UpdateNextFrameStatus

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Followup to bug 1144481 comment 8.

Comment 1

[Bobby Holley (:bholley)]

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

Comment 2

[Bobby Holley (:bholley)]

Splitting part of this out into bug 1158916.

Comment 3

[Bobby Holley (:bholley)]

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

Comment 4

[Bobby Holley (:bholley)]

Attachment: Stop manually notifying MediaDecoder::mReadyStateWatchTarget. v1

If we've got our state graph set up properly, the watch target should be
notified automatically whenever anything relevant changes.

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 1 - Be more disciplined about where we call UpdateNextFrameStatus. v1

This value depends on mState and the audio/video queue states. Given that, we can
just stick these calls in a few choke points.

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 2 - Use state-watching machinery for UpdateNextFrameStatus. v1

The ergonomics here aren't ideal. I'm going to file a bug to improve them.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8598225 [details] [diff] [review]
Stop manually notifying MediaDecoder::mReadyStateWatchTarget. v1

This should be moved to the other bug, sorry.

Comment 8

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

Comment on attachment 8598281 [details] [diff] [review]
Part 2 - Use state-watching machinery for UpdateNextFrameStatus. v1

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +259,5 @@
>                          "MediaDecoderStateMachine::mNextFrameStatus (Canonical)");
>  
> +  // Skip the initial notification we get when we Watch the value, since we're
> +  // not on the right thread yet.
> +  mNextFrameStatusUpdater->Watch(mState, /* aSkipInitialNotify = */ true);

Can we do this inside Watch() which will skip or delay the 1st notification if not on the right thread yet? I would like to free the callsite from worrying about the internal thread model of our watch mechanics.

Comment 9

[Bobby Holley (:bholley)]

(In reply to JW Wang [:jwwang] from comment #8)
> Comment on attachment 8598281 [details] [diff] [review]
> Part 2 - Use state-watching machinery for UpdateNextFrameStatus. v1
> 
> Review of attachment 8598281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +259,5 @@
> >                          "MediaDecoderStateMachine::mNextFrameStatus (Canonical)");
> >  
> > +  // Skip the initial notification we get when we Watch the value, since we're
> > +  // not on the right thread yet.
> > +  mNextFrameStatusUpdater->Watch(mState, /* aSkipInitialNotify = */ true);
> 
> Can we do this inside Watch() which will skip or delay the 1st notification
> if not on the right thread yet? I would like to free the callsite from
> worrying about the internal thread model of our watch mechanics.

The problem is that watchers/watchables don't have any way of knowing what their owner thread is. I'm going to combine these things into some sort of manager and try to clean this up in another bug.

Comment 10

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d57204624a8b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7e444c42499

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d57204624a8b
https://hg.mozilla.org/mozilla-central/rev/f7e444c42499