Closed Bug 1406080 Opened 7 years ago Closed 7 years ago

[EME] Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9

Categories

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

Product:
Core
Component:
Audio/Video: GMP
Version:
55 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng)

References

Details

Attachments

(2 files)

Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1406080 - Part2 - Reject creating with unsupported cdm version in clearkey cdm and cdm-fake.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
Currently, we create CDM version 8[1] and if we got creation failure, we do not retry for another interface.

We should try create(cdm::ContentDecryptionModule_9::kVersion first and try 
create(cdm::ContentDecryptionModule_8::kVersion if we cannot create version 9.

This makes Firefox be able to handle old(8) and new version(9) CDM if we update CDM to 1.4.9.

[1]
http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/dom/media/gmp/ChromiumCDMAdapter.cpp#138
Attachment #8915915 - Flags: review?(cpearce)
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review192198


C/C++ static analysis found 2 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`

If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/media/gmp/ChromiumCDMAdapter.cpp:66
(Diff revision 1)
>    }
> -  return static_cast<cdm::Host_8*>(aUserData);
> +  auto tuple = static_cast<mozilla::Tuple<cdm::Host_8*, cdm::Host_9*, int&>*>(aUserData);
> +  if (aHostInterfaceVersion == cdm::Host_8::kVersion) {
> +    mozilla::Get<2>(*tuple) = cdm::ContentDecryptionModule_8::kVersion;
> +    return static_cast<cdm::Host_8*>(mozilla::Get<0>(*tuple));
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

::: dom/media/gmp/ChromiumCDMAdapter.cpp:178
(Diff revision 1)
> -              aPluginAPI,
> +                aPluginAPI,
> -              aDecryptorId,
> +                aDecryptorId,
> -              this);
> +                this,
> +                cdm::ContentDecryptionModule_8::kVersion);
> -      return GMPGenericErr;
> +        return GMPGenericErr;
> +      } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Attachment #8916037 - Flags: review?(cpearce)
Upload Part2 to fix treeherder error due to mis-casting the CDM from CMD8 to CDM9.

Revise Part1 to fix static analysis defects.
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review192726

Thanks, but we need some revisions here. See https://hg.mozilla.org/mozilla-central/rev/355453560577 which removed the code that used to do this for supporting multiple CDM versions.

::: dom/media/gmp/ChromiumCDMAdapter.cpp:152
(Diff revision 2)
> +      create(cdm::ContentDecryptionModule_9::kVersion,
> +             kEMEKeySystemWidevine.get(),
> +             kEMEKeySystemWidevine.Length(),
> +             &ChromiumCdmHost,
> +             aHostAPI));
> +    if (!cdm9) {

This would be clearer if you inverted the check and returned early, i.e.

  if (cdm9) {
    \*aPluginAPI = cdm9;
    return GMPNoErr;
  }

  // Check for CDM8...

This is clearer, as there is fewer levels of indent to keep in your head at once.

::: dom/media/gmp/ChromiumCDMAdapter.cpp:161
(Diff revision 2)
> +              aHostAPI,
> +              aPluginAPI,
> +              aDecryptorId,
> +              this,
> +              cdm::ContentDecryptionModule_9::kVersion);
> +      auto cdm8 = reinterpret_cast<cdm::ContentDecryptionModule_8*>(

We're casting to the CDM type, and assigning that to aPluginAPI.

The type of aPluginAPI is void\*\*

Doing a reinterpret_cast shouldn't change the pointer.

So we're not doing anything useful here, as right after we do the cast from void\*\* to the CDM type, we assign to a void\*\* and lose the type.

So there's not much point doing the extra cast. We could stop doing the casts here, I think.

::: dom/media/gmp/ChromiumCDMChild.h:102
(Diff revision 2)
> +                            uint32_t aErrorMessageLength) override;
> +
>    void GiveBuffer(ipc::Shmem&& aBuffer);
>  
>  protected:
> +  class ContentDecryptionModuleWrapperBase {

The difference between CDM_8 and CDM_9 interfacecs is the OnQueryOutputProtectionStatus() and OnStorageId() functions. So instead of defining a wrapper base, you can instead create an implementation of CDM_9 that wraps CDM and calls through to the wrapped CDM_8 interface for functions that exist on both CDM_9 and CDM_8, and just ignores the functions that CDM_9 has that CDM_8 does not have.

You can deallocate the wrapper in the CDM_9::Destroy() implementation.

::: dom/media/gmp/ChromiumCDMChild.h:219
(Diff revision 2)
>  
>    template<typename MethodType, typename... ParamType>
>    void CallOnMessageLoopThread(const char* const, MethodType, ParamType&&...);
>  
>    GMPContentChild* mPlugin = nullptr;
> -  cdm::ContentDecryptionModule_8* mCDM = nullptr;
> +  UniquePtr<ContentDecryptionModuleWrapperBase> mCDM;

Just make this a ContentDecryptionModule_8*.

::: dom/media/gmp/GMPContentChild.cpp:143
(Diff revision 2)
> -  cdm::Host_8* host = child;
> +  cdm::Host_8* host8 = child;
> +  cdm::Host_9* host9 = child;
> +  int cdmVersion = 0;
> +  // When we successfully create cdm, the cdm will call ChromiumCdmHost,
> +  // we will write the current cdm version into cdmVersion.
> +  mozilla::Tuple<cdm::Host_8*, cdm::Host_9*, int&> hostInfo{ host8, host9, cdmVersion };

In general, it's not advisable to use tuples , and you're better to define a custom struct with thoughtfully named fields for your data, as that means when you get/set the values it's clear what the data is. Rather than get(2) = blah, etc. Which is unclear.

::: dom/media/gmp/GMPContentChild.cpp:146
(Diff revision 2)
> +  // When we successfully create cdm, the cdm will call ChromiumCdmHost,
> +  // we will write the current cdm version into cdmVersion.
> +  mozilla::Tuple<cdm::Host_8*, cdm::Host_9*, int&> hostInfo{ host8, host9, cdmVersion };
>  
>    void* cdm = nullptr;
> -  GMPErr err = mGMPChild->GetAPI(CHROMIUM_CDM_API, host, &cdm);
> +  GMPErr err = mGMPChild->GetAPI(CHROMIUM_CDM_API, &hostInfo, &cdm);

The CHROMIUM_CDM_API is defined as "chromium-cdm8-host4" here:
https://searchfox.org/mozilla-central/rev/1033bfa26f6d42c1ef48621909f04e734a7ed8a3/dom/media/gmp/GMPUtils.h#17

You should update the value of that string to reflect the new CDM version we support, and add a new string define CHROMIUM_CDM_API_BACKCWARD_COMPAT for the old string value and query for both here.

In the case where the query for CHROMIUM_CDM_API_BACKCWARD_COMPAT succeeds, you'll have a version 8 CDM, and so you'll need to create the CDM_9 wrapper that adapts CDM version 8 to the CDM version 9 interface. Then you can call ChromiumCDMChild::Init() with a version 9 CDM always; the actual CDM if it supports version 9, or an adapter implementation of CDM version 9 that wraps the version 8 CDM.

Here's the commit where I removed the code to handle multiple CDM versions at once:
https://hg.mozilla.org/mozilla-central/rev/355453560577

What I'm suggesting here is similar to the code I removed in the commit above.
Attachment #8915915 - Flags: review?(cpearce) → review-
Comment on attachment 8916037 [details]
Bug 1406080 - Part2 - Reject creating with unsupported cdm version in clearkey cdm and cdm-fake.

https://reviewboard.mozilla.org/r/187300/#review192760
Attachment #8916037 - Flags: review?(cpearce) → review+
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review192768

::: dom/media/gmp/ChromiumCDMAdapter.cpp:153
(Diff revision 2)
> +             kEMEKeySystemWidevine.get(),
> +             kEMEKeySystemWidevine.Length(),
> +             &ChromiumCdmHost,
> +             aHostAPI));
> +    if (!cdm9) {
> +      GMP_LOG("ChromiumCDMAdapter::GMPGetAPI(%s, 0x%p, 0x%p, %u) this=0x%p "

A follow up on these two comments here; you may not need to address them once you've addressed my other comment about moving the wrapper to a CDM_9 instance.
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review192726

> The difference between CDM_8 and CDM_9 interfacecs is the OnQueryOutputProtectionStatus() and OnStorageId() functions. So instead of defining a wrapper base, you can instead create an implementation of CDM_9 that wraps CDM and calls through to the wrapped CDM_8 interface for functions that exist on both CDM_9 and CDM_8, and just ignores the functions that CDM_9 has that CDM_8 does not have.
> 
> You can deallocate the wrapper in the CDM_9::Destroy() implementation.

I think the difference between CDM_8 and CDM_9 interfacecs is OnStorageId and GetStatusForPolicy.

> Just make this a ContentDecryptionModule_8*.

I think its a typo, ContentDecryptionModule_9* right?
I've addressed the review feedback and upload a revised patch.

Please review it again.

And attach treeherder link for reference
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98f5662c4801889b77122d002eed11d7afc14d18

Thank you.
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review194774

Thanks, almost there.

::: dom/media/gmp/ChromiumCDMAdapter.cpp:126
(Diff revision 3)
>            aHostAPI,
>            aPluginAPI,
>            aDecryptorId,
>            this);
> -  if (!strcmp(aAPIName, CHROMIUM_CDM_API)) {
> +  bool isCDM9 = !strcmp(aAPIName, CHROMIUM_CDM_API);
> +  bool isCDM8 = !strcmp(aAPIName, CHROMIUM_CDM_API_BACKWARD_COMPACT);

s/COMPACT/COMPAT/

BACKWARDS_COMPAT stands for "backwards compatible".

::: dom/media/gmp/ChromiumCDMChild.cpp:352
(Diff revision 3)
>                                     uint32_t aLegacyDestinationUrlLength)
>  {
> +  OnSessionMessage(aSessionId, aSessionIdSize, aMessageType, aMessage, aMessageSize);
> +}
> +
> +// new interface on cdm::Host_9

I think this comment would be better as "cdm::Host_9 interface". If you put "new" in there, it will soon not be new and the comment will be out of date.

::: dom/media/gmp/GMPContentChild.cpp:157
(Diff revision 3)
> +  }
> +
> +  void GetStatusForPolicy(uint32_t aPromiseId,
> +                          const cdm::Policy& policy) override
> +  {
> +    //Only support on version 9 CDM.

The comment for GetStatusForPolicy() says: 

  // The CDM must respond by calling either Host::OnResolveKeyStatusPromise()
  // with the result key status or Host::OnRejectPromise() if an unexpected
  // error happened or this method is not supported.

So you should reject the promise here, and when you come to implement the HDCP check on the Firefox side, handle that rejection sensibly.

::: dom/media/gmp/GMPContentChild.cpp:287
(Diff revision 3)
> +  GMPErr err = mGMPChild->GetAPI(CHROMIUM_CDM_API, host9, &cdm);
> +  if (err != GMPNoErr || !cdm) {
> +    // Try to create older version 8 CDM.
> +    cdm::Host_8* host8 = child;
> +    err = mGMPChild->GetAPI(CHROMIUM_CDM_API_BACKWARD_COMPACT, host8, &cdm);
> +    cdm =

Operator new is infallible; if we fail to allocate, we abort the process and shutdown. So you don't need to null-check the result of new.

::: dom/media/gmp/GMPUtils.h:17
(Diff revision 3)
>  #include "nsStringFwd.h"
>  #include "nsTArray.h"
>  #include "nsCOMPtr.h"
>  #include "nsClassHashtable.h"
>  
> -#define CHROMIUM_CDM_API "chromium-cdm8-host4"
> +#define CHROMIUM_CDM_API_BACKWARD_COMPACT "chromium-cdm8-host4"

s/COMPACT/COMPAT/
Attachment #8915915 - Flags: review?(cpearce) → review-
Comment on attachment 8915915 [details]
Bug 1406080 - Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9.

https://reviewboard.mozilla.org/r/187178/#review194808

::: dom/media/gmp/GMPContentChild.cpp:299
(Diff revision 5)
> +    // Try to create older version 8 CDM.
> +    cdm::Host_8* host8 = child;
> +    err = mGMPChild->GetAPI(CHROMIUM_CDM_API_BACKWARD_COMPAT, host8, &cdm);
> +    cdm =
> +      new ChromiumCDM8BackwardsCompat(
> +        host8,

You probably want the Host9 interface here, so that you can call any new Host9 functions in future.
Attachment #8915915 - Flags: review?(cpearce) → review+
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb69687aa69a
Part1 - Make ChromiumCDMAdapter and ChromiumCDMChild compatible with CDM version 8 and 9. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/54bf60ede8e8
Part2 - Reject creating with unsupported cdm version in clearkey cdm and cdm-fake. r=cpearce
Blocks: 1417332
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: