7,900 instances of "CDM returned NoKeyErr" emitted from dom/media/gmp/GMPCDMProxy.cpp during linux64 debug testing

RESOLVED FIXED in Firefox 54

Status

()

Core
Audio/Video: GMP
P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: erahm, Assigned: jay.harris)

Tracking

(Blocks: 1 bug)

Trunk
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

> 7949 WARNING: CDM returned NoKeyErr: file dom/media/gmp/GMPCDMProxy.cpp, line 747

This warning [1] shows up in the following test suites:

>   5291 - desktop-test-linux64/debug-mochitest-media mda
>   2658 - desktop-test-linux64/debug-mochitest-media-e10s mda

It shows up in 2 tests. A few of the most prevalent:

>   5291 -        dom/media/test/test_eme_non_mse_fails.html
>   2658 - [e10s] dom/media/test/test_eme_non_mse_fails.html

[1] https://hg.mozilla.org/mozilla-central/annotate/02876b8cf2b0/dom/media/gmp/GMPCDMProxy.cpp#l747
Rank: 25
Priority: -- → P2
This one seems intermittent, but when it shows up it's by far the #1 most verbose warning during testing. It's hard to bisect as the m-media test isn't run as often on inbound and it only shows up in one test.

edwin and cpearce it looks like you were both involved in adding the test, any idea what's going on here?
Flags: needinfo?(edwin)
Flags: needinfo?(cpearce)
Hmm.

This should only occur if a sample is reaching the CDM. ISTR adding a check to prevent this from happening in the non-fragmented case.

Those checks are gone now; we just need to re-add a guard before calls to SetCDMProxy in HTMLMediaElement to make sure that mMediaSource exists.

Although, I'm assuming that we know at that point whether we have fragmented media -- is that still the case?
Flags: needinfo?(edwin)
This continues to be the most verbose warning during testing.

(In reply to Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin] from comment #2)
> Hmm.
> 
> This should only occur if a sample is reaching the CDM. ISTR adding a check
> to prevent this from happening in the non-fragmented case.
> 
> Those checks are gone now; we just need to re-add a guard before calls to
> SetCDMProxy in HTMLMediaElement to make sure that mMediaSource exists.
> 
> Although, I'm assuming that we know at that point whether we have fragmented
> media -- is that still the case?

I'm not sure who this question is directed to. cpearce, Edwin no longer appears to be active, do you have any ideas?
Flags: needinfo?(cpearce)
Flags: needinfo?(cpearce)
I've been working on other things.

I suspect what's happening here is that because we only stop the load (when we detect EME being used in non-MSE content) after we've reached HTMLMediaElement::MetadataLoaded(), we'll have a CDMProxy attached, and have started the decode pipeline. We should be not trying to decode samples for which we don't have a key for (the SamplesWaitingForKey class manages that). So I don't know why we are trying (and subsequently failing) to decode samples; we should be assuming all keys are not usable and thus not trying to decode. For some reason we must be thinking the keys with which the samples are encrypted are usable, and trying to decode.

Jay: Do you want to look into this? Bryce may be able to mentor you; we did some work around our "waitingforkey" code, which is close to this area.
Flags: needinfo?(cpearce) → needinfo?(jharris)
(Assignee)

Comment 5

a year ago
So it looks like this issue is due to the way we deal with encryption on unfragmented videos. We only parse CENC data from the video if the video is fragmented (`Index::Index#243`) which means that when we check to see if we need to wait for a key (`SamplesWaitingForKey::WaitIfKeyNotUsable#31`), `aSample->mCrypto.mValid` is false, so we don't wait. Speaking with Chris, we have two possible solutions:

1. To read the CENC information on unfragmented videos, so our existing guards know not to play the video
2. Detect encryption in non-fragmented videos and fail more pro-actively (we don't support it at all). This might involve detecting encryption atoms in the parser and reporting a failure.

Chris has suggested that the second option is probably the better of the two.
Flags: needinfo?(jharris)
(Assignee)

Updated

a year ago
Assignee: nobody → jharris
Comment hidden (mozreview-request)
(Assignee)

Comment 7

a year ago
As we're going to be replacing the parser in the near future, I've changed the NS_WARNING to an EME_LOG, so you shouldn't see the warnings unless you enable the log. This should naturally be fixed with the new parser.

Comment 8

a year ago
mozreview-review
Comment on attachment 8832222 [details]
Bug 1316650 - Changes the NS_WARNING to an EME_LOG

https://reviewboard.mozilla.org/r/108574/#review109728
Attachment #8832222 - Flags: review?(cpearce) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 9

a year ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29cf209dcba4
Changes the NS_WARNING to an EME_LOG r=cpearce
Keywords: checkin-needed

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/29cf209dcba4
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.