1161402 - assertions and thread-interaction clarification changes for WMFMediaDataDecoder

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: document that MediaDataDecoder::IsHardwareAccelerated() is called from the state machine task queue

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: remove async MediaDataDecoder::Drain() from the list of blocking examples

Async since bug 1039128.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: add MediaDataDecoderCallback::OnReaderTaskQueue()

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.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: assert that some public methods are called on reader task queue

No assert is added for Init() and Shutdown() because SharedDecoderProxy does
not set the MediaDataDecoderCallback in these methods.

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: remove unnecessary mIsDecodeTaskDispatched wait condition and notify only when mIsFlushing

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.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: assert that public methods are not called after shutdown

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.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: remove debug-only locking around assert that should be true even without the locking

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.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: assert there is no mActiveProxy during SharedDecoderManager::Shutdown() r?

Comment 9

[Karl Tomlinson (:karlt)]

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.

Comment 10

[Karl Tomlinson (:karlt)]

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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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?

Comment 13

[Karl Tomlinson (:karlt)]

(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).

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: more thread doc for MediaDataDecoder use from IsVideoAccelerated()

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

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: assert active proxy if callback is called

This is the assert noted as failing in comment 9.

Comment 16

[Pulsebot]

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

Comment 17

[Carsten Book [:Tomcat]]

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

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18d25adcfce8

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/18d25adcfce8