1232514 - [EME] GMPDecryptsAndDecodesH264 actually checks for AAC rather than H264

Status: RESOLVED FIXED

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

Description

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

GMPDecryptsAndDecodesH264() in MediaKeySystemAccess.cpp has this code:

static bool
GMPDecryptsAndDecodesH264(mozIGeckoMediaPluginService* aGMPS,
                          const nsAString& aKeySystem)
{
  MOZ_ASSERT(HaveGMPFor(aGMPS,
                        NS_ConvertUTF16toUTF8(aKeySystem),
                        NS_LITERAL_CSTRING(GMP_API_DECRYPTOR)));
  return HaveGMPFor(aGMPS,
                    NS_ConvertUTF16toUTF8(aKeySystem),
                    NS_LITERAL_CSTRING(GMP_API_AUDIO_DECODER),
                    NS_LITERAL_CSTRING("aac"));
}

But it's supposed to check for GMP_API_VIDEO_DECODER/"h264" rather than GMP_API_AUDIO_DECODER/"aac".

Oops!

Fortunately, every GMP which supports decoding with AAC (Adobe's GMP) also supports decoding with H.264, so we won't break anything by shipping this (as this mistake is in Firefox43 which goes out this week).

Comment 1

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

(In reply to Chris Pearce (:cpearce) from comment #0)
> Fortunately, every GMP which supports decoding with AAC (Adobe's GMP) also
> supports decoding with H.264, so we won't break anything by shipping this
> (as this mistake is in Firefox43 which goes out this week).

This isn't strictly true, as gmp-clearkey also supports decoding AAC and H.264, and in theory if the user is on Windows N, or if they've messed with their codecs using Codec Tweak Tool, they could get into a state where AAC works but H.264 doesn't.

gmp-clearkey isn't used enough yet to warrant a dot release however.

Comment 2

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

Attachment: MozReview Request: Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang

Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang

Comment 3

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

Attachment: MozReview Request: Bug 1232527 - Call into WMF PDM to determine if WMF can decode instead of using GMPVideoDecoderTrialCreator. r?jwwang

Bug 1232527 - Call into WMF PDM to determine if WMF can decode instead of using GMPVideoDecoderTrialCreator. r?jwwang

Resurrect WMFDecoderModule::HasAAC() and HasH264(), and use those in
MediaKeySystemAccess.cpp to figure out whether we gmp-clearkey can decode,
rather than assuming Vista and later is always able to decode, as that's not
a valid assumption; Vista may not have the required Platfor Update installed,
or we may be on Windows N or KN without the Media Feature Pack.

Comment 4

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

Attachment: MozReview Request: Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

Remove GMPVideoDecoderTrialCreator, and the tests and IPC/IDL supporting it.

Comment 5

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

Comment on attachment 8698294 [details]
MozReview Request: Bug 1232527 - Call into WMF PDM to determine if WMF can decode instead of using GMPVideoDecoderTrialCreator. r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27925/diff/1-2/

Comment 6

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

Comment on attachment 8698295 [details]
MozReview Request: Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/27927/diff/1-2/

Comment 7

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

Comment on attachment 8698292 [details]
MozReview Request: Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang

https://reviewboard.mozilla.org/r/27923/#review25043

Comment 8

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

Comment on attachment 8698294 [details]
MozReview Request: Bug 1232527 - Call into WMF PDM to determine if WMF can decode instead of using GMPVideoDecoderTrialCreator. r?jwwang

https://reviewboard.mozilla.org/r/27925/#review25045

Comment 9

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

Comment on attachment 8698295 [details]
MozReview Request: Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

https://reviewboard.mozilla.org/r/27927/#review25047

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0cacf8bf5f9373b09e70337df1a81bdc05639e
Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r=jwwang

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b0cacf8bf5f

Comment 12

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

Comment on attachment 8698292 [details]
MozReview Request: Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang

Approval Request Comment
[Feature/regressing bug #]: ClearKey EME (i.e. the cross browser EME baseline/demo; i.e. *not* Netflix).
[User impact if declined]: Some Windows systems with broken video codecs (I believe broken by "Codec Tweak Tool") will incorrectly report that they support playback of encrypted H.264 ClearyKey streams, when in fact they do not since our ClearKey implementation relies on the Windows system decoders, which are broken on 0.5% of all systems.
[Describe test coverage new/current, TreeHerder]: We have lots of EME mochitests, however none on systems with broken codecs.
[Risks and why]: Low; this should only be a behaviour change on Windows systems with broken codecs (~0.5% of population) on ClearKey demos. ClearKey is not used in production EME sites, it's not robust enough.
[String/UUID change made/needed]: None.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8698292 [details]
MozReview Request: Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang

Given that this will impact only a very small percentage of end-users, seems safe to uplift to Aurora45 and Beta44.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Chris, not sure if you noticed but the patch you are requesting to uplift to Beta also modifies a UUID in mozIGeckoMediaPluginService.idl. Is that ok? Will that break some add-on compatibility?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Chris, not sure if you noticed but the patch you are requesting to uplift to
> Beta also modifies a UUID in mozIGeckoMediaPluginService.idl. Is that ok?
> Will that break some add-on compatibility?

Ok, this is confusing. When I open the MozReview request in this bug, it shows 3 changesets, two from bug 1232527. I think the Sheriffs will know which ones to land (or at least I hope so).

Comment 16

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

Yeah, mozreview puts all review requests from different bugs into the first commit in the changeset list's bug.

I requested uplift separately in bug 1232527 for the patch there.

I didn't request uplift for the patch which changed the UUID.

Comment 17

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

There are try pushes for the patch here in Aurora and Beta in bug 1232527.

Comment 18

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

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

Comment 19

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f36808a4aeff

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/f36808a4aeff