Closed Bug 1290830 Opened 4 years ago Closed 4 years ago

Make Decryptor APIs reusable by not only GMP framework

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(2 files, 1 obsolete file)

The original Decryptor callback interface[1] is bound with GMP prefix.
We would need a interface to callback Fennec CDMProxy(will call it MediaDrmCDMProxy).
So I detach the interface from GMPDecryptorProxyCallback.

[1] https://dxr.mozilla.org/mozilla-central/rev/4a18b5cacb1b21a3e8b4b1dada6b2dd3dba51cb1/dom/media/gmp/GMPDecryptorProxy.h#17
https://reviewboard.mozilla.org/r/68284/#review65320

::: dom/media/gmp/GMPCDMProxy.h
(Diff revision 1)
>  
>    GMPCrashHelper* mCrashHelper;
>  
>    GMPDecryptorProxy* mCDM;
>  
> -  CDMCaps mCapabilites;

This member has been already defined by base class, so remove it.
https://reviewboard.mozilla.org/r/68284/#review65548

::: dom/media/eme/CDMDecryptorCallback.h:9
(Diff revision 1)
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef CDMDecryptorCallback_h_
> +#define CDMDecryptorCallback_h_
> +
> +class CDMDecryptorCallback {

CDM stands for Content Decryption Module, so that would make the name Content Decryption Module Decryptor Proxy Callback.

It seems weird having Decryption mentioned more than once.

So let's call it DecryptorProxyCallback.
Attachment #8776519 - Flags: review?(cpearce) → review+
Comment on attachment 8776519 [details]
Bug 1290830 - [Part1] Detach the GMPDecryptorProxyCallback interface to DecryptorProxyCallback.

https://reviewboard.mozilla.org/r/68284/#review65550

r+ with comment addressed.
Comment on attachment 8776519 [details]
Bug 1290830 - [Part1] Detach the GMPDecryptorProxyCallback interface to DecryptorProxyCallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68284/diff/1-2/
Thanks for reviewing and remind me of this...

I've addressed the issue.

Thank you:)
Keywords: checkin-needed
Keywords: checkin-needed
Review commit: https://reviewboard.mozilla.org/r/68512/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/68512/
Attachment #8776519 - Attachment description: Bug 1290830 - Make Decryptor APIs reusable by not only GMP framework. → Bug 1290830 - [Part1] Detach the GMPDecryptorProxyCallback interface to DecryptorProxyCallback.
Attachment #8776844 - Flags: review?(cpearce)
Comment on attachment 8776519 [details]
Bug 1290830 - [Part1] Detach the GMPDecryptorProxyCallback interface to DecryptorProxyCallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68284/diff/2-3/
Hi Chris,
Sorry that I noticed that the interface contained some GMP specified types.

So I create the patch to replace them.

GMPMediaKeyStatus     => mozilla::dom::MediaKeyStatus
GMPSessionMessageType => mozilla::dom::MediaKeyMessageType
GMPErr                => mozilla::DecryptStatus
GMPTimeStamp          => int64_t

Please help me to review this patch ,many thanks.
Comment on attachment 8776844 [details]
Bug 1290830 - [Part2] Remove the GMP types using genral types instead.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68512/diff/1-2/
Rank: 25
Priority: -- → P2
Comment on attachment 8776844 [details]
Bug 1290830 - [Part2] Remove the GMP types using genral types instead.

https://reviewboard.mozilla.org/r/68512/#review66974

