Closed Bug 1404230 Opened 6 years ago Closed 5 years ago

[EME] Support HDCP Policy check on MediaKeys

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: JamesCheng, Assigned: JamesCheng, NeedInfo)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 5 obsolete files)

Bug 1404230 - Part1 - Add media.eme.hdcp-policy-check.enabled for flexibility turning on/off this feature.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.
59 bytes, text/x-review-board-request
cpearce
: review+
smaug
: review+
Details
Bug 1404230 - Part3 - Add GetStatusForPolicy method in CDMProxy and its derived classes.
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it by calling CDM.
59 bytes, text/x-review-board-request
cpearce
: review+
Details 
Note:
1. Assume that we only support this feature on widevine CDM.
2. I will provide Part3 which may ask CDM the policy and check if the CDM has supported this API(available in cdm::Host_9::GetStatusForPolicy).
3. If we could land these patches before updating to cdm::Host_9 CDM, the pref should be off.
Rank: 15
Priority: -- → P2
Depends on: 1406080
Group: mozilla-employee-confidential
Blocks: 1411205
Comment on attachment 8921400 [details]
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.

https://reviewboard.mozilla.org/r/192444/#review197572


C/C++ static analysis found 1 defect 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/eme/MediaKeysPolicy.cpp:30
(Diff revision 1)
> +    mMinHdcpVersion(aMinHdcpVersion)
> +
> +{
> +}
> +
> +MediaKeysPolicy::~MediaKeysPolicy()

Warning: Use '= default' to define a trivial destructor [clang-tidy: modernize-use-equals-default]
I plan to let cpearce to review first and find a DOM peer to review Part2.

Meanwhile, I will do the test case in Bug 1411205.
Attachment #8921399 - Flags: review?(cpearce)
Attachment #8921400 - Flags: review?(cpearce)
Attachment #8921401 - Flags: review?(cpearce)
Attachment #8921402 - Flags: review?(cpearce)
Comment on attachment 8921402 [details]
Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it by calling CDM.

https://reviewboard.mozilla.org/r/192448/#review197580


