Closed Bug 1208289 Opened 4 years ago Closed 4 years ago

[EME] Log outstanding frames in GMPVideoDecoder DrainComplete() and when ResetComplete() is late

Categories

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Product:
Core
Component:
Audio/Video: MediaStreamGraph
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla44
Tracking Flags:
Tracking Status
firefox42 --- fixed
firefox43 --- fixed
firefox44 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Patch 1: Add SimpleTimer
3.55 KB, patch
jwwang
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Patch 2: Log outstanding frames in Drain() and when ResetComplete isn't called
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.
This is used in the next patch to detect if the GMP forgets to call GMPVideoDecoderCallback::ResetComplete().
Attachment #8665730 - Flags: review?(jwwang)
* 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 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 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+
Backed out, so don't close bug on merge of bad changesets.
Keywords: leave-open
Keywords: leave-open
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)
Backed out, as per comment 13. So reopening.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(cpearce)
https://hg.mozilla.org/mozilla-central/rev/25677e506eef
https://hg.mozilla.org/mozilla-central/rev/37e7c82c0fc3
Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
Resolution: --- → FIXED
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?
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?
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+
Attachment #8665734 - Flags: approval-mozilla-beta?
Attachment #8665734 - Flags: approval-mozilla-beta+
Attachment #8665734 - Flags: approval-mozilla-aurora?
Attachment #8665734 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.