1214710 - Implement support for simultaneous decoder limiting

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Jan Henning [:JanH]]

Attachment: logcat output

This bug was filed from the Socorro interface and is 
report bp-d59bd1a6-6df1-4eea-85d2-29b2e2151014.
=============================================================
I have a feeling that since Firefox 41 was released, the frequency of this crash has increased greatly, and Crash Stats seem to back up my impression:
https://crash-stats.mozilla.com/signature/?date=%3E%3D2015-08-01&date=%3C2015-10-14T00%3A00%3A00+00%3A00&product=FennecAndroid&signature=libsomxcore.so%400x1826&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#graphs

It's now around #25 in the top crashes list for Firefox 41.

This crash seems to affect only certain Samsung devices, with ~70 % of them being a variety of the Galaxy S3 Mini.

It usually happens at some point after playing an MP3 file, whether by opening it directly, using some media site (like soundcloud.com) or using the TTS on Google Translate.

In fact, I can now relatively reliably reproduce it by going to Google Translate, entering some text, using the TTS feature and then closing the tab.
On my main profile in FF41, this usually crashes on the first attempt, while with my Nightly profile in FF44, I can crash it on the second attempt, i.e. after closing the tab, I open another new tab with Google Translate, use TTS and close it again.

I've attached the logcat output for Gecko and ACodec.
To help orient you:
16:38:20 Closing the Google Translate tab in Firefox 41, Firefox dies at 16:38:44
16:40:54 First attempt to reproduce in Firefox 44, no success
16:44:23 Second attempt, Firefox finally dies at 16:44:57
16:47:00 Having restarted Firefox Nightly, another attempt, again without success on the first try
16:49:31 Second attempt, Firefox dies at 16:50:02

Comment 1

[Jan Henning [:JanH]]

Attachment: logcat output as xlsx

The same logcat output, but with some helpful highlighting in Excel.

Comment 2

[Jan Henning [:JanH]]

[Tracking Requested - why for this release]:
This crash is now #12 among Samsung devices in general and by a large margin (65 % of all crashes, 13x as many reports as the next biggest crash) the #1 crash on the Galaxy S3 Mini (GT-I8190) family.

Comment 3

[Jan Henning [:JanH]]

The regression range is https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c4c89dc9fc4b122cfb7097423c7cce43b48eaea7&tochange=3be8a68878e6032da32f4d339222cf585b20da85, i.e. bug 1168374, which enabled the new MP3 decoder code for Android API 16+, i.e. Jelly Bean (4.1) and above.

Comment 4

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

41 was EOL'd last week. This is now a wontfix for FF41.

Comment 5

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

[Tracking Requested - why for this release]: Nom'ing for tracking 43. It is possible that this may not be super important for us to track but I did not fall it to fall off our radar completely without looking at 43 crash-stats.

Comment 6

[Jan Henning [:JanH]]

42 has definitively improved the situation - whereas before playing an MP3 file almost guaranteed a crash within the next few minutes, this is no longer the case. However I have a feeling that these crashes are still happening more frequently than pre-41.
Crash stats seems to show a similar trend:
https://crash-stats.mozilla.com/signature/?date=%3E%3D2015-09-01&date=%3C2015-11-12T00%3A00%3A00+00%3A00&product=FennecAndroid&signature=libsomxcore.so%400x1826&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&page=1#graphs

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

That's still quite a few crashes, most of them in 42 on release.  Tracking for 43. 

Did we really fix this in 43, or is it one of those crashes that we don't see reflected in the user population until a version hits the release channel?

Ioanna can your team try to reproduce this (if you have the right device around?)

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Kevin just checking that you are also aware of this.

Comment 9

[Kevin Brosnan [Ex-Mozilla]]

Yes it is a crash that affects mostly the Galaxy S3 mini. Accounting for 60% of the affected devices in a 1 month period. It is just barely within the top 50. It would be up to Snorp's team to see if they have bandwidth to address this.

Given that it is an older phone I am not surprised that this is mostly a release crash. The beta population trends much higher in the early adopter devices.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