C/C++ static analysis found 1 defect 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/ChromiumCDMChild.cpp:638
(Diff revision 1)
> +ToCDMHdcpVersion(const nsCString& aMinHdcpVersion)
> +{
> +  // String compare with ignoring case.
> +  if (aMinHdcpVersion.IsEmpty()) {
> +    return cdm::HdcpVersion::kHdcpVersionNone;
> +  } else if (aMinHdcpVersion.EqualsIgnoreCase("hdcp-1.0")) {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
obsolete the patches.
Attachment #8914253 - Attachment is obsolete: true
Attachment #8914254 - Attachment is obsolete: true
Attachment #8914255 - Attachment is obsolete: true
Attachment #8917437 - Attachment is obsolete: true
Comment on attachment 8921400 [details]
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.

https://reviewboard.mozilla.org/r/192444/#review199738

::: dom/media/eme/MediaKeys.cpp:612
(Diff revision 3)
> +  if (aRv.Failed()) {
> +    return nullptr;
> +  }
> +
> +  // Currently, only widevine CDM supports for this API.
> +  if (!IsWidevineKeySystem(mKeySystem)) {

It would be good to have some kind of automated test, i.e. it would be good to support something in ClearKey here.

::: dom/webidl/MediaKeysPolicy.webidl:10
(Diff revision 3)
> + *
> + * Copyright © 2014 W3C® (MIT, ERCIM, Keio, Beihang), All Rights Reserved.
> + * W3C liability, trademark and document use rules apply.
> + */
> + 
> +// https://github.com/WICG/media-capabilities/blob/master/eme-extension-policy-check.md

Trailing whitespace here.
Attachment #8921400 - Flags: review?(cpearce) → review+
Comment on attachment 8921402 [details]
Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it by calling CDM.

https://reviewboard.mozilla.org/r/192448/#review199744

::: dom/media/eme/MediaKeys.cpp:642
(Diff revision 3)
> +  RefPtr<DetailedPromise> promise(RetrievePromise(aId));
> +  if (!promise) {
> +    return;
> +  }
> +  RefPtr<MediaKeys> keys(this);
> +  EME_LOG("MediaKeys[%p]::ResolvePromiseWithKeyStatus() resolve promise id=%d", this, aId);

You should log the status here too.

::: dom/media/gmp/ChromiumCDMChild.cpp:685
(Diff revision 3)
> +    // ChromiumCDM8BackwardsCompat::GetStatusForPolicy will reject the promise
> +    // since this API is only supported by CDM version 9.
> +    // CDM will callback by OnResolveKeyStatusPromise when it successfully executes.
> +    policy.min_hdcp_version = ToCDMHdcpVersion(aMinHdcpVersion);
> +    mCDM->GetStatusForPolicy(aPromiseId, policy);
> +  }

You should reject if mCDM is null. Other Recv functions do this, as a shutdown can easily occur at unexpected times, and something waiting on the promise to resolve may wait forever if we don't reject it.
Attachment #8921402 - Flags: review?(cpearce) → review+
Comment on attachment 8921401 [details]
Bug 1404230 - Part3 - Add GetStatusForPolicy method in CDMProxy and its derived classes.

https://reviewboard.mozilla.org/r/192446/#review199740

::: dom/media/eme/CDMProxy.h:229
(Diff revision 3)
>  
>    virtual void GetSessionIdsForKeyId(const nsTArray<uint8_t>& aKeyId,
>                                       nsTArray<nsCString>& aSessionIds) = 0;
>  
> +  // Main thread only.
> +  // Sends a new certificate to the CDM.

"Sends a new certificate to the CDM" is a copy paste mistake I assume.

::: dom/media/eme/mediadrm/MediaDrmCDMProxy.cpp:381
(Diff revision 3)
> +MediaDrmCDMProxy::GetStatusForPolicy(PromiseId aPromiseId,
> +                                     const nsAString& aMinHdcpVersion)
> +{
> +  // TODO: Implement GetStatusForPolicy.
> +  RejectPromise(aPromiseId, NS_ERROR_DOM_NOT_SUPPORTED_ERR,
> +                NS_LITERAL_CSTRING("Currently Fennec did not support GetStatusForPolicy"));

s/did/does/
Attachment #8921401 - Flags: review?(cpearce) → review+
Comment on attachment 8921399 [details]
Bug 1404230 - Part1 - Add media.eme.hdcp-policy-check.enabled for flexibility turning on/off this feature.

https://reviewboard.mozilla.org/r/192442/#review199756

I feel like this pref should be disabled by default until we've had a chance to test this with a real CDM.
Attachment #8921399 - Flags: review?(cpearce) → review-
Comment on attachment 8921402 [details]
Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it by calling CDM.

https://reviewboard.mozilla.org/r/192448/#review199766

::: dom/media/gmp/ChromiumCDMChild.cpp:635
(Diff revision 3)
>  }
>  
> +static cdm::HdcpVersion
> +ToCDMHdcpVersion(const nsCString& aMinHdcpVersion)
> +{
> +  // String compare with ignoring case.

It's probably worth sticking a comment with a link to https://cs.chromium.org/chromium/src/media/blink/webcontentdecryptionmodule_impl.cc?rcl=9d4e17194fbae2839d269e0b625520eac09efa9b&l=40 so it's clear where these magic strings come from; I couldn't see them in any spec.
(In reply to Chris Pearce (:cpearce) from comment #23)
> Comment on attachment 8921399 [details]
> Bug 1404230 - Part1 - Add media.eme.hdcp-policy-check.enabled for
> flexibility turning on/off this feature.
> 
> https://reviewboard.mozilla.org/r/192442/#review199756
> 
> I feel like this pref should be disabled by default until we've had a chance
> to test this with a real CDM.

Yes, as comment 4 said.

So I've changed to default false.
Comment on attachment 8921400 [details]
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.

https://reviewboard.mozilla.org/r/192444/#review199738

> It would be good to have some kind of automated test, i.e. it would be good to support something in ClearKey here.

Currently, I didn't know what should I do in clearkey. I will create a follow-up bug and wait for this API phase in  standard spec and consider what to do in Clearkey implementation, thanks.
(In reply to Chris Pearce (:cpearce) from comment #21)
> Comment on attachment 8921402 [details]
> Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it
> by calling CDM.

> ::: dom/media/gmp/ChromiumCDMChild.cpp:685
> (Diff revision 3)
> > +    // ChromiumCDM8BackwardsCompat::GetStatusForPolicy will reject the promise
> > +    // since this API is only supported by CDM version 9.
> > +    // CDM will callback by OnResolveKeyStatusPromise when it successfully executes.
> > +    policy.min_hdcp_version = ToCDMHdcpVersion(aMinHdcpVersion);
> > +    mCDM->GetStatusForPolicy(aPromiseId, policy);
> > +  }
> 
> You should reject if mCDM is null. Other Recv functions do this, as a
> shutdown can easily occur at unexpected times, and something waiting on the
> promise to resolve may wait forever if we don't reject it.

I didn't see any Recv functions with `aPromiseId` that handle the `mCDM is nullptr` case.

I noticed that I thought it is not necessary to check....

I will file a new bug to handle those Recv functions with promise id once.

ni? you if I misunderstand your word.

Thanks.
Flags: needinfo?(cpearce)
Blocks: 1413480
Blocks: 1413481
Attachment #8921399 - Flags: review?(cpearce)
Attachment #8921400 - Flags: review?(bugs)
Comment on attachment 8921399 [details]
Bug 1404230 - Part1 - Add media.eme.hdcp-policy-check.enabled for flexibility turning on/off this feature.

I don't know why I cancel the review.... r? again via bugzilla...
Attachment #8921399 - Flags: review?(cpearce)
Hi smaug,

Would like your help to review the change of webidl.

Thank you.
Is there a spec for this stuff?
Or is it just that random github readme file?
I don't understand the API. Why do we need MediaKeysPolicy? Just to pass it as a param to getStatusForPolicy? Why getStatusForPolicy doesn't take MediaKeysPolicyInit as a param?
Comment on attachment 8921400 [details]
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.

https://reviewboard.mozilla.org/r/192444/#review200438

The API looks weird, and the interface object is exposed to the global object even when media.eme.hdcp-policy-check.enabled is false.
Please explain why the odd looking API is reasonable (and exposed the interface only when media.eme.hdcp-policy-check.enabled is set), or get the API changed and implement that.

::: dom/webidl/MediaKeysPolicy.webidl:17
(Diff revision 4)
> +dictionary MediaKeysPolicyInit {
> +  DOMString minHdcpVersion = "";
> +};
> +
> +[Constructor(optional MediaKeysPolicyInit init), Exposed=Window]
> +interface MediaKeysPolicy {

So you expose MediaKeysPolicy to the global scope even if media.eme.hdcp-policy-check.enabled isn't set.
Attachment #8921400 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #37)
> Comment on attachment 8921400 [details]
> Bug 1404230 - Part2 - Add a web api to support HDCP policy check on
> MediaKeys.
> 
> https://reviewboard.mozilla.org/r/192444/#review200438
> 
> The API looks weird, and the interface object is exposed to the global
> object even when media.eme.hdcp-policy-check.enabled is false.
> Please explain why the odd looking API is reasonable (and exposed the
> interface only when media.eme.hdcp-policy-check.enabled is set), or get the
> API changed and implement that.
> 
Sorry, I didn't provide the related information about this API.

https://github.com/WICG/media-capabilities/blob/master/eme-extension-policy-check.md

This API has been discussed in WICG and has implemented by Chrome

https://www.chromestatus.com/feature/5652917147140096

Since we used Widevine Content Decryption Module(CDM) for our DRM decryption backend.

Google will provide this API's backend CDM support in Q4.

I agree that there's no need to have a struct, getStatusForPolicy(with a simple string) will be enough.

But I saw the discussion in WICG, it sounds like the MediaKeysPolicy will be extended with other policies.

We will keep monitoring the change but now I just follow the WICG example to implement.

Hope you can accept my explaination.

Thank you.
> ::: dom/webidl/MediaKeysPolicy.webidl:17
> (Diff revision 4)
> > +dictionary MediaKeysPolicyInit {
> > +  DOMString minHdcpVersion = "";
> > +};
> > +
> > +[Constructor(optional MediaKeysPolicyInit init), Exposed=Window]
> > +interface MediaKeysPolicy {
> 
> So you expose MediaKeysPolicy to the global scope even if
> media.eme.hdcp-policy-check.enabled isn't set.

Thanks, will update a new patch for review.
I should not accept the patch, based on not having a spec or anything.
Let me ask some Google folks what is happening here.
(I'll probably r+, but need to understand why we're in this bad situation again - implementing random stuff without proper specs)
So, I was asking someone from Google, and apparently there hasn't been intent-to-ship yet, so we have chance to improve the API.
I did file https://github.com/WICG/media-capabilities/issues/49 and would like to see either some clarification why the API looks like what it is, or get it simplified.
Thanks, I thought I can search the code in Chromium means they ship it...
My understanding is that they have just had intent-to-implement, not intent-to-ship.
But please correct me if I'm wrong.
(In reply to James Cheng[:JamesCheng] from comment #31)
> (In reply to Chris Pearce (:cpearce) from comment #21)
> > Comment on attachment 8921402 [details]
> > Bug 1404230 - Part4 - Add GetStatusForPolicy method in ipdl and implement it
> > by calling CDM.
> 
> > ::: dom/media/gmp/ChromiumCDMChild.cpp:685
> > (Diff revision 3)
> > > +    // ChromiumCDM8BackwardsCompat::GetStatusForPolicy will reject the promise
> > > +    // since this API is only supported by CDM version 9.
> > > +    // CDM will callback by OnResolveKeyStatusPromise when it successfully executes.
> > > +    policy.min_hdcp_version = ToCDMHdcpVersion(aMinHdcpVersion);
> > > +    mCDM->GetStatusForPolicy(aPromiseId, policy);
> > > +  }
> > 
> > You should reject if mCDM is null. Other Recv functions do this, as a
> > shutdown can easily occur at unexpected times, and something waiting on the
> > promise to resolve may wait forever if we don't reject it.
> 
> I didn't see any Recv functions with `aPromiseId` that handle the `mCDM is
> nullptr` case.

https://searchfox.org/mozilla-central/rev/f8ed355d1932e884104df3379f21c2753d549acb/dom/media/gmp/ChromiumCDMChild.cpp#523

It's necessary to check whether mCDM is null, as the CDM can shutdown after messages have been inserted in the child process' message queue but before those message have been processed.

> I will file a new bug to handle those Recv functions with promise id once.

Thanks.
Flags: needinfo?(cpearce)
Comment on attachment 8921399 [details]
Bug 1404230 - Part1 - Add media.eme.hdcp-policy-check.enabled for flexibility turning on/off this feature.

https://reviewboard.mozilla.org/r/192442/#review201702
Attachment #8921399 - Flags: review?(cpearce) → review+
FWIW, sounds like the proposal will change to be less weird
https://github.com/WICG/media-capabilities/issues/49
No longer blocks: 1413480
(In reply to Olli Pettay [:smaug] from comment #48)
> FWIW, sounds like the proposal will change to be less weird
> https://github.com/WICG/media-capabilities/issues/49

I've updated my patches according to the spec change.

Thank you smaug for pushing the API more reasonable.
Comment on attachment 8921400 [details]
Bug 1404230 - Part2 - Add a web api to support HDCP policy check on MediaKeys.

https://reviewboard.mozilla.org/r/192444/#review203334

r+ for the .webidl, but the implementation of the method doesn't seem to follow the spec proposal.

::: dom/media/eme/MediaKeys.cpp:614
(Diff revision 6)
> +
> +  // Currently, only widevine CDM supports for this API.
> +  if (!IsWidevineKeySystem(mKeySystem)) {
> +    EME_LOG("MediaKeys[%p]::GetStatusForPolicy() HDCP policy check on unsupported keysystem ", this);
> +    NS_WARNING("Tried to query without a CDM");
> +    promise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR,

Why this rejection?
The readme says just
'If HDCP is available at the specified version, the promise should return a MediaKeyStatus of "usable". Otherwise, the promise should return a MediaKeyStatus of "output-restricted".'

::: dom/media/eme/MediaKeys.cpp:621
(Diff revision 6)
> +    return promise.forget();
> +  }
> +
> +  if (!mProxy) {
> +   NS_WARNING("Tried to use a MediaKeys without a CDM");
> +   promise->MaybeReject(NS_ERROR_DOM_INVALID_STATE_ERR,

Why this rejection?
The readme says just
'If HDCP is available at the specified version, the promise should return a MediaKeyStatus of "usable". Otherwise, the promise should return a MediaKeyStatus of "output-restricted".'
Attachment #8921400 - Flags: review?(bugs) → review+
If the API is supposed to reject the promise in some cases, that should be spec'ed.
(In reply to Olli Pettay [:smaug] from comment #55)
> If the API is supposed to reject the promise in some cases, that should be
> spec'ed.

I opened an issue [1]!
Thank you for reminding. I think rejecting the promise should be appropriate.


[1]
https://github.com/WICG/media-capabilities/issues/56
Attachment #8921751 - Attachment is obsolete: true
(In reply to James Cheng[:JamesCheng] from comment #56)
> (In reply to Olli Pettay [:smaug] from comment #55)
> > If the API is supposed to reject the promise in some cases, that should be
> > spec'ed.
> 
> I opened an issue [1]!
> Thank you for reminding. I think rejecting the promise should be appropriate.
> 
> 
> [1]
> https://github.com/WICG/media-capabilities/issues/56

The spec will be updated as rejecting the promise with NotSupportedError...

I'll land these patches with pref off and keep monitoring the spec change and do the following actions.
Blocks: EME
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/78a7e39eaa87
Part1 - Add media.eme.hdcp-policy-check.enabled for flexibility turning on/off this feature. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/6e33c46788b4
Part2 - Add a web api to support HDCP policy check on MediaKeys. r=cpearce,smaug
https://hg.mozilla.org/integration/autoland/rev/4958b7135ea3
Part3 - Add GetStatusForPolicy method in CDMProxy and its derived classes. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/5cad2a87e2dd
Part4 - Add GetStatusForPolicy method in ipdl and implement it by calling CDM. r=cpearce

I'll land these patches with pref off and keep monitoring the spec change and do the following actions.

This seems to be shipping in Chrome, right? Is there progress on the spec here?

Flags: needinfo?(ayumiqmazaky)

Seeing as 1573976 is resolved, can Firefox already start checking HDCP capabilities natively without the media.eme.hdcp-policy-check.enabled pref on?

Flags: needinfo?(bvandyk)

(In reply to Tomas Bucher from comment #65)

Seeing as 1573976 is resolved, can Firefox already start checking HDCP capabilities natively without the media.eme.hdcp-policy-check.enabled pref on?

:smaug, given your previous involvement here, do you have any feelings about the above?

Flags: needinfo?(bugs)

Martin, do you have any suggestions about exposing this without a pref and/or other stakeholders?

Flags: needinfo?(mt)

https://github.com/WICG/media-capabilities/blob/master/eme-extension-policy-check.md is gone. Is there a new location for the same material? I'm looking at https://github.com/WICG/hdcp-detection/blob/master/explainer.md right now.

I would ask Henri Sivonen, who did a solid job of ensuring that EME wasn't a total privacy disaster the first time around.

Tom Ritter has been looking at fingerprinting more broadly and that seems to be one of the primary problems here. However, it seems like HDCP level can be learned by other methods (just by attempting to play HDCP-protected content), so maybe it isn't new information. The risk seems to be limited to reducing the effort required to extract the information.

Did this ever make it into the standards positions repository? I don't see it there and it might make sense to do so. That process is designed to find people with a stake more easily.

Flags: needinfo?(mt)

(I'm not the right person to answer to this. Martin gave some better names :) )

Flags: needinfo?(bugs)

Henri, do you have any thoughts on this?

Flags: needinfo?(bvandyk) → needinfo?(hsivonen)

(In reply to Martin Thomson [:mt:] (vacation) from comment #68)

https://github.com/WICG/media-capabilities/blob/master/eme-extension-policy-check.md is gone. Is there a new location for the same material? I'm looking at https://github.com/WICG/hdcp-detection/blob/master/explainer.md right now.

As you say, I believe https://github.com/WICG/hdcp-detection/blob/master/explainer.md is where this now lives.

Did this ever make it into the standards positions repository? I don't see it there and it might make sense to do so. That process is designed to find people with a stake more easily.

I've raised https://github.com/mozilla/standards-positions/issues/243 to track this.

You need to log in before you can comment on or make changes to this bug.