1317779 - Crash after media playback on Samsung S3 Mini

Status: RESOLVED FIXED

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

Description

[Eugen Sawin [:esawin]]

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.

Comment 1

[Matt Woodrow (:mattwoodrow)]

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.

Comment 2

[Eugen Sawin [:esawin]]

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

[Matt Woodrow (:mattwoodrow)]

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

Comment 4

[Eugen Sawin [:esawin]]

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.

Comment 5

[Eugen Sawin [:esawin]]

Missing link:
[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaFormatReader.cpp#132

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

(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

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 9

[Eugen Sawin [:esawin]]

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

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1317779-1.0-Remove-audio-only-track-decoder-limi.patch

Comment 11

[JW Wang [:jwwang] [:jw_wang]]

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

Comment 12

[Eugen Sawin [:esawin]]

(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

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 14

[Eugen Sawin [:esawin]]

Attachment: 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.

Comment 15

[JW Wang [:jwwang] [:jw_wang]]

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(...);

Comment 16

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1317779-1.2-Separate-audio-and-video-decoder-all.patch

Separate DecoderAllocPolicy instances for audio and video.

Comment 17

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 18

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1317779-1.3-Separate-audio-and-video-decoder-all.patch

Addressed comments.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/062e97fe3eba
[1.3] Separate audio and video decoder allocation policies. r=jwwang

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaf04d327cc0
[2.0] [bustage fix, CLOSED TREE] Mark implicit conversion constructors explicit. r=me

Comment 21

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1317779-1.4-Separate-audio-and-video-decoder-all.patch

Merged bustage fix into patch 1.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/062e97fe3eba
https://hg.mozilla.org/mozilla-central/rev/eaf04d327cc0

Comment 23

[Eugen Sawin [:esawin]]

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.

Comment 24

[Julien Cristau [:jcristau]]

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

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8ec375514353