Closed
Bug 1208289
Opened 7 years ago
Closed 7 years ago
[EME] Log outstanding frames in GMPVideoDecoder DrainComplete() and when ResetComplete() is late
Categories
(Core :: Audio/Video: MediaStreamGraph, defect, P2)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla44
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
3.55 KB,
patch
|
jwwang
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.07 KB,
patch
|
jwwang
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
Adobe are having trouble Drain()ing correctly, so logging the number of frames we have outstanding when DrainComplete() is called would help them. Also they sometimes don't call ResetComplete(), so adding a timer to detect this would help them too.
Assignee | ||
Comment 1•7 years ago
|
||
This is used in the next patch to detect if the GMP forgets to call GMPVideoDecoderCallback::ResetComplete().
Attachment #8665730 -
Flags: review?(jwwang)
Assignee | ||
Comment 2•7 years ago
|
||
* Log to browser console the number of outstanding frames when DrainComplete is called. * Log to browser console if GMPVideoDecoderCallback::ResetComplete() isn't called within 5 seconds of GMPVideoDecoder::Reset() being called.
Attachment #8665734 -
Flags: review?(jwwang)
Comment 3•7 years ago
|
||
Comment on attachment 8665730 [details] [diff] [review] Patch 1: Add SimpleTimer Review of attachment 8665730 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/VideoUtils.cpp @@ +345,5 @@ > > +void > +SimpleTimer::Cancel() { > + if (mTimer) { > + mTimer->Cancel(); It is not safe to cancel a timer on a thread other than the target thread. Add an assertion to ensure this function is not misused. @@ +360,5 @@ > + mTask = nullptr; > + } > + return NS_OK; > +} > + space. @@ +385,5 @@ > + rv = timer->InitWithCallback(this, aTimeoutMs, nsITimer::TYPE_ONE_SHOT); > + if (NS_FAILED(rv)) { > + return rv; > + } > + rv = timer->SetTarget(aTarget); This should be done before InitWithCallback in case the timer fires before we change the event target.
Attachment #8665730 -
Flags: review?(jwwang) → review+
Comment 4•7 years ago
|
||
Comment on attachment 8665734 [details] [diff] [review] Patch 2: Log outstanding frames in Drain() and when ResetComplete isn't called Review of attachment 8665734 [details] [diff] [review]: ----------------------------------------------------------------- r+ if my concern is addressed. ::: dom/media/gmp/GMPVideoDecoderParent.cpp @@ +190,5 @@ > + self->mResetCompleteTimeout = nullptr; > + LogToBrowserConsole(NS_LITERAL_STRING("GMPVideoDecoderParent timed out waiting for ResetComplete()")); > + }); > + CancelResetCompleteTimeout(); > + mResetCompleteTimeout = SimpleTimer::Create(task, 5000); SimpleTimer defaults to the main thread. It seems we should specify the GMP thread instead of the main thread.
Attachment #8665734 -
Flags: review?(jwwang) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc6762f52775 https://hg.mozilla.org/integration/mozilla-inbound/rev/28ede868e68c
Assignee | ||
Comment 9•7 years ago
|
||
Backed out, so don't close bug on merge of bad changesets.
Keywords: leave-open
Comment 11•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/967c555a595c https://hg.mozilla.org/integration/mozilla-inbound/rev/efcfe0c08c24
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 12•7 years ago
|
||
Chris, LogToBrowserConsole is defined in EMEUtils.cpp which isn't being compiled on B2G and others. I suggest to move it to dom/media/VideoUtils.cpp let me know if you want me to do that. It's still not best
Flags: needinfo?(cpearce)
Comment 13•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf3fa9171a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/0a449a9aa674
Comment 14•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bc6762f52775 https://hg.mozilla.org/mozilla-central/rev/28ede868e68c https://hg.mozilla.org/mozilla-central/rev/a0f4ddc23411 https://hg.mozilla.org/mozilla-central/rev/40eb9a8ebfec https://hg.mozilla.org/mozilla-central/rev/c1093088302a https://hg.mozilla.org/mozilla-central/rev/ce4c3fe31f57
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Assignee | ||
Comment 15•7 years ago
|
||
Backed out, as per comment 13. So reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3791ce8e5d7a
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/25677e506eef https://hg.mozilla.org/integration/mozilla-inbound/rev/37e7c82c0fc3
Updated•7 years ago
|
Flags: needinfo?(cpearce)
Comment 18•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/25677e506eef https://hg.mozilla.org/mozilla-central/rev/37e7c82c0fc3
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 19•7 years ago
|
||
Comment on attachment 8665730 [details] [diff] [review] Patch 1: Add SimpleTimer Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: This patch adds Browser Console logging to detect when the Adobe EME plugin is misbehaving. It's a prerequisite for Bug 1209385 which terminates misbehaving EME plugins. So without this patch, we can't land Bug 1209385, which will make Netflix more vulnerable to hung EME plugins. [Describe test coverage new/current, TreeHerder]: Lots of EME mochitests. [Risks and why]: Low [String/UUID change made/needed]: None.
Attachment #8665730 -
Flags: approval-mozilla-beta?
Attachment #8665730 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 20•7 years ago
|
||
Comment on attachment 8665734 [details] [diff] [review] Patch 2: Log outstanding frames in Drain() and when ResetComplete isn't called Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: This patch adds Browser Console logging to detect when the Adobe EME plugin is misbehaving. It's a prerequisite for Bug 1209385 which terminates misbehaving EME plugins. So without this patch, we can't land Bug 1209385, which will make Netflix more vulnerable to hung EME plugins. [Describe test coverage new/current, TreeHerder]: Lots of EME mochitests. [Risks and why]: Low [String/UUID change made/needed]: None.
Attachment #8665734 -
Flags: approval-mozilla-beta?
Attachment #8665734 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 21•7 years ago
|
||
Aurora Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d96adc84f158 Beta Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e657c22d317 Rebased patches can be found in these pushes.
Updated•7 years ago
|
status-firefox42:
--- → affected
status-firefox43:
--- → affected
Comment 22•7 years ago
|
||
Comment on attachment 8665730 [details] [diff] [review] Patch 1: Add SimpleTimer Improvement for EME diagnostics, taking it.
Attachment #8665730 -
Flags: approval-mozilla-beta?
Attachment #8665730 -
Flags: approval-mozilla-beta+
Attachment #8665730 -
Flags: approval-mozilla-aurora?
Attachment #8665730 -
Flags: approval-mozilla-aurora+
Updated•7 years ago
|
Attachment #8665734 -
Flags: approval-mozilla-beta?
Attachment #8665734 -
Flags: approval-mozilla-beta+
Attachment #8665734 -
Flags: approval-mozilla-aurora?
Attachment #8665734 -
Flags: approval-mozilla-aurora+
Comment 23•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/6b781dafff21 https://hg.mozilla.org/releases/mozilla-beta/rev/a79d0844eb82
Comment 24•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/6b781dafff21 https://hg.mozilla.org/releases/mozilla-beta/rev/a79d0844eb82
Comment 25•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/7c2733198a53 https://hg.mozilla.org/releases/mozilla-aurora/rev/95baa1b51926
You need to log in
before you can comment on or make changes to this bug.
Description
•