Closed Bug 1232514 Opened 4 years ago Closed 4 years ago

[EME] GMPDecryptsAndDecodesH264 actually checks for AAC rather than H264

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox44 --- fixed
firefox45 --- fixed
firefox46 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: cpearce, Assigned: cpearce)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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).
(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.
Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r?jwwang
Attachment #8698292 - Flags: review?(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.
Attachment #8698294 - Flags: review?(jwwang)
Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

Remove GMPVideoDecoderTrialCreator, and the tests and IPC/IDL supporting it.
Attachment #8698295 - Flags: review?(jwwang)
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 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/
Attachment #8698292 - Flags: review?(jwwang) → review+
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 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
Attachment #8698294 - Flags: review?(jwwang) → review+
Attachment #8698295 - Flags: review?(jwwang) → review+
Comment on attachment 8698295 [details]
MozReview Request: Bug 1232527 - Remove GMPVideoDecoderTrialCreator and friends. r?jwwang

https://reviewboard.mozilla.org/r/27927/#review25047
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b0cacf8bf5f9373b09e70337df1a81bdc05639e
Bug 1232514 - Make GMPDecryptsAndDecodesH264() actually check for H.264 rather than AAC. r=jwwang
https://hg.mozilla.org/mozilla-central/rev/6b0cacf8bf5f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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.
Flags: needinfo?(cpearce)
Attachment #8698292 - Flags: approval-mozilla-beta?
Attachment #8698292 - Flags: approval-mozilla-aurora?
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.
Attachment #8698292 - Flags: approval-mozilla-beta?
Attachment #8698292 - Flags: approval-mozilla-beta+
Attachment #8698292 - Flags: approval-mozilla-aurora?
Attachment #8698292 - Flags: approval-mozilla-aurora+
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?
(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).
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.
Flags: needinfo?(cpearce)
There are try pushes for the patch here in Aurora and Beta in bug 1232527.
You need to log in before you can comment on or make changes to this bug.