Closed Bug 1278574 Opened 3 years ago Closed 3 years ago

Crash in libsomxcore.so@0x1826 after MP3 playback on Samsung devices

Categories

(Firefox for Android :: Audio/Video, defect)

48 Branch
All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
firefox50 --- fixed

People

(Reporter: esawin, Assigned: esawin)

References

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files, 4 obsolete files)

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.
No longer depends on: 1214710
Depends on: 1214710
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.
Attachment #8761574 - Flags: review?(snorp)
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.
Attachment #8761574 - Flags: review?(snorp) → review?(jmuizelaar)
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.
Attachment #8761593 - Flags: review?(jyavenard)
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).
Attachment #8761574 - Attachment is obsolete: true
Attachment #8761574 - Flags: review?(jmuizelaar)
Attachment #8761775 - Flags: review?(snorp)
Attachment #8761775 - Flags: review?(jyavenard)
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.
Attachment #8761593 - Flags: review?(jyavenard) → review+
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.
Attachment #8761775 - Flags: review?(jyavenard)
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.
Attachment #8761593 - Attachment is obsolete: true
Attachment #8761775 - Attachment is obsolete: true
Attachment #8761775 - Flags: review?(snorp)
Attachment #8762887 - Flags: review?(snorp)
Attachment #8762887 - Flags: review?(jyavenard)
Attachment #8762887 - Flags: review?(snorp) → review+
Attachment #8762887 - Flags: review?(jyavenard) → review+
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
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
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.
Attachment #8764032 - Flags: review?(jyavenard)
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.
Attachment #8764032 - Attachment is obsolete: true
Attachment #8764195 - Flags: review?(jyavenard)
Attachment #8764195 - Flags: review?(jyavenard) → review+
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
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Duplicate of this bug: 1272874
You need to log in before you can comment on or make changes to this bug.