We fixed bug 1204483 in 42, which definitely could've fixed or improved this one too. I don't think we have a S3 mini anywhere (or at least I don't have one), so I think it's unlikely we do much about this unless it blows up.

Comment 11

[Mihaela Velimiroviciu (:mihaelav)]

Reproduced the crash with Samsung Galaxy S3 Mini/Firefox 42.0.1
https://crash-stats.mozilla.com/report/index/2d9ed7dd-1ae8-4e28-9a31-3fabc2151123

Steps:
1. Go to translate.google.com
2. type text to translate
3. Play audio for text
4. Tap on the URL
5. Tap on the TTS icon
6. Speak

Result: Firefox crashed

Note: I was able to reproduce the crash only once

Comment 12

[Ioana Chiorean]

We do not have the device in house but I will try to coordinate with Mihaela Velimiroviciu  (^) to try it with the latest beta.

Comment 13

[Mihaela Velimiroviciu (:mihaelav)]

Still crashing: https://crash-stats.mozilla.com/report/index/04bee5f4-6598-4667-8d60-3cc362151126

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

[Tracking Requested - why for this release]:

Too late to fix in 43. Is this still an issue in 44+?

Comment 15

[Kevin Brosnan [Ex-Mozilla]]

Yes

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44 as it's a crash and at a decent volume (55 hits in 28 days).

Comment 17

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Margaret, Snorp: This is a crash on Fennec Android that seems to have decent volume and worth investigating for Beta44. Could you help find an owner?

28 days of data on 44 looks like:

FennecAndroid 	44.0b1 	1.13% 	94
FennecAndroid 	44.0b2 	1.40% 	116
FennecAndroid 	44.0b4 	0.58% 	48

Comment 18

[Kevin Brosnan [Ex-Mozilla]]

This is something for the platform team.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Eugen, can you get your hands on a S3 mini and look at this? I think we have some other bugs on that device too.

Comment 20

[Eugen Sawin [:esawin]]

This crash occurs on decoder shutdown when multiple decoders are active simultaneously. Since decoders aren't shutdown immediately on EOS, this is also triggered by successive playbacks.

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

I've found similar reports on the S3 crashing when multiple video decoders are used.
To prevent such failures, we should add mechanics to limit the number of simultaneous decoders allowed (which puts us in line with Chrome) and enable them for older devices (using the API version as a heuristic should be enough).

Comment 21

[James Willcox (:snorp) ([e-mail]) (he/him)]

There used to be some generic stuff in dom/media for this, because B2G had that limitation as well.

Comment 22

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.1-Enforce-platform-restriction-on-numb.patch

With this patch, we shutdown a previously instanced decoder before creating a new decoder on the affected platform/MIME-type combinations.

There is currently no straight-forward way to restrict the number of active decoders. To enable clean decoder shutdown and reuse, we would need to make major adjustments to our shutdown mechanics with limited benefits.

This solution is minimally invasive, touches only Android code and would provide the expected user experience for the majority of cases (e.g., see STR comment 20).
However, it's specialized (would need modifications should more than MP3 become affected) and the shutdown isn't clean, which leaves the aborted media in a non-resumable state.

Since decoder lifetime mechanics are undergoing changes, this might be a "good enough" intermediate solution.

Comment 23

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8714450 [details] [diff] [review]
0001-Bug-1214710-1.1-Enforce-platform-restriction-on-numb.patch

Review of attachment 8714450 [details] [diff] [review]:
-----------------------------------------------------------------

Couple nits, but otherwise lgtm

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +391,5 @@
> +      cModel.Equals("GT-I8552", nsCaseInsensitiveCStringComparator()) ||
> +      cModel.Equals("GT-I8160", nsCaseInsensitiveCStringComparator()) ||
> +      cModel.Equals("GT-I9070", nsCaseInsensitiveCStringComparator()) ||
> +      cModel.Equals("GT-P3113", nsCaseInsensitiveCStringComparator()) ||
> +      cModel.Equals("GT-P5110", nsCaseInsensitiveCStringComparator())) {

Brace on the newline for these multi-line ifs

@@ +408,5 @@
> +    // Nothing to do on platforms without decoder limits.
> +    return;
> +  }
> +
> +  if (sActiveDecoder && sActiveDecoder != aActiveDecoder) {

I had to read this three times. Maybe just use aDecoder for the argument.

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8714450 [details] [diff] [review]
0001-Bug-1214710-1.1-Enforce-platform-restriction-on-numb.patch

Review of attachment 8714450 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/platforms/android/AndroidDecoderModule.cpp
@@ +382,5 @@
> +  }
> +
> +  sLimit = kUnlimited;
> +  NS_LossyConvertUTF16toASCII cModel(model);
> +  if (cModel.Equals("GT-I8190", nsCaseInsensitiveCStringComparator()) ||

I've been told LowerCaseEqualsLiteral() is faster than Equals() because it knows the length of the string in advance. You'd need to lowercase your device id string of course.

@@ +422,5 @@
>  MediaCodecDataDecoder::InitDecoder(Surface::Param aSurface)
>  {
> +  // Some platforms don't allow multiple simultaneous decoders, we shutdown the
> +  // existing decoder in such a case before creating the new one.
> +  EnsureActiveDecoder(this);

I don't think what you're doing is threadsafe.

MediaDataDecoder::Init() can be called on multiple threads at the same time for different decoder instances. So you need a StaticMutex or somesuch to synchronize accesses to sActiveDecoder.

You also need to ensure that when you release the StaticMutex after setting `this` decoder as the active one, you're in a state where it's safe to have another instance immediately call Shutdown() on `this` on a different thread. i.e. you probably need to hold onto the mutex until the decoder has finished initializing.

You also need to ensure that at any time all of your MediaCodecDataDecoder functions are able to exit cleanly if Shutdown() is called on another thread. I think that should be the case, but you should double check.

Comment 25

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.2-Enforce-platform-restriction-on-numb.patch

Addressed comment 23 and comment 24.

I've split EnsureActiveDecoder into two functions to allow for explicit locking mechanics.

I'm reusing sActiveDecoderMonitor for locking in InitDecoder. Alternatively, we can introduce a dedicated init-phase mutex to have more consistent locking mechanics in (Ensure|Release)ActiveDecoder.

I've also moved the MIME-check in DecoderLimit to avoid repeated MIME-string comparisons on non-restricted platforms.

Comment 26

[Eugen Sawin [:esawin]]

Comment on attachment 8714911 [details] [diff] [review]
0001-Bug-1214710-1.2-Enforce-platform-restriction-on-numb.patch

Cancelling review since we need a more general solution to include restricting video decoders (bug 1244292).

Comment 27

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.3-Enforce-simultaneous-MediaDataDecode.patch

Let's explore how we can properly limit the number of simultaneous decoder-pairs (audio/video) while allowing to resume playback.

With this patch we would limit the number of active MediaDataDecoders based on the "media.decoder.limit" (= N) pref.
ReaderQueue would guarantee to keep at most N decoders active at all times by suspending all decoder(-pair)s past the limit. Once an active decoder(-pair) is shutdown, a previously instantiated decoder(-pair) will be resumed.

With this kind of mechanics, it will be possible to set N to 1 on (Android) platforms which do not support multiple decoders of the same MIME type.

Currently, resuming is based on MFR shutdown, which is triggered when the decoder is "hidden", specifics are controlled via the "media.decoder.heuristic.dormant.*" prefs, which could be adjusted for the blacklisted devices.

Comment 28

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.4-Enforce-simultaneous-MediaDataDecode.patch

Split reader queue into active and suspended queues for easier multi-decoder (N > 1) handling.

Comment 29

[Eugen Sawin [:esawin]]

With attachment 8717457 [details] [diff] [review], resuming audio-only media works, resuming video tracks is broken.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Too late for Fx44, now a wontfix

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8717457 [details] [diff] [review]
0001-Bug-1214710-1.4-Enforce-simultaneous-MediaDataDecode.patch

Review of attachment 8717457 [details] [diff] [review]:
-----------------------------------------------------------------

Structural things to the MediaFormatReader should really be reviewed by jya. Sorry for the delay.

Comment 32

[Jean-Yves Avenard [:jya]]

If I understand properly, we want to ensure the decoder are shutdown.

We already have the dormant mechanism in place to do that. It just that right now it only shutdown the video decoding part.
So we really only need to also shutdown the audio decoding.

As it is, this patch would leave the MFR in non clean state: not draining, shutting down, demuxer in unusable state. It will also be unable to resume (as decoding would restart to a non-keyframe).

While I'm not a fan of the dormant implementation, what is missing with it? 

Doing what this patch is doing would I think only a matter of adding to MediaFormatReader::ReleaseMediaResources() mAudio.ShutdownDecoder()

And then ensure that dormant is enabled on android and immediately started.

And I'd like to give more thought and a more generic implementation to limit the number of simultaneous decoders as this is something we've been talking about for a while.

Comment 33

[Eugen Sawin [:esawin]]

(In reply to Jean-Yves Avenard [:jya] from comment #32)
> If I understand properly, we want to ensure the decoder are shutdown.
> 
> We already have the dormant mechanism in place to do that. It just that
> right now it only shutdown the video decoding part.
> So we really only need to also shutdown the audio decoding.

We have mechanics to shutdown dormant decoders, what we want is to shutdown active decoders to ensure that at most N (=1 in this case) decoder pairs are active at any time.

This is different to what we do, as for this we need to keep track of the number of active decoder pairs and add mechanics to shutdown and resume decoder pairs based on their "suspend" states.

> As it is, this patch would leave the MFR in non clean state: not draining,
> shutting down, demuxer in unusable state. It will also be unable to resume
> (as decoding would restart to a non-keyframe).

The patch suspends and resumes audio-only streams as required (within the limits of our dormant implementation). I was hoping to get some feedback on what needs to be done to enable proper resuming of video streams, which is something the patch doesn't do correctly, as you've noted.

> And I'd like to give more thought and a more generic implementation to limit
> the number of simultaneous decoders as this is something we've been talking
> about for a while.

This is really what this patch is about, unless you think of decoder reuse?

We have identified platforms, which have broken multi-decoder support, i.e., new decoder initializations could fail or trigger a crash on shutdown, the only way to address this is having strict control over the number of simultaneous decoders.

Comment 34

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #33)
> This is different to what we do, as for this we need to keep track of the
> number of active decoder pairs and add mechanics to shutdown and resume
> decoder pairs based on their "suspend" states.

ok then..

But your changes won't work for video as there's no way it will resume properly. And surely, if you get issues with multiple audio, you'll also get issues with multiple videos.

It will work for MP3 audio only because all frames are keyframes.

For opus, it won't resume on the proper stream as you need to account for rolling.

> The patch suspends and resumes audio-only streams as required (within the
> limits of our dormant implementation). I was hoping to get some feedback on
> what needs to be done to enable proper resuming of video streams, which is
> something the patch doesn't do correctly, as you've noted.
> 

You basically need to start the internal seek machinery by storing where you're up to
You'll need to drain the decoder, see what frames your up to, store it in the internal seek target.

And upon resume, you simply restart it all ; decoders will be recreated automatically of they have been shutdown. it will seek internally where it needs to be and resume.

> > And I'd like to give more thought and a more generic implementation to limit
> > the number of simultaneous decoders as this is something we've been talking
> > about for a while.
> 
> This is really what this patch is about, unless you think of decoder reuse?

I'd prefer to have the ReaderQueue implementation separated from the MediaFormatReader and place this so it also manages MediaDecoderReader

I think it should reuse the dormant API and expand ReleaseMediaResources so it also shutdown the audio decoder.

It may be best if you coordinate with kamidphish, because he has something that does almost all that you need to do (that is suspend and destroy the decoder and resume when the tab becomes visible again).
Almost all the bricks that he needed will do what you want here.



> 
> We have identified platforms, which have broken multi-decoder support, i.e.,
> new decoder initializations could fail or trigger a crash on shutdown, the
> only way to address this is having strict control over the number of
> simultaneous decoders.

that sucks :)

Comment 35

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Are you still working on this?

Comment 36

[Eugen Sawin [:esawin]]

As per comment 34, I was waiting for feedback regarding the decoder suspension work to avoid conflicts there. I'll move forward with my approach then and see how I can get video resuming to work.

Comment 37

[u480271]

Hi, the approach I'm working on for bug 1224973 is to stop decoding frames, but not destroy the decoder.

I agree with jya, that we want a better dormant mode but I don't have any work that might be directly relevant. I had a prototype of triggering dormant mode when a HTMLMediaElement went into a background tab, but we decided not to go with that method.

Comment 38

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Blake - this could be your cue to find someone to fix up dormant mode.

Comment 39

[Alfredo Yang (:alfredo)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #38)
> Blake - this could be your cue to find someone to fix up dormant mode.

It looks like MediaDataDecoder on Fennec needs to use MediaSystemResourceService to control decoder's lifetime. So it guarantees only one decoder is active at the same time.

However, there is one limitation on MediaSystemResourceService: it allows only one decoder active at the same time. Most Android devices should be able to decode more than one videos simultaneously.
We need to know the decoder capability of device.

Comment 40

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Alfredo Yang (:alfredo) from comment #39)
> However, there is one limitation on MediaSystemResourceService: it allows
> only one decoder active at the same time. Most Android devices should be
> able to decode more than one videos simultaneously.
> We need to know the decoder capability of device.

We need to be able to arbitrate to figure out who gets the decoder. Consider the situation where one decoder is visible and one is off screen. The visible one should get the decoder.

Comment 41

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.5-Enforce-simultaneous-MediaDataDecode.patch

Fixed video resuming.

Comment 42

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.6-Enforce-simultaneous-MediaDataDecode.patch

Some minor changes to resuming mechanics.

Comment 43

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.7-Enforce-simultaneous-MediaDataDecode.patch

With bug 1224973 landed, we can reuse some of the new mechanics to make decoding suspending and resuming work properly.

This patch adds a way to suspend and resume a reader and maintain it's state. We also extend ReleaseMediaResource to release the audio decoder during a suspend action.

By default, a reader starts in suspended state and gets added to the ReaderQueue.
The ReaderQueue will resume the last suspended reader once the number of active readers drops below the max decoder limit (controlled by pref "media.decoder.limit", defaults to -1 ~ unlimited).

Comment 44

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1214710-2.1-Recover-decode-seek-position-when-re.patch

Part 2: here we mirror the visibility tracking to manage the reader suspend state.
For this we generalize Initiate(Video)DecodeRecoverySeek to allow for video-audio seek recovery.

TODO: update function comments.

Comment 45

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755569 [details] [diff] [review]
0001-Bug-1214710-1.7-Enforce-simultaneous-MediaDataDecode.patch

Review of attachment 8755569 [details] [diff] [review]:
-----------------------------------------------------------------

A good start, but I'm afraid that just won't work.

You could hijack the dormant mode to resume I guess.

JW may have some other ideas on how to best proceed.

::: dom/media/MediaDecoderReader.cpp
@@ +65,5 @@
>  
> +class ReaderQueue
> +{
> +public:
> +  typedef MediaDecoderReader Reader;

I don't really like having such typedef, it just makes things confusing for the reader (not the MediaDecoderReader, people like me that is :) )

@@ +66,5 @@
> +class ReaderQueue
> +{
> +public:
> +  typedef MediaDecoderReader Reader;
> +  typedef RefPtr<Reader> ReaderPtr;

do not use this, do not pass RefPtr<MediaDecoderReader> as argument; unless you use move semantic in which case you can pass RefPtr<MediaDecoderReader>&& but I don't think you'll want that here

@@ +75,5 @@
> +  { }
> +
> +  void MaxNumActive(int32_t aNumActive)
> +  {
> +    MonitorAutoLock lock(mMonitor);

You don't need the monitor. You will only ever Push/Remove in the constructor of the MediaDecoderReader which is only ever called on the main thread.

So MOZ_ASSERT for each member that it's only ever called on the main thread.

@@ +84,5 @@
> +      mNumMaxActive = aNumActive;
> +    }
> +  }
> +
> +  void Push(ReaderPtr aReader)

MediaDecoderReader*

Do not pass a RefPtr as argument.

@@ +106,5 @@
> +      DispatchSuspendResume(suspendReader, aReader);
> +    }
> +  }
> +
> +  void Remove(ReaderPtr aReader)

You shouldn't pass RefPtr in argument here. It should take a MediaDecoderReader* instead

@@ +130,5 @@
> +
> +private:
> +  void DispatchResume(ReaderPtr aReader) const
> +  {
> +    nsCOMPtr<nsIRunnable> task = NewRunnableMethod(

aReader->OwnerThread()->Dispatch(NewRunnable...

@@ +142,5 @@
> +      [aSuspend, aResume] () {
> +        aSuspend->Suspend();
> +        nsCOMPtr<nsIRunnable> task = NewRunnableMethod(
> +            aResume, &Reader::Resume);
> +        aResume->OwnerThread()->Dispatch(task.forget());

aResume->OwnerThread()->Dispatch(NewRunnableMethod(...)

no need to use an intermediary.

@@ +144,5 @@
> +        nsCOMPtr<nsIRunnable> task = NewRunnableMethod(
> +            aResume, &Reader::Resume);
> +        aResume->OwnerThread()->Dispatch(task.forget());
> +    });
> +    aSuspend->OwnerThread()->Dispatch(task.forget());

why don't you simply dispatch two tasks in a row? Why dispatch a task in which you will dispatch another task?

seems overly complicated to me.

@@ +147,5 @@
> +    });
> +    aSuspend->OwnerThread()->Dispatch(task.forget());
> +  }
> +
> +  std::list<ReaderPtr> mActive;

please use a nsTArray<RefPtr<MediaDecoderReader>>, you don't need the stl. would be more efficient here too as you have O(1) access to access the last element

@@ +154,5 @@
> +
> +  mutable Monitor mMonitor;
> +};
> +
> +static ReaderQueue sReaderQueue;

You should be allocating this on the heap and keep the reference in a StaticAutoPtr (defined in "mozilla/StaticPtr.h").
A MediaDecoderReader is always created on the main thread, so please make sReaderQueue a static StaticAutoPtr and test in the MediaDecoderReader constructor if sReaderQueue is null, and create it when needed.

@@ +180,4 @@
>        mTaskQueue, this, &MediaDecoderReader::NotifyDataArrived);
>    }
>  
> +  sReaderQueue.MaxNumActive(Preferences::GetInt("media.decoder.limit", -1));

