This bug was filed from the Socorro interface and is report bp-d59bd1a6-6df1-4eea-85d2-29b2e2151014. Moved out of bug 1214710, where we track the platform support for decoder limiting. In this bug we'll address the affected platforms by setting the decoder limit prefs accordingly.

STR copy from bug 1214710: > STR: > 1. Open translate.google.com. > 2. Type "I". > 3. Press TTS icon. > 4. Press TTS icon again. > 5. Close tab and wait (for decoder shutdown).

This could be an OK place to set the default pref value.

Comment on attachment 8761574 [details] [diff] [review] 0001-Bug-1278574-1.1-Allow-only-a-single-media-decoder-pa.patch Review of attachment 8761574 [details] [diff] [review]: ----------------------------------------------------------------- I guess I'm ok with this, but wish there was a better way. I'm going to defer to a gfx person though.

Is there a reason why these functions are declared private? Making them public would allow for compile-time checks of media pref names, as in this case.

Comment on attachment 8761574 [details] [diff] [review] 0001-Bug-1278574-1.1-Allow-only-a-single-media-decoder-pa.patch Review of attachment 8761574 [details] [diff] [review]: ----------------------------------------------------------------- We don't want to persist this so it seems like it might better to enforce the limit at the reader of pref.

We can avoid the default pref setting, if we move the special casing directly into the ReaderQueue. However, we need to resort to Preferences in this case, since MediaPrefs doesn't provide a way to differentiate between a user-set value and the default value defined in MediaPrefs.h (assuming both are equal). This patch also contains a minor cleanup. Patch 2 remains valuable (after rebasing).

Comment on attachment 8761593 [details] [diff] [review] 0002-Bug-1278574-2.1-Make-Get-PrefName-and-Get-PrefDefaul.patch Review of attachment 8761593 [details] [diff] [review]: ----------------------------------------------------------------- Should remove the need for the macro TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE and other global entries like SUSPEND_BACKGROUND_VIDEO_DELAY_MS while at it.

Comment on attachment 8761593 [details] [diff] [review] 0002-Bug-1278574-2.1-Make-Get-PrefName-and-Get-PrefDefaul.patch Review of attachment 8761593 [details] [diff] [review]: ----------------------------------------------------------------- Should remove the need for the macro TEST_PREFERENCE_FAKE_RECOGNITION_SERVICE and other global entries like SUSPEND_BACKGROUND_VIDEO_DELAY_MS while at it. Actually, my bad... No point for this, SetIntPref will not work with e10s... ::: widget/android/GfxInfo.cpp @@ +227,4 @@ > MOZ_ASSERT(prefserv); > prefserv->GetDefaultBranch(nullptr, getter_AddRefs(prefs)); > MOZ_ASSERT(prefs); > + prefs->SetIntPref(MediaPrefs::GetMediaDecoderLimitPrefName(), maxNumDecoders); If this is anywhere like the normal preferences, it will not work with e10s.

Comment on attachment 8761775 [details] [diff] [review] 0001-Bug-1278574-1.2-Allow-only-a-single-media-decoder-pa.patch Review of attachment 8761775 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaDecoderReader.cpp @@ +91,5 @@ > + if (AndroidBridge::Bridge() && > + AndroidBridge::Bridge()->GetAPIVersion() < 18) { > + // Older Android versions have broken support for multiple simultaneous > + // decoders, see bug 1278574. > + maxNumActive = Preferences::GetInt("media.decoder.limit", 1); Didn't you make changes in patch 1 precisely so we don't need to use the literal here? To me this should all be done in MediaPrefs.

With this patch, we set the MediaPref::MediaDecoderLimit default to 1 for Android API levels 15-17. Since AndroidBridge is not available when nsLayoutStatics::Initialize is called, we have to defer MediaPrefs instantiation to GfxInfoBase::Init/gfxPlatform::Init.

Looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d094549cfc0

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/87d0b9d9da94 [1.3] Allow only a single media decoder (pair) on Android 4.2 and earlier. r=jya,snorp

Sure glad I only have to report them, not explain them. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9f80505cec8547dca8939efefb47e96986cd1973 for apparently WinXP-only assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=30329410&repo=mozilla-inbound

Some XPCShell tests on Windows XP don't hit any of the MediaPrefs' initialization paths by calling GMPDecoderModule::HasGMPFor [1] -> WMFDecoderModule::SupportsMimeType [2]. We should call MediaPrefs::GetSingleton to ensure it's initialized somewhere on the path. I suggest putting it in WMFDecoderModule's constructor since none of the ::Init functions are called on this path. Try run with calling MediaPrefs::GetSingleton in GMPDecoderModule::UpdateUsableCodecs looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8f15b5750db [1] https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/agnostic/gmp/GMPDecoderModule.cpp#116 [2] https://dxr.mozilla.org/mozilla-central/source/dom/media/platforms/wmf/WMFDecoderModule.cpp#213

See previous comment. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a247f3a56371

Comment on attachment 8764032 [details] [diff] [review] 0001-Bug-1278574-2.1-Ensure-MediaPrefs-is-initialized-for.patch Review of attachment 8764032 [details] [diff] [review]: ----------------------------------------------------------------- There must be a better place. Especially as WMF doesn't work on Windows XP. Also, the decoders can be first call off the main thread, in which case you would get an assert. I would prefer something not related to the platforms decoder Doing something unique to Android and reverting the earlier change is fine by me.

For the lack of a better place, we can special-case the Android behavior by delaying the instantiation there to the AndroidBridge construction. (In reply to Jean-Yves Avenard (On PTO until July 10) [:jya] from comment #16) > There must be a better place. Especially as WMF doesn't work on Windows XP. > Also, the decoders can be first call off the main thread, in which case you > would get an assert. I would prefer something not related to the platforms > decoder WMF may not work on XP, but it's instantiated to call WMFDecoderModule::SupportsMimeType, which is where MediaPrefs is accessed. An alternative placement would be GMPDecoderModule::UpdateUsableCodecs, which is called on the main thread, but I think it would be an odd place. I was trying to avoid more Android-specific code, but given that the dependence on AndroidBridge is unique to the platform, this might be a sensible choice.

None of the failures seem related: https://treeherder.mozilla.org/#/jobs?repo=try&tochange=063d9796ade5&fromchange=0029872ed5f3daa9f076ff2bbd9ebd7086edde8b. Let's have another try.

Pushed by esawin@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4f222b71be2 [1.3] Allow only a single media decoder (pair) on Android 4.2 and earlier. r=jya,snorp https://hg.mozilla.org/integration/mozilla-inbound/rev/b39bb445f4eb [2.2] Instantiate MediaPrefs on Android during AndroidBridge construction. r=jya

https://hg.mozilla.org/mozilla-central/rev/b4f222b71be2 https://hg.mozilla.org/mozilla-central/rev/b39bb445f4eb