Closed Bug 1147689 Opened 5 years ago Closed 5 years ago

[EME] Pass Session ID into EME Decode+Decrypt CDMs

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: eflores, Assigned: eflores)

References

Details

Attachments

(3 files)

No description provided.
still working on v6 compat patch.
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+
Attachment #8583504 - Flags: review?(cpearce) → review+
Attachment #8583563 - Flags: review?(cpearce) → review+
Needs uplift to 38.
Flags: needinfo?(cpearce)
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 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 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?
Attachment #8583503 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8583504 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8583563 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.