Please use the new MediaPrefs...

::: dom/media/MediaDecoderReader.h
@@ +292,5 @@
> +  {
> +    return mIsSuspended;
> +  }
> +
> +  virtual void IsSuspended(bool aState)

this is a very confusing name. Shouldn't you name this like SetSuspendedState or something?

::: dom/media/MediaFormatReader.cpp
@@ +111,5 @@
> +  IsSuspended(false);
> +
> +  TrackType track = HasVideo() ? TrackType::kVideoTrack
> +                               : TrackType::kAudioTrack;
> +  ScheduleUpdate(track);

how would that work? so if you have video, you have suspended both audio and video, yet you only ever schedule an update for video?

In any case, this will not work.

After a Reset() nothing will ever happen until you either seek or call Request*Data() once again.

which the MDSM will never do again because you have rejected the MediaDataPromise and as such, your media element is now in error.

you Resume() is broken really, I don't see how that could work.

@@ +959,4 @@
>      return;
>    }
>  
> +  if (IsSuspended()) {

don't need this, you've called Reset() so decoding has been disabled. so NeedInput() will always return false.

Comment 46

[Jean-Yves Avenard [:jya]]

Comment on attachment 8755570 [details] [diff] [review]
0002-Bug-1214710-2.1-Recover-decode-seek-position-when-re.patch

Review of attachment 8755570 [details] [diff] [review]:
-----------------------------------------------------------------

Passing the request to JW, the MDSM is his thing really. I guess most of my doubts for P1.7 are cleared with this patch...

Comment 47

[Eugen Sawin [:esawin]]

Just to clarify, P2.1 is only required to sync audio in video streams. With P1.7 only, everything would work but the audio seek position would not be recovered when resuming video playback.

Comment 48

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

Comment on attachment 8755569 [details] [diff] [review]
0001-Bug-1214710-1.7-Enforce-simultaneous-MediaDataDecode.patch

Review of attachment 8755569 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.cpp
@@ +84,5 @@
> +      mNumMaxActive = aNumActive;
> +    }
> +  }
> +
> +  void Push(ReaderPtr aReader)

