Crash after media playback on Samsung S3 Mini

RESOLVED FIXED in Firefox 52

Status

()

Firefox for Android
Audio/Video
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

51 Branch
Firefox 53
All
Android
Points:
---

Firefox Tracking Flags

(firefox52 fixed, firefox53 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

a year ago
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

a year ago
Summary: Crash in MP3 playback on Samsung S3 Mini → Crash after media playback on Samsung S3 Mini
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

a year 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
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

a year 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.
Blocks: 1312337
No longer blocks: 1288618, 1315510
Flags: needinfo?(jwwang)
(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)
(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?
(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

a year 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

a year ago
Created attachment 8811723 [details] [diff] [review]
0001-Bug-1317779-1.0-Remove-audio-only-track-decoder-limi.patch
Assignee: nobody → esawin
Attachment #8811723 - Flags: review?(jwwang)
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

a year 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 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.
Priority: -- → P1
(Assignee)

Comment 14

a year ago
Created attachment 8812182 [details] [diff] [review]
0001-Bug-1317779-1.1-Enforce-decoder-limits-based-on-deco.patch

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 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

a year ago
Created attachment 8812765 [details] [diff] [review]
0001-Bug-1317779-1.2-Separate-audio-and-video-decoder-all.patch

Separate DecoderAllocPolicy instances for audio and video.
Attachment #8812182 - Attachment is obsolete: true
Attachment #8812182 - Flags: review?(jwwang)
Attachment #8812765 - Flags: review?(jwwang)
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

a year ago
Created attachment 8813272 [details] [diff] [review]
0001-Bug-1317779-1.3-Separate-audio-and-video-decoder-all.patch

Addressed comments.
Attachment #8812765 - Attachment is obsolete: true
Attachment #8813272 - Flags: review+

Comment 19

a year 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

a year 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

a year ago
Created attachment 8813315 [details] [diff] [review]
0001-Bug-1317779-1.4-Separate-audio-and-video-decoder-all.patch

Merged bustage fix into patch 1.
Attachment #8813272 - Attachment is obsolete: true
Attachment #8813315 - Flags: review+

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/062e97fe3eba
https://hg.mozilla.org/mozilla-central/rev/eaf04d327cc0
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
(Assignee)

Comment 23

a year 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 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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/8ec375514353
status-firefox52: --- → fixed
You need to log in before you can comment on or make changes to this bug.