Closed
Bug 1290830
Opened 8 years ago
Closed 8 years ago
Make Decryptor APIs reusable by not only GMP framework
Categories
(Core :: Audio/Video: GMP, defect, P2)
Core
Audio/Video: GMP
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
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/68284/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68284/
Attachment #8776519 -
Flags: review?(cpearce)
Assignee | ||
Comment 2•8 years ago
|
||
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.
Comment 3•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8776519 -
Flags: review?(cpearce) → review+
Comment 4•8 years ago
|
||
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.
Assignee | ||
Comment 5•8 years ago
|
||
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/
Assignee | ||
Comment 6•8 years ago
|
||
Thanks for reviewing and remind me of this... I've addressed the issue. Thank you:)
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•8 years ago
|
||
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)
Assignee | ||
Comment 8•8 years ago
|
||
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/
Assignee | ||
Comment 9•8 years ago
|
||
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.
Assignee | ||
Comment 10•8 years ago
|
||
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/
Assignee | ||
Comment 11•8 years ago
|
||
Attach treeherder result for reference: https://treeherder.mozilla.org/#/jobs?repo=try&revision=04e42a94c7f6
Updated•8 years ago
|
Rank: 25
Priority: -- → P2
Comment 12•8 years ago
|
||
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-
Assignee | ||
Comment 13•8 years ago
|
||
(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)
Comment 14•8 years ago
|
||
(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)
Assignee | ||
Comment 15•8 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 18•8 years ago
|
||
mozreview-review |
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-
Assignee | ||
Comment 19•8 years ago
|
||
Thanks for your suggestion and advice. Learn a lot! I will refine the patch soon, thank you.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 23•8 years ago
|
||
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.
Assignee | ||
Comment 24•8 years ago
|
||
Attach treeherder result to ensure those are compilable. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa817ac56ea
Comment 25•8 years ago
|
||
mozreview-review |
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 26•8 years ago
|
||
mozreview-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 27•8 years ago
|
||
mozreview-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-
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8780996 -
Attachment is obsolete: true
Assignee | ||
Comment 30•8 years ago
|
||
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
Comment 31•8 years ago
|
||
This can't be autolanded until the open issues on Part 2 are marked as resolved in MozReview.
Keywords: checkin-needed
Comment 32•8 years ago
|
||
(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.
Assignee | ||
Comment 33•8 years ago
|
||
(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.
Assignee | ||
Comment 34•8 years ago
|
||
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
Comment 35•8 years ago
|
||
(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)
Comment 36•8 years ago
|
||
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
Comment 37•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3942a7c1224c https://hg.mozilla.org/mozilla-central/rev/65b00e9620d1
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1299627
You need to log in
before you can comment on or make changes to this bug.
Description
•