Closed Bug 1067216 Opened 5 years ago Closed 5 years ago

Make MediaKeys.isTypeSupported() more accurate

Categories

(Core :: Audio/Video, defect)

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox37 --- fixed
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

Currently MediaKeys.isTypeSupported() only tests the keysystem string, it doesn't test that we can play the contentType passed in. We need to be able to determine whether we have a CDM that can decode the contentType passed in, or if we're using ClearKey, whether we have a PDM that we can use to decode the decrypted stream.

mozIGeckoMediaPluginService.hasPluginForAPI() will be useful here, though the GMP .info format doesn't (yet) provide a way for a GMP to specify what profile it supports for H.264/AAC.

This is independent of whether isTypeSupported should be async.
Assignee: nobody → cpearce
Attached patch Patch v1 (obsolete) — Splinter Review
* Make MediaKeys.isTypeSupported() actually query the GMPService to see if we have a plugin that can play the types we're querying for.
* Make GMPService.hasPluginForAPI block if we've not scanned MOZ_GMP_PATH until we have.

Edwin: can you please review MediaKeys changes. Comments on GMPService welcome.
Jesup: can you please review the GMPService changes.
Attachment #8499315 - Flags: review?(rjesup)
Attachment #8499315 - Flags: review?(edwin)
Comment on attachment 8499315 [details] [diff] [review]
Patch v1

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

::: content/media/eme/MediaKeys.cpp
@@ +185,5 @@
> +                 NS_LITERAL_CSTRING("h264")) &&
> +      HaveGMPFor(NS_LITERAL_CSTRING("com.adobe.access"),
> +                 NS_LITERAL_CSTRING("decode-audio"),
> +                 NS_LITERAL_CSTRING("aac"))) {
> +      return true;

Choose - 2 or 4 byte indents.  (Really, match file, which appears 2)
Admittedly, an evil if()

@@ +203,5 @@
>  {
>    // TODO: Should really get spec changed to this is async, so we can wait
>    //       for user to consent to running plugin.
> +  bool supported = IsTypeSupported(aKeySystem, aInitDataType, aContentType);
> +  

space

::: content/media/test/test_encryptedMediaExtensions.html
@@ +157,2 @@
>  function beginTest() {
> +  testIsTypeSupported();       

spaces
Attachment #8499315 - Flags: review?(rjesup) → review+
Urgh. Problem. IsTypeSupported fails if we've instantiated a plugin of the same type already, since HaveGMPFor() in MediaKeys.cpp passes an emptry string as the node id, implying that the plugin should be for shared origins, and them GMPService::HasPluginForAPI passes aCloneCrossNodeIds=false to GMPService::SelectPluginForAPI, so it returns nullptr, and we that we can't play that type.
Attached patch Patch 2 (obsolete) — Splinter Review
It's a bit of a hassle to get the nodeId in the MediaKeys class before we've created a CDM, because we have to hit the disk to check to see if one's stored. But we can remove the need for the nodeId parameter in GMPService::hasPluginForAPI if we copy the logic from SelectPluginForAPI. We can take advantage of the fact that once we've found a plugin that supports all APIs we're querying for we know that we can at least clone this plugin, even if its nodeId doesn't match. This makes calling the API simpler.

Included test to catch this case.
Attachment #8500826 - Flags: review?(rjesup)
Comment on attachment 8500826 [details] [diff] [review]
Patch 2

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

::: content/media/gmp/GMPService.cpp
@@ +777,5 @@
>          continue;
>        }
> +
> +      // Note: We either have a plugin that we can clone, or that we can
> +      // use directly if it's sharable or it's node id matches.

"use directly if it's sharable or its node id matches".
Comment on attachment 8500826 [details] [diff] [review]
Patch 2

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

I'll r+, but I'd really prefer to see the common code factored out somehow.

::: content/media/gmp/GMPService.cpp
@@ +760,5 @@
>  
>    GMPParent* gmpToClone = nullptr;
>    {
> +    // Note: This logic below needs to be kept in sync with HasPluginForAPI()
> +    // above.

Since a big block of this is virtually identical (except for aTags->ElementAt(t) vs aTags[t]), can we please not duplicate it?
Attachment #8500826 - Flags: review?(rjesup) → review+
* Fold two patches together.
* Added GMPService::FindPluginForAPIFrom() so that we don't need the duplicated logic anymore in GMPService::SelectPluginForAPI and GMPService::HasPluginForAPI.
Attachment #8499315 - Attachment is obsolete: true
Attachment #8500826 - Attachment is obsolete: true
Attachment #8502954 - Flags: review?(rjesup)
Comment on attachment 8502954 [details] [diff] [review]
Patch: Review comments addressed

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

::: content/media/gmp/GMPService.cpp
@@ +743,5 @@
> +  mMutex.AssertCurrentThreadOwns();
> +  for (size_t i = aSearchStartIndex; i < mPlugins.Length(); i++) {
> +    GMPParent* gmp = mPlugins[i];
> +    bool supportsAllTags = true;
> +    for (uint32_t t = 0; t < aTags.Length(); t++) {

Length() isn't uint32_t (size_t is good enough).  I realize this code was just moved here...
Attachment #8502954 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #8)
> Comment on attachment 8502954 [details] [diff] [review]
> Patch: Review comments addressed
> 
> Review of attachment 8502954 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/gmp/GMPService.cpp
> @@ +743,5 @@
> > +  mMutex.AssertCurrentThreadOwns();
> > +  for (size_t i = aSearchStartIndex; i < mPlugins.Length(); i++) {
> > +    GMPParent* gmp = mPlugins[i];
> > +    bool supportsAllTags = true;
> > +    for (uint32_t t = 0; t < aTags.Length(); t++) {
> 
> Length() isn't uint32_t (size_t is good enough).  I realize this code was
> just moved here...

Whoops I didn't make this change; This comment wasn't in the review-granted email, just in the bugmail that accompanies the review-granted email... Odd... Will follow up...
Testing build fix, and also fixed the size_t issue:
https://tbpl.mozilla.org/?tree=Try&rev=2c418bbc010d
https://hg.mozilla.org/mozilla-central/rev/f5c56af59b28
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Mass update firefox-status to track EME uplift.
You need to log in before you can comment on or make changes to this bug.