I would prefer the names like Push/Pop or Add/Remove.

@@ +93,5 @@
> +      // Below active limit, resume the new reader.
> +      mActive.push_front(aReader);
> +      DispatchResume(aReader);
> +    } else if (mActive.empty()) {
> +      // Limit must be set to 0, no active readers allowed.

Just assert |mNumMaxActive == 0| to be clear. Btw how is 0 a sensible value for mNumMaxActive?

@@ +100,5 @@
> +      // We're past the active limit, suspend an old reader and resume the new.
> +      mActive.push_front(aReader);
> +      auto it = mActive.rbegin();
> +      auto suspendReader = *it;
> +      mActive.erase(--it.base());

Why is pre decrement needed here?

@@ +144,5 @@
> +        nsCOMPtr<nsIRunnable> task = NewRunnableMethod(
> +            aResume, &Reader::Resume);
> +        aResume->OwnerThread()->Dispatch(task.forget());
> +    });
> +    aSuspend->OwnerThread()->Dispatch(task.forget());

I guess this is because each reader has its own task queue. This is the way to guarantee one reader is suspended before another to be resumed provided Suspend() will release resources synchronously.

@@ +151,5 @@
> +  std::list<ReaderPtr> mActive;
> +  std::list<ReaderPtr> mSuspended;
> +  uint32_t mNumMaxActive;
> +
> +  mutable Monitor mMonitor;

