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

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Comment 1

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1147689-gmp-encrypted-sessions.patch

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1147689-increment-eme-version.patch

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

still working on v6 compat patch.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1147689-eme-version-backcompat.patch

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

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

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8290eed26786

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Needs uplift to 38.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

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