Closed Bug 1161402 Opened 5 years ago Closed 5 years ago

assertions and thread-interaction clarification changes for WMFMediaDataDecoder

Categories

(Core :: Audio/Video, defect)

defect
Not set

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.
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)
No assert is added for Init() and Shutdown() because SharedDecoderProxy does
not set the MediaDataDecoderCallback in these methods.
Attachment #8601308 - Flags: review?(cpearce)
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)
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)
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)
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.
Blocks: 1129455
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)
Attachment #8601325 - Flags: review?(cpearce)
Attachment #8601287 - Flags: review?(cpearce) → review+
Attachment #8601303 - Flags: review?(cpearce) → review+
Attachment #8601307 - Flags: review?(cpearce) → review+
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+
Attachment #8601324 - Flags: review?(cpearce) → review+
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?
Attachment #8601331 - Flags: review?(cpearce) → review+
(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).
(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)
This is the assert noted as failing in comment 9.
Attachment #8601914 - Flags: review?(ajones)
Attachment #8601913 - Flags: review?(cpearce) → review+
Attachment #8601914 - Flags: review?(ajones) → review+
You need to log in before you can comment on or make changes to this bug.