Closed
Bug 1161402
Opened 9 years ago
Closed 9 years ago
assertions and thread-interaction clarification changes for WMFMediaDataDecoder
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: karlt, Assigned: karlt)
References
Details
Attachments
(10 files)
2.24 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.37 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
3.87 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.87 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.44 KB,
patch
|
Details | Diff | Splinter Review | |
3.65 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
1.31 KB,
patch
|
Details | Diff | Splinter Review | |
946 bytes,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
2.53 KB,
patch
|
cpearce
:
review+
|
Details | Diff | Splinter Review |
2.29 KB,
patch
|
ajones
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8601287 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•9 years ago
|
||
Async since bug 1039128.
Attachment #8601303 -
Flags: review?(cpearce)
Assignee | ||
Comment 3•9 years ago
|
||
This allows MediaDataDecoder methods to check whether they are on this task queue. It falls over a bit because SharedDecoderManager doesn't always have an active callback, but it is still useful often enough.
Attachment #8601307 -
Flags: review?(cpearce)
Assignee | ||
Comment 4•9 years ago
|
||
No assert is added for Init() and Shutdown() because SharedDecoderProxy does not set the MediaDataDecoderCallback in these methods.
Attachment #8601308 -
Flags: review?(cpearce)
Assignee | ||
Comment 5•9 years ago
|
||
I find this a little easier to understand as it is only Flush() waiting for the NotifyAll from Decode(), and no new decode tasks are dispatched during Flush(). It also saves a NotifyAll() in the common decode case. I don't mind if the existing code makes more sense in your mental model though.
Attachment #8601314 -
Flags: review?(cpearce)
Assignee | ||
Comment 6•9 years ago
|
||
Bug 1141241 suggests that this may be happening. Although mIsShutDown is accessed from the state machine thread in IsHardwareAccelerated() I haven't used an atomic or locking because the caller must use some barriers, etc. to ensure this is not called before shutdown.
Attachment #8601324 -
Flags: review?(cpearce)
Assignee | ||
Comment 7•9 years ago
|
||
Assuming Flush() has been called, its monitor Wait() provides the necessary memory consistency. If this hasn't happened, then I think the assert is more likely to notice the problem without the wait for a lock. Also removing the lock allows TSAN to detect any such problem.
Attachment #8601325 -
Flags: review?(cpearce)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8601331 -
Flags: review?(cpearce)
Assignee | ||
Comment 9•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce9d7faa07b8 includes an additional assert failing in Linux M3 that I'll come back to after filing a bug for the failure.
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8601314 [details] [diff] [review] remove unnecessary mIsDecodeTaskDispatched wait condition and notify only when mIsFlushing Removing review request from a couple of patches that may become irrelevant through some other changes I'm working on.
Attachment #8601314 -
Flags: review?(cpearce)
Assignee | ||
Updated•9 years ago
|
Attachment #8601325 -
Flags: review?(cpearce)
Updated•9 years ago
|
Attachment #8601287 -
Flags: review?(cpearce) → review+
Updated•9 years ago
|
Attachment #8601303 -
Flags: review?(cpearce) → review+
Updated•9 years ago
|
Attachment #8601307 -
Flags: review?(cpearce) → review+
Comment 11•9 years ago
|
||
Comment on attachment 8601308 [details] [diff] [review] assert that some public methods are called on reader task queue Review of attachment 8601308 [details] [diff] [review]: ----------------------------------------------------------------- We also call Init() and Shutdown() on the main thread in MP4Reader::IsVideoAccelerated(), which is called by the code that generates about:support's HTML. So the thread-use documentation in patch 1 probably should also note that.
Attachment #8601308 -
Flags: review?(cpearce) → review+
Updated•9 years ago
|
Attachment #8601324 -
Flags: review?(cpearce) → review+
Comment 12•9 years ago
|
||
Comment on attachment 8601324 [details] [diff] [review] assert that public methods are not called after shutdown Review of attachment 8601324 [details] [diff] [review]: ----------------------------------------------------------------- Are diagnostic asserts compiled in in release builds? i.e. are we going to crash users in the field? Or are you hoping to catch these and fix them in pre-release channels?
Updated•9 years ago
|
Attachment #8601331 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 13•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #12) > Are diagnostic asserts compiled in in release builds? No. When RELEASE_BUILD is set, it is just like MOZ_ASSERT. i.e. only in debug builds. https://hg.mozilla.org/mozilla-central/annotate/5907a8eca521/mfbt/Assertions.h#l298 > i.e. are we going to crash users in the field? > Or are you hoping to catch these and fix them in pre-release channels? It may cause crashes in nightly and aurora / dev edition (only).
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #11) > We also call Init() and Shutdown() on the main thread in > MP4Reader::IsVideoAccelerated(), which is called by the code that generates > about:support's HTML.
Attachment #8601913 -
Flags: review?(cpearce)
Assignee | ||
Comment 15•9 years ago
|
||
This is the assert noted as failing in comment 9.
Attachment #8601914 -
Flags: review?(ajones)
Updated•9 years ago
|
Attachment #8601913 -
Flags: review?(cpearce) → review+
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e7ffb4d019b https://hg.mozilla.org/integration/mozilla-inbound/rev/29a7ef7e5599 https://hg.mozilla.org/integration/mozilla-inbound/rev/471636f085f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d12bde97ccb https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3c121c456e https://hg.mozilla.org/integration/mozilla-inbound/rev/8f47810b0ec2 https://hg.mozilla.org/integration/mozilla-inbound/rev/321602a95beb
Updated•9 years ago
|
Attachment #8601914 -
Flags: review?(ajones) → review+
Comment 17•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2e7ffb4d019b https://hg.mozilla.org/mozilla-central/rev/29a7ef7e5599 https://hg.mozilla.org/mozilla-central/rev/471636f085f7 https://hg.mozilla.org/mozilla-central/rev/6d12bde97ccb https://hg.mozilla.org/mozilla-central/rev/bb3c121c456e https://hg.mozilla.org/mozilla-central/rev/8f47810b0ec2 https://hg.mozilla.org/mozilla-central/rev/321602a95beb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•