Closed
Bug 1278574
Opened 8 years ago
Closed 8 years ago
Crash in libsomxcore.so@0x1826 after MP3 playback on Samsung devices
Categories
(Firefox for Android Graveyard :: Audio/Video, defect)
Tracking
(firefox50 fixed)
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)
(deleted),
patch
|
jya
:
review+
snorp
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jya
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 1•8 years ago
|
||
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).
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 4•8 years ago
|
||
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 5•8 years ago
|
||
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.
Assignee | ||
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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 8•8 years ago
|
||
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 9•8 years ago
|
||
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)
Assignee | ||
Comment 10•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8762887 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 11•8 years ago
|
||
Comment 12•8 years ago
|
||
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
Comment 13•8 years ago
|
||
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
Assignee | ||
Comment 14•8 years ago
|
||
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
Assignee | ||
Comment 15•8 years ago
|
||
See previous comment.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a247f3a56371
Attachment #8764032 -
Flags: review?(jyavenard)
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8764195 -
Flags: review?(jyavenard) → review+
Assignee | ||
Comment 18•8 years ago
|
||
None of the failures seem related: https://treeherder.mozilla.org/#/jobs?repo=try&tochange=063d9796ade5&fromchange=0029872ed5f3daa9f076ff2bbd9ebd7086edde8b.
Let's have another try.
Comment 19•8 years ago
|
||
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
Comment 20•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4f222b71be2
https://hg.mozilla.org/mozilla-central/rev/b39bb445f4eb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•