Closed
Bug 1329543
Opened 7 years ago
Closed 7 years ago
Remove Adobe Primetime supporting code
Categories
(Core :: Audio/Video: Playback, defect, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: cpearce, Assigned: cpearce)
References
(Blocks 1 open bug)
Details
Attachments
(17 files)
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
glandium
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
impossibus
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mconley
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
spohl
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
spohl
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
We're going to stop shipping Adobe Primetime. So we can remove its supporting code.
Chris, I noticed that in bug 1329538 you said you wanted to uplift it to Firefox 52 so ESR gets it. If at all possible, please don't do the same with this bug, as you'd be removing the only way for XP Firefox users to view H.264 video without having to use Flash. ESR 52 looks to be the last supported version on XP, so it would be quite helpful for many to keep Primetime usable in it, even if no longer visible by default as per bug 1329538.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8827010 [details] Bug 1329543 - Remove PGMPAudioDecoder. https://reviewboard.mozilla.org/r/104820/#review105526 Review comments so far: (keeping 'r?', will review again...) GMPLoader.cpp mentions "Adobe" in comments three times (lines 175, 176, 181), you'll probably want to update these. In particular, at lines 180-181 you wrote "PassThroughGMPAdapter's code must remain in this file so that it's covered by Adobe's plugin-container voucher", meaning PassThroughGMPAdapter could be moved elsewhere -- but I guess it's not that important. ::: dom/media/gmp/moz.build (Diff revision 1) > ] > > IPDL_SOURCES += [ > 'GMPTypes.ipdlh', > 'PGMP.ipdl', > - 'PGMPAudioDecoder.ipdl', You forgot to remove the file itself!
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8827010 [details] Bug 1329543 - Remove PGMPAudioDecoder. https://reviewboard.mozilla.org/r/104820/#review105526 I will follow up on the referenes to "Adobe" in GMPLoader, as it's a bigger change to clean up the GMPLoader. I also need to remove the device binding code and GMP async shutdown.
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8827001 [details] Bug 1329543 - Remove use of gmp-eme-adobe* prefs from Gecko. https://reviewboard.mozilla.org/r/104802/#review105552
Attachment #8827001 -
Flags: review?(gsquelart) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8827002 [details] Bug 1329543 - Remove IsPrimetimeKeySystem(string) from Gecko. https://reviewboard.mozilla.org/r/104804/#review105554
Attachment #8827002 -
Flags: review?(gsquelart) → review+
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8827005 [details] Bug 1329543 - Remove Primetime from MediaKeySystemAccess. https://reviewboard.mozilla.org/r/104810/#review105556
Attachment #8827005 -
Flags: review?(gsquelart) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8827006 [details] Bug 1329543 - Remove kEMEKeySystemPrimetime. https://reviewboard.mozilla.org/r/104812/#review105558
Attachment #8827006 -
Flags: review?(gsquelart) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8827007 [details] Bug 1329543 - Remove Primetime SystemId from PSSH parser. https://reviewboard.mozilla.org/r/104814/#review105562
Attachment #8827007 -
Flags: review?(gsquelart) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8827008 [details] Bug 1329543 - Remove GMPAudioDecoder and unencrypted GMP decoding. https://reviewboard.mozilla.org/r/104816/#review105568 r+, but may need a follow-up: ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp (Diff revision 1) > } > > - if (aParams.mDiagnostics) { > - const Maybe<nsCString> preferredGMP = PreferredGMP(aParams.mConfig.mMimeType); > - if (preferredGMP.isSome()) { > - aParams.mDiagnostics->SetGMP(preferredGMP.value()); Decoder Doctor won't know anymore which GMP is used. Now, this was only used in debug logs, and we only have one real GMP to worry about, so it's probably not a real loss. But to tidy things up, I'd suggest that either we record the GMP name somewhere, or we just remove the SetGMP API -- I'd be happy to work on that in a separate bug.
Attachment #8827008 -
Flags: review?(gsquelart) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8827009 [details] Bug 1329543 - Remove GMP storage migration. https://reviewboard.mozilla.org/r/104818/#review105574
Attachment #8827009 -
Flags: review?(gsquelart) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8827010 [details] Bug 1329543 - Remove PGMPAudioDecoder. https://reviewboard.mozilla.org/r/104820/#review105576
Attachment #8827010 -
Flags: review?(gsquelart) → review+
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8827014 [details] Bug 1329543 - Amend comment in EMEDecoderModule to not mention Adobe. https://reviewboard.mozilla.org/r/104828/#review105578
Attachment #8827014 -
Flags: review?(gsquelart) → review+
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8827017 [details] Bug 1329543 - Remove obsolete GMPDecryptor7 interface that was only used by Primetime. https://reviewboard.mozilla.org/r/104838/#review105580
Attachment #8827017 -
Flags: review?(gsquelart) → review+
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8827003 [details] Bug 1329543 - Remove use of gmp-eme-adobe prefs from browser-media.js. https://reviewboard.mozilla.org/r/104806/#review105654 ::: browser/base/content/browser-media.js:27 (Diff revision 1) > isKeySystemVisible(keySystem) { > - if (!keySystem) { > - return false; > - } > - if (keySystem.startsWith("com.adobe") && > + return keySystem && > + keySystem == "com.widevine.alpha" && > + Services.prefs.getPrefType("media.gmp-widevinecdm.visible") && > + Services.prefs.getBoolPref("media.gmp-widevinecdm.visible"); This changes the behaviour for keysystems other than adobe and widevine. I'm assuming that's not desirable, or am I missing something?
Attachment #8827003 -
Flags: review?(gijskruitbosch+bugs)
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8827011 [details] Bug 1329543 - Remove Adobe from GMP backup downloader. https://reviewboard.mozilla.org/r/104822/#review105746
Attachment #8827011 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8827003 [details] Bug 1329543 - Remove use of gmp-eme-adobe prefs from browser-media.js. https://reviewboard.mozilla.org/r/104806/#review105766 ::: browser/base/content/browser-media.js:27 (Diff revision 1) > isKeySystemVisible(keySystem) { > - if (!keySystem) { > - return false; > - } > - if (keySystem.startsWith("com.adobe") && > + return keySystem && > + keySystem == "com.widevine.alpha" && > + Services.prefs.getPrefType("media.gmp-widevinecdm.visible") && > + Services.prefs.getBoolPref("media.gmp-widevinecdm.visible"); Oh d'oh. Yup, that's a mistake on my part. Thanks!
Assignee | ||
Comment 40•7 years ago
|
||
(In reply to Gerald Squelart [:gerald] from comment #32) > Comment on attachment 8827008 [details] > Bug 1329543 - Remove GMPAudioDecoder and unencrypted GMP decoding. > > https://reviewboard.mozilla.org/r/104816/#review105568 > > r+, but may need a follow-up: > > ::: dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp > (Diff revision 1) > > } > > > > - if (aParams.mDiagnostics) { > > - const Maybe<nsCString> preferredGMP = PreferredGMP(aParams.mConfig.mMimeType); > > - if (preferredGMP.isSome()) { > > - aParams.mDiagnostics->SetGMP(preferredGMP.value()); > > Decoder Doctor won't know anymore which GMP is used. > Now, this was only used in debug logs, and we only have one real GMP to > worry about, so it's probably not a real loss. > But to tidy things up, I'd suggest that either we record the GMP name > somewhere, or we just remove the SetGMP API -- I'd be happy to work on that > in a separate bug. It would be great if you can take over the clean up on the decoder doctor pieces. I also noticed that browser-media.js handles "adobe-cdm-not-found" and "adobe-cdm-not-activated", if you could clean up that too, that would be much appreciated. Thanks!
Blocks: 1331484
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8827003 [details] Bug 1329543 - Remove use of gmp-eme-adobe prefs from browser-media.js. https://reviewboard.mozilla.org/r/104806/#review105768
Attachment #8827003 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 57•7 years ago
|
||
mozreview-review |
Comment on attachment 8826999 [details] Bug 1329543 - Remove Adobe from ac_add_options --enable-eme. https://reviewboard.mozilla.org/r/104798/#review105778
Attachment #8826999 -
Flags: review?(mh+mozilla) → review+
Comment 58•7 years ago
|
||
mozreview-review |
Comment on attachment 8827000 [details] Bug 1329543 - Remove uses of MOZ_ADOBE_EME. https://reviewboard.mozilla.org/r/104800/#review105780
Attachment #8827000 -
Flags: review?(mh+mozilla) → review+
Comment 59•7 years ago
|
||
mozreview-review |
Comment on attachment 8827012 [details] Bug 1329543 - Remove 'adobe' from GMP download code. https://reviewboard.mozilla.org/r/104824/#review105994
Attachment #8827012 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 60•7 years ago
|
||
mozreview-review |
Comment on attachment 8827013 [details] Bug 1329543 - Rename eme-adobe_description string to cdm_description. https://reviewboard.mozilla.org/r/104826/#review105996
Attachment #8827013 -
Flags: review?(spohl.mozilla.bugs) → review+
Comment 61•7 years ago
|
||
mozreview-review |
Comment on attachment 8827004 [details] Bug 1329543 - Remove use of gmp-eme-adobe* prefs from external media tests. https://reviewboard.mozilla.org/r/104808/#review106038
Attachment #8827004 -
Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 79•7 years ago
|
||
Pushed by cpearce@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9a193223d4aa Remove Adobe from ac_add_options --enable-eme. r=glandium https://hg.mozilla.org/integration/autoland/rev/8916048a9bab Remove uses of MOZ_ADOBE_EME. r=glandium https://hg.mozilla.org/integration/autoland/rev/d47e700dbc36 Remove use of gmp-eme-adobe* prefs from Gecko. r=gerald https://hg.mozilla.org/integration/autoland/rev/be251a397f2d Remove IsPrimetimeKeySystem(string) from Gecko. r=gerald https://hg.mozilla.org/integration/autoland/rev/1d8062b87249 Remove use of gmp-eme-adobe prefs from browser-media.js. r=Gijs https://hg.mozilla.org/integration/autoland/rev/ac1981b1296c Remove use of gmp-eme-adobe* prefs from external media tests. r=maja_zf https://hg.mozilla.org/integration/autoland/rev/79f11e99a81a Remove Primetime from MediaKeySystemAccess. r=gerald https://hg.mozilla.org/integration/autoland/rev/f4956aa25be6 Remove kEMEKeySystemPrimetime. r=gerald https://hg.mozilla.org/integration/autoland/rev/a54e6cd31eb6 Remove Primetime SystemId from PSSH parser. r=gerald https://hg.mozilla.org/integration/autoland/rev/182ae7543ea4 Remove GMPAudioDecoder and unencrypted GMP decoding. r=gerald https://hg.mozilla.org/integration/autoland/rev/58e504f4afdc Remove GMP storage migration. r=gerald https://hg.mozilla.org/integration/autoland/rev/8b358c1267b2 Remove PGMPAudioDecoder. r=gerald https://hg.mozilla.org/integration/autoland/rev/c4594994e460 Remove Adobe from GMP backup downloader. r=mconley https://hg.mozilla.org/integration/autoland/rev/ee5ca22bfa36 Remove 'adobe' from GMP download code. r=spohl https://hg.mozilla.org/integration/autoland/rev/943f58381a5f Rename eme-adobe_description string to cdm_description. r=spohl https://hg.mozilla.org/integration/autoland/rev/7ab977888d47 Amend comment in EMEDecoderModule to not mention Adobe. r=gerald https://hg.mozilla.org/integration/autoland/rev/355453560577 Remove obsolete GMPDecryptor7 interface that was only used by Primetime. r=gerald
Comment 80•7 years ago
|
||
for information only (no action needed), the set of patches here increased the number of constructors measured: == Change summary for alert #4830 (as of January 18 2017 00:48 UTC) == Regressions: 1% compiler_metrics num_constructors linux32 debug 183 -> 184 1% compiler_metrics num_constructors linux64 debug 183 -> 184 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4830
Comment 81•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a193223d4aa https://hg.mozilla.org/mozilla-central/rev/8916048a9bab https://hg.mozilla.org/mozilla-central/rev/d47e700dbc36 https://hg.mozilla.org/mozilla-central/rev/be251a397f2d https://hg.mozilla.org/mozilla-central/rev/1d8062b87249 https://hg.mozilla.org/mozilla-central/rev/ac1981b1296c https://hg.mozilla.org/mozilla-central/rev/79f11e99a81a https://hg.mozilla.org/mozilla-central/rev/f4956aa25be6 https://hg.mozilla.org/mozilla-central/rev/a54e6cd31eb6 https://hg.mozilla.org/mozilla-central/rev/182ae7543ea4 https://hg.mozilla.org/mozilla-central/rev/58e504f4afdc https://hg.mozilla.org/mozilla-central/rev/8b358c1267b2 https://hg.mozilla.org/mozilla-central/rev/c4594994e460 https://hg.mozilla.org/mozilla-central/rev/ee5ca22bfa36 https://hg.mozilla.org/mozilla-central/rev/943f58381a5f https://hg.mozilla.org/mozilla-central/rev/7ab977888d47 https://hg.mozilla.org/mozilla-central/rev/355453560577
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Assignee | ||
Comment 82•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #80) > for information only (no action needed), the set of patches here increased > the number of constructors measured: > == Change summary for alert #4830 (as of January 18 2017 00:48 UTC) == > > Regressions: > > 1% compiler_metrics num_constructors linux32 debug 183 -> 184 > 1% compiler_metrics num_constructors linux64 debug 183 -> 184 > > For up to date results, see: > https://treeherder.mozilla.org/perf.html#/alerts?id=4830 I struggle to believe these measurements are correct; in these patches I removed an IPDL protocol, which will generate a number of classes with constructors.
Comment 83•7 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #82) > (In reply to Joel Maher ( :jmaher) from comment #80) > > for information only (no action needed), the set of patches here increased > > the number of constructors measured: > > == Change summary for alert #4830 (as of January 18 2017 00:48 UTC) == > > > > Regressions: > > > > 1% compiler_metrics num_constructors linux32 debug 183 -> 184 > > 1% compiler_metrics num_constructors linux64 debug 183 -> 184 > > > > For up to date results, see: > > https://treeherder.mozilla.org/perf.html#/alerts?id=4830 > > I struggle to believe these measurements are correct; in these patches I > removed an IPDL protocol, which will generate a number of classes with > constructors. Joel, can we investigate this (in a separate bug)? Is there maybe a symbolicated list of constructors that could be compared so we know why this 'triggered' and/or what's broken?
Flags: needinfo?(jmaher)
Comment 84•7 years ago
|
||
:froydnj, can you comment as to if we should be doing anything? If we collect these metrics we should be prepared to do something with them, so far this is causing more confusion and I really don't understand what these metrics represent.
Flags: needinfo?(jmaher) → needinfo?(nfroyd)
Comment 85•7 years ago
|
||
(In reply to Joel Maher ( :jmaher) from comment #84) > :froydnj, can you comment as to if we should be doing anything? If we > collect these metrics we should be prepared to do something with them, so > far this is causing more confusion and I really don't understand what these > metrics represent. This is probably OK, with the same explanation as bug 1323957 comment 17.
Flags: needinfo?(nfroyd)
You need to log in
before you can comment on or make changes to this bug.
Description
•