Closed
Bug 1289004
Opened 7 years ago
Closed 7 years ago
Remove MediaDecoder::mShuttingDown
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
Details
Attachments
(3 files)
It is confusing to have both mShuttingDown and PLAY_STATE_SHUTDOWN. We should remove mShuttingDown and use |mPlayState == PLAY_STATE_SHUTDOWN| instead.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jwwang
Priority: -- → P3
Assignee | ||
Comment 1•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66756/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66756/
Attachment #8774214 -
Flags: review?(cpearce)
Attachment #8774215 -
Flags: review?(cpearce)
Attachment #8774216 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66758/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66758/
Assignee | ||
Comment 3•7 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/66760/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/66760/
Comment 4•7 years ago
|
||
Comment on attachment 8774214 [details] Bug 1289004. Part 1 - Constify and devirtualize some functions. . https://reviewboard.mozilla.org/r/66756/#review63536
Attachment #8774214 -
Flags: review?(cpearce) → review+
Comment 5•7 years ago
|
||
Comment on attachment 8774215 [details] Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown(). https://reviewboard.mozilla.org/r/66758/#review63538
Attachment #8774215 -
Flags: review?(cpearce) → review+
Comment 6•7 years ago
|
||
Comment on attachment 8774216 [details] Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown. https://reviewboard.mozilla.org/r/66760/#review63540
Attachment #8774216 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 7•7 years ago
|
||
https://hg.mozilla.org/try/rev/f1671931bae24f1345a43eac38d4a5b82e9abde2#l1.105 This assertion fails in the mochitest. https://treeherder.mozilla.org/logviewer.html#?job_id=24443345&repo=try#L32512 stack: XUL!mozilla::MediaDecoder::ChangeState(mozilla::MediaDecoder::PlayState) [MediaDecoder.cpp:4bf5f73310db : 1355 + 0x0] XUL!mozilla::dom::HTMLMediaElement::Pause(mozilla::ErrorResult&) [HTMLMediaElement.cpp:4bf5f73310db : 1935 + 0x9] XUL!<name omitted> [HTMLMediaElement.cpp:4bf5f73310db : 1955 + 0x8] XUL!mozilla::dom::HTMLMediaElement::UnbindFromTree(bool, bool) [HTMLMediaElement.cpp:4bf5f73310db : 3144 + 0xc] XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6] XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10] XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6] XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10] XUL!mozilla::dom::Element::UnbindFromTree(bool, bool) [Element.cpp:4bf5f73310db : 1861 + 0x6] XUL!nsGenericHTMLElement::UnbindFromTree(bool, bool) [nsGenericHTMLElement.cpp:4bf5f73310db : 583 + 0x10] XUL!mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) [HTMLSharedElement.cpp:4bf5f73310db : 312 + 0xf] XUL!nsDocument::cycleCollection::Unlink(void*) [nsDocument.cpp:4bf5f73310db : 1955 + 0x6] XUL!nsHTMLDocument::cycleCollection::Unlink(void*) [nsHTMLDocument.cpp:4bf5f73310db : 189 + 0xb] XUL!nsCycleCollector::CollectWhite() [nsCycleCollector.cpp:4bf5f73310db : 3343 + 0x3] XUL!nsCycleCollector::Collect(ccType, js::SliceBudget&, nsICycleCollectorListener*, bool) [nsCycleCollector.cpp:4bf5f73310db : 3689 + 0x8] XUL!nsCycleCollector::ShutdownCollect() [nsCycleCollector.cpp:4bf5f73310db : 3607 + 0x15] XUL!nsCycleCollector_shutdown() [nsCycleCollector.cpp:4bf5f73310db : 4217 + 0x5] This is because HTMLMediaElement::Pause() calls MediaDecoder::Pause() which calls ChangeState() after MediaShutdownManager::BlockShutdown() happens. I will work around this problem by early return from MediaDecoder::Pause() when IsShutdown() is true. However, we should fix this problem since HTMLMediaElement should not call into MediaDecoder functions once Shutdown() begins. Checking IsShutdown() everywhere makes the code fragile and logic unclear.
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8774214 [details] Bug 1289004. Part 1 - Constify and devirtualize some functions. . Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66756/diff/1-2/
Assignee | ||
Comment 9•7 years ago
|
||
Comment on attachment 8774215 [details] Bug 1289004. Part 2 - Add MediaDecoder::IsShutdown(). Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66758/diff/1-2/
Assignee | ||
Comment 10•7 years ago
|
||
Comment on attachment 8774216 [details] Bug 1289004. Part 3 - Remove MediaDecoder::mShuttingDown. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66760/diff/1-2/
Comment 11•7 years ago
|
||
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/48c409dc9e93 Part 1 - Constify and devirtualize some functions. r=cpearce. https://hg.mozilla.org/integration/autoland/rev/412d21e8fddc Part 2 - Add MediaDecoder::IsShutdown(). r=cpearce https://hg.mozilla.org/integration/autoland/rev/6bb275091417 Part 3 - Remove MediaDecoder::mShuttingDown. r=cpearce
Comment 12•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/48c409dc9e93 https://hg.mozilla.org/mozilla-central/rev/412d21e8fddc https://hg.mozilla.org/mozilla-central/rev/6bb275091417
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•