Remove some IsShutdown() checks from MediaDecoder

RESOLVED FIXED in Firefox 51

Status

()

Core
Audio/Video: Playback
P3
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

unspecified
mozilla51
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(10 attachments)

58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
58 bytes, text/x-review-board-request
kaku
: review+
Details | Review
(Assignee)

Description

a year ago
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

a year ago
Assignee: nobody → jwwang
Blocks: 1289288
Priority: -- → P3
(Assignee)

Comment 1

a year ago
Created attachment 8775436 [details]
Bug 1289976. Part 1 - Remove the IsShutdown() check from MediaDecoder::ConstructMediaTracks().

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

a year ago
Created attachment 8775437 [details]
Bug 1289976. Part 2 - Remove the IsShutdown() check from MediaDecoder::DumpDebugInfo() which happens before Shutdown().

Review commit: https://reviewboard.mozilla.org/r/67620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67620/
(Assignee)

Comment 3

a year ago
Created attachment 8775438 [details]
Bug 1289976. Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate().

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

a year ago
Created attachment 8775439 [details]
Bug 1289976. Part 4 - Remove the IsShutdown() check from MediaDecoder::NotifyOwnerActivityChanged() which happens before Shutdown().

Review commit: https://reviewboard.mozilla.org/r/67624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67624/
(Assignee)

Comment 5

a year ago
Created attachment 8775440 [details]
Bug 1289976. Part 5 - Remove the IsShutdown() check from MediaDecoder::Pause().

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

a year ago
Created attachment 8775441 [details]
Bug 1289976. Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks().

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

a year ago
Created attachment 8775442 [details]
Bug 1289976. Part 7 - Remove the IsShutdown() check from MediaDecoder::Seek().

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

a year ago
Created attachment 8775443 [details]
Bug 1289976. Part 8 - Remove the IsShutdown() check from UpdateReadyState(). The callback is disconnected by the watch manager in Shutdown().

Review commit: https://reviewboard.mozilla.org/r/67632/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67632/
(Assignee)

Comment 9

a year ago
Created attachment 8775444 [details]
Bug 1289976. Part 9 - Remove the IsShutdown() check from MediaDecoder::StartDormantTimer().

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

a year ago
Created attachment 8775445 [details]
Bug 1289976. Part 10 - Remove the IsShutdown() check from MediaDecoder::UpdateDormantState().

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 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 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

a year ago
Attachment #8775438 - Flags: review?(kaku) → review+
Comment on attachment 8775438 [details]
Bug 1289976. Part 3 - Remove the IsShutdown() check from MediaDecoder::FireTimeUpdate().

https://reviewboard.mozilla.org/r/67622/#review65012
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 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

a year ago
Attachment #8775441 - Flags: review?(kaku) → review+
Comment on attachment 8775441 [details]
Bug 1289976. Part 6 - Remove the IsShutdown() check from MediaDecoder::RemoveMediaTracks().

https://reviewboard.mozilla.org/r/67628/#review65018
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 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

a year ago
Attachment #8775444 - Flags: review?(kaku) → review+
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 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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

a year 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
backed out for causing assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=1198713&repo=autoland
Flags: needinfo?(jwwang)

Comment 34

a year 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
(Assignee)

Comment 35

a year ago
Need to fix bug 1290338 first.
Depends on: 1290338
(Assignee)

Updated

a year ago
Blocks: 1290780

Comment 36

a year 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

a year 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
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
(Assignee)

Updated

a year ago
Flags: needinfo?(jwwang)
You need to log in before you can comment on or make changes to this bug.