Closed
Bug 1289976
Opened 8 years ago
Closed 8 years ago
Remove some IsShutdown() checks from MediaDecoder
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla51
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(10 files)
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
kaku
:
review+
|
Details |
After fixing bug 1289649, we ensure HTMLMediaElement will never hold a MediaDecoder that is shutting down. This allows us to remove some checks for IsShutdown().
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
1. ConstructMediaTracks() is called from ChangeState() when |mPlayState == PLAY_STATE_PLAYING|. 2. ConstructMediaTracks() is called from MetadataLoaded() which asserts |!IsShutdown()|. Review commit: https://reviewboard.mozilla.org/r/67618/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67618/
Attachment #8775436 -
Flags: review?(kaku)
Attachment #8775437 -
Flags: review?(kaku)
Attachment #8775438 -
Flags: review?(kaku)
Attachment #8775439 -
Flags: review?(kaku)
Attachment #8775440 -
Flags: review?(kaku)
Attachment #8775441 -
Flags: review?(kaku)
Attachment #8775442 -
Flags: review?(kaku)
Attachment #8775443 -
Flags: review?(kaku)
Attachment #8775444 -
Flags: review?(kaku)
Attachment #8775445 -
Flags: review?(kaku)
Assignee | ||
Comment 2•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67620/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67620/
Assignee | ||
Comment 3•8 years ago
|
||
FireTimeUpdate() is only called from UpdateLogicalPositionInternal() which returns early when IsShutdown() is true. Review commit: https://reviewboard.mozilla.org/r/67622/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67622/
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67624/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67624/
Assignee | ||
Comment 5•8 years ago
|
||
1. Pause() is called from HTMLMediaElement and happens before Shutdown(). 2. Pause() is called from SetPlaybackRate() which is called from HTMLMediaElement. Review commit: https://reviewboard.mozilla.org/r/67626/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67626/
Assignee | ||
Comment 6•8 years ago
|
||
1. It is called from ChangeState() when IsEnded() is true. 2. It is called from OnMetadataUpdate(). The callback is disconnected in Shutdown(). Review commit: https://reviewboard.mozilla.org/r/67628/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67628/
Assignee | ||
Comment 7•8 years ago
|
||
1. It is called from DurationChanged() which returns early when IsShutdown() is true. 2. It is called from Play() when IsEnded() is true. Review commit: https://reviewboard.mozilla.org/r/67630/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67630/
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/67632/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67632/
Assignee | ||
Comment 9•8 years ago
|
||
We don't need to check IsShutdown() which is a subset of |mPlayState != PLAY_STATE_PAUSED && !IsEnded()|. Review commit: https://reviewboard.mozilla.org/r/67634/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67634/
Assignee | ||
Comment 10•8 years ago
|
||
1. It is called from DormantTimerExpired(). The timer is canceled in Shutdown(). 2. It is called from NotifyOwnerActivityChanged() which happens before Shutdown(). 3. It is called from Play() which happens before Shutdown(). 4. It is called from Seek() which happens before Shutdown(). Review commit: https://reviewboard.mozilla.org/r/67636/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/67636/
Comment 11•8 years ago
|
||
Comment on attachment 8775436 [details] Bug 1289976. Part 1 - Remove the IsShutdown() check from MediaDecoder::ConstructMediaTracks(). https://reviewboard.mozilla.org/r/67618/#review65008
Attachment #8775436 -
Flags: review?(kaku) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8775437 [details] Bug 1289976. Part 2 - Remove the IsShutdown() check from MediaDecoder::DumpDebugInfo() which happens before Shutdown(). https://reviewboard.mozilla.org/r/67620/#review65010
Attachment #8775437 -
Flags: review?(kaku) → review+
Updated•8 years ago
|
Attachment #8775438 -
Flags: review?(kaku) → review+
Comment 13•8 years ago
|
||
Comment on attachment 8775438 [details] Bug 1289976. Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate(). https://reviewboard.mozilla.org/r/67622/#review65012
Comment 14•8 years ago
|
||
Comment on attachment 8775439 [details] Bug 1289976. Part 4 - Remove the IsShutdown() check from MediaDecoder::NotifyOwnerActivityChanged() which happens before Shutdown(). https://reviewboard.mozilla.org/r/67624/#review65014
Attachment #8775439 -
Flags: review?(kaku) → review+
Comment 15•8 years ago
|
||
Comment on attachment 8775440 [details] Bug 1289976. Part 5 - Remove the IsShutdown() check from MediaDecoder::Pause(). https://reviewboard.mozilla.org/r/67626/#review65016
Attachment #8775440 -
Flags: review?(kaku) → review+
Updated•8 years ago
|
Attachment #8775441 -
Flags: review?(kaku) → review+
Comment 16•8 years ago
|
||
Comment on attachment 8775441 [details] Bug 1289976. Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks(). https://reviewboard.mozilla.org/r/67628/#review65018
Comment 17•8 years ago
|
||
Comment on attachment 8775442 [details] Bug 1289976. Part 7 - Remove the IsShutdown() check from MediaDecoder::Seek(). https://reviewboard.mozilla.org/r/67630/#review65020
Attachment #8775442 -
Flags: review?(kaku) → review+
Comment 18•8 years ago
|
||
Comment on attachment 8775443 [details] Bug 1289976. Part 8 - Remove the IsShutdown() check from UpdateReadyState(). The callback is disconnected by the watch manager in Shutdown(). https://reviewboard.mozilla.org/r/67632/#review65000 ::: dom/media/MediaDecoder.h (Diff revision 1) > } > > void UpdateReadyState() > { > MOZ_ASSERT(NS_IsMainThread()); > - if (!IsShutdown()) { Add |MOZ_ASSERT(!IsShutdown());| here?
Attachment #8775443 -
Flags: review?(kaku) → review+
Updated•8 years ago
|
Attachment #8775444 -
Flags: review?(kaku) → review+
Comment 19•8 years ago
|
||
Comment on attachment 8775444 [details] Bug 1289976. Part 9 - Remove the IsShutdown() check from MediaDecoder::StartDormantTimer(). https://reviewboard.mozilla.org/r/67634/#review65004 Not sure why shutdown is a subset of |not pause && not end|, but MediaDecoder::StartDormantTimer() is only called by MediaDecoder::NotifyOwnerActivityChanged() and MediaDecoder::ChangeState() which are both assert to be |!IsShoutdown()|. ::: dom/media/MediaDecoder.cpp (Diff revision 1) > > void > MediaDecoder::StartDormantTimer() > { > MOZ_ASSERT(NS_IsMainThread()); > - if (!IsHeuristicDormantSupported()) { Add |MOZ_ASSERT(!IsShutdown());| here?
Comment 20•8 years ago
|
||
Comment on attachment 8775445 [details] Bug 1289976. Part 10 - Remove the IsShutdown() check from MediaDecoder::UpdateDormantState(). https://reviewboard.mozilla.org/r/67636/#review65022
Attachment #8775445 -
Flags: review?(kaku) → review+
Assignee | ||
Comment 21•8 years ago
|
||
https://reviewboard.mozilla.org/r/67634/#review65004 Let a denote IsShutdown() and b denote |not paused && not ended|. If a is false, |if (a || b)| can be reduced to |if (b)|. if a is true, b will also be true. |if (a || b)| is equivalent to |if (b)|. Therefore, |if (a || b)| can always be reduced to |if (b)|. > Add |MOZ_ASSERT(!IsShutdown());| here? We can't. ChangeState(PLAY_STATE_SHUTDOWN) will change mPlayState to PLAY_STATE_SHUTDOWN and then call StartDormantTimer(). IsShutdown() will be true inside StartDormantTimer().
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8775436 [details] Bug 1289976. Part 1 - Remove the IsShutdown() check from MediaDecoder::ConstructMediaTracks(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67618/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8775437 [details] Bug 1289976. Part 2 - Remove the IsShutdown() check from MediaDecoder::DumpDebugInfo() which happens before Shutdown(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67620/diff/1-2/
Assignee | ||
Comment 24•8 years ago
|
||
Comment on attachment 8775438 [details] Bug 1289976. Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67622/diff/1-2/
Assignee | ||
Comment 25•8 years ago
|
||
Comment on attachment 8775439 [details] Bug 1289976. Part 4 - Remove the IsShutdown() check from MediaDecoder::NotifyOwnerActivityChanged() which happens before Shutdown(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67624/diff/1-2/
Assignee | ||
Comment 26•8 years ago
|
||
Comment on attachment 8775440 [details] Bug 1289976. Part 5 - Remove the IsShutdown() check from MediaDecoder::Pause(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67626/diff/1-2/
Assignee | ||
Comment 27•8 years ago
|
||
Comment on attachment 8775441 [details] Bug 1289976. Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67628/diff/1-2/
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8775442 [details] Bug 1289976. Part 7 - Remove the IsShutdown() check from MediaDecoder::Seek(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67630/diff/1-2/
Assignee | ||
Comment 29•8 years ago
|
||
Comment on attachment 8775443 [details] Bug 1289976. Part 8 - Remove the IsShutdown() check from UpdateReadyState(). The callback is disconnected by the watch manager in Shutdown(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67632/diff/1-2/
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8775444 [details] Bug 1289976. Part 9 - Remove the IsShutdown() check from MediaDecoder::StartDormantTimer(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67634/diff/1-2/
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8775445 [details] Bug 1289976. Part 10 - Remove the IsShutdown() check from MediaDecoder::UpdateDormantState(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67636/diff/1-2/
Comment 32•8 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9966bbe6b534 Part 1 - Remove the IsShutdown() check from MediaDecoder::ConstructMediaTracks(). r=kaku https://hg.mozilla.org/integration/autoland/rev/2cb402f74a51 Part 2 - Remove the IsShutdown() check from MediaDecoder::DumpDebugInfo() which happens before Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/7368d06f677b Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate(). r=kaku https://hg.mozilla.org/integration/autoland/rev/93a556b5e244 Part 4 - Remove the IsShutdown() check from MediaDecoder::NotifyOwnerActivityChanged() which happens before Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/bac5f65e6931 Part 5 - Remove the IsShutdown() check from MediaDecoder::Pause(). r=kaku https://hg.mozilla.org/integration/autoland/rev/ce6b4f84c86e Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks(). r=kaku https://hg.mozilla.org/integration/autoland/rev/cd25be005e38 Part 7 - Remove the IsShutdown() check from MediaDecoder::Seek(). r=kaku https://hg.mozilla.org/integration/autoland/rev/f648be2df9ec Part 8 - Remove the IsShutdown() check from UpdateReadyState(). The callback is disconnected by the watch manager in Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/2931db85cc99 Part 9 - Remove the IsShutdown() check from MediaDecoder::StartDormantTimer(). r=kaku https://hg.mozilla.org/integration/autoland/rev/ba01eb85da0a Part 10 - Remove the IsShutdown() check from MediaDecoder::UpdateDormantState(). r=kaku
Comment 33•8 years ago
|
||
backed out for causing assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1198713&repo=autoland
Flags: needinfo?(jwwang)
Comment 34•8 years ago
|
||
Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/75f47838f78d Backed out changeset ba01eb85da0a https://hg.mozilla.org/integration/autoland/rev/99165d03ee19 Backed out changeset 2931db85cc99 https://hg.mozilla.org/integration/autoland/rev/aced69398af2 Backed out changeset f648be2df9ec https://hg.mozilla.org/integration/autoland/rev/3bc17ae80dc8 Backed out changeset cd25be005e38 https://hg.mozilla.org/integration/autoland/rev/055698b4f81d Backed out changeset ce6b4f84c86e https://hg.mozilla.org/integration/autoland/rev/98c45193a812 Backed out changeset bac5f65e6931 https://hg.mozilla.org/integration/autoland/rev/7b54c336bdf0 Backed out changeset 93a556b5e244 https://hg.mozilla.org/integration/autoland/rev/1cc9be4bf034 Backed out changeset 7368d06f677b https://hg.mozilla.org/integration/autoland/rev/1ea93130cc67 Backed out changeset 2cb402f74a51 https://hg.mozilla.org/integration/autoland/rev/5469a910b4a1 Backed out changeset 9966bbe6b534 for assertion failures in MediaDecoder.h
Comment 36•8 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1fa73e3d834d Part 1 - Remove the IsShutdown() check from MediaDecoder::ConstructMediaTracks(). r=kaku https://hg.mozilla.org/integration/autoland/rev/5e333fc27567 Part 2 - Remove the IsShutdown() check from MediaDecoder::DumpDebugInfo() which happens before Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/af2250994011 Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate(). r=kaku https://hg.mozilla.org/integration/autoland/rev/37e8b9029975 Part 4 - Remove the IsShutdown() check from MediaDecoder::NotifyOwnerActivityChanged() which happens before Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/912087c585ee Part 5 - Remove the IsShutdown() check from MediaDecoder::Pause(). r=kaku https://hg.mozilla.org/integration/autoland/rev/88bb8e3f4749 Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks(). r=kaku https://hg.mozilla.org/integration/autoland/rev/7ec72a79735d Part 7 - Remove the IsShutdown() check from MediaDecoder::Seek(). r=kaku https://hg.mozilla.org/integration/autoland/rev/54df7081706d Part 8 - Remove the IsShutdown() check from UpdateReadyState(). The callback is disconnected by the watch manager in Shutdown(). r=kaku https://hg.mozilla.org/integration/autoland/rev/a9f48e09d979 Part 9 - Remove the IsShutdown() check from MediaDecoder::StartDormantTimer(). r=kaku https://hg.mozilla.org/integration/autoland/rev/728ec5d19e75 Part 10 - Remove the IsShutdown() check from MediaDecoder::UpdateDormantState(). r=kaku
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1fa73e3d834d https://hg.mozilla.org/mozilla-central/rev/5e333fc27567 https://hg.mozilla.org/mozilla-central/rev/af2250994011 https://hg.mozilla.org/mozilla-central/rev/37e8b9029975 https://hg.mozilla.org/mozilla-central/rev/912087c585ee https://hg.mozilla.org/mozilla-central/rev/88bb8e3f4749 https://hg.mozilla.org/mozilla-central/rev/7ec72a79735d https://hg.mozilla.org/mozilla-central/rev/54df7081706d https://hg.mozilla.org/mozilla-central/rev/a9f48e09d979 https://hg.mozilla.org/mozilla-central/rev/728ec5d19e75
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(jwwang)
You need to log in
before you can comment on or make changes to this bug.
Description
•