Closed
Bug 1317779
Opened 8 years ago
Closed 8 years ago
Crash after media playback on Samsung S3 Mini
Categories
(Firefox for Android Graveyard :: Audio/Video, defect, P1)
Tracking
(firefox52 fixed, firefox53 fixed)
RESOLVED
FIXED
Firefox 53
People
(Reporter: esawin, Assigned: esawin)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
esawin
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
The ReaderQueue enforcement (bug 1214710) of a single simultaneous decoder has been broken in bug 1288618 which brings back the crash from bug 1278574 on some Android devices.
In particular, the changes from patch 13 in layout/build/nsLayoutStatics.cpp are causing it.
Although, bug 1315510 does revert the changes in nsLayoutStatics.cpp, it seems to replace it with something that equally breaks the ReaderQueue enforcement.
Assignee | ||
Updated•8 years ago
|
Summary: Crash in MP3 playback on Samsung S3 Mini → Crash after media playback on Samsung S3 Mini
Comment 1•8 years ago
|
||
This is super weird!
The nsLayoutStatics changes just call two functions, but neither of them should do anything on Android.
VideoDecoderManagerChild::Initialize is pretty much all #ifdef XP_WIN and VideoDecoderManagerChild::Shutdown only does things if we actually initialized.
The only code that actually runs is MediaPrefs::GetSingleton(), is it that line that's breaking things? That is very unexpected.
Assignee | ||
Comment 2•8 years ago
|
||
There is an issue with calling MediaPrefs::GetSingleton() before the AndroidBridge is up because of [1].
We have to prevent that from happening on Fennec as it's done in [2].
It's an ugly issue to have, but that's the current workaround.
[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaPrefs.h#175
[2] https://dxr.mozilla.org/mozilla-central/source/layout/build/nsLayoutStatics.cpp#314
Comment 3•8 years ago
|
||
Hmm, ok, that seems really fragile.
Can you help me figure out which caller is the broken one now?
Maybe this one? [1]
We should try make this harder to break. A few ideas:
* Add an assertion if we call MediaDecoderLimitDefault on Android, but the bridge isn't available.
* Add a function to MediaPrefs that forces it to recompute the cached value for MediaDecoderLimit, call it when we initialize the AndroidBridge (maybe even assert that we don't try check the value of MediaDecoderLimit before this has happened).
[1] https://dxr.mozilla.org/mozilla-central/source/gfx/ipc/GPUProcessManager.cpp#739
Assignee | ||
Comment 4•8 years ago
|
||
After some more testing, it turns out that bug 1315510 had fixed the regression from bug 1288618 since it removed the early MediaPrefs instantiation.
However, bug 1312337 has regressed on the same feature in the meanwhile.
The decoder limit should be enforced for audio-only tracks [1], in fact that's the most reliable crashing scenario we have found on the tested device.
Also, the new decoder policy seems to block the creation of new decoders instead of suspending old ones (from my limited testing), which would essentially break media playback in most situations on devices with set decoder limits.
Assignee | ||
Comment 5•8 years ago
|
||
Comment 6•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #4)
> After some more testing, it turns out that bug 1315510 had fixed the
> regression from bug 1288618 since it removed the early MediaPrefs
> instantiation.
>
> However, bug 1312337 has regressed on the same feature in the meanwhile.
How does bug 1312337 cause early MediaPrefs instantiation to happen before Android bridge is available?
Flags: needinfo?(jwwang) → needinfo?(esawin)
Comment 7•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #4)
> The decoder limit should be enforced for audio-only tracks [1], in fact
> that's the most reliable crashing scenario we have found on the tested
> device.
IIUC, it's the video decoders which allocate hardware resource that we want to enforce the decoder limit, right?
Comment 8•8 years ago
|
||
(In reply to Eugen Sawin [:esawin] from comment #4)
> Also, the new decoder policy seems to block the creation of new decoders
> instead of suspending old ones (from my limited testing), which would
> essentially break media playback in most situations on devices with set
> decoder limits.
The creation of new decoders is blocked until an existing one is released. It doesn't break media playback, but just somewhat change the behavior.
With ReaderQueue: (latest come first serve)
1. M1 plays.
2. M2 plays.
3. M1 is forced to released decoders and stop playback.
4. M2 finishes playback and releases decoders.
5. M1 resumes playback.
With DecoderAllocPolicy: (first come first serve)
1. M1 plays.
2. M2 plays.
3. M2 waits for decoders to be released.
4. M1 finishes playback and releases decoders.
5. M2 creates decoders and starts playback.
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #6)
> (In reply to Eugen Sawin [:esawin] from comment #4)
> > After some more testing, it turns out that bug 1315510 had fixed the
> > regression from bug 1288618 since it removed the early MediaPrefs
> > instantiation.
> >
> > However, bug 1312337 has regressed on the same feature in the meanwhile.
>
> How does bug 1312337 cause early MediaPrefs instantiation to happen before
> Android bridge is available?
It doesn't, that was bug 1288618, which has been fixed in bug 1315510.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #7)
> (In reply to Eugen Sawin [:esawin] from comment #4)
> > The decoder limit should be enforced for audio-only tracks [1], in fact
> > that's the most reliable crashing scenario we have found on the tested
> > device.
>
> IIUC, it's the video decoders which allocate hardware resource that we want
> to enforce the decoder limit, right?
The purpose of the ReaderQueue (bug 1214710) was to prevent crashes on devices with broken support for multiple simultaneous decoders (bug 1278574), both video and audio. The MP3 decoder crash is the most reliable to reproduce.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
> (In reply to Eugen Sawin [:esawin] from comment #4)
> > Also, the new decoder policy seems to block the creation of new decoders
> > instead of suspending old ones (from my limited testing), which would
> > essentially break media playback in most situations on devices with set
> > decoder limits.
>
> The creation of new decoders is blocked until an existing one is released.
> It doesn't break media playback, but just somewhat change the behavior.
>
> With ReaderQueue: (latest come first serve)
> 1. M1 plays.
> 2. M2 plays.
> 3. M1 is forced to released decoders and stop playback.
> 4. M2 finishes playback and releases decoders.
> 5. M1 resumes playback.
>
> With DecoderAllocPolicy: (first come first serve)
> 1. M1 plays.
> 2. M2 plays.
> 3. M2 waits for decoders to be released.
> 4. M1 finishes playback and releases decoders.
> 5. M2 creates decoders and starts playback.
I see that the dormant mechanics have changed, so now finished and paused streams are being properly and immediately released, this wasn't the case before.
In this case the current solution is good, it doesn't break the TTS use case on translate.google.com, assuming audio-only track whitelisting is removed.
Flags: needinfo?(esawin)
Assignee | ||
Comment 10•8 years ago
|
||
Assignee: nobody → esawin
Attachment #8811723 -
Flags: review?(jwwang)
Comment 11•8 years ago
|
||
Comment on attachment 8811723 [details] [diff] [review]
0001-Bug-1317779-1.0-Remove-audio-only-track-decoder-limi.patch
Review of attachment 8811723 [details] [diff] [review]:
-----------------------------------------------------------------
With media.decoder.limit=1, do we want to allow (one video decoder and one audio decoder) or (one video decoder or one audio decoder)?
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #11)
> Comment on attachment 8811723 [details] [diff] [review]
> 0001-Bug-1317779-1.0-Remove-audio-only-track-decoder-limi.patch
>
> Review of attachment 8811723 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> With media.decoder.limit=1, do we want to allow (one video decoder and one
> audio decoder) or (one video decoder or one audio decoder)?
It should allow decoder pairs, so 1 audio or 1 audio + 1 video (or 1 video) decoder.
Comment 13•8 years ago
|
||
Comment on attachment 8811723 [details] [diff] [review]
0001-Bug-1317779-1.0-Remove-audio-only-track-decoder-limi.patch
Review of attachment 8811723 [details] [diff] [review]:
-----------------------------------------------------------------
IIUC, we can allow 2 media elements to play concurrently where one is video only and the other is audio only.
So this patch doesn't give us what we want because it allows only one audio _or_ video decoder when media.decoder.limit=1.
We should split mDecoderLimit into mAudioDecoderLimit and mVideoDecoderLimit.
Updated•8 years ago
|
Priority: -- → P1
Assignee | ||
Comment 14•8 years ago
|
||
Split decoder limit into audio and video and pass the track type through.
Attachment #8811723 -
Attachment is obsolete: true
Attachment #8811723 -
Flags: review?(jwwang)
Attachment #8812182 -
Flags: review?(jwwang)
Comment 15•8 years ago
|
||
Comment on attachment 8812182 [details] [diff] [review]
0001-Bug-1317779-1.1-Enforce-decoder-limits-based-on-deco.patch
Review of attachment 8812182 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaFormatReader.cpp
@@ +108,2 @@
> // Requests to acquire tokens.
> std::queue<RefPtr<PromisePrivate>> mPromises;
We need 2 promise queues, one for audio and the other for video.
It might be easier to have 2 singletons to deal with audio and video respectively:
DecoderAllocPolicy&
DecoderAllocPolicy::Instance(TrackType aTrack)
{
StaticMutexAutoLock lock(sMutex);
if (aTrack == TrackInfo::kAudioTrack) {
static auto sPolicy = new DecoderAllocPolicy();
return *sPolicy;
} else {
static auto sPolicy = new DecoderAllocPolicy();
return *sPolicy;
}
}
DecoderAllocPolicy::Instance(aTrack).Alloc()->Then(...);
Assignee | ||
Comment 16•8 years ago
|
||
Separate DecoderAllocPolicy instances for audio and video.
Attachment #8812182 -
Attachment is obsolete: true
Attachment #8812182 -
Flags: review?(jwwang)
Attachment #8812765 -
Flags: review?(jwwang)
Comment 17•8 years ago
|
||
Comment on attachment 8812765 [details] [diff] [review]
0001-Bug-1317779-1.2-Separate-audio-and-video-decoder-all.patch
Review of attachment 8812765 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/media/MediaFormatReader.cpp
@@ +87,5 @@
> ReentrantMonitor mMonitor;
> // The number of decoders available for creation.
> int mDecoderLimit;
> + // Track type.
> + TrackType mTrack;
const
@@ +107,5 @@
> + {
> + DecoderAllocPolicy::Instance(mTrack).Dealloc();
> + }
> +
> + TrackType mTrack;
const
@@ +135,3 @@
> {
> StaticMutexAutoLock lock(sMutex);
> + static DecoderAllocPolicy* sPolicies[2] = {
I would prefer:
if (aTrack == TrackType::kAudioTrack) {
static auto sAudioPolicy = new DecoderAllocPolicy(TrackType::kAudioTrack);
return *sAudioPolicy;
} else {
static auto sVideoPolicy = new DecoderAllocPolicy(TrackType::kVideoTrack);
return *sVideoPolicy;
}
for lazy initialization. sVideoPolicy will never be allocated until we need a video decoder.
Attachment #8812765 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 18•8 years ago
|
||
Addressed comments.
Attachment #8812765 -
Attachment is obsolete: true
Attachment #8813272 -
Flags: review+
Comment 19•8 years ago
|
||
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/062e97fe3eba
[1.3] Separate audio and video decoder allocation policies. r=jwwang
Comment 20•8 years ago
|
||
Pushed by esawin@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf04d327cc0
[2.0] [bustage fix, CLOSED TREE] Mark implicit conversion constructors explicit. r=me
Assignee | ||
Comment 21•8 years ago
|
||
Merged bustage fix into patch 1.
Attachment #8813272 -
Attachment is obsolete: true
Attachment #8813315 -
Flags: review+
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/062e97fe3eba
https://hg.mozilla.org/mozilla-central/rev/eaf04d327cc0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8813315 [details] [diff] [review]
0001-Bug-1317779-1.4-Separate-audio-and-video-decoder-all.patch
Approval Request Comment
[Feature/Bug causing the regression]: Bug 1312337.
[User impact if declined]: Crash after media playback on some devices.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Yes (by assignee only).
[Needs manual test from QE? If yes, steps to reproduce]: STR can be found in bug 1278574 (affected device is required).
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: It does change media playback queuing behavior, however, this will only have an effect on restricted devices, which would have major media playback issues (crashes) without it.
[String changes made/needed]: None.
Attachment #8813315 -
Flags: approval-mozilla-aurora?
Comment 24•8 years ago
|
||
Comment on attachment 8813315 [details] [diff] [review]
0001-Bug-1317779-1.4-Separate-audio-and-video-decoder-all.patch
fennec crash fix for some devices, take in aurora52
Attachment #8813315 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 25•8 years ago
|
||
bugherder uplift |
status-firefox52:
--- → fixed
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
•