1148071 - [EME] Handle CDM updating during playback

Status: VERIFIED FIXED

(Core :: Audio/Video, defect, P1)

Description

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

If the CDM is updated while the CDM is in use, we need to ensure that playback does not fail.

I suspect that it could be the case that the GMPDecryptor from the old version would be in use, but a new GMPVideoDecoder will be created from the new plugin.

The nodeId will get us part of the way, but we may need to do more work.

Comment 1

[Stephen A Pohl [:spohl]]

Instructions to test a CDM update during playback:

1. Download a copy of the update.xml [1].
2. Obtain two Primetime CDM zip files of different versions, both able to play back encrypted content. These will be referred to as "v1" and "v2" respectively in the steps below.
3. Obtain or generate the sha512 of both Primetime zip files.
4. Host the CDM zip files on a web server.
5. Change the URL for "gmp-eme-adobe" in the update.xml file (obtained in point 1) to point to the v1 Primetime CDM hosted in point 4.
6. Change the hashValue for "gmp-eme-adobe" in the update.xml file to the sha512 of the v1 Primetime CDM.
7. Host the update.xml file on a web server.
8. Start Firefox with a new profile.
9. Type "about:config" in the URL bar.
10. Create a new string key "media.gmp-manager.url.override". Set the value to the URL of your update.xml file hosted in point 7.
11. Reset the "media.gmp-manager.lastCheck" pref if one is present.
12. Quickly shut down Firefox (but do NOT kill process via Activity Manager or similar).
13. Remove the gmp-eme-adobe directory (if any) from your user profile.
14. Start Firefox and play back encrypted content. Wait for content to play back before proceeding to the next step. This will ensure that the v1 Primetime CDM has been downloaded properly.
15. At this point, you may want to create a bookmark to the encrypted content player to have quick access to this page for step 21.
16. Change the URL for "gmp-eme-adobe" in the update.xml file on the web server (step 7) to point to the v2 Primetime CDM hosted in point 4.
17. Change the hashValue for "gmp-eme-adobe" in the update.xml file to the sha512 of the v2 Primetime CDM.
18. Open a new tab and type "about:config" in the URL bar.
19. Reset the "media.gmp-manager.lastCheck" pref.
20. Shut down Firefox.
21. Start Firefox and quickly play back encrypted content (the bookmark from step 15 may be handy). Content playback needs to start within 60 seconds of Firefox startup to ensure that the v1 Primetime CDM is used.
22. Wait several minutes to ensure that the v2 Primetime CDM is downloaded and registered. You can monitor this by opening a new tab, entering "about:addons" in the URL bar and clicking on Plugins. Once the v2 Primetime CDM has been downloaded, the new version will be reflected there.

[1] https://aus4.mozilla.org/update/3/GMP/39.0a1/20150320030211/WINNT_x86-msvc/en-US/nightly/Windows_NT%206.1.1.0%20%28x64%29/default/default/update.xml

Comment 2

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1148071-handle-stream-switch-pre-e10s.patch

