1123535 - Make Mp4Reader dormant capable in desktop Firefox

Status: RESOLVED FIXED

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

Description

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

We need to make MP4Readers in Desktop Firefox that aren't being used dormant, to reduce resource usage.

This is pretty easy; we just need to shutdown the MediaDataDecoders and the PDM. We don't need to tear down the demuxer. We can re-create the PDM and decoders if the MP4Reader comes out of dormant when ReadMetadata() is called again.

We'll need to change the logic in HTMLMediaElement to not be so aggressive in making <video> elements dormant; currently it makes the element dormant if it becomes non-visible. This is fine for B2G, but on desktop I think it makes sense to only make paused non-visible videos dormant after a timeout. 60s would be fine. We can do this in another bug.

Comment 1

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

Attachment: WIP Patch: Make MP4Reader dormant capable

Implement MP4Reader::ReleaseMediaResources(), to make the MP4Reader able to become dormant.

This patch is based on top of Blake's patch in bug 1113527.

MP4Reader goes dormant by simply shutting down and destroying the decoders and PDM. We do not shutdown the demuxer. When we want to leave dormant state, we will act as if we were loading metadata; we create a new PDM and decoders, which may still need to wait for resources.

I've tested this patch on Windows, and it makes videos non-visible dormant, and re-activates them when the videos become visible again.

Blake: Can you please test this patch on MP4Reader on B2G? I don't have a B2G device.

Sotaro: Any feedback here is welcome.

Comment 2

[Blake Wu [:bwu][:blakewu]]

Comment on attachment 8551591 [details] [diff] [review]
WIP Patch: Make MP4Reader dormant capable

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

I have tested it on my Flame. This patch can only work on non-MSE case (local playback) and on MSE it cannot work well. The difference is for MSE on B2G SharedDecoderManager plays a proxy role between MP4Reader and video decoder.