::: dom/media/eme/CDMCaps.cpp:73
(Diff revision 2)
>  {
>    mData.mMonitor.AssertCurrentThreadOwns();
>    KeyStatus key(aKeyId, aSessionId, aStatus);
>    auto index = mData.mKeyStatuses.IndexOf(key);
>  
> -  if (aStatus == kGMPUnknown) {
> +  if (aStatus == dom::MediaKeyStatus::Internal_error) {

This is not going to work. If the CDM marks a key as "internal-error" status, we need to expose that status to the web application. But here we'd end up removing the key from our MediaKeyStatusMap.

So doing this would result in non-conformant behaviour.

So you need to retain a way to signal to the CDMCaps when a key becomes unknown.

One option, would be to add a new message to PGMPDecryptor, and have the GMPDecryptorChild detect the GMP setting a keystatus of kUnknown, and then sending the new message instead of sending a KeyStatusChanged message.
Attachment #8776844 - Flags: review?(cpearce) → review-
(In reply to Chris Pearce (:cpearce) from comment #12)
> Comment on attachment 8776844 [details]
> Bug 1290830 - [Part2] Remove the GMP types using genral types instead.
> 
> https://reviewboard.mozilla.org/r/68512/#review66974
> 
> ::: dom/media/eme/CDMCaps.cpp:73
> (Diff revision 2)
> >  {
> >    mData.mMonitor.AssertCurrentThreadOwns();
> >    KeyStatus key(aKeyId, aSessionId, aStatus);
> >    auto index = mData.mKeyStatuses.IndexOf(key);
> >  
> > -  if (aStatus == kGMPUnknown) {
> > +  if (aStatus == dom::MediaKeyStatus::Internal_error) {
> 
> This is not going to work. If the CDM marks a key as "internal-error"
> status, we need to expose that status to the web application. But here we'd
> end up removing the key from our MediaKeyStatusMap.
> 
> So doing this would result in non-conformant behaviour.
> 

Hi Chris,

Thanks for pointing out this problem.

But I traced the calling flow,

Once we received the KeyStatusChanged event and call cap.SetKeysStatus [1] with kGMPUnknown, |mData.mKeyStatuses.RemoveElement(key)| will return true and keep on the event dispatching procedure.

The flow will be

CDMProxy::OnKeyStatusesChange -> session->DispatchKeyStatusesChange() -> UpdateKeyStatusMap() -> Translate GMPStatus to MediaKeyStatus [3] -> asyncDispatcher->PostDOMEvent()  keystatuseschange.

The kGMPUnknown will be translated into MediaKeyStatus::Internal_error eventually and fire a event to DOM side. 

IIUC, should I treat kGMPUnknown as a special case to handle?


[1] https://dxr.mozilla.org/mozilla-central/rev/d42aacfe34af25e2f5110e2ca3d24a210eabeb33/dom/media/gmp/GMPCDMCallbackProxy.cpp#304

[2] https://dxr.mozilla.org/mozilla-central/rev/d42aacfe34af25e2f5110e2ca3d24a210eabeb33/dom/media/gmp/GMPCDMCallbackProxy.cpp#308

[3] https://dxr.mozilla.org/mozilla-central/rev/d42aacfe34af25e2f5110e2ca3d24a210eabeb33/dom/media/eme/MediaKeyStatusMap.cpp#130


> So you need to retain a way to signal to the CDMCaps when a key becomes
> unknown.
> 
> One option, would be to add a new message to PGMPDecryptor, and have the
> GMPDecryptorChild detect the GMP setting a keystatus of kUnknown, and then
> sending the new message instead of sending a KeyStatusChanged message.

I will also modify the patch to handle this if above understanding is wrong.

Thank you very much.
Flags: needinfo?(cpearce)
(In reply to James Cheng[:JamesCheng] from comment #13)
> Once we received the KeyStatusChanged event and call cap.SetKeysStatus [1]
> with kGMPUnknown, |mData.mKeyStatuses.RemoveElement(key)| will return true
> and keep on the event dispatching procedure.

The problem is not with "keystatuschanged" event dispatch. The problem is that if you treat status "internal-error" as meaning "remove the key from the MediaKeyStatusMap", then when a key has status "internal-error", we won't expose that as status in the MediaKeyStatusMap, we'll instead remove the keyId from the map.

The CDM may legitimately need to express a key status of "internal-error" for a keyId in the MediaKeyStatusMap. So we can't have "internal-error" mean "remove from the map", otherwise we can't express the "internal-error" status.

The old code had a special case for "unknown", which told Gecko to remove a key from the MediaKeyStatusMap. If you're limiting yourself to just using the DOM MediaKeyStatus enum values, then you need another way to express that a key should be removed from the MediaKeyStatusMap.

Does that make sense?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #14)
> (In reply to James Cheng[:JamesCheng] from comment #13)
> > Once we received the KeyStatusChanged event and call cap.SetKeysStatus [1]
> > with kGMPUnknown, |mData.mKeyStatuses.RemoveElement(key)| will return true
> > and keep on the event dispatching procedure.
> 
> The problem is not with "keystatuschanged" event dispatch. The problem is
> that if you treat status "internal-error" as meaning "remove the key from
> the MediaKeyStatusMap", then when a key has status "internal-error", we
> won't expose that as status in the MediaKeyStatusMap, we'll instead remove
> the keyId from the map.
> 
> The CDM may legitimately need to express a key status of "internal-error"
> for a keyId in the MediaKeyStatusMap. So we can't have "internal-error" mean
> "remove from the map", otherwise we can't express the "internal-error"
> status.
> 
> The old code had a special case for "unknown", which told Gecko to remove a
> key from the MediaKeyStatusMap. If you're limiting yourself to just using
> the DOM MediaKeyStatus enum values, then you need another way to express
> that a key should be removed from the MediaKeyStatusMap.
> 
> Does that make sense?
Yes, I got it.

I should treat unknown case independently.

I will try to handle this in next patch

Thanks.
Comment on attachment 8776844 [details]
Bug 1290830 - [Part2] Remove the GMP types using genral types instead.

https://reviewboard.mozilla.org/r/68512/#review68496

::: dom/media/eme/CDMCaps.h:63
(Diff revision 3)
>  
>      bool IsKeyUsable(const CencKeyId& aKeyId);
>  
>      // Returns true if key status changed,
>      // i.e. the key status changed from usable to expired.
> -    bool SetKeyStatus(const CencKeyId& aKeyId, const nsString& aSessionId, GMPMediaKeyStatus aStatus);
> +    bool SetKeyStatus(const CencKeyId& aKeyId, const nsString& aSessionId,

I would prefer separate public ForgetKeyStatus() and SetKeyStatus() functions, rather than having a boolean parameter to express that you're removing a key.

I think it's confusing and messy to require callers to specify a bogus key status when removing keys from the map.

Having a boolean parameter on the internal implementation is OK, though I still don't really like it.

Another option would be to make the MediaKeyStatus you pass in Optional<MediaKeyStatus>, and have the KeyStatus not being passed signify that the key should be removed.

::: dom/media/eme/DecryptorProxyCallback.h:33
(Diff revision 3)
>  
>    virtual void SessionMessage(const nsCString& aSessionId,
> -                              GMPSessionMessageType aMessageType,
> +                              mozilla::dom::MediaKeyMessageType aMessageType,
>                                const nsTArray<uint8_t>& aMessage) = 0;
>  
>    virtual void ExpirationChange(const nsCString& aSessionId,

Please have a typedef for the time units used here (see comment below).

::: dom/media/gmp/GMPCDMCallbackProxy.cpp:290
(Diff revision 3)
>                                        const nsTArray<uint8_t>& aKeyId,
> -                                      GMPMediaKeyStatus aStatus)
> +                                      dom::MediaKeyStatus aStatus)
> +{
> +  MOZ_ASSERT(mProxy->IsOnOwnerThread());
> +
> +  KeyStatusChangedInternal(aSessionId, aKeyId, aStatus, false);

Boolean parameters in general aren't great, as it's not obvious when reading code that calls the function what the parameters mean.

So please either:
1. Annotate with comments what the values mean, i.e.: 
  KeyStatusChangedInternal(aSessionId, aKeyId, aStatus, /* Reset status */ false), or:
2. Define an enum { kSetKeyStatus, kForgetKeyStatus}, and use that instead of the bool.

Option 1 is easier, option 2 is enforced by the compiler.

::: dom/media/gmp/PGMPDecryptor.ipdl:85
(Diff revision 3)
>                       nsCString aMessage);
>  
>    async KeyStatusChanged(nsCString aSessionId, uint8_t[] aKey,
>                           GMPMediaKeyStatus aStatus);
>  
> +  async KeyStatusUnknown(nsCString aSessionId, uint8_t[] aKey);

Please call this  "ForgetKeyStatus". So that the name has a "do something" form. Rather than a "state is X" form.

The "state is X" form can be confused with a function that tests whether the state is X.

Please also rename the other functions you called "KeyStatusUnknown".

::: dom/media/gtest/TestGMPCrossOrigin.cpp:1376
(Diff revision 3)
>    void ResolvePromise(uint32_t aPromiseId) override {}
>    void RejectPromise(uint32_t aPromiseId,
>                       nsresult aException,
>                       const nsCString& aSessionId) override { }
>    void ExpirationChange(const nsCString& aSessionId,
> -                        GMPTimestamp aExpiryTime) override {}
> +                        int64_t aExpiryTime) override {}

Instead of using raw ints for timestamps, please "typedef int64_t UnixTime" somewhere, and use that instead.

That way it's clear what you're passing around (though it still not typesafe).

Please make this change elsewhere that you replaces GMPTimestamp.
Attachment #8776844 - Flags: review?(cpearce) → review-
Thanks for your suggestion and advice.

Learn a lot! I will refine the patch soon, thank you.
Hi Chris,

I am not sure why ExpirationChange is designed by passing double. 

I think changing into int64_t is OK by checking all the callers, so I create Part3 for modifying this.

Thanks.
Attach treeherder result to ensure those are compilable.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa817ac56ea
Comment on attachment 8776844 [details]
Bug 1290830 - [Part2] Remove the GMP types using genral types instead.

https://reviewboard.mozilla.org/r/68512/#review71290

::: dom/media/gmp/PGMPDecryptor.ipdl:85
(Diff revision 4)
>                       nsCString aMessage);
>  
>    async KeyStatusChanged(nsCString aSessionId, uint8_t[] aKey,
>                           GMPMediaKeyStatus aStatus);
>  
> +  async KeyStatusUnknown(nsCString aSessionId, uint8_t[] aKey);

Please call this ForgetKeyStatus. So it's a "do something" form of name, rather than a name which can be confused with a "state is X" name.
Attachment #8776844 - Flags: review?(cpearce) → review-
Comment on attachment 8776844 [details]
Bug 1290830 - [Part2] Remove the GMP types using genral types instead.

https://reviewboard.mozilla.org/r/68512/#review71292
Attachment #8776844 - Flags: review- → review+
Comment on attachment 8780996 [details]
Bug 1290830 - [Part3] Fix timestamp type from double to int64_t for ExpirationChange in PGMPDecryptor.ipdl.

https://reviewboard.mozilla.org/r/71504/#review71294

The expiration parameter corresponds to the MediaKeySession.expiration DOM attribute, which is defined to be a double in the EME spec:
https://w3c.github.io/encrypted-media/#dom-mediakeysession-expiration

So we should keep this parameter as a double, so we don't end up losing precision when we convert from a double, to an in64_t and the back to a double when we expose this to JavaScript.
Attachment #8780996 - Flags: review?(cpearce) → review-
Attachment #8780996 - Attachment is obsolete: true
Thanks for the feedback and reviewing. 

(In reply to Chris Pearce (:cpearce) from comment #27)
> Comment on attachment 8780996 [details]
> Bug 1290830 - [Part3] Fix timestamp type from double to int64_t for
> ExpirationChange in PGMPDecryptor.ipdl.
> 
> https://reviewboard.mozilla.org/r/71504/#review71294
> 
> The expiration parameter corresponds to the MediaKeySession.expiration DOM
> attribute, which is defined to be a double in the EME spec:
> https://w3c.github.io/encrypted-media/#dom-mediakeysession-expiration
> 
> So we should keep this parameter as a double, so we don't end up losing
> precision when we convert from a double, to an in64_t and the back to a
Do you mean that in this case double value will never bigger than  std::numeric_limits<int64_t>::max() which is 9223372036854775807? So the conversion will never lose precision. 
> double when we expose this to JavaScript.

Thanks for the feedback and reviewing.
Keywords: checkin-needed
This can't be autolanded until the open issues on Part 2 are marked as resolved in MozReview.
Keywords: checkin-needed
(In reply to James Cheng[:JamesCheng] from comment #30)
> > So we should keep this parameter as a double, so we don't end up losing
> > precision when we convert from a double, to an in64_t and the back to a
> Do you mean that in this case double value will never bigger than 
> std::numeric_limits<int64_t>::max() which is 9223372036854775807? So the
> conversion will never lose precision. 
> > double when we expose this to JavaScript.

doubles can represent "real numbers" like 0.5, whereas ints can only represent "whole numbers" (i.e. 0, 1, 2...). So if the CDM wants to express 0.5, with your change, we'd end up rounding that down to 0.
(In reply to Chris Pearce (:cpearce) from comment #32)
> (In reply to James Cheng[:JamesCheng] from comment #30)
> > > So we should keep this parameter as a double, so we don't end up losing
> > > precision when we convert from a double, to an in64_t and the back to a
> > Do you mean that in this case double value will never bigger than 
> > std::numeric_limits<int64_t>::max() which is 9223372036854775807? So the
> > conversion will never lose precision. 
> > > double when we expose this to JavaScript.
> 
> doubles can represent "real numbers" like 0.5, whereas ints can only
> represent "whole numbers" (i.e. 0, 1, 2...). So if the CDM wants to express
> 0.5, with your change, we'd end up rounding that down to 0.

Thanks,

I just wonder that 
https://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPDecryptorParent.cpp?q=gmpdecryptorparent.cpp&redirect_type=direct#295

will lose the fractional numbers(or the double value is bigger than int64) when it calls callback. 

For representing the timestamp, I think it's OK to conversion between double and int64 in this case.
Hi RyanVM,

Do you mean that I should mark all comments as "Fixed" in mozreview?(I did it)

I wonder if I'm just Level1 without the privilege to press  "Automation->Land Commit" in mozreview.  

Does it also go through "autoland"?

Or no matter using "autoland" or "marking check-in-needed", I must mark all comments as "Fixed" before landing code.

Thank you!
Flags: needinfo?(ryanvm)
Keywords: checkin-needed
(In reply to James Cheng[:JamesCheng] from comment #34)
> Do you mean that I should mark all comments as "Fixed" in mozreview?(I did
> it)

Yep, that worked.
Flags: needinfo?(ryanvm)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3942a7c1224c
[Part1] Detach the GMPDecryptorProxyCallback interface to DecryptorProxyCallback. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/65b00e9620d1
[Part2] Remove the GMP types using genral types instead. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3942a7c1224c
https://hg.mozilla.org/mozilla-central/rev/65b00e9620d1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.