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

Status: RESOLVED FIXED

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

Description

[James Cheng[:JamesCheng]]

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.

Comment 1

[James Cheng[:JamesCheng]]

Attachment: [mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

Review commit: https://reviewboard.mozilla.org/r/62268/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62268/

Comment 2

[James Cheng[:JamesCheng]]

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.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 4

[James Cheng[:JamesCheng]]

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/

Comment 5

[James Cheng[:JamesCheng]]

Attachment: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8767901 [details]
[mq]: Bug 1284192 - get rid of GMPErr from CDMProxy base class.

https://reviewboard.mozilla.org/r/62268/#review61110

Comment 8

[James Cheng[:JamesCheng]]

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/

Comment 9

[James Cheng[:JamesCheng]]

Attachment: Make CDMProxy be a base class and move the logic into subclass. r=cpearce

Sorry I messed up with Mozreview... So fallback to patch-based review.

Carry r+ from cpearce.

Comment 10

[James Cheng[:JamesCheng]]

Attachment: Get rid of GMPErr from CDMProxy base class. r=cpearce

Thank you Chris for your comment 6.

Please help me for reviewing this patch.

Thank you:)

Comment 11

[James Cheng[:JamesCheng]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c96c3fd5800

Comment 12

[James Cheng[:JamesCheng]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9a28aa25590

Comment 13

[James Cheng[:JamesCheng]]

Attachment: Make CDMProxy be a base class and move the logic into subclass. r?cpearce

Updated and carry r+ and fix static analysis failed as 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e822588995c&selectedJob=23868514

Comment 14

[James Cheng[:JamesCheng]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=732375b3a873

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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...

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Also, please use mozreview for review requests.

Comment 17

[James Cheng[:JamesCheng]]

Attachment: Part2-get rid of GMPErr from CDMProxy base class. r?cpearce

Thanks for the reviewing, sorry that I will use mozreview next time.

Addressed comment 15.

Carry r+ from cpearce.

Comment 18

[James Cheng[:JamesCheng]]

Attachment: Part1- Make CDMProxy be a base class and move the logic into subclass. r?cpearce

Rebased.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/813a61c61c6c
https://hg.mozilla.org/mozilla-central/rev/340f2259b820