::: dom/media/fmp4/MP4Reader.cpp
@@ +1020,3 @@
>    if (mVideo.mDecoder) {
> +    Flush(kVideo);
> +    mVideo.mDecoder->Shutdown();

This can only work for non-MSE case(local playback). For MSE case, ShutDown will finally go to SharedDecoderManager's setIdle() @[1] which will not really release resource.
mVideo.mDecoder->ReleaseMediaResources() should be the correct function to call.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/media/fmp4/SharedDecoderManager.cpp#173

Comment 3

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8551591 [details] [diff] [review]
WIP Patch: Make MP4Reader dormant capable

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

I saw the following diagram. 
  https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaSource_FirefoxOS_2_2.pdf?raw=true

From the diagram, When MSE is used, MediaSourceReader is instantiation of MediaDecoderReader from playback point of view.
The control flow from MediaSourceReader is like the following and multiple MP4Readers could exist.
- MediaSourceReader => TrackBuffer => SourceBufferDecoder => MP4Reader.

If we want to enable dormant on MES, such control like for MES seems to becomes necessary. And it might be better to limit dormant only to widows and b2g.

Comment 4

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

Attachment: WIP Patch #2

OK, another attempt. Make MP4Reader's B2G path also work on Windows. So the WMF MediaDataDecoder implements AllocateMediaResources and ReleaseMediaResources. The SharedDecoderManager also now calls these.

Dormancy works with MSE on YouTube with this patch, as well as non-MSE video.

I still think we should refactor the dormant code to shutdown the decoder instead of asking it to release/reallocate resources, but I think that will be easiest when we've refactored MSE to have multiple demuxers and one decoder instead of multiple Readers, each of which have (a proxy to) a decoder.

Comment 5

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

Attachment: Patch 1: Call Reader::ReleaseMediaResources on decode task queue

Patch 1: Split the dispatch to call Reader::ReleaseMediaResources() on the decode task queue out of bwu's patch in bug 1113527. I need this for my subsequent patch.

Comment 6

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

Attachment: Patch 2: ReleaseMediaResources on Windows/MP4Reader

* Use SharedDecoderManager on Windows to share a single decoder in MSReader using MP4Readers.
* Implement WMF MediaDataDecoder ReleaseMediaResources, and pipe that through the SharedDecoderProxy.
* SharedDecoderManager uses the MP4Reader's PDM to create the MediaDataDecoder wrapped by SharedDecoderProxy. This is so that if we're decoding EME content, the PDM CDM wrapper is wrapped by the SharedDecoderProxy. This will work fine provided we don't happen to create the wrapped decoder first on a non-encrypted video, and try to subsequently use the decoder on an encrypted video. I haven't figured out how to handle that yet.

This works for YouTube/MSE and non-MSE MP4 videos on Windows.

Comment 7

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

Attachment: Patch 2: ReleaseMediaResources on Windows/MP4Reader

* Based on Sotaro's dormant patch in bug 1123452.
* Make MP4Reader use the SharedDecoderManager on Windows, not just B2G.
* Use the same "ReleaseMediaResources" approach as B2G. We can clean that up and shutdown the decoders properly once we've refactored the MSReader to have multiple demuxers and an explicit single decoder, rather than multiple sub-readers.

Comment 8

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

Attachment: Patch 1: Call Reader::ReleaseMediaResources on decode task queue

Run Reader::ReleaseMediaResources on decode task queue, rather than the state machine thread.

I also noticed that we don't need to call AwaitIdle() before dispatching the ReleaseMediaResources() task from RunStateMachine either; the FlushDecoding() we call earlier in that case will do an AwaitIdle() anyway.

Comment 9

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

Attachment: Patch 3: Turn on MP4Reader dormant on Windows.

Pref's match Sotaro's from bug 1123452.

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f50d403202

Comment 11

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

Comment on attachment 8554398 [details] [diff] [review]
Patch 1: Call Reader::ReleaseMediaResources on decode task queue

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1966,5 @@
>  
>    SAMPLE_LOG("EnsureAudioDecodeTaskQueued isDecoding=%d status=%d",
>                IsAudioDecoding(), mAudioRequestStatus);
>  
> +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {

I wonder if we should move this check to NeedToDecode{Audio,Video} to centralize the logic of checking whether to decode next audio/video samples.

Comment 12

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

(In reply to Chris Pearce (:cpearce) from comment #10)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f50d403202

The failures in this push are caused by calling MP4Reader::PreReadMetadata() with the decoder monitor held. Stack:

02:38:28     INFO -  10  xul.dll!NS_DispatchToMainThread(nsIRunnable *,unsigned int) [nsThreadUtils.cpp:e7f50d403202 : 177 + 0x41]
02:38:28     INFO -      rip = 0x000007fe26d989d7   rsp = 0x00000090db1bf000
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  11  xul.dll!mozilla::WMFVideoMFTManager::InitializeDXVA() [WMFVideoMFTManager.cpp:e7f50d403202 : 163 + 0xc]
02:38:28     INFO -      rip = 0x000007fe28b1bff7   rsp = 0x00000090db1bf040
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  12  xul.dll!mozilla::WMFVideoMFTManager::Init() [WMFVideoMFTManager.cpp:e7f50d403202 : 172 + 0x4]
02:38:28     INFO -      rip = 0x000007fe28b1baa3   rsp = 0x00000090db1bf080
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  13  xul.dll!mozilla::WMFMediaDataDecoder::AllocateMediaResources() [WMFMediaDataDecoder.cpp:e7f50d403202 : 158 + 0x37]
02:38:28     INFO -      rip = 0x000007fe28b1a03a   rsp = 0x00000090db1bf140
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  14  xul.dll!mozilla::MP4Reader::RequestCodecResource() [MP4Reader.cpp:e7f50d403202 : 260 + 0xc]
02:38:28     INFO -      rip = 0x000007fe28b13298   rsp = 0x00000090db1bf180
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  15  xul.dll!mozilla::MP4Reader::PreReadMetadata() [MP4Reader.cpp:e7f50d403202 : 338 + 0x4]
02:38:28     INFO -      rip = 0x000007fe28b1218b   rsp = 0x00000090db1bf1c0
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  16  xul.dll!mozilla::MediaDecoderStateMachine::DecodeMetadata() [MediaDecoderStateMachine.cpp:e7f50d403202 : 2182 + 0x2d]
02:38:28     INFO -      rip = 0x000007fe289c58e0   rsp = 0x00000090db1bf1f0
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  17  xul.dll!mozilla::MediaDecoderStateMachine::CallDecodeMetadata() [MediaDecoderStateMachine.cpp:e7f50d403202 : 2169 + 0x7]
02:38:28     INFO -      rip = 0x000007fe289bfeaa   rsp = 0x00000090db1bf340
02:38:28     INFO -      Found by: call frame info
02:38:28     INFO -  18  xul.dll!nsRunnableMethodImpl<void ( mozilla::MediaDecoderStateMachine::*)(void),void,1>::Run() [nsThreadUtils.h:e7f50d403202 : 386 + 0x6]
02:38:28     INFO -      rip = 0x000007fe289f7d2f   rsp = 0x00000090db1bf3c0


The WMFMediaDataDecoder::AllocateMediaResources() path calls InitializeDXVA which dispatches synchronously to the main thread to create the DXVA context. The main thred is blocked waiting for the monitor. The fix is simple; don't call MediaDecoderReader::PreReadMetadata() with the monitor held.

(Ideally, we should not need a PreReadMetadata() function, but we can clean that up later.)

Comment 13

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

Attachment: Patch 4: Don't hold decoder monitor while calling MDR::PrereadMetadata

Don't hold decoder monitor while calling MDR::PrereadMetadata.

Comment 14

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2fb9caa9d283

Comment 15

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

Comment on attachment 8554396 [details] [diff] [review]
Patch 2: ReleaseMediaResources on Windows/MP4Reader

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

::: dom/media/fmp4/MP4Reader.cpp
@@ +1005,5 @@
>  
>  bool MP4Reader::IsDormantNeeded()
>  {
> +#if defined(MOZ_GONK_MEDIACODEC) || defined(XP_WIN)
> +  return 

nit: Dreaded whitespace.

::: dom/media/fmp4/SharedDecoderManager.cpp
@@ +60,4 @@
>    , mActiveCallback(nullptr)
>    , mWaitForInternalDrain(false)
>    , mMonitor("SharedDecoderProxy")
> +  , mDecoderReleasedResources(false)

Comment: It is a bit yuck that we have to store this state here. All it does is reduce the number of times we call AllocateMediaResources(). Making it fast and calling it all the time would make the code less stateful.

::: dom/media/fmp4/wmf/WMFMediaDataDecoder.h
@@ +94,5 @@
> +    ReleaseMftManager,
> +    RetainMftManager
> +  };
> +
> +  void ProcessShutdown(ReleaseMode aMode);

Commnent: It might be cleaner to just have two functions. ProcessTemporaryShutdown() and ProcessShutdown() rather than passing a flag to ProcessShutdown().

Comment 16

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

Comment on attachment 8554400 [details] [diff] [review]
Patch 3: Turn on MP4Reader dormant on Windows.

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

::: modules/libpref/init/all.js
@@ +263,5 @@
>  pref("media.play-stand-alone", true);
>  
> +#if defined(XP_WIN)
> +pref("media.decoder.heuristic.dormant.enabled", true);
> +pref("media.decoder.heuristic.dormant.timeout", 60000); 

nit: whitespace

::: testing/profiles/prefs_general.js
@@ +284,5 @@
>  
>  user_pref("media.eme.enabled", true);
>  
> +#if defined(XP_WIN)
> +user_pref("media.decoder.heuristic.dormant.timeout", 0); 

nit: whitespace

Comment 17

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

(In reply to JW Wang [:jwwang] from comment #11)
> Comment on attachment 8554398 [details] [diff] [review]
> Patch 1: Call Reader::ReleaseMediaResources on decode task queue
> 
> Review of attachment 8554398 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1966,5 @@
> >  
> >    SAMPLE_LOG("EnsureAudioDecodeTaskQueued isDecoding=%d status=%d",
> >                IsAudioDecoding(), mAudioRequestStatus);
> >  
> > +  if (mState >= DECODER_STATE_COMPLETED || mState == DECODER_STATE_DORMANT) {
> 
> I wonder if we should move this check to NeedToDecode{Audio,Video} to
> centralize the logic of checking whether to decode next audio/video samples.

I too like centralizing logic, but I don't think the above would work, as the NeedToDecodeVideo() etc are called and then the DecodeVideo() task is run, so the state could change in between NeedToDecodeVideo() being called and the DecodeVideo() task being run, and so we would not catch this case.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bec74def09
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdcfd5fda7b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7348cb10d7b0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6da914b2a93

Ralph: We'll need this uplifted, along with bug 1125472 and bug 113452, but I think we should let them bake for a few days first in Nightly if possible.

Comment 19

[Carsten Book [:Tomcat]]

sorry had to back this out in https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=043a03d002f2 seems one of this changes caused a Crashtest bustage on Windows Tests like:

https://treeherder.mozilla.org/logviewer.html#?job_id=5955531&repo=mozilla-inbound

Comment 20

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/29304c69eff6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2efa0319f04
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab05b7115f6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4eb1cc07186

( try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=502284bbf3b3 )

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Backed out because something in the push was causing Linux32 opt mochitest-3 timeouts and Windows w-p-t failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e16804bd0547

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5997815&repo=mozilla-inbound
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5999248&repo=mozilla-inbound

Comment 22

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

Bug 1125472 by itself was green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a4fad808ffa
I've landed that.

Patch 1 by itself was green:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a4fad808ffa

Patch 4 caused the Linux test failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb7af25bbc7

Enabling dormant for MP4Reader caused the WPT-4 failure:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d43d549352e5

(Clearing ni?rillian until this is green and landed.)

Comment 23

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/63bccd7cd93f
https://hg.mozilla.org/integration/mozilla-inbound/rev/b787a2e8334d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee07b0a66b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6cecb2d01ee


Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9888dba8f3bc

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63bccd7cd93f
https://hg.mozilla.org/mozilla-central/rev/b787a2e8334d
https://hg.mozilla.org/mozilla-central/rev/7ee07b0a66b3
https://hg.mozilla.org/mozilla-central/rev/a6cecb2d01ee

Comment 25

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

Comment on attachment 8554396 [details] [diff] [review]
Patch 2: ReleaseMediaResources on Windows/MP4Reader

I'm requesting a blanket approval for all patches in this bug.

Approval Request Comment
[Feature/regressing bug #]: Media Source
[User impact if declined]: Out Of Memory crashes. This patch shuts down video decoders that are paused and not visible for 60 seconds, releasing GPU and CPU memory. This should reduce memory consumption of idle video in background tabs, reducing chance of out of memory crashes on YouTube, which have spiked in the last week or so.
[Describe test coverage new/current, TreeHerder]: This has been in m-c for a day and tested locally.
[Risks and why]: There's a chance this could break video decoding on Windows completely. Most of the platform agnostic infrastructure has shipped in B2G already, this is just the Windows specific stuff, so I'm pretty confident it will be OK. We need this for reducing out of memory crashes, so I think the risk is worth it.
[String/UUID change made/needed]: None.

Comment 26

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

https://hg.mozilla.org/projects/holly/rev/52661fcbc1d8
https://hg.mozilla.org/projects/holly/rev/a0494361053f
https://hg.mozilla.org/projects/holly/rev/b2b594928bb3
https://hg.mozilla.org/projects/holly/rev/086edcd190df

Comment 27

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8554396 [details] [diff] [review]
Patch 2: ReleaseMediaResources on Windows/MP4Reader

I am concerned about the potential risk for beta.
Anyway, the next good to build is next Monday. Until then, I am going to take in aurora to get more testing.

Comment 28

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2e2e25e3587
https://hg.mozilla.org/releases/mozilla-aurora/rev/864660ce7095
https://hg.mozilla.org/releases/mozilla-aurora/rev/e41e13cff724
https://hg.mozilla.org/releases/mozilla-aurora/rev/533ac462e0e5

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8554396 [details] [diff] [review]
Patch 2: ReleaseMediaResources on Windows/MP4Reader

Taking it for beta 6. Go to build tomorrow.

Comment 30

[Ralph Giles (:rillian)]

https://hg.mozilla.org/releases/mozilla-beta/rev/0280782f6bac
https://hg.mozilla.org/releases/mozilla-beta/rev/e884a5b5ff18
https://hg.mozilla.org/releases/mozilla-beta/rev/f94e2c0e2971
https://hg.mozilla.org/releases/mozilla-beta/rev/1192f29eb59a

Comment 31

[Ralph Giles (:rillian)]

This is failing mochitest on beta with:

Assertion failure: mState == DECODER_STATE_SEEKING || mState == DECODER_STATE_SHUTDOWN || mState == DECODER_STATE_DORMANT, at c:\builds\moz2_slave\m-beta-w32-d-00000000000000000\build\dom\media\MediaDecoderStateMachine.cpp:1631

I assume this means I forgot to uplift something. Any ideas, Chris?

Comment 32

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

Maybe bug 1124844. That's the only thing I can find that's not been uplifted in the bugs blocking Bug 1050031, the dormant tracking bug.

Comment 33

[Ralph Giles (:rillian)]

Pushed the patch from bug 1124844 on top of beta to try to see if that resolves the assertion. I don't see it with aurora though, so this may just re-arranging the timing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=534a3717e242

Comment 34

[Ralph Giles (:rillian)]

Backed out for the test_reactivate.html debug asserts. The patch from bug 1124844 didn't help.

https://hg.mozilla.org/releases/mozilla-beta/rev/bbc98a8c8142

Comment 35

[Ralph Giles (:rillian)]

I get a timeout with that test on a beta on my Mac (with the pref flipped) but haven't been able to reproduce the asserts.

Comment 36

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

I think the assert here is benign, and we can just fix the assert to handle this call path.

I think what's happening here is we're running the state machine for DORMANT state, and inside the call to FlushDecoding() while we drop the monitor to flush the task queue the main thread moves the decoder out of dormant state into DECODER_STATE_DECODING_NONE (in MDSM::SetDormant()), and so when FlushDecoding re-takes the monitor and calls ResetPlayback(), the assertion on MDSM::mState fails.

We can just change this assertion to include DECODER_STATE_DECODING_NONE state; we should complete the going dormant before we resume from being dormant.

I think this is a timing issue. We should make the same fix on Aurora and Beta too.

Comment 37

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

Relanded on beta, with fix.

https://hg.mozilla.org/releases/mozilla-beta/rev/41fac15f269a
https://hg.mozilla.org/releases/mozilla-beta/rev/f74e583e724f

Comment 38

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

Attachment: Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)

This patch applies to mozilla-central. We'll need it on Aurora too.

My diagnosis of the problem:

(In reply to Chris Pearce (:cpearce) from comment #36)
> I think the assert here is benign, and we can just fix the assert to handle
> this call path.
> 
> I think what's happening here is we're running the state machine for DORMANT
> state, and inside the call to FlushDecoding() while we drop the monitor to
> flush the task queue the main thread moves the decoder out of dormant state
> into DECODER_STATE_DECODING_NONE (in MDSM::SetDormant()), and so when
> FlushDecoding re-takes the monitor and calls ResetPlayback(), the assertion
> on MDSM::mState fails.
> 
> We can just change this assertion to include DECODER_STATE_DECODING_NONE
> state; we should complete the going dormant before we resume from being
> dormant.

Ideally once the dust settles, we can change the state machine to only change its state on a single task queue, so we don't have these out-of-order issues.

Comment 39

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

Comment on attachment 8558256 [details] [diff] [review]
Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)

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

This is fine but I wonder if the state could even change again before we enter the monitor in some extreme case. We should add a comment for as code changes it is getting harder to understand why we assert some states in a function. And I like the idea to only allow state transition in-between state machine runs not in the middle of it.

Comment 40

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

(In reply to JW Wang [:jwwang] from comment #39)
> Comment on attachment 8558256 [details] [diff] [review]
> Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)

Landed, with comment added:
https://hg.mozilla.org/integration/mozilla-inbound/rev/347b91bccf4d

Will need to uplift this to aurora 37, but it's already on beta 36. ni?me so I don't forget.

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/347b91bccf4d

Comment 42

[Ralph Giles (:rillian)]

Comment on attachment 8558256 [details] [diff] [review]
Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)

Request uplift of this follow-up patch. This fixed test failures on beta; we should have it in 37 as well in case further changes trigger the same problem.

Approval Request Comment
[Feature/regressing bug #]: MSE
[User impact if declined]: Possible asserts (crashes) on debug builds.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, beta.
[Risks and why]: Risk is low. At worst we'd crash somewhere less useful for debugging.
[String/UUID change made/needed]: None

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8558256 [details] [diff] [review]
Patch 5: Make dormant ResetPlayback assertion more lenient (m-c patch)

Aurora+ for the follow-up.

Comment 44

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/f95708ec343e