A few things going on in this patch:
 * Add CDM version to node ID hash so that we get the same CDM every time for a given CDMProxy.
 * Add a `CanDefer' flag to removePluginDirectory and RemoveOnGMPThread to tell the GMPService to wait for the CDM to die before removing it properly.
 * Make sure the child process using the CDM DLL is dead before trying to delete it.

Comment 3

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8588896 [details] [diff] [review]
1148071-handle-stream-switch-pre-e10s.patch

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

Aurora-based patch. Patch for trunk coming soon to a bug near you (this one).

Comment 4

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

Comment on attachment 8588896 [details] [diff] [review]
1148071-handle-stream-switch-pre-e10s.patch

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

r+ with nits.

Really need an automated test for this somehow.

::: dom/media/gmp/GMPService.cpp
@@ +793,5 @@
>      nsCString api(aAPI);
> +    size_t index = 0;
> +
> +    while (GMPParent* gmp = FindPluginForAPIFrom(index, api, *aTags, &index)) {
> +      if (aOutVersion.IsEmpty() || gmp->GetVersion() > aOutVersion) {

You're doing a string comparison here. This won't actually work, as "9" < "10" returns false for example. You need to actually convert the version string to a number. This is complicated by the fact that OpenH264 uses an X.x format, and EME plugins using a X format. So you probably need to parse the version to a float, and do the less than comparision on that.

::: dom/media/gmp/mozIGeckoMediaPluginService.idl
@@ +103,3 @@
>     * @note Main-thread API.
>     */
> +  void removeAndDeletePluginDirectory(in AString directory,

Need to rev the uuid.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Handle stream switch. Review comments addressed; based on Aurora.

Carry over r+ from comment 4.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: Handle stream switch. Rebased for Beta.

Carry over r+ from comment 4.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8590041 [details] [diff] [review]
Handle stream switch. Review comments addressed; based on Aurora.

Approval Request Comment
[Feature/regressing bug #]: Encrypted Media Extensions/Gecko Media Plugins
[User impact if declined]: Playback stops when an EME CDM is updated, and the old CDM is not deleted from disk. Further, the CDM is not deleted from disk when EME is disabled.
[Describe test coverage new/current, TreeHerder]: None. This patch cannot be applied to trunk since bug 1057908 landed and differs significantly from the trunk version.
[Risks and why]: Small possibility of breaking encrypted media playback.
[String/UUID change made/needed]: Touches mozIGeckoMediaPluginService UUID.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Green on Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e14b189c9d1

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1148071-handle-stream-switch-post-e10s.patch

Now with more e10s action

Comment 10

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1148071-handle-stream-switch-post-e10s.patch

Now with more e10s _and_ |hg qref| action!

Comment 11

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

Comment on attachment 8590103 [details] [diff] [review]
1148071-handle-stream-switch-post-e10s.patch

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

r+, but if it's possible to remove the extra round trip through the main thread in CDMProxy::Init and friends, that would be good.

::: dom/media/eme/CDMProxy.cpp
@@ +197,5 @@
> +  // Add plugin version string to node ID so that we get the same
> +  // CDM every time during a stream's playback.
> +  // We have to do this on the main thread in |mt_GetPluginVersionRunnable|
> +  // which then calls back to gmp_InitGetNodeId().
> +  NS_DispatchToMainThread(new mt_GetPluginVersionRunnable(this, Move(aData)));

Why can't you read the plugin version from the main thread in CDMProxy::Init() and just add it as a field to struct InitData? Why does Init() have to dispatch to the GMP thread, only to have it dispatch back tot he main thread and then dispatch back to the GMP thread to continue initializing?

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Edwin: does this all need to uplift together once it lands?  Or is the patch in comment 7 the part that's different for aurora, and the rest of the work you're doing will stay on 40/nightly?

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

I think I see now. These are the same patch but rebased for beta, aurora, and nightly. 

Let's uplift this now and verify it on aurora using the steps in comment 1.  Florin if your team can work on verification on Monday that would be awesome.

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8590041 [details] [diff] [review]
Handle stream switch. Review comments addressed; based on Aurora.

Approving for uplift to aurora; while there is some risk we do have steps to verify the fix.

Comment 15

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/ed573da17975

Comment 16

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/260b09772972

Comment 17

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

Comment on attachment 8590042 [details] [diff] [review]
Handle stream switch. Rebased for Beta.

Approval Request Comment
[Feature/regressing bug #]: Encrypted Media Extensions/Gecko Media Plugins
[User impact if declined]: Playback stops when an EME CDM is updated, and the old CDM is not deleted from disk. Further, the CDM is not deleted from disk when EME is disabled. Adobe will be spinning a new CDM version soon, so it would be good to get this in before the CDM updates.
[Describe test coverage new/current, TreeHerder]: The aurora and central version of this patch have been landed for a few days now.
[Risks and why]: Small possibility of breaking encrypted media playback.
[String/UUID change made/needed]: Touches mozIGeckoMediaPluginService UUID.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/260b09772972

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Jorge, is that acceptable? Thanks
> [String/UUID change made/needed]: Touches mozIGeckoMediaPluginService UUID.

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Liz Henry (:lizzard) from comment #13)
> I think I see now. These are the same patch but rebased for beta, aurora,
> and nightly. 
> 
> Let's uplift this now and verify it on aurora using the steps in comment 1. 
> Florin if your team can work on verification on Monday that would be awesome.

Sorry, Monday was a holiday for us, so no verification possible. Assigning to Alexandra since she monitors EME related fixes.

Comment 21

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Jorge, is that acceptable? Thanks
> > [String/UUID change made/needed]: Touches mozIGeckoMediaPluginService UUID.

Looks okay to me.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590042 [details] [diff] [review]
Handle stream switch. Rebased for Beta.

Jorge approved, I am happy!
Should be in 38 beta 5.

Comment 23

[Ada [:adalucinet]]

Verified as fixed with latest Developer Edition 39.0a2 (2015-04-14) and Nightly 40.0a1 (2015-04-15) with str provided in comment 1: while CDM is in use via http://people.mozilla.org/~cpearce/mse-clearkey/, updates are properly applied and playback does not fail. Verified with updates from:
- version 6 (bug 1149876) to version 7 (bug 1150781) on 39.0a2
- version 3 (bug 1140944) to version 7 and version 7 to version 8 (bug 1154590) on 40.0a1

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6c7e8d9f955c

Comment 25

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

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #23)
> Verified as fixed with latest Developer Edition 39.0a2 (2015-04-14) and
> Nightly 40.0a1 (2015-04-15) with str provided in comment 1: while CDM is in
> use via http://people.mozilla.org/~cpearce/mse-clearkey/, updates are
> properly applied and playback does not fail. Verified with updates from:
> - version 6 (bug 1149876) to version 7 (bug 1150781) on 39.0a2
> - version 3 (bug 1140944) to version 7 and version 7 to version 8 (bug
> 1154590) on 40.0a1

http://people.mozilla.org/~cpearce/mse-clearkey/ does not use the Adobe CDM, it uses the "ClearKey CDM", which is not updated by the update mechanism in question here. 

So you tested that playback using the ClearKey CDM is not affected while the Adobe CDM is updated.

Whereas we need to test that playback using the *Adobe* CDM is not affected while the Adobe CDM is updated.

So please can you retest, but using this URL:

http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/

This uses the Adobe CDM.

Note that if you're testing in beta, you will also need to set the pref media.mediasource.whitelist to false.

Thanks!

Comment 26

[Ada [:adalucinet]]

Chris, thanks for the heads up!
Encountered the same results with http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/ test page: updates are properly applied and playback does not fail on latest Developer Edition 39.0a2 (2015-04-15) and Nightly 40.0a1 (2015-04-15).

Comment 27

[Ada [:adalucinet]]

Verified as fixed on Firefox 38 beta 5 (Build ID: 20150416143048) as well, under Windows 8.1 64 bit.