Closed Bug 1157797 Opened 10 years ago Closed 10 years ago

Remove unnecessary calls to UpdateNextFrameStatus

Categories

(Core :: Audio/Video, defect)

Product:
Core
Component:
Audio/Video
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla40
Tracking Flags:
Tracking Status
firefox40 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files, 1 obsolete file)

Part 1 - Be more disciplined about where we call UpdateNextFrameStatus. v1
(deleted), patch
jwwang
: review+
Details | Diff | Splinter Review
Part 2 - Use state-watching machinery for UpdateNextFrameStatus. v1
(deleted), patch
jwwang
: review+
Details | Diff | Splinter Review
Followup to bug 1144481 comment 8.
Splitting part of this out into bug 1158916.
Summary: Remove unnecessary calls to mReadyStateWatchTarget->Notify() and UpdateNextFrameStatus → Remove unnecessary calls to UpdateNextFrameStatus
If we've got our state graph set up properly, the watch target should be notified automatically whenever anything relevant changes.
Attachment #8598225 - Flags: review?(jwwang)
This value depends on mState and the audio/video queue states. Given that, we can just stick these calls in a few choke points.
Attachment #8598280 - Flags: review?(jwwang)
The ergonomics here aren't ideal. I'm going to file a bug to improve them.
Attachment #8598281 - Flags: review?(jwwang)
Comment on attachment 8598225 [details] [diff] [review] Stop manually notifying MediaDecoder::mReadyStateWatchTarget. v1 This should be moved to the other bug, sorry.
Attachment #8598225 - Attachment is obsolete: true
Attachment #8598225 - Flags: review?(jwwang)
Attachment #8598280 - Flags: review?(jwwang) → review+
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.
Attachment #8598281 - Flags: review?(jwwang) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/d57204624a8b https://hg.mozilla.org/mozilla-central/rev/f7e444c42499
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: