Closed Bug 1420836 Opened 2 years ago Closed 2 years ago

Implement cdm::Host_9::RequestStorageId

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(2 files, 7 obsolete files)

59 bytes, text/x-review-board-request
cpearce
: review+
Details
59 bytes, text/x-review-board-request
cpearce
: review+
Details
The patch is a hack way for making CDM 1.4.9.1056 run normally on Firefox.
I should ask for more implementation detail about the buffer passing to the OnStorageId.



[WIP] Implement cdm::Host_9::RequestStorageId.

MozReview-Commit-ID: 7qgNgbUDSfr
Chromium compute their Id by

https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.cc?l=69&rcl=f54676dcee771be7e1271c60aa3440e9574b3c7c

The hash code is generated by ``machine_id + origin + storage_id_key + profile_salt``

I cannot easily find the corresponding factors in gecko.
Hi Chris,

Regarding to comment 2,

Ideally, we should find some identifier to generate the hash value, then provide it into CDM.

Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that I'm not sure if gecko has the corresponding method to retreive those values.

Two questions need to consult you,

1. Do you know is there any similar facility/utility method in Gecko that we can leverage directly?
If not, do you know who might be the expert of this domain that I can ask for help?

2. What do you think about the implementation of this API? Am I in the right direction or we don't need to take reference of Chromium's impl.

I'll upload a patch as a prototype in Part1 for review, and do the complicated things in Part2.

Thank you!
Flags: needinfo?(cpearce)
Attachment #8933581 - Flags: review?(cpearce)
(In reply to James Cheng[:JamesCheng] from comment #3)
> Hi Chris,
> 
> Regarding to comment 2,
> 
> Ideally, we should find some identifier to generate the hash value, then
> provide it into CDM.
> 
> Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that
> I'm not sure if gecko has the corresponding method to retreive those values.
> 
> Two questions need to consult you,
> 
> 1. Do you know is there any similar facility/utility method in Gecko that we
> can leverage directly?

Chromium uses rlz to calculate the machine ID. We imported that code for use with Adobe EME, but removed it when we removed Adobe EME, in Bug 1332530. So you could start by restoring the code in Bug 1332530 which adds librlz, and then ensuring it's up to date with upstream.

You do *not* need to restore the code that ensured the device binding is linked into plugin-container; it can live in libxul now.

You may also need the Mac code added in bug 1214018.

The storage_id_key parameter ( https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.cc?l=101&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) is tricker. That looks like it's not checked into Chromium's public repo ( https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id_key.cc?l=14&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) so we'll need to ask them about that.


> If not, do you know who might be the expert of this domain that I can ask
> for help?
> 
> 2. What do you think about the implementation of this API? Am I in the right
> direction or we don't need to take reference of Chromium's impl.

We should proceed to implement it.

> 
> I'll upload a patch as a prototype in Part1 for review, and do the
> complicated things in Part2.
> 
> Thank you!
Flags: needinfo?(cpearce)
Comment on attachment 8933581 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

https://reviewboard.mozilla.org/r/204522/#review210418

r+ with leak fixed.

::: dom/media/gmp/ChromiumCDMParent.cpp:305
(Diff revision 1)
> +  GMP_LOG("ChromiumCDMParent::RecvRequestStorageId(this=%p, version=%u",
> +          this,
> +          aVersion);
> +
> +  nsAutoCString storageId;
> +  nsICryptoHash* hasher = nullptr;

Doesn't 'hasher' need to be an nsCOMPtr, otherwise you'll leak the instance?

::: dom/media/gmp/ChromiumCDMParent.cpp:307
(Diff revision 1)
> +          aVersion);
> +
> +  nsAutoCString storageId;
> +  nsICryptoHash* hasher = nullptr;
> +  nsresult rv;
> +  if (!hasher) {

You don't need the !hasher check here; you know it's null since you assign it to null on line 305 above.
Attachment #8933581 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #5)
> (In reply to James Cheng[:JamesCheng] from comment #3)
> > Hi Chris,
> > 
> > Regarding to comment 2,
> > 
> > Ideally, we should find some identifier to generate the hash value, then
> > provide it into CDM.
> > 
> > Chromium uses ``machine_id + origin + storage_id_key + profile_salt`` that
> > I'm not sure if gecko has the corresponding method to retreive those values.
> > 
> > Two questions need to consult you,
> > 
> > 1. Do you know is there any similar facility/utility method in Gecko that we
> > can leverage directly?
> 
> Chromium uses rlz to calculate the machine ID. We imported that code for use
> with Adobe EME, but removed it when we removed Adobe EME, in Bug 1332530. So
> you could start by restoring the code in Bug 1332530 which adds librlz, and
> then ensuring it's up to date with upstream.
> 
> You do *not* need to restore the code that ensured the device binding is
> linked into plugin-container; it can live in libxul now.
> 
> You may also need the Mac code added in bug 1214018.

Thanks for your guidance. 
> 
> The storage_id_key parameter (
> https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id.
> cc?l=101&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) is tricker. That
> looks like it's not checked into Chromium's public repo (
> https://cs.chromium.org/chromium/src/chrome/browser/media/cdm_storage_id_key.
> cc?l=14&rcl=e0e3bb53c23b95fff838908a8296dc84ce722b99 ) so we'll need to ask
> them about that.
> 
Thanks,
I noticed that, and I will ask them about this parameter.

> 
> > If not, do you know who might be the expert of this domain that I can ask
> > for help?
> > 
> > 2. What do you think about the implementation of this API? Am I in the right
> > direction or we don't need to take reference of Chromium's impl.
> 
> We should proceed to implement it.
> 
> > 
> > I'll upload a patch as a prototype in Part1 for review, and do the
> > complicated things in Part2.
> > 
> > Thank you!
Depends on: 1422669
Attachment #8933581 - Attachment is obsolete: true
Attachment #8937177 - Attachment is obsolete: true
Attachment #8937178 - Attachment is obsolete: true
Comment on attachment 8932750 [details] [diff] [review]
[WIP] Implement cdm::Host_9::RequestStorageId

># HG changeset patch
># User James Cheng <jacheng@mozilla.com>
>
>Bug 1420836 - [WIP] Implement cdm::Host_9::RequestStorageId.
>
>MozReview-Commit-ID: 7qgNgbUDSfr
>
>diff --git a/dom/media/gmp/ChromiumCDMChild.cpp b/dom/media/gmp/ChromiumCDMChild.cpp
>index 519df1cdf373..b74b73712816 100644
>--- a/dom/media/gmp/ChromiumCDMChild.cpp
>+++ b/dom/media/gmp/ChromiumCDMChild.cpp
>@@ -471,16 +471,30 @@ ChromiumCDMChild::CreateFileIO(cdm::FileIOClient * aClient)
>   MOZ_ASSERT(IsOnMessageLoopThread());
>   GMP_LOG("ChromiumCDMChild::CreateFileIO()");
>   if (!mPersistentStateAllowed) {
>     return nullptr;
>   }
>   return new WidevineFileIO(aClient);
> }
> 
>+void
>+ChromiumCDMChild::RequestStorageId(uint32_t aVersion)
>+{
>+  MOZ_ASSERT(IsOnMessageLoopThread());
>+  GMP_LOG("ChromiumCDMChild::RequestStorageId() aVersion = %u", aVersion);
>+  if (aVersion >= 0x80000000) {
>+    mCDM->OnStorageId(aVersion, nullptr, 0);
>+    return;
>+  }
>+
>+  //TODO: Need to provide a menaingful buffer instead of a dummy one.
>+  mCDM->OnStorageId(aVersion, new uint8_t[1024*1024], 1024 * 1024);
>+}
>+
> ChromiumCDMChild::~ChromiumCDMChild()
> {
>   GMP_LOG("ChromiumCDMChild:: dtor this=%p", this);
> }
> 
> bool
> ChromiumCDMChild::IsOnMessageLoopThread()
> {
>diff --git a/dom/media/gmp/ChromiumCDMChild.h b/dom/media/gmp/ChromiumCDMChild.h
>index 4a0b08c5709a..4d47017c97e5 100644
>--- a/dom/media/gmp/ChromiumCDMChild.h
>+++ b/dom/media/gmp/ChromiumCDMChild.h
>@@ -66,17 +66,17 @@ public:
>                              uint32_t aServiceIdSize,
>                              const char* aChallenge,
>                              uint32_t aChallengeSize) override {}
>   void EnableOutputProtection(uint32_t aDesiredProtectionMask) override {}
>   void QueryOutputProtectionStatus() override {}
>   void OnDeferredInitializationDone(cdm::StreamType aStreamType,
>                                     cdm::Status aDecoderStatus) override {}
>   // cdm::Host_9 interface
>-  void RequestStorageId(uint32_t aVersion) override {}
>+  void RequestStorageId(uint32_t aVersion) override;
>   cdm::FileIO* CreateFileIO(cdm::FileIOClient* aClient) override;
> 
>   // cdm::Host_8 implementation
>   void OnSessionMessage(const char* aSessionId,
>                         uint32_t aSessionIdSize,
>                         cdm::MessageType aMessageType,
>                         const char* aMessage,
>                         uint32_t aMessageSize,
>
Attachment #8932750 - Attachment is obsolete: true
Attachment #8937189 - Flags: review?(cpearce)
Attachment #8937190 - Flags: review?(cpearce)
Attachment #8937191 - Flags: review?(cpearce)
Comment on attachment 8937189 [details]
Bug 1420836 - Part1 - Restore sha256.c from Bug 1332530 back.

https://reviewboard.mozilla.org/r/207890/#review214316

You should be able to compute the storage Id in the parent process before sending it to the GMP process. So then you can just use the SHA256 code in Gecko to calculate the storage Id.
Attachment #8937189 - Flags: review?(cpearce) → review-
Comment on attachment 8937190 [details]
Bug 1420836 - Part2 - Add a class to compute the storage id.

https://reviewboard.mozilla.org/r/207892/#review214296

I think that using a template class for this is overkill, and doesn't make the code easier to understand. However if you want to continue with the template class approach, you should at least move the template class into the .cpp file, and then only expose a single public function which takes as a parameter the origin salt and returns the storage id for version 1. You don't need the consumer of the storage id to know all the details of what objects are required to calculate the storage id.

::: dom/media/gmp/CDMStorageIdProvider.h:17
(Diff revision 1)
> +
> +#ifdef SUPPORT_STORAGE_ID
> +#include "rlz/lib/string_utils.h"
> +#include "rlz/sha256.h"
> +#endif
> +

See my comment in the next commit in your series, but it seems to me this interface is way more complicated that it needs to be.

It seems since you only use version 1, you can just publically expose a function that takes as parameter the origin salt, and returns the storage id for the latest version. The fact that you use a complicated template class to separate the calculation of the storage id for different algorithms doesn't need to be exposed to the user of your API. The template class stuff can be in the .cpp file, hidden behind a single public function which just returns the storage id.

::: dom/media/gmp/CDMStorageIdProvider.h:73
(Diff revision 1)
> +
> +  CDMStorageIdProvider(const CDMStorageIdProvider&) = delete;
> +  void operator=(const CDMStorageIdProvider&) = delete;
> +};
> +
> +//Current version.

Nit: space after // before comment.

::: dom/media/gmp/CDMStorageIdProvider.h:88
(Diff revision 1)
> +
> +  struct PrerequisiteFactors : PrerequisiteFactorsBase
> +  {
> +    PrerequisiteFactors(std::string aMachineId, std::string aOriginSalt)
> +      : mMachineId(aMachineId),
> +        mOriginSalt(aOriginSalt)

Layout of args should be with leading ',' i.e.:

  : mMachineId(aMachineId)
  , mOriginSalt(aOriginSalt)

::: dom/media/gmp/CDMStorageIdProvider.h:123
(Diff revision 1)
> +    MOZ_ASSERT_UNREACHABLE("This platform does not support computing storage id.");
> +    return false;
> +#endif
> +  }
> +
> +  int GetVersion() override { return 1; }

Optional nit: you write a literal 1 here and above in the template argument. Is it possible to have a constant used in both places?

::: dom/media/gmp/CDMStorageIdProvider.h:135
(Diff revision 1)
> +UniquePtr<CDMStorageIdProviderBase::PrerequisiteFactorsBase>
> +MakePrerequisiteFactors(int aVersion, Args&&... aArgs)
> +{
> +  switch (aVersion) {
> +      case CDMStorageIdProviderBase::kCDMRequestLatestVersion:
> +        return MakeUnique<CDMStorageIdProvider<CDMStorageIdProviderBase::kCurrentVersion>::PrerequisiteFactors>(Forward<Args>(aArgs)...);

The behaviour of LatestVersion and CurrentVersion is the same. I don't understand what value having this switch statement adds.
Attachment #8937190 - Flags: review?(cpearce) → review-
Comment on attachment 8937191 [details]
Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

https://reviewboard.mozilla.org/r/207894/#review214310

This can be made a lot simpler.

::: commit-message-d077e:1
(Diff revision 1)
> +Bug 1420836 - Part3 - Try to compute the storage id in GMP process and implement RequestStorageId for CDM.

Either do, or do not, there is no try.

::: dom/media/gmp/ChromiumCDMChild.cpp:496
(Diff revision 1)
> +  }
> +  if (aVersion > CDMStorageIdProviderBase::kCurrentVersion) {
> +    mCDM->OnStorageId(aVersion, nullptr, 0);
> +    return;
> +  }
> +  // mCurrentStorageId will be empty if we compute storage failed.

What should the behaviour be if aVersion is less than kCurrentVersion?

::: dom/media/gmp/GMPChild.cpp:278
(Diff revision 1)
> +    LOGD("%s get machineId failed.", __FUNCTION__);
> +    return IPC_OK();
> +  }
> +  std::string originSalt(aOriginSalt.BeginReading(), aOriginSalt.Length());
> +  auto provider = CDMStorageIdProviderBase::CreateProvider(CDMStorageIdProviderBase::kCDMRequestLatestVersion);
> +  auto factors = MakePrerequisiteFactors(CDMStorageIdProviderBase::kCDMRequestLatestVersion,

I think this code is more complicated that it needs to be.

You only ever compute the storage id for the lastest version currently.

You're calling a number of functions here in order to get the storage Id, but really the only variable input to the procedure is the origin salt.

So I think it makes sense to wrap all of the code related to generating the storage id into a single function that takes just the origin salt as a parameter, and does all the intermediate work. Then it's simpler for the caller.

I also don't see why the storage Id needs to be calculated in the GMP process. It could be calculated in the parent process I think, and then sent pre-calculated to the GMP process. Then you'd not need to re-import the old SHA256 code, you could use the existing code in Gecko for calculating SHA256.
Attachment #8937191 - Flags: review?(cpearce) → review-
Attachment #8937189 - Attachment is obsolete: true
Attachment #8937190 - Attachment is obsolete: true
Attachment #8937191 - Attachment is obsolete: true
Attachment #8939502 - Flags: review?(cpearce)
Attachment #8939503 - Flags: review?(cpearce)
Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

https://reviewboard.mozilla.org/r/209836/#review218668

r+, but please can you check whether mStorageId.get() will be nullptr if we failed to generate the storage ID.

::: dom/media/gmp/ChromiumCDMChild.cpp:495
(Diff revision 1)
> +  }
> +  if (aVersion > CDMStorageIdProvider::kCurrentVersion) {
> +    mCDM->OnStorageId(aVersion, nullptr, 0);
> +    return;
> +  }
> +

If we fail to generate the storage ID in the parent, will mStorageId.get() be nullptr here? Maybe we need to also explicitly check whether mStorageId.IsEmpty() here, and pass nullptr if so?
Attachment #8939503 - Flags: review?(cpearce) → review+
Comment on attachment 8939502 [details]
Bug 1420836 - Part1 - Provide a utility function to compute the storage id.

https://reviewboard.mozilla.org/r/209834/#review218670

That's a lot better, thanks!
Attachment #8939502 - Flags: review?(cpearce) → review+
Comment on attachment 8939503 [details]
Bug 1420836 - Part2 - Pass the storage id from content process to GMP process then provide it to CDM.

https://reviewboard.mozilla.org/r/209836/#review218668

> If we fail to generate the storage ID in the parent, will mStorageId.get() be nullptr here? Maybe we need to also explicitly check whether mStorageId.IsEmpty() here, and pass nullptr if so?

Thanks for reminding. It will return non-null pointer from get() when mStorageId is empty. I will explicitly check it and pass nullptr instead of calling get().
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1595c6002aa6
Part1 - Provide a utility function to compute the storage id. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5a46db143897
Part2 - Pass the storage id from content process to GMP process then provide it to CDM. r=cpearce
https://hg.mozilla.org/mozilla-central/rev/1595c6002aa6
https://hg.mozilla.org/mozilla-central/rev/5a46db143897
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.