[EME] Delay navigator.requestMediaKeySystemAccess while we wait for CDM to download

RESOLVED FIXED in Firefox 38

Status

()

P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla40
x86
Windows Vista
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed, firefox39 fixed, firefox40 fixed)

Details

Attachments

(4 attachments, 7 obsolete attachments)

1.49 KB, patch
Gijs
: review+
Details | Diff | Splinter Review
3.35 KB, patch
markh
: review+
Details | Diff | Splinter Review
23.00 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.45 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
If an EME CDM is enabled, but not yet downloaded, we should delay the resolution of the promise returned by navigator.requestMediaKeySystemAccess() until the CDM has finished downloading.

We can do this by just inspecting the prefs set by the GMP downloading code.
Note that our beta test partner chooses EME or Silverlight based on the User-Agent's version, not client-side feature detection. If they don't want to change their EME detection, can we postpone this fix until after MVP?
Flags: needinfo?(cpearce)
(Assignee)

Comment 2

4 years ago
If we do this fix now, then a page using EME will eventually work, with a way for the streaming partner to detect this condition. If we don't do anything, the user will see an error instead, though the page will work if they refresh often enough to pass the 60s window.

It seems to me that adding an extra delay and having the page eventually work will probably be a better UX than having the page fail with an error.
Flags: needinfo?(cpearce)
(Assignee)

Comment 3

4 years ago
We'll need to uplift this.
Flags: needinfo?(cpearce)
Priority: -- → P1
(Assignee)

Comment 4

4 years ago
STR (32bit Firefox on Windows only):
1. Open Options > Content and toggle "Play DRM Content" off and on again.
2. Quit Firefox quickly (so it doesn't have time to download the Adobe CDM again).
3. Start Firefox.
4. Open http://drmtest2.adobe.com/HTML5AAXSPlayer/2/mse-access/ and press "Play" button
5. Observe "Firefox is installing components requried to play" drop down, and observe video playback doesn't start.

Expected result, eventually playback starts...
(Assignee)

Comment 6

4 years ago
Created attachment 8584822 [details] [diff] [review]
Patch 1v1: Delay navigator.requestMediaKeySystemAccess until CDM downloads

* Create a delegate to delay creating/returning a MediaKeySystemAccess until the CDM is ready.
* When we get a "CDM needs update" or "CDM not yet installed", we'll wait for 60 seconds, or until the CDM is added to the GMPService by the GMP downloader (in Chrome code).
* If we retry a request after the GMPService reports its added a new GMP and the Adobe CDM still isn't available, we'll fail. I think this could only cause an unnecessary fail if we end up having an OpenH264 and Adobe CDM update happening at the same time, but that's probably acceptable.

Ehsan: Can you check the cycle collector stuff in MediaKeySystemAccessManager.cpp please and Navigator::Invalidate() changes? I'm confident JW can handle the rest.
Attachment #8584822 - Flags: review?(jwwang)
Attachment #8584822 - Flags: review?(ehsan)
(Assignee)

Comment 7

4 years ago
Created attachment 8584826 [details] [diff] [review]
Patch 2v1: Make GMPProvider respond when updates are needed

Make GMPProvider listen for "mediakeys-request" observer service notification, and respond to "cdm-not-installed" or "cdm-insufficient-version" statuses by triggering a new update check.

Will the observer service work in e10s?
Flags: needinfo?(cpearce)
Attachment #8584826 - Flags: review?(spohl.mozilla.bugs)
(Assignee)

Comment 8

4 years ago
Created attachment 8584827 [details] [diff] [review]
Patch 3v1: Hide "can't play EME notification box" when we can play

With the above two patches, we can end up displaying a drmContentCDMInsufficientVersion or drmContentCDMInstalling and then recovering and playing EME content. It's weird to leave those notification boxes displaying when that happens so this patch hides them.
Attachment #8584827 - Flags: review?(florian)
(Assignee)

Updated

4 years ago
Flags: needinfo?(cpearce)
Comment on attachment 8584826 [details] [diff] [review]
Patch 2v1: Make GMPProvider respond when updates are needed

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

(In reply to Chris Pearce (:cpearce) from comment #7)
> Will the observer service work in e10s?

I believe so, but you should double-check with one of our e10s pros.

::: toolkit/mozapps/extensions/internal/GMPProvider.jsm
@@ +369,5 @@
> +      }
> +      this._isUpdateCheckPending = false;
> +    }, delay);
> +  },
> +  

nit: whitespace