You want a Mutex if you don't need Notify() and Wait().

@@ +154,5 @@
> +
> +  mutable Monitor mMonitor;
> +};
> +
> +static ReaderQueue sReaderQueue;

Static initialization will slow launch time. Please refer to MediaDecoder::InitStatics() which is called in nsLayoutStatics::Initialize().

@@ +180,4 @@
>        mTaskQueue, this, &MediaDecoderReader::NotifyDataArrived);
>    }
>  
> +  sReaderQueue.MaxNumActive(Preferences::GetInt("media.decoder.limit", -1));

MaxNumActive() should only be called once since we don't want to change the limit to disturb the reader queue management.

::: dom/media/MediaDecoderReader.h
@@ +288,4 @@
>    // Notified by the OggReader during playback when chained ogg is detected.
>    MediaEventSource<void>& OnMediaNotSeekable() { return mOnMediaNotSeekable; }
>  
> +  virtual bool IsSuspended() const

Why is this needed? Won't ReaderQueue::mSuspended tell you if this reader is suspended?

Comment 49

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

Comment on attachment 8755570 [details] [diff] [review]
0002-Bug-1214710-2.1-Recover-decode-seek-position-when-re.patch

Review of attachment 8755570 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1360,4 @@
>    }
>  
>    // Start video-only seek to the current time...
> +  InitiateDecodeRecoverySeek(MediaDecoderReader::VIDEO_ONLY);

We need to resume both audio and video if last call is |Reset(MediaDecoderReader::AUDIO_VIDEO)|, right?

