Make CDMProxy be a base class and move the logic into subclass

RESOLVED FIXED in Firefox 50

Status

()

defect
P2
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

(Blocks 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(2 attachments, 5 obsolete attachments)

Currently the CDMProxy is bound with GMP implementation deeply,

We decided to make CDMProxy be a super class and movethe logic into GMPCDMProxy.

For Fennec EME implmentation, we want to have a CDMProxy (FennecCDMPRoxy) which is not based on GMP architecture.
Assignee: nobody → jacheng
Priority: -- → P2
Blocks: 1264840
Blocks: 1284431
Blocks: 1284434
Hi JW,
We would like to implment non-GMP type of CDMProxy, so first is to make CDMProxy to be a base class.

I've also filed a Bug 1284431 which is intended to remove the remaining types of GMP in CDMProxy.h.

Could you please review this patch?

Thank you very much.
https://reviewboard.mozilla.org/r/62268/#review59298

::: dom/media/eme/CDMCallbackProxy.h:58
(Diff revision 1)
>    void Terminated() override;
>  
>    ~CDMCallbackProxy() {}
>  
>  private:
> -  friend class CDMProxy;
> +  friend class GMPCDMProxy;

This is not scalable when we have more CDMProxy sub-classes.

::: dom/media/eme/CDMCallbackProxy.cpp:51
(Diff revision 1)
>  
>  void
>  CDMCallbackProxy::SetSessionId(uint32_t aToken,
>                                 const nsCString& aSessionId)
>  {
> -  MOZ_ASSERT(mProxy->IsOnGMPThread());
> +  MOZ_ASSERT(mProxy->IsOnDesignatedThread());

Maybe call this "OnOwnerThread" or "OnWorkerThread".

::: dom/media/eme/GMPCDMProxy.h:1
(Diff revision 1)
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Use "hg cp" in order not to lose the history.
Depends on: 1284809
Comment on attachment 8767901 [details]
[mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62268/diff/1-2/
Attachment #8767901 - Attachment description: Bug 1284192 - Make CDMProxy be a base class and move the logic into subclass. → [mq]: Bug 1284192 - Make CDMProxy be a base class and move the logic into subclass.
Hi Chris,

We have tried to removed all the GMP types from CDMProxy.

But for |GMPErr|, I am not sure how to deal with the error code type.

Intuitively, 

GMPErr is an enum equivalent to int, so I just use |int| for the error code type.

CDMProxy will be overridden by different implementation e.g. GMPCDMProxy or FennecCDMProxy, so the error code is implementation-dependent, I think using |int| for all error code is OK.

Do you have any suggestions for this point? 

Thank you.
Attachment #8770462 - Flags: feedback?(cpearce)
Attachment #8770462 - Attachment is patch: true
Comment on attachment 8770462 [details] [diff] [review]
Bug 1284192 - get rid of GMPErr from CDMProxy base class.

Review of attachment 8770462 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/eme/CDMProxy.h
@@ -23,4 @@
>      : mStatus(aStatus)
>      , mSample(aSample)
>    {}
> -  GMPErr mStatus;

The only user of DecryptResult::mStatus is here:
https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/agnostic/eme/EMEDecoderModule.cpp#68

You'll note that it cares about 3 cases; no-key-error, success, and failure. So I think you'd be better to create an enum in CDMProxy with those 3 cases, and have the GMPCDMProxy convert from GMPErr to the DecryptResult's enum.

It's better to use enums rather than ints where possible, as enums are type safe. So the type system enforces that the values are in a specified set, whereas you don't get that with integers.
Attachment #8770462 - Flags: feedback?(cpearce)
Comment on attachment 8767901 [details]
[mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

https://reviewboard.mozilla.org/r/62268/#review61110
Attachment #8767901 - Flags: review+
Comment on attachment 8767901 [details]
[mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62268/diff/2-3/
Attachment #8767901 - Attachment description: [mq]: Bug 1284192 - Make CDMProxy be a base class and move the logic into subclass. → [mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.
Attachment #8767901 - Flags: review?(jwwang)
Attachment #8767901 - Attachment is obsolete: true
Attachment #8770462 - Attachment is obsolete: true
Sorry I messed up with Mozreview... So fallback to patch-based review.

Carry r+ from cpearce.
Attachment #8770857 - Flags: review+
Thank you Chris for your comment 6.

Please help me for reviewing this patch.

Thank you:)
Attachment #8770858 - Flags: review?(cpearce)
Comment on attachment 8770858 [details] [diff] [review]
Get rid of GMPErr from CDMProxy base class. r=cpearce

Review of attachment 8770858 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed.

::: dom/media/eme/CDMProxy.h
@@ +30,2 @@
>  struct DecryptResult {
> +  DecryptResult(CDMErr aStatus, MediaRawData* aSample)

I'm not a fan of the XXXErr convention we have in GMP for result status'.

You also don't need the CDMLastErr.

So let's make that:

  enum DecryptStatus {
    Ok = 0,
    GenericErr = 1,
    NoKeyErr = 2,
    AbortedErr = 3,
  };

Then use:

DecryptStatus::Ok, etc...
Attachment #8770858 - Flags: review?(cpearce) → review+
Also, please use mozreview for review requests.
Thanks for the reviewing, sorry that I will use mozreview next time.

Addressed comment 15.

Carry r+ from cpearce.
Attachment #8770858 - Attachment is obsolete: true
Attachment #8772279 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/813a61c61c6c
Make CDMProxy be a base class and move the logic into subclass. r=cpearce
https://hg.mozilla.org/integration/mozilla-inbound/rev/340f2259b820
get rid of GMPErr from CDMProxy base class. r=cpearce
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/813a61c61c6c
https://hg.mozilla.org/mozilla-central/rev/340f2259b820
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Duplicate of this bug: 1284431
You need to log in before you can comment on or make changes to this bug.