@@ +374,5 @@
> +  observe: function(subject, topic, data) {
> +    let parsedData;
> +    try {
> +      parsedData = JSON.parse(data);
> +    } catch (ex) {

nit: no space between catch and (ex)

@@ +375,5 @@
> +    let parsedData;
> +    try {
> +      parsedData = JSON.parse(data);
> +    } catch (ex) {
> +      Cu.reportError("Malformed EME video message with data: " + data);

We should use the GMP logging here instead.

@@ +378,5 @@
> +    } catch (ex) {
> +      Cu.reportError("Malformed EME video message with data: " + data);
> +      return;
> +    }
> +    let {status: status, keySystem: keySystem} = parsedData;    

nit: whitespace

@@ +383,5 @@
> +    if (status == "cdm-not-installed" || status == "cdm-insufficient-version") {
> +      this.checkForUpdates(0);
> +    }
> +  },
> +  

nit: whitespace
Attachment #8584826 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8584822 [details] [diff] [review]
Patch 1v1: Delay navigator.requestMediaKeySystemAccess until CDM downloads

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

LGTM.

::: dom/base/Navigator.cpp
@@ +2636,4 @@
>    if (aRv.Failed()) {
>      return nullptr;
>    }
> +  if (!mMediaKeySystemAccessManager) {

Since we want to initialize mMediaKeySystemAccessManager lazily, move this if statement down to initialize mMediaKeySystemAccessManager as lazy as possible.

@@ +2641,2 @@
>    }
> +  if (aKeySystem.IsEmpty() || aOptions.WasPassed() && aOptions.Value().IsEmpty()) {

Warning: -Wparentheses in dom/base/Navigator.cpp: suggest parentheses around ‘&&’ within ‘||’

@@ +2645,3 @@
>    }
>  
> +  auto options = aOptions.WasPassed() ? aOptions.Value()

Take const reference instead of a value copy.

::: dom/base/Navigator.h
@@ +335,5 @@
>    RequestMediaKeySystemAccess(const nsAString& aKeySystem,
>                                const Optional<Sequence<MediaKeySystemOptions>>& aOptions,
>                                ErrorResult& aRv);
> +private:
> +  void RequestMediaKeySystemAccess(Promise* aPromise,

Where is this function defined?

@@ +339,5 @@
> +  void RequestMediaKeySystemAccess(Promise* aPromise,
> +                                   const nsAString& aKeySystem,
> +                                   const Optional<Sequence<MediaKeySystemOptions>>& aOptions);
> +
> +  nsRefPtr<MediaKeySystemAccessManager> mMediaKeySystemAccessManager;

Don't we need to add it to the cycle collection traverse?

::: dom/media/eme/MediaKeySystemAccessManager.cpp
@@ +110,5 @@
> +    aPromise->MaybeReject(NS_ERROR_DOM_NOT_SUPPORTED_ERR);
> +    return;
> +  }
> +
> +  if (aOptions.IsEmpty() ||

We should move all checks for options (including those in Navigator::RequestMediaKeySystemAccess()) into MediaKeySystemAccess::IsSupported() so we have only one place to decide what options are valid and what are not.

@@ +171,5 @@
> +
> +void
> +MediaKeySystemAccessManager::RetryRequest(PendingRequest& aRequest)
> +{
> +  if (aRequest.mTimer) {

How could mTimer be null here?

@@ +230,5 @@
> +
> +  nsTArray<PendingRequest> requests(Move(mRequests));
> +  for (PendingRequest& request : requests) {
> +    // Cancel all requests; we're shutting down.
> +    if (request.mPromise) {

Is it possible for mPromise to be null?

@@ +233,5 @@
> +    // Cancel all requests; we're shutting down.
> +    if (request.mPromise) {
> +      request.mPromise->MaybeReject(NS_ERROR_DOM_INVALID_ACCESS_ERR);
> +    }
> +    if (request.mTimer) {

Ditto.

::: dom/media/eme/MediaKeySystemAccessManager.h
@@ +2,5 @@
> +* License, v. 2.0. If a copy of the MPL was not distributed with this
> +* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_MediaKeySystemAccessManager_h
> +#define mozilla_dom_MediaKeySystemAccessManager_h

The naming convention is inconsistent with the rest headers in dom/media/eme that don't include name space.

@@ +66,5 @@
> +
> +  void RetryRequest(PendingRequest& aRequest);
> +
> +  nsTArray<PendingRequest> mRequests;
> + 

space.

::: dom/media/gmp/GMPService.cpp
@@ +964,5 @@
>  
>    return gmp.get();
>  }
>  
> +class GMPAddedTask : public nsRunnable {

The class can be merged with StorageClearedTask by passing the topic string.
Attachment #8584822 - Flags: review?(jwwang) → review+

Comment 11

4 years ago
Comment on attachment 8584822 [details] [diff] [review]
Patch 1v1: Delay navigator.requestMediaKeySystemAccess until CDM downloads

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

r- for the cycle collection issues.

::: dom/base/Navigator.h
@@ +339,5 @@
> +  void RequestMediaKeySystemAccess(Promise* aPromise,
> +                                   const nsAString& aKeySystem,
> +                                   const Optional<Sequence<MediaKeySystemOptions>>& aOptions);
> +
> +  nsRefPtr<MediaKeySystemAccessManager> mMediaKeySystemAccessManager;

Yes, this should indeed be added to the Navigator's cycle collection participant.

::: dom/media/eme/MediaKeySystemAccessManager.cpp
@@ +1,1 @@
> +#include "MediaKeySystemAccessManager.h"

Did you forget the license header of this file?

@@ +10,5 @@
> +namespace dom {
> +
> +NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION(MediaKeySystemAccessManager)
> +  NS_INTERFACE_MAP_ENTRY(nsIObserver)
> +  NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIObserver)

Hmm, why can't you just use NS_INTERFACE_MAP_ENTRY(nsISupports)?  There is only one nsISupports base class here, so there should be no ambiguity.

@@ +19,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_CLASS(MediaKeySystemAccessManager)
> +
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(MediaKeySystemAccessManager)
> +  tmp->Shutdown();

You should unlink the stuff that you traverse here.  Shutdown() doesn't seem to do that.  If you specialized ImplCycleCollectionUnlink for PendingRequest similar to what I described below, you could use the macros after you called Shutdown().

There is another issue here.  Cycle collected objects are not guaranteed to die through the cycle collector.  It's possible that the object gets deleted normally when its last reference is dropped.  So you should call Shutdown() in the destructor as well.

@@ +24,5 @@
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_END
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(MediaKeySystemAccessManager)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mWindow)
> +  for (size_t i = 0; i < tmp->mRequests.Length(); ++i) {

<https://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1784> is a helper that lets you iterate over the array.  It would be nicer if you specialized ImplCycleCollectionTraverse for PendingRequest and get it to call ImplCycleCollectionTraverse on mPromise.  Then you could just do NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mRequests) here and everything would work.

@@ +223,5 @@
> +  return mAddedObservers;
> +}
> +
> +void
> +MediaKeySystemAccessManager::Shutdown()

Once you call Shutdown() in the dtor, you should also ensure that it's safe to be called multiple times.  But I think that property holds for this function as it is.  Please just double check that.

::: dom/media/gmp/GMPService.cpp
@@ +964,5 @@
>  
>    return gmp.get();
>  }
>  
> +class GMPAddedTask : public nsRunnable {

Please make this final and add a private destructor.

@@ +966,5 @@
>  }
>  
> +class GMPAddedTask : public nsRunnable {
> +public:
> +  NS_IMETHOD Run() {

Please mark this as override.
Attachment #8584822 - Flags: review?(ehsan) → review-
(Assignee)

Comment 12

4 years ago
Created attachment 8585067 [details] [diff] [review]
Patch 1v2: Delay navigator.requestMediaKeySystemAccess until CDM downloads

Reworked for Ehsan's and JW's comments.
Attachment #8584822 - Attachment is obsolete: true
Attachment #8585067 - Flags: review?(ehsan)
(Assignee)

Comment 13

4 years ago
Created attachment 8585068 [details] [diff] [review]
Interdiff of Patch 1 v1 vs v2

Interdiff of Patch 1 between v1 and v2.
Attachment #8585068 - Flags: feedback?(ehsan)
(Assignee)

Updated

4 years ago
Flags: needinfo?(cpearce)
(Assignee)

Comment 16

4 years ago
> (In reply to Chris Pearce (:cpearce) from comment #7)
> > Will the observer service work in e10s?

Apparently the observer service does not work in e10s.
(Assignee)

Comment 17

4 years ago
Created attachment 8585250 [details] [diff] [review]
Patch 2vx: Make GMPProvider respond when updates are needed

Review comments addressed. Carry forward r=spohl.
Attachment #8584826 - Attachment is obsolete: true
Attachment #8585250 - Flags: review+
(Assignee)

Comment 18

4 years ago
Created attachment 8585251 [details] [diff] [review]
Patch 4: Make GMPWrapper e10s compatible

Fix up my patch 2, so that we use the global message manager instead of the observer service. We should then work in e10s Firefox too.
Attachment #8585251 - Flags: review?(mhammond)
Comment on attachment 8585251 [details] [diff] [review]
Patch 4: Make GMPWrapper e10s compatible

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

looks fine to me.
Attachment #8585251 - Flags: review?(mhammond) → review+
Keywords: checkin-needed
Hi, seems part 1 didn't apply cleanly:

renamed 1146201 -> delay-promise-resolution.patch
applying delay-promise-resolution.patch
patching file dom/base/Navigator.h
Hunk #2 FAILED at 103
1 out of 3 hunks FAILED -- saving rejects to file dom/base/Navigator.h.rej
patching file dom/media/eme/MediaKeySystemAccess.h
Hunk #1 FAILED at 16
1 out of 1 hunks FAILED -- saving rejects to file dom/media/eme/MediaKeySystemAccess.h.rej
Keywords: checkin-needed
(Assignee)

Comment 21

4 years ago
Comment on attachment 8584827 [details] [diff] [review]
Patch 3v1: Hide "can't play EME notification box" when we can play

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

Florian is on PTO, Gigs (maybe also on PTO?) or Dolske, can you review this?
Attachment #8584827 - Flags: review?(gijskruitbosch+bugs)
Attachment #8584827 - Flags: review?(florian)
Attachment #8584827 - Flags: review?(dolske)
(Assignee)

Comment 22

4 years ago
Created attachment 8585373 [details] [diff] [review]
Patch 1v3: Delay navigator.requestMediaKeySystemAccess until CDM downloads

Rebased to fix bitrot.

We still need ehsan's review before we can land.
Attachment #8585067 - Attachment is obsolete: true
Attachment #8585067 - Flags: review?(ehsan)
Attachment #8585373 - Flags: review?(ehsan)

Comment 23

4 years ago
Comment on attachment 8585373 [details] [diff] [review]
Patch 1v3: Delay navigator.requestMediaKeySystemAccess until CDM downloads

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

::: dom/media/eme/MediaKeySystemAccessManager.cpp
@@ +51,5 @@
> +}
> +
> +MediaKeySystemAccessManager::~MediaKeySystemAccessManager()
> +{
> +  Shutdown();

Hmm, not sure what changed to make this unnecessary during Unlink now.  Note that Unlink clears out the array so you won't have a chance to reject the promises and whatnot if the object gets freed through the CC, which makes this seem incorrect to me.
Attachment #8585373 - Flags: review?(ehsan) → review-

Updated

4 years ago
Attachment #8585068 - Flags: feedback?(ehsan)

Comment 24

4 years ago
Comment on attachment 8584827 [details] [diff] [review]
Patch 3v1: Hide "can't play EME notification box" when we can play

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

(In reply to Chris Pearce (:cpearce) from comment #21)
> Gigs

I love autocomplete. Can you tell me which phone manufacturer I should file a bug with? :-)

::: browser/base/content/browser-eme.js
@@ +163,5 @@
> +     "drmContentCDMInstalling"
> +     ].forEach(function (value) {
> +        var notification = box.getNotificationWithValue(value);
> +        if (notification)
> +          box.removeNotification(notification);

I wonder if we should pass the second parameter. You can pass |true| in order to skip the animation for the closing, which might be more fluent if there are situations where we only show the notification bar very briefly?

Maybe I'm overthinking this, though - despite the loop, only one of these will ever show at the same time, and I don't know if there are cases where we show the one notification but only briefly...

@@ +164,5 @@
> +     ].forEach(function (value) {
> +        var notification = box.getNotificationWithValue(value);
> +        if (notification)
> +          box.removeNotification(notification);
> +      });    

Nit: trailing whitespace
Attachment #8584827 - Flags: review?(gijskruitbosch+bugs)
Attachment #8584827 - Flags: review?(dolske)
Attachment #8584827 - Flags: review+
(Assignee)

Comment 25

4 years ago
Created attachment 8585819 [details] [diff] [review]
Patch 1v4: Delay navigator.requestMediaKeySystemAccess until CDM downloads

I'm having a hard time hitting ImplCycleCollectionTraverse etc specializations in a debugger, so I'll stick with a manual loop in the traverse and unlink...
Attachment #8585373 - Attachment is obsolete: true
Attachment #8585819 - Flags: review?(ehsan)
(Assignee)

Comment 27

4 years ago
(In reply to :Gijs Kruitbosch from comment #24)
> Comment on attachment 8584827 [details] [diff] [review]
> Patch 3v1: Hide "can't play EME notification box" when we can play
> 
> Review of attachment 8584827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Chris Pearce (:cpearce) from comment #21)
> > Gigs
> 
> I love autocomplete. Can you tell me which phone manufacturer I should file
> a bug with? :-)

Error between sleep-deprived user and keyboard. ;)


> ::: browser/base/content/browser-eme.js
> @@ +163,5 @@
> > +     "drmContentCDMInstalling"
> > +     ].forEach(function (value) {
> > +        var notification = box.getNotificationWithValue(value);
> > +        if (notification)
> > +          box.removeNotification(notification);
> 
> I wonder if we should pass the second parameter. You can pass |true| in
> order to skip the animation for the closing, which might be more fluent if
> there are situations where we only show the notification bar very briefly

In my opinion we may as well animate it so that it's smooth and not jarring. I think this is indeed an edge case, so spending a lot of time on it isn't worthwhile. One option would be to set a timer in the "drmContentCDMInsufficientVersion" and "drmContentCDMInstalling" case and only show the notification after some number of elapsed seconds.

Up to you though, I'm not a Firefox dev. ;)

Comment 28

4 years ago
Comment on attachment 8585819 [details] [diff] [review]
Patch 1v4: Delay navigator.requestMediaKeySystemAccess until CDM downloads

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

r=me with the below fixed.

::: dom/media/eme/MediaKeySystemAccessManager.cpp
@@ +28,5 @@
> +  for (size_t i = 0; i < tmp->mRequests.Length(); i++) {
> +    tmp->mRequests[i].RejectPromise();
> +    tmp->mRequests[i].CancelTimer();
> +    NS_IMPL_CYCLE_COLLECTION_UNLINK(mRequests[i].mPromise)
> +  }

Please empty out mRequests here so that Shutdown() called from the dtor doesn't end up calling RejectPromise and CancelTimer twice.
Attachment #8585819 - Flags: review?(ehsan) → review+
(Assignee)

Comment 30

4 years ago
Created attachment 8585926 [details] [diff] [review]
Patch 1v5: Delay navigator.requestMediaKeySystemAccess until CDM downloads

Review comments addressed, rebased.
Attachment #8585819 - Attachment is obsolete: true
Attachment #8585926 - Flags: review+
(Assignee)

Comment 31

4 years ago
Created attachment 8585927 [details] [diff] [review]
Patch 2v3: Make GMPProvider respond when updates are needed

Review comments addressed, rebased.
Attachment #8585068 - Attachment is obsolete: true
Attachment #8585250 - Attachment is obsolete: true
Attachment #8585927 - Flags: review+
(Assignee)

Comment 34

4 years ago
Comment on attachment 8584827 [details] [diff] [review]
Patch 3v1: Hide "can't play EME notification box" when we can play

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: If the user happens to install an EME capable Firefox, or creates a clear profile in an EME capable Firefox, and immediately tries to play EME content, they'll get a "I can't play DRM video", until the Adobe EME plugin downloads. By default we check for updates and download the Adobe EME plugin 60 seconds after startup. With the patches in this bug, we will initiate the update check as soon as a page requests EME. This means users won't have to wait for 60 seconds after startup before we start the EME plugin download, it'll happen immediately.
[Describe test coverage new/current, TreeHerder]: We have test coverage for the existing code paths that are modified here. We don't have unit tests to cover the new code paths added here yet, we're working with Adobe on an end-to-end test that will test downloading and installing the EME plugin and playing EME videos.
[Risks and why]: Seems pretty low risk; this adds new code paths to cover an edge case, and the existing code path that is modified is well tested.
[String/UUID change made/needed]: None.
Attachment #8584827 - Flags: approval-mozilla-beta?
Attachment #8584827 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 35

4 years ago
Comment on attachment 8585251 [details] [diff] [review]
Patch 4: Make GMPWrapper e10s compatible

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: If the user happens to install an EME capable Firefox, or creates a clear profile in an EME capable Firefox, and immediately tries to play EME content, they'll get a "I can't play DRM video", until the Adobe EME plugin downloads. By default we check for updates and download the Adobe EME plugin 60 seconds after startup. With the patches in this bug, we will initiate the update check as soon as a page requests EME. This means users won't have to wait for 60 seconds after startup before we start the EME plugin download, it'll happen immediately.
[Describe test coverage new/current, TreeHerder]: We have test coverage for the existing code paths that are modified here. We don't have unit tests to cover the new code paths added here yet, we're working with Adobe on an end-to-end test that will test downloading and installing the EME plugin and playing EME videos.
[Risks and why]: Seems pretty low risk; this adds new code paths to cover an edge case, and the existing code path that is modified is well tested.
[String/UUID change made/needed]: None.
Attachment #8585251 - Flags: approval-mozilla-beta?
Attachment #8585251 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 36

4 years ago
Comment on attachment 8585926 [details] [diff] [review]
Patch 1v5: Delay navigator.requestMediaKeySystemAccess until CDM downloads

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: If the user happens to install an EME capable Firefox, or creates a clear profile in an EME capable Firefox, and immediately tries to play EME content, they'll get a "I can't play DRM video", until the Adobe EME plugin downloads. By default we check for updates and download the Adobe EME plugin 60 seconds after startup. With the patches in this bug, we will initiate the update check as soon as a page requests EME. This means users won't have to wait for 60 seconds after startup before we start the EME plugin download, it'll happen immediately.
[Describe test coverage new/current, TreeHerder]: We have test coverage for the existing code paths that are modified here. We don't have unit tests to cover the new code paths added here yet, we're working with Adobe on an end-to-end test that will test downloading and installing the EME plugin and playing EME videos.
[Risks and why]: Seems pretty low risk; this adds new code paths to cover an edge case, and the existing code path that is modified is well tested.
[String/UUID change made/needed]: None.
Attachment #8585926 - Flags: approval-mozilla-beta?
Attachment #8585926 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 37

4 years ago
Comment on attachment 8585927 [details] [diff] [review]
Patch 2v3: Make GMPProvider respond when updates are needed

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: If the user happens to install an EME capable Firefox, or creates a clear profile in an EME capable Firefox, and immediately tries to play EME content, they'll get a "I can't play DRM video", until the Adobe EME plugin downloads. By default we check for updates and download the Adobe EME plugin 60 seconds after startup. With the patches in this bug, we will initiate the update check as soon as a page requests EME. This means users won't have to wait for 60 seconds after startup before we start the EME plugin download, it'll happen immediately.
[Describe test coverage new/current, TreeHerder]: We have test coverage for the existing code paths that are modified here. We don't have unit tests to cover the new code paths added here yet, we're working with Adobe on an end-to-end test that will test downloading and installing the EME plugin and playing EME videos.
[Risks and why]: Seems pretty low risk; this adds new code paths to cover an edge case, and the existing code path that is modified is well tested.
[String/UUID change made/needed]: None.
Attachment #8585927 - Flags: approval-mozilla-beta?
Attachment #8585927 - Flags: approval-mozilla-aurora?
status-firefox38: --- → affected
status-firefox39: --- → affected
Comment on attachment 8585927 [details] [diff] [review]
Patch 2v3: Make GMPProvider respond when updates are needed

Taking it as I guess this scenario is going to happen a lot of time...

Should be in beta 2
Attachment #8585927 - Flags: approval-mozilla-beta?
Attachment #8585927 - Flags: approval-mozilla-beta+
Attachment #8585927 - Flags: approval-mozilla-aurora?
Attachment #8585927 - Flags: approval-mozilla-aurora+
Attachment #8585926 - Flags: approval-mozilla-beta?
Attachment #8585926 - Flags: approval-mozilla-beta+
Attachment #8585926 - Flags: approval-mozilla-aurora?
Attachment #8585926 - Flags: approval-mozilla-aurora+
Attachment #8585251 - Flags: approval-mozilla-beta?
Attachment #8585251 - Flags: approval-mozilla-beta+
Attachment #8585251 - Flags: approval-mozilla-aurora?
Attachment #8585251 - Flags: approval-mozilla-aurora+
Attachment #8584827 - Flags: approval-mozilla-beta?
Attachment #8584827 - Flags: approval-mozilla-beta+
Attachment #8584827 - Flags: approval-mozilla-aurora?
Attachment #8584827 - Flags: approval-mozilla-aurora+
(Assignee)

Updated

4 years ago
Flags: needinfo?(peterv)
(Assignee)

Updated

4 years ago
Flags: needinfo?(cpearce)
You need to log in before you can comment on or make changes to this bug.