@@ +1445,5 @@
> +  if (mSeekTask || mQueuedSeek.Exists()) {
> +    return;
> +  }
> +
> +  InitiateDecodeRecoverySeek(MediaDecoderReader::AUDIO_VIDEO);

We don't want to resume video if mIsVisible is still false.

::: dom/media/MediaDecoderStateMachine.h
@@ +377,4 @@
>  
>    void BufferedRangeUpdated();
>  
> +  void SuspendedChanged();

Call this |ReaderSuspendedChanged|.

@@ +970,4 @@
>    // The buffered range. Mirrored from the decoder thread.
>    Mirror<media::TimeIntervals> mBuffered;
>  
> +  Mirror<bool> mIsSuspended;

Call this |mIsReaderSuspended|.

Comment 50

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.8-Implement-ReaderQueue-for-simultaneo.patch

I've address all the comments unless noted otherwise below. Also, I've changed the initial reader state to active instead of suspended to guarantee no change in semantics if no decoder limit is set.

(In reply to Jean-Yves Avenard [:jya] from comment #45)
> You don't need the monitor. You will only ever Push/Remove in the
> constructor of the MediaDecoderReader which is only ever called on the main
> thread.

Remove is called in Shutdown, which is called on the task queue.

> please use a nsTArray<RefPtr<MediaDecoderReader>>, you don't need the stl.
> would be more efficient here too as you have O(1) access to access the last
> element

We remove elements from random positions in Remove. Since this lists will hold decoder pairs (N ~ 1-10), we shouldn't expect O(n) access and the memory overhead to become a performance bottleneck.

(In reply to JW Wang [:jwwang] from comment #48)
> > +      mActive.erase(--it.base());
> 
> Why is pre decrement needed here?

In the conversion from a reverse_iterator to an iterator, we need to account for the reverse_iterator's base offset. Alternatively, we can write |std::next(it).base()| or |(++i).base()| instead.
 
> I guess this is because each reader has its own task queue. This is the way
> to guarantee one reader is suspended before another to be resumed provided
> Suspend() will release resources synchronously.

Correct.
 
> Static initialization will slow launch time. Please refer to
> MediaDecoder::InitStatics() which is called in nsLayoutStatics::Initialize().

I've moved ReaderQueue's access into a singleton, but I can refactor that if preferred.

Comment 51

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1214710-2.2-Recover-decode-seek-position-when-re.patch

(In reply to JW Wang [:jwwang] from comment #49)
> We need to resume both audio and video if last call is
> |Reset(MediaDecoderReader::AUDIO_VIDEO)|, right?

We don't want to do it in VisabilityChanged, see following reply.

> We don't want to resume video if mIsVisible is still false.

We don't want to resume video decoding, but we need to recover the audio and video seek positions in sync. This way, if mIsReaderSuspended = false and mIsVisible = false holds, the seek position is recovered properly for the audio-only background playback.

Comment 52

[Jean-Yves Avenard [:jya]]

Comment on attachment 8758383 [details] [diff] [review]
0001-Bug-1214710-1.8-Implement-ReaderQueue-for-simultaneo.patch

Review of attachment 8758383 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.cpp
@@ +169,5 @@
> +
> +  static StaticAutoPtr<ReaderQueue> sInstance;
> +
> +  std::list<RefPtr<MediaDecoderReader>> mActive;
> +  std::list<RefPtr<MediaDecoderReader>> mSuspended;

Again, please don't use a stdc++, a nsTArray will be more efficient.

::: dom/media/MediaFormatReader.cpp
@@ +90,5 @@
> +  if (IsSuspended()) {
> +    return;
> +  }
> +
> +  SetIsSuspended(true);

IMHO, this should be done in the caller of suspend. It should be up to the dispatcher to set the suspend flag.

I don't see anything here unique to MediaFormatReader ; all of this is applicable with all MediaDecoderReader

I believe MediaFormatReader::Suspend/Resume should be removed and set the flag as suspended in DispatchSuspendResume instead.

@@ +109,5 @@
> +  }
> +
> +  SetIsSuspended(false);
> +
> +  TrackType track = HasVideo() ? TrackType::kVideoTrack

But having said that, I don't think this is necessary to schedule an update anyway. It will be done once the next resume will occur.

Scheduling an update will not resume the system properly. It must be reset/seeked/request*data ; until then the reader is now in non-working state.

Comment 53

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.9-Implement-ReaderQueue-for-simultaneo.patch

(In reply to Jean-Yves Avenard [:jya] from comment #52) 
> IMHO, this should be done in the caller of suspend. It should be up to the
> dispatcher to set the suspend flag.
> 
> I don't see anything here unique to MediaFormatReader ; all of this is
> applicable with all MediaDecoderReader
> 
> I believe MediaFormatReader::Suspend/Resume should be removed and set the
> flag as suspended in DispatchSuspendResume instead.

That's right, we can move it all out of MFR if we publicly expose HasVideo instead.

I've worked it into this patch version.

Comment 54

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

Comment on attachment 8758849 [details] [diff] [review]
0001-Bug-1214710-1.9-Implement-ReaderQueue-for-simultaneo.patch

Review of attachment 8758849 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.cpp
@@ +104,5 @@
> +      // Below active limit, resume the new reader.
> +      mActive.AppendElement(aReader);
> +      DispatchResume(aReader);
> +    } else if (mActive.IsEmpty()) {
> +      MOZ_ASSERT(mNumMaxActive == 0);

I still can't understand how |mNumMaxActive == 0| is sensible. Do we really want to change mNumMaxActive dynamically?

@@ +112,5 @@
> +      mActive.AppendElement(aReader);
> +      MediaDecoderReader* suspendReader = mActive.ElementAt(0);
> +      mSuspended.AppendElement(suspendReader);
> +      mActive.RemoveElementAt(0);
> +      DispatchSuspendResume(suspendReader, aReader);

Since this is called in the constructor of |aReader|, we don't need to resume |aReader| which defaults to |mIsSuspended=false|. It appears to be a race between suspending |suspendReader| and creating decoders for |aReader|.

@@ +122,5 @@
> +    MutexAutoLock lock(mMutex);
> +
> +    if (aReader->IsSuspended()) {
> +      // Removing suspended readers has no immediate side-effects.
> +      mSuspended.RemoveElement(aReader);

Assert return value is true so we can guarantee |aReader->IsSuspended() == true| iff |aReader is in mSuspended| and |aReader->IsSuspended() == false| iff |aReader is in mActive|.

@@ +125,5 @@
> +      // Removing suspended readers has no immediate side-effects.
> +      mSuspended.RemoveElement(aReader);
> +    } else {
> +      // For each removed active reader, we resume a suspended one.
> +      mActive.RemoveElement(aReader);

Ditto.

@@ +159,5 @@
> +    }
> +    aReader->SetIsSuspended(true);
> +
> +    if (aReader->HasVideo()) {
> +      aReader->ResetDecode(MediaDecoderReader::AUDIO_VIDEO);

Why do we reset both audio and video?

::: dom/media/MediaDecoderReader.h
@@ +289,5 @@
>    MediaEventSource<void>& OnMediaNotSeekable() { return mOnMediaNotSeekable; }
>  
> +  virtual bool IsSuspended() const
> +  {
> +    return mIsSuspended;

Assert this is called on the reader queue thread.

@@ +294,5 @@
> +  }
> +
> +  virtual void SetIsSuspended(bool aState)
> +  {
> +    mIsSuspended = aState;

Ditto.

Comment 55

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #53)
> Created attachment 8758849 [details] [diff] [review]
> 0001-Bug-1214710-1.9-Implement-ReaderQueue-for-simultaneo.patch
> 
> (In reply to Jean-Yves Avenard [:jya] from comment #52) 
> > IMHO, this should be done in the caller of suspend. It should be up to the
> > dispatcher to set the suspend flag.
> > 
> > I don't see anything here unique to MediaFormatReader ; all of this is
> > applicable with all MediaDecoderReader
> > 
> > I believe MediaFormatReader::Suspend/Resume should be removed and set the
> > flag as suspended in DispatchSuspendResume instead.
> 
> That's right, we can move it all out of MFR if we publicly expose HasVideo
> instead.
> 

This information is already available via the MediaDecoderReader::mInfo member.

But why would you need this anyway?

to suspend/resume I don't see why it matters, it's all handled in ReleaseMediaResources.

> I've worked it into this patch version.

Comment 56

[Jean-Yves Avenard [:jya]]

Comment on attachment 8758849 [details] [diff] [review]
0001-Bug-1214710-1.9-Implement-ReaderQueue-for-simultaneo.patch

Review of attachment 8758849 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with methods removed and follow changed reworked accordingly.

In any case, you don't want something unique to MediaFormatReader

::: dom/media/MediaFormatReader.h
@@ +100,4 @@
>    // Used for debugging purposes.
>    void GetMozDebugReaderData(nsAString& aString);
>  
> +  bool HasVideo() override { return mVideo.mTrackDemuxer; }

you don't need this.

you have the mInfo member that will tell you if you have audio and/or video.

HasVideo() has a different meaning/use within the MediaFormatReader that is to know if the demuxer has been created.
you have the final value in mInfo

Comment 57

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.10-Implement-ReaderQueue-for-simultane.patch

(In reply to JW Wang [:jwwang] from comment #54)
> I still can't understand how |mNumMaxActive == 0| is sensible. Do we really
> want to change mNumMaxActive dynamically?

With the latest patch version, we don't allow dynamic changes to it, but allowing 0 would be an easy way to disable all decoding activity, should this be required on a platform to, e.g., avoid crashes until a proper fix has landed.

> Since this is called in the constructor of |aReader|, we don't need to
> resume |aReader| which defaults to |mIsSuspended=false|.

I think it's safer for the ReaderQueue to not make assumptions about the initial state of the reader. The redundant calls to Resume/Suspend are semantically NOPs.

> It appears to be a race between suspending |suspendReader| and creating decoders for |aReader|.

That's the reason I've set it to mIsSuspended=true initially, but upon further investigation, I think there is no potential race condition. All decoder initialization is processed on the task queue and with the ReaderQueue::Add call in MediaDecoderReader's constructor, the resuming/suspending is guaranteed to be processed first. Is there an initialization path that I'm missing?

> Why do we reset both audio and video?

We actually don't need to, I've changed it to video-only.

(In reply to Jean-Yves Avenard [:jya] from comment #55)
> This information is already available via the MediaDecoderReader::mInfo
> member.
> 
> But why would you need this anyway?
> 
> to suspend/resume I don't see why it matters, it's all handled in
> ReleaseMediaResources.

We need to call ResetDecode when suspending video decoding. I've exposed HasVideo() now in MediaDecoderReader for ReaderQueue's access, is this ok?

Comment 58

[Jean-Yves Avenard [:jya]]

(In reply to Eugen Sawin [:esawin] from comment #57)

> We need to call ResetDecode when suspending video decoding. I've exposed
> HasVideo() now in MediaDecoderReader for ReaderQueue's access, is this ok?

You don't need to.

You have mInfo which is a protected member, and you can use MediaDecoderReader::mInfo.HasVideo() instead.
So I'd prefer you to use GetInfo() or simply use the existing ReadUpdatedMetadata.

Having said that, I question why you're handling video differently than audio. You shouldn't. If you need to call ResetDecode for video, then you also need to do so for Audio.

So simply calling ResetDecode(AUDIO_VIDEO) and not having to worry about having either audio and video would be preferable. In which case there's no need to have a HasVideo, if you need a different handling to suspend audio and video, then it should be the responsibility of the reader itself as it owns the MediaDataDecoder. But I don't see why you would need to do that anyway.

Comment 59

[Jean-Yves Avenard [:jya]]

Btw, my question "why do you need to reset both audio and video" wasn't about arguing that only video needs to be reset (as opposed to audio only). My question was about why we need reset *at all*

So by that I meant either you reset both, or your reset none. And when you seek back automatically, the MDSM will already perform a reset. So it seems to me that you now have duplicated logic in different places.

Comment 60

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1214710-1.11-Implement-ReaderQueue-for-simultane.patch

Updated according to latest comments (jya).

(In reply to JW Wang [:jwwang] from comment #54)
> I still can't understand how |mNumMaxActive == 0| is sensible. Do we really
> want to change mNumMaxActive dynamically?

With the latest patch version, we don't allow dynamic changes to it, but
allowing 0 would be an easy way to disable all decoding activity, should
this be required on a platform to, e.g., avoid crashes until a proper fix
has landed.

> Since this is called in the constructor of |aReader|, we don't need to
> resume |aReader| which defaults to |mIsSuspended=false|.

I think it's safer for the ReaderQueue to not make assumptions about the
initial state of the reader. The redundant calls to Resume/Suspend are
semantically NOPs.

> It appears to be a race between suspending |suspendReader| and creating decoders for |aReader|.

I've set it back to mIsSuspended=true initially. I was under the impression that there is no possible race condition since all the initialization is being done on the task queue.

> Why do we reset both audio and video?

See comment 59, apparently we don't need to reset video anymore.

Comment 61

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1214710-2.3-Recover-decode-seek-position-when-re.patch

Comment 62

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

Comment on attachment 8760313 [details] [diff] [review]
0001-Bug-1214710-1.11-Implement-ReaderQueue-for-simultane.patch

Review of attachment 8760313 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderReader.cpp
@@ +133,5 @@
> +      MOZ_ASSERT(result, "Non-suspended reader must be in mActive");
> +      if (mSuspended.IsEmpty()) {
> +        return;
> +      }
> +      MediaDecoderReader* resumeReader = mSuspended.LastElement();

It looks like we will starve he oldest reader if we always resume the latest one.

Comment 63

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

Comment on attachment 8760314 [details] [diff] [review]
0002-Bug-1214710-2.3-Recover-decode-seek-position-when-re.patch

Review of attachment 8760314 [details] [diff] [review]:
-----------------------------------------------------------------

When the reader is suspended, both audio and video decoding are suspended. There is no need to seek at all when resuming a reader, right?

Comment 64

[Jean-Yves Avenard [:jya]]

(In reply to JW Wang [:jwwang] from comment #63)
> Comment on attachment 8760314 [details] [diff] [review]
> 0002-Bug-1214710-2.3-Recover-decode-seek-position-when-re.patch
> 
> Review of attachment 8760314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> When the reader is suspended, both audio and video decoding are suspended.
> There is no need to seek at all when resuming a reader, right?

yes you have to.

When shutting down, the decoder is first flushed. To resume playback you must restart at a keyframe. That's what seeking does.

Comment 65

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1214710-3.1-Reset-video-decode-suspend-timer-whe.patch

With bug 1275481 landed, we need a way to reset the timer in the case when the media has become visible, while the reader was suspended.

Also, after rebasing, the resuming mechanics have changed so that suspended media is automatically paused now and resuming requires user interaction. I don't know which bug has changed this behavior, but I think this is not a disadvantage. Eventually, we can introduce more sophisticated resuming heuristics based on visibility/dormant state.

Comment 66

[Eugen Sawin [:esawin]]

Try looks OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=923203b1b904&selectedJob=22118673

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/74a7d364f5a3
[1.11] Implement ReaderQueue for simultaneous decoder limit enforcement. r=jya,jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/d64ca44c11ce
[2.4] Recover decode seek position when resuming suspended playback. r=jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a796ebc560f
[3.1] Reset video decode suspend timer when resuming reader. r=jwwang

Comment 68

[Carsten Book [:Tomcat]]

sorry had to back out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=29771068&repo=mozilla-inbound

Comment 69

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/222cf54605e3
Backed out changeset 8a796ebc560f 
https://hg.mozilla.org/integration/mozilla-inbound/rev/2daeefb5d0d0
Backed out changeset d64ca44c11ce 
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8e2176b90c6
Backed out changeset 74a7d364f5a3 for bustage on a CLOSED TREE

Comment 70

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1214710-2.5-Recover-decode-seek-position-when-re.patch

Rebased on bug 1276570 and merged patch 2 and 3.

Comment 71

[Eugen Sawin [:esawin]]

Looks good again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbc480e74f12

Comment 72

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b94fc162494
[1.11] Implement ReaderQueue for simultaneous decoder limit enforcement. r=jya,jwwang
https://hg.mozilla.org/integration/mozilla-inbound/rev/42747e600949
[2.5] Recover decode seek position when resuming suspended playback. r=jwwang

Comment 73

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b94fc162494
https://hg.mozilla.org/mozilla-central/rev/42747e600949