Closed
Bug 1147689
Opened 9 years ago
Closed 9 years ago
[EME] Pass Session ID into EME Decode+Decrypt CDMs
Categories
(Core :: Audio/Video, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: eflores, Assigned: eflores)
Details
Attachments
(3 files)
12.21 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
6.38 KB,
patch
|
cpearce
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8583503 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8583504 -
Flags: review?(cpearce)
Assignee | ||
Comment 3•9 years ago
|
||
still working on v6 compat patch.
Comment 4•9 years ago
|
||
Comment on attachment 8583503 [details] [diff] [review] 1147689-gmp-encrypted-sessions.patch Review of attachment 8583503 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/fmp4/MP4Reader.cpp @@ +752,5 @@ > + if (sample && sample->mMp4Sample && sample->mMp4Sample->crypto.valid) { > + CryptoSample& crypto = sample->mMp4Sample->crypto; > + MOZ_ASSERT(crypto.session_ids.IsEmpty()); > + > + nsRefPtr<CDMProxy> proxy = mDecoder->GetCDMProxy(); You should hold the decoder monitor while you call GetCDMProxy(), so that no-one unsets if while you're running. MediaDecoder::GetCDMProxy() asserts that you hold the lock, so I'd have thought you'd have hit that already. ::: dom/media/gmp/GMPEncryptedBufferDataImpl.cpp @@ +27,5 @@ > : mKeyId(aCrypto.key) > , mIV(aCrypto.iv) > , mClearBytes(aCrypto.plain_sizes) > , mCipherBytes(aCrypto.encrypted_sizes) > + , mSessionIdList(new GMPStringListImpl(aCrypto.session_ids)) Why don't you decaler GMPStringListImpl in GMPEncryptedBufferDataImpl.h, and then make it a member of GMPEncryptedBufferDataImpl? Then you don't need to allocate it here separately? @@ +122,5 @@ > +GMPStringListImpl::StringAt(uint32_t aIndex, > + const char** aOutString, > + uint32_t *aOutLength) const > +{ > + MOZ_ASSERT(aIndex < Size()); I think we're better to enforce the out-of-bounds check rather than just doing an assert. ::: dom/media/gmp/gmp-api/gmp-decryption.h @@ +51,5 @@ > virtual const uint32_t* CipherBytes() const = 0; > > virtual ~GMPEncryptedBufferMetadata() {} > + > + virtual const GMPStringList* SessionIds() const = 0; Add a comment here about what this does, and the lifecycle of the GMPStringList, i.e. warn not to hang onto it, etc.
Attachment #8583503 -
Flags: review?(cpearce) → review+
Updated•9 years ago
|
Attachment #8583504 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Attachment #8583563 -
Flags: review?(cpearce)
Updated•9 years ago
|
Attachment #8583563 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 6•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ecd8227fe3 https://hg.mozilla.org/integration/mozilla-inbound/rev/06330b23ca2a https://hg.mozilla.org/integration/mozilla-inbound/rev/1ec44f6cdb36
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8290eed26786
Comment 8•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/74ecd8227fe3 https://hg.mozilla.org/mozilla-central/rev/06330b23ca2a https://hg.mozilla.org/mozilla-central/rev/1ec44f6cdb36 https://hg.mozilla.org/mozilla-central/rev/8290eed26786
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 10•9 years ago
|
||
Comment on attachment 8583503 [details] [diff] [review] 1147689-gmp-encrypted-sessions.patch Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: Some edge cases in how the Adobe CDM handle multiple decryption sessions won't work correctly, some videos could incorrectly have their quality reduced when it's not necessary. [Describe test coverage new/current, TreeHerder]: Local testing, our ClearKey CDM doesn't need to use these, so we'll need to add coverage for this later. [Risks and why]: pretty low, just basically adding more data to what we send to the CDM. [String/UUID change made/needed]: None.
Attachment #8583503 -
Flags: approval-mozilla-aurora?
Comment 11•9 years ago
|
||
Comment on attachment 8583504 [details] [diff] [review] 1147689-increment-eme-version.patch Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: This patch means our existing test CDMs work with the Gecko Media Plugin interface changes from the previous patch. [Describe test coverage new/current, TreeHerder]: Our unit tests would fail if this didn't work correctly. [Risks and why]: Low; it's a metadata change. [String/UUID change made/needed]: None.
Attachment #8583504 -
Flags: approval-mozilla-aurora?
Comment 12•9 years ago
|
||
Comment on attachment 8583563 [details] [diff] [review] 1147689-eme-version-backcompat.patch Approval Request Comment [Feature/regressing bug #]: EME [User impact if declined]: This makes Firefox handle Gecko Media Plugins that support both the Gecko Media Plugin API before patch 1 in this bug and after. This means we don't break Adobe EME until they get a chance to update their CDM. [Describe test coverage new/current, TreeHerder]: I've tested locally with Adobe's old CDM, and it still works. We use the "new" path on mochitests on mozilla-central, so I'm confident that both paths work. [Risks and why]: as above, mochitests cover the new path, so I'm pretty sure this is low risk. [String/UUID change made/needed]: None.
Attachment #8583563 -
Flags: approval-mozilla-aurora?
Updated•9 years ago
|
status-firefox38:
--- → affected
Updated•9 years ago
|
Attachment #8583503 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8583504 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•9 years ago
|
Attachment #8583563 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 13•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/730f3ecb847b https://hg.mozilla.org/releases/mozilla-aurora/rev/4ff57be7b888 https://hg.mozilla.org/releases/mozilla-aurora/rev/255a5791fab3 https://hg.mozilla.org/releases/mozilla-aurora/rev/7ce924fcb7a6
Updated•9 years ago
|
Flags: needinfo?(cpearce)
You need to log in
before you can comment on or make changes to this bug.
Description
•