Making MP4Reader dormant clears the YouTube end-of-video speed dial

RESOLVED FIXED in Firefox 38

Status

()

Core
Audio/Video
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: cpearce, Assigned: sotaro)

Tracking

(Blocks: 1 bug)

unspecified
mozilla39
x86_64
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox37+ wontfix, firefox38+ fixed, firefox39 fixed)

Details

Attachments

(5 attachments, 28 obsolete attachments)

1.68 KB, text/plain
Details
3.03 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
4.06 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
50.62 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
4.67 KB, patch
Bobby Holley (parental leave - send mail for anything urgent)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

3 years ago
If a YouTube video plays to the end, it will display a "speed dial" of recommended other videos to play.

Once this happens, if we move the YouTube video to a background tab, and wait 60 seconds, we'll make that video "dormant". If you re-focus the tab, you'll get a black screen, you won't get the "speed dial". You should get the "speed dial".

You can change the pref "media.dormant-when-idle.timeout" to 0 to change the wait from 60 to 0 seconds.
(Reporter)

Comment 1

3 years ago
With a zero dormant timeout, I've also seen on Vimeo ( http://vimeo.com/channels/staffpicks/118489997 ) that if you open a video, click, quickly twice (so it starts loading, but pauses), and switch tabs, the rendered frame is all black, not the first frame as it should be.
Blocks: 1123535
(Reporter)

Comment 2

3 years ago
We should probably just disable heuristic dormant on beta 36, so we don't break non-MSE video.
(Reporter)

Comment 3

3 years ago
There are two problems here:

Firstly, when we go dormant, we record the current time (in this case the end time) and when we come out of dormant we seek to the previous current time. When this happens we dispatch "seeking" and "seeked" events, which YouTube's causes the video frame to update. We should not do this; dormant state should not be observable to script as much as is possible.

Secondly, the MediaSourceReader::Seek() checks whether the seek target is in its audio and video sub-readers, and if not it waits. It does this by calling AttemptSeek() -> TrackBuffersContainTime(). However the checks in TrackBuffersContainTime() use TimeRanges::Find(), which tests every buffered range of each track buffer against the seek target by checking if the end time of the range is *less* than the seek target, but in the case of the audio stream on this media it's *exactly* the end time, so that check fails.

To make matters worse, the video stream ends *before* the end of the audio stream, so the end time of the media is the end time of the audio stream, which is not contained in any of the video stream buffers, so the test in TrackBuffersContainTime() fails, and so the seek bails out waiting for more data.

Adding tolerance to the range end time check in TimeRanges::Find() seems cause breakages in other places.

The seek also does not appear to report that it's waiting for data, but I'm not really sure how that works.
(Reporter)

Comment 4

3 years ago
Created attachment 8561710 [details] [diff] [review]
Patch 1: Add extra dormant logging.
(Reporter)

Comment 5

3 years ago
Created attachment 8561712 [details] [diff] [review]
WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

This patch makes YouTube not clear its speed dial. But the seek back to previous playback position when coming out of dormant still fails.
(Reporter)

Comment 6

3 years ago
Created attachment 8561713 [details] [diff] [review]
WIP Patch 3: Make range end time check more tolerant

This makes the end time check of the MSReader more tolerant, so the seek to end succeeds. We end up in a hot loop if we seek to near the end of stream and play a bit of data though.
(Reporter)

Comment 7

3 years ago
jya: STR in comment 0, the problem happens if you don't apply WIP Patch 3; you won't be able to seek after following the STR in comment 0.
Flags: needinfo?(jyavenard)
(Assignee)

Comment 8

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #5)
> Created attachment 8561712 [details] [diff] [review]
> WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant
> mode
> 
> This patch makes YouTube not clear its speed dial. But the seek back to
> previous playback position when coming out of dormant still fails.

I am going to try different way to suppress the event.

Updated

3 years ago
Priority: -- → P1
(Reporter)

Comment 9

3 years ago
https://www.youtube.com/watch?v=2fFYObYJG1k was the video I was testing on where the video stream was about 100ms shorter than the audio stream.
cpearce: I'm unable to reproduce the problem with nightly using a few videos I've played with. 
What I do is:
start a video, seek toward the end. let it finish playing. Once the speed dial shows up, I switch tab (media.dormant-when-idle.timeout is set to 0)
then go back to the previous tab. For a very brief moment you see a waiting thing and then the video comes back where it left off.
Speed dial is there and I can seek.

Now, you could very well have hit bug 1125469, where we would discard all video frames after a seek, that would have caused entering WAITING_FOR_DATA status and stall.

Only patch 1 and 2 are applied. I don't think we need patch 3.
Flags: needinfo?(jyavenard) → needinfo?(cpearce)
(Reporter)

Comment 11

3 years ago
OK, my local build is based on top of this changeset:

changeset:   228058:3436787a82d0
tag:         qparent
parent:      228053:50ea2cac6d64
parent:      228057:bc48a81f3f7b
user:        Phil Ringnalda <philringnalda@gmail.com>
date:        Sun Feb 08 17:40:44 2015 -0800
summary:     Merge m-i to m-c, a=merge

I can repro on https://www.youtube.com/watch?v=2fFYObYJG1k with the 144p stream.
If I have patches 1 an 2 applied, but not patch 3.

STR:
1. Set media.decoder.heuristic.dormant.timeout = 0 (sorry, pref name wrong in comment 0)
2. Load https://www.youtube.com/watch?v=2fFYObYJG1k , seek to near the end, play to end.
3. Make YouTube tag in background.
4. Focus tab YT tab.
5. Seek to somewhere near end.

Expected result:
* Video should resume playback at new position.

Observed result:
* Video frame at new position shows, and video UI shows video as playing, but video is not playing.

I will try in latest nightly, maybe something else that landed already fixed this...
Flags: needinfo?(cpearce)
(Reporter)

Comment 12

3 years ago
I don't see the seek problem in today's Nightly, though the last speed dial is still lost.

So we can hold off looking at the EOS/seek problem until we know it's a problem again.
(Reporter)

Comment 13

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> (In reply to Chris Pearce (:cpearce) from comment #5)
> > Created attachment 8561712 [details] [diff] [review]
> > WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant
> > mode
> > 
> > This patch makes YouTube not clear its speed dial. But the seek back to
> > previous playback position when coming out of dormant still fails.
> 
> I am going to try different way to suppress the event.

Sotaro: Do you want to take this bug? You're welcome to. :)
Flags: needinfo?(sotaro.ikeda.g)
(Reporter)

Comment 14

3 years ago
Comment on attachment 8561713 [details] [diff] [review]
WIP Patch 3: Make range end time check more tolerant

Seems this is unnecessary now. Probably fixed by c955793299aa.
Attachment #8561713 - Attachment is obsolete: true
(Assignee)

Comment 15

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] from comment #8)
> > (In reply to Chris Pearce (:cpearce) from comment #5)
> > > Created attachment 8561712 [details] [diff] [review]
> > > WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant
> > > mode
> > > 
> > > This patch makes YouTube not clear its speed dial. But the seek back to
> > > previous playback position when coming out of dormant still fails.
> > 
> > I am going to try different way to suppress the event.
> 
> Sotaro: Do you want to take this bug? You're welcome to. :)

I want to take this bug. By the way, I stay in Taipei office this week.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Updated

3 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

3 years ago
Blocks: 1050031
(Assignee)

Comment 16

3 years ago
Created attachment 8563929 [details] [diff] [review]
WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
Attachment #8561712 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
attachment 8563929 [details] [diff] [review] fixed the "YouTube end-of-video speed dial" problem. But during checking, I saw a crash. It seems not related to the change.

I seeks the problem by the following.

[1] Set pref "media.dormant-when-idle.timeout" to 0.
[2] Open youtube site and show "YouTube end-of-video speed dial".
[3] Change to Different tab and quickly move back to youtube tab.

Continue [3] until crash happen.
(Assignee)

Comment 18

3 years ago
Created attachment 8563989 [details]
crash log of Comment 17
(Assignee)

Comment 19

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #18)
> Created attachment 8563989 [details]
> crash log of Comment 17

Correction: stack dump.
(Assignee)

Comment 20

3 years ago
Created attachment 8564070 [details] [diff] [review]
WIP Patch 3: Add CancelSeek call  to FlushDecoding()
(Assignee)

Comment 21

3 years ago
Created attachment 8564469 [details] [diff] [review]
WIP Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Update comment.
Attachment #8563929 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8564470 [details] [diff] [review]
Patch 3: Add CancelSeek call to FlushDecoding()

Fix a mistake.
Attachment #8564070 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
The patches fixed the "speed dial" problem. But it does not fix the video playback problem. After dormant resume, MediaDecoder does not in PLAY_STATE_ENDED state.
(Assignee)

Comment 24

3 years ago
attachment 8564470 [details] [diff] [review] fixed the different problem than this bug. It seems better to split to a different bug.
(Assignee)

Updated

3 years ago
Blocks: 1133167
(Assignee)

Updated

3 years ago
No longer blocks: 1133167
Depends on: 1133167
(Assignee)

Updated

3 years ago
Attachment #8564470 - Attachment is obsolete: true
Tracking all MSE P1 bugs for Firefox 37.
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox37: --- → +
(Assignee)

Comment 26

3 years ago
Created attachment 8565551 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
Attachment #8564469 - Attachment is obsolete: true
Sotaro - is this likely to make the uplift?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Updated

3 years ago
Attachment #8561710 - Attachment description: WIP Patch 1: Add extra dormant logging. → Patch 1: Add extra dormant logging.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 28

3 years ago
Created attachment 8566030 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Some code clean up.
Attachment #8565551 - Attachment is obsolete: true
(Assignee)

Comment 29

3 years ago
attachment 8566030 [details] [diff] [review] has one problem. After applying the patch, "speed dial" did not disappear. But the following sequence did not work correctly. Somehow, youtube site did not call play, just call only seek. Then video playback did not re-start.

[1] show "speed dial"
[2] Set MediaDecoder to dormant by setting the tab to background
[3] Push "play" button.
(Assignee)

Comment 30

3 years ago
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #27)
> Sotaro - is this likely to make the uplift?

The patch still have one problem. Need to fix it.
(Assignee)

Comment 31

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> attachment 8566030 [details] [diff] [review] has one problem. After applying
> the patch, "speed dial" did not disappear. But the following sequence did
> not work correctly. Somehow, youtube site did not call play, just call only
> seek. Then video playback did not re-start.
> 
> [1] show "speed dial"
> [2] Set MediaDecoder to dormant by setting the tab to background
> [3] Push "play" button.

I understand the cause of the problem. In the STR, dormant recovery seek and re-start seek(seek to 0) happens consecutively. I suppress, the second seek's seek start callback. It change the how HTMLMediaElement::UpdateReadyStateForData().

Without dormant, the STR go through HAVE_METADATA state. It state is triggered  by  "seek start callback". With dormant, "seek start callback" is suppressed, then the STR did not go through HAVE_METADATA state.
(Assignee)

Comment 32

3 years ago
From the investigation, in video replay from "speed dial" use case, youtube seems to check "seeking" and "timeupdate" event that are triggered by seek. It is actually called from HTMLMediaElement::SeekStarted(). Especially "seeking" event seems important. Disabling "seeking" causes the problem. It caused the playback start problem like comment 29.

In this bug, Skip of Disabling HTMLMediaElement::SeekStarted() happened because of seek overlap between "dormant resume seek" and a seek for re-play.
(Assignee)

Comment 33

3 years ago
And HTMLMediaElement::SeekStarted() is also used to fix Bug 1048171. Therefore, it seems better not to skip to call HTMLMediaElement::SeekStarted().
(Assignee)

Comment 34

3 years ago
The problem of comment 32, comment 33 seems a bit different problem from this bug. It seems better to handle it as a different bug.
(Assignee)

Updated

3 years ago
Blocks: 1134523
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1134523
(Assignee)

Comment 36

3 years ago
Created attachment 8566393 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Add SeekStarted() handling. By it, comment 29 problem is fixed in some cases, but by the timing, there were cases that SeekStarted() is not fixed yet depend on timing. More correct fix seems necessary.
(Assignee)

Updated

3 years ago
Attachment #8566030 - Attachment is obsolete: true
There's an issue with our seek implementation: it doesn't Fire a seeking event in some circumstances (linked to bug 1132851). Could be related
(Assignee)

Comment 38

3 years ago
(In reply to Jean-Yves Avenard [:jya] from comment #37)
> There's an issue with our seek implementation: it doesn't Fire a seeking
> event in some circumstances (linked to bug 1132851). Could be related

Thanks for the info :-)
(Assignee)

Comment 39

3 years ago
Comment on attachment 8566393 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2580,5 @@
> +      ReentrantMonitorAutoExit exitMon(mDecoder->GetReentrantMonitor());
> +      nsCOMPtr<nsIRunnable> startEvent =
> +        NS_NewRunnableMethodWithArg<bool>(mDecoder,
> +                                          &MediaDecoder::SeekingStarted,
> +                                          restored);

This part was incorrect. I am going to update it.
(Assignee)

Comment 40

3 years ago
Created attachment 8566503 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
Attachment #8566393 - Attachment is obsolete: true
(Assignee)

Comment 41

3 years ago
Comment on attachment 8566503 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

::: dom/media/MediaDecoder.h
@@ +242,2 @@
>      : mTime(aTimeUsecs)
> +    , mRestoredFromDormant(aRestoredFromDormant)

Unintentional change. mType is removed by mistake.
(Assignee)

Comment 42

3 years ago
Created attachment 8566624 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
Attachment #8566503 - Attachment is obsolete: true
(Assignee)

Comment 43

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d68186ea671
(Assignee)

Comment 44

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #43)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d68186ea671

some test failures exist.
(Assignee)

Comment 45

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #44)
> (In reply to Sotaro Ikeda [:sotaro] from comment #43)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d68186ea671
> 
> some test failures exist.

All tests failures seems to related to MozCaptureStream.
(Assignee)

Comment 46

3 years ago
The following test failure was caused by incorrect HTMLMediaElement's destruction. MediaDecoder have a dependency to HTMLMediaElement::mAudioTrackList and HTMLMediaElement::mVideoTrackList. They are unlinked from HTMLMediaElement before HTMLMediaElement::~HTMLMediaElement() call. When there is a MediaDecoder exist during HTMLMediaElement::~HTMLMediaElement(), it create dangling pointer. And it could cause the following crash.

> PROCESS-CRASH | dom/media/test/test_video_to_canvas.html | application crashed [@ mozilla::dom::MediaTrackList::~MediaTrackList()]
(Assignee)

Comment 47

3 years ago
Comment 46 is different problem from this bug. I am going to create a different bug.
(Assignee)

Updated

3 years ago
Blocks: 1135304
(Assignee)

Updated

3 years ago
No longer blocks: 1135304
(Assignee)

Updated

3 years ago
Depends on: 1135304
(Assignee)

Comment 48

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f18298b6434d
(Assignee)

Comment 49

3 years ago
Created attachment 8567931 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

un-bitrot.
Attachment #8566624 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8561710 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8567931 - Flags: review?(cpearce)
(Reporter)

Comment 50

3 years ago
Comment on attachment 8561710 [details] [diff] [review]
Patch 1: Add extra dormant logging.

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

::: dom/media/MediaDecoder.cpp
@@ +150,5 @@
>    }
>  
> +  DECODER_LOG("UpdateDormantState aTimeout=%d aActivity=%d mIsDormant=%d "
> +              "ownerActive=%d ownerHidden=%d mIsHeuristicDormant=%d mPlayState=%s",
> +              aDormantTimeout, aActivity, mIsDormant, mOwner->IsActive(), mOwner->IsHidden(), mIsHeuristicDormant, PlayStateStr());

Nit: You should probably break line 153, it's quite long.
Attachment #8561710 - Flags: review?(cpearce) → review+
(Reporter)

Comment 51

3 years ago
Comment on attachment 8567931 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

I don't want to r+ this because it adds MediaDecoder::IsEndedState() when we already have a similarly named MediaDecoder::IsEnded(). I would like to avoid adding two methods that do almost the same thing; can they just be the same method?

Also, I think Bobby Holley should review this, as he's working on Bug 1135170 which refactors seeking.

Can we get a mochitest to cover seeking events being suppressed when coming out of dormant mode? We don't want to regress this.

::: dom/html/HTMLMediaElement.cpp
@@ +3422,1 @@
>    if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&

It is not clear from the comment why we need to do not do this on B2G.

Also, would it be better to instead only call ChangeReadyState(HAVE_METADATA) if the decoder is not dormant?

i.e.:

  if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&
      mDecoder && !mDecoder->IsDormant() &&
      GetImageContainer() && !GetImageContainer()->HasCurrentImage()) {

?

::: dom/media/MediaDecoder.cpp
@@ +186,5 @@
>    if (mIsDormant) {
>      // enter dormant state
>      mDecoderStateMachine->SetDormant(true);
> +    if (IsEndedState()) {
> +      mEmulateEndedState = true;

Why do you need to add mEmulateEndedState?

@@ +187,5 @@
>      // enter dormant state
>      mDecoderStateMachine->SetDormant(true);
> +    if (IsEndedState()) {
> +      mEmulateEndedState = true;
> +      mNextState = PLAY_STATE_PAUSED;

Why do you not want mNextState = PLAY_STATE_ENDED?

@@ +992,5 @@
>  
>  bool MediaDecoder::IsEnded() const
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  return IsEndedState() || mPlayState == PLAY_STATE_SHUTDOWN;

Why do you need the mPlayState == PLAY_STATE_SHUTDOWN check on IsEnded(), but not on IsEndedState()?

::: dom/media/MediaDecoder.h
@@ +239,3 @@
>    {
>    }
> +  SeekTarget(int64_t aTimeUsecs, Type aType, bool aRestoredFromDormant = false)

Please declare another enum to use here that describes whether we're restoring from dormant.

Then you don't need /* aRestoredFromDormant */ comments every time you construct a SeekTarget.

What aRestoredFromDormant==true means is that we should suppress event firing.

So how about we use this:

enum EventObservability {
  EventsObservable,
  EventsSuppressed
};

Please use that, or something like it, everywhere you're passing a bool aRestoredFromDormant.

@@ +1021,5 @@
>  
>    // Cancel a timer for heuristic dormant.
>    void CancelDormantTimer();
>  
> +  bool IsEndedState() const;

We already have an IsEnded() function on this class, it is confusing to have another similarly named IsEndedState() function.

How is this function different from IsEnded()? Why do we need two similar functions? Why can't we change IsEnded() to have the behaviour we need?

@@ +1191,5 @@
>    bool mIsDormant;
>  
> +  // True if MediaDecoder is emulating PLAY_STATE_ENDED state.
> +  // When MediaCodec is in dormant during PLAY_STATE_ENDED state, PlayState
> +  // becomes different from PLAY_STATE_ENDED. But the MediaDecoder need to act

*Why* is ended-while-dormant different from a non-dormant ended state?

Comments should explain what code cannot: *why* code does what it does. *What* the code does can be figured out by reading the code.

Can you infer that you need to emulate ended state while dormant instead of adding state to track it?

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1516,3 @@
>        }
> +    } else {
> +      mQueuedSeekTarget = SeekTarget(mCurrentFrameTime,

Can (mState == DECODER_STATE_SEEKING && mQueuedSeekTarget.IsValid()) be true? If so, we'll be overwriting mQueuedSeekTarget here, even though something else wrote a value into it an expected it to work. Is this safe?

@@ +2580,5 @@
> +      nsCOMPtr<nsIRunnable> startEvent =
> +        NS_NewRunnableMethodWithArg<bool>(mDecoder,
> +                                          &MediaDecoder::SeekingStarted,
> +                                          mCurrentSeekTarget.mRestoredFromDormant);
> +      NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);

I don't like having a sync dispatch here, but we do it elsewhere for the SeekingStarted call so I guess we can run with this for now... Until Bug 1135170 lands and the sync dispatch will be removed. :)
Attachment #8567931 - Flags: review?(cpearce)
Attachment #8567931 - Flags: review?(bobbyholley)
Attachment #8567931 - Flags: review-
(Assignee)

Comment 52

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #51)
> Comment on attachment 8567931 [details] [diff] [review]
> Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
> 
> Review of attachment 8567931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't want to r+ this because it adds MediaDecoder::IsEndedState() when we
> already have a similarly named MediaDecoder::IsEnded(). I would like to
> avoid adding two methods that do almost the same thing; can they just be the
> same method?
> 
> Also, I think Bobby Holley should review this, as he's working on Bug
> 1135170 which refactors seeking.
> 
> Can we get a mochitest to cover seeking events being suppressed when coming
> out of dormant mode? We don't want to regress this.
> 
> ::: dom/html/HTMLMediaElement.cpp
> @@ +3422,1 @@
> >    if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&
> 
> It is not clear from the comment why we need to do not do this on B2G.
> 
> Also, would it be better to instead only call
> ChangeReadyState(HAVE_METADATA) if the decoder is not dormant?
> 
> i.e.:
> 
>   if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&
>       mDecoder && !mDecoder->IsDormant() &&
>       GetImageContainer() && !GetImageContainer()->HasCurrentImage()) {
> 
> ?
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +186,5 @@
> >    if (mIsDormant) {
> >      // enter dormant state
> >      mDecoderStateMachine->SetDormant(true);
> > +    if (IsEndedState()) {
> > +      mEmulateEndedState = true;
> 
> Why do you need to add mEmulateEndedState?

Thanks for the review.

Cpearce, how do you keep playback ended state even whwn MedaDecoder's state becomes PLAY_STATE_LOADING during dormant state? Can you explain your idea?

In current dormant implementation, MedaDecoder's internal state transition show actual MediaDecoder's internal state. But it tells lie to HTMLMediaElement as there is not dormant. To connect the difference between MediaElement's internal state and state that showed to HTMLMediaElement, mEmulateEndedState becomes necessary. 



> 
> @@ +187,5 @@
> >      // enter dormant state
> >      mDecoderStateMachine->SetDormant(true);
> > +    if (IsEndedState()) {
> > +      mEmulateEndedState = true;
> > +      mNextState = PLAY_STATE_PAUSED;
> 
> Why do you not want mNextState = PLAY_STATE_ENDED?
> 

I want to keep internal state transition as consistent as possible state like the following.

https://github.com/sotaroikeda/firefox-diagrams/blob/master/media/dom_media_MediaDecoder_state_FirefoxOS_2_2.pdf?raw=true
Flags: needinfo?(cpearce)
Comment on attachment 8567931 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +2580,5 @@
> +      nsCOMPtr<nsIRunnable> startEvent =
> +        NS_NewRunnableMethodWithArg<bool>(mDecoder,
> +                                          &MediaDecoder::SeekingStarted,
> +                                          mCurrentSeekTarget.mRestoredFromDormant);
> +      NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);

In bug 1135170 all this junk goes away, and we just always use the machinery in DecodeSeek. So as long as this is just try to do the same in both cases there should be no conflict there.

The rest of these changes look mostly orthogonal to what I'm doing.
Attachment #8567931 - Flags: review?(bobbyholley) → feedback+
(Reporter)

Comment 54

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #52)
> (In reply to Chris Pearce (:cpearce) from comment #51)
> > ::: dom/media/MediaDecoder.cpp
> > @@ +186,5 @@
> > >    if (mIsDormant) {
> > >      // enter dormant state
> > >      mDecoderStateMachine->SetDormant(true);
> > > +    if (IsEndedState()) {
> > > +      mEmulateEndedState = true;
> > 
> > Why do you need to add mEmulateEndedState?
> 
> Thanks for the review.
> 
> Cpearce, how do you keep playback ended state even whwn MedaDecoder's state
> becomes PLAY_STATE_LOADING during dormant state? Can you explain your idea?
> 
> In current dormant implementation, MedaDecoder's internal state transition
> show actual MediaDecoder's internal state. But it tells lie to
> HTMLMediaElement as there is not dormant. To connect the difference between
> MediaElement's internal state and state that showed to HTMLMediaElement,
> mEmulateEndedState becomes necessary. 

So what you're saying is that mEmulateEndedState is true when we go dormant while in ended state, so that we still report consistently to the media element that we're ended after we've gone dormant, as we'll change the play state to LOADING. OK.

I think a better name would make it easier to understand. Like mWasEndedWhenEnteredDormant?

I would still like IsEnded() and IsEndedState() to be merged into one function if possible.
Flags: needinfo?(cpearce)
(Assignee)

Comment 55

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #54)
> state to LOADING. OK.
> 
> I think a better name would make it easier to understand. Like
> mWasEndedWhenEnteredDormant?

I will update in a next patch.

> I would still like IsEnded() and IsEndedState() to be merged into one
> function if possible.

IsEnded() include PLAY_STATE_SHUTDOWN state. But IsEndedState() does not include PLAY_STATE_SHUTDOWN. IsEndedState() is used as PLAY_STATE_ENDED( + mWasEndedWhenEnteredDormant). Then, something different function becomes necessary.
(Assignee)

Comment 56

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #54)
> 
> I would still like IsEnded() and IsEndedState() to be merged into one
> function if possible.

How about to re-name IsEndedState() as Is_PLAY_STATE_ENDED()?
(Assignee)

Comment 57

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #51)
> Comment on attachment 8567931 [details] [diff] [review]
> Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
> 
> Review of attachment 8567931 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't want to r+ this because it adds MediaDecoder::IsEndedState() when we
> already have a similarly named MediaDecoder::IsEnded(). I would like to
> avoid adding two methods that do almost the same thing; can they just be the
> same method?

As explained in Comment 55, it could not be merged. I will re-name IsEndedState() as Is_PLAY_STATE_ENDED() in a next patch.


> >    if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&
> 
> It is not clear from the comment why we need to do not do this on B2G.
> 
> Also, would it be better to instead only call
> ChangeReadyState(HAVE_METADATA) if the decoder is not dormant?

It is not easy to provide mDecoder->IsDormant() and work as expected. This part was necessary during implementation. I will remove it in a next patch.

> i.e.:
> 
>   if (IsVideo() && mHasVideo && !IsPlaybackEnded() &&
>       mDecoder && !mDecoder->IsDormant() &&
>       GetImageContainer() && !GetImageContainer()->HasCurrentImage()) {
> 
> ?
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +186,5 @@
> >    if (mIsDormant) {
> >      // enter dormant state
> >      mDecoderStateMachine->SetDormant(true);
> > +    if (IsEndedState()) {
> > +      mEmulateEndedState = true;
> 
> Why do you need to add mEmulateEndedState?

It will re-name to mWasEndedWhenEnteredDormant in  a next patch.

> 
> @@ +187,5 @@
> >      // enter dormant state
> >      mDecoderStateMachine->SetDormant(true);
> > +    if (IsEndedState()) {
> > +      mEmulateEndedState = true;
> > +      mNextState = PLAY_STATE_PAUSED;
> 
> Why do you not want mNextState = PLAY_STATE_ENDED?

It was changed from state machine state transition point of view. But it could be removed. I will remove it in a next patch.

> 
> @@ +992,5 @@
> >  
> >  bool MediaDecoder::IsEnded() const
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> > +  return IsEndedState() || mPlayState == PLAY_STATE_SHUTDOWN;
> 
> Why do you need the mPlayState == PLAY_STATE_SHUTDOWN check on IsEnded(),
> but not on IsEndedState()?

As explained in Comment 55, it could not be merged. I will re-name IsEndedState() as Is_PLAY_STATE_ENDED() in a next patch.

> 
> ::: dom/media/MediaDecoder.h
> @@ +239,3 @@
> >    {
> >    }
> > +  SeekTarget(int64_t aTimeUsecs, Type aType, bool aRestoredFromDormant = false)
> 
> Please declare another enum to use here that describes whether we're
> restoring from dormant.
> 
> Then you don't need /* aRestoredFromDormant */ comments every time you
> construct a SeekTarget.
> 
> What aRestoredFromDormant==true means is that we should suppress event
> firing.
> 
> So how about we use this:
> 
> enum EventObservability {
>   EventsObservable,
>   EventsSuppressed
> };
> 
> Please use that, or something like it, everywhere you're passing a bool
> aRestoredFromDormant.

I will update in a next patch.


> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1516,3 @@
> >        }
> > +    } else {
> > +      mQueuedSeekTarget = SeekTarget(mCurrentFrameTime,
> 
> Can (mState == DECODER_STATE_SEEKING && mQueuedSeekTarget.IsValid()) be
> true? If so, we'll be overwriting mQueuedSeekTarget here, even though
> something else wrote a value into it an expected it to work. Is this safe?

There might be a possibility. I will update in a next patch.

> 
> @@ +2580,5 @@
> > +      nsCOMPtr<nsIRunnable> startEvent =
> > +        NS_NewRunnableMethodWithArg<bool>(mDecoder,
> > +                                          &MediaDecoder::SeekingStarted,
> > +                                          mCurrentSeekTarget.mRestoredFromDormant);
> > +      NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);
> 
> I don't like having a sync dispatch here, but we do it elsewhere for the
> SeekingStarted call so I guess we can run with this for now... Until Bug
> 1135170 lands and the sync dispatch will be removed. :)

In current seek implementation, it is necessary. It seems be removed by bug 1135170.
(Assignee)

Comment 58

3 years ago
> So how about we use this:
> 
> enum EventObservability {
>   EventsObservable,
>   EventsSuppressed
> };
> 
> Please use that, or something like it, everywhere you're passing a bool
> aRestoredFromDormant.

If we replace "bool aRestoredFromDormant." by the enum, the meaning of RestoredFromDormant will disappear. Is it ok for you?
(Assignee)

Comment 59

3 years ago
Created attachment 8568639 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Apply the comments.
Attachment #8567931 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
No longer blocks: 1134523
(Assignee)

Updated

3 years ago
Attachment #8568639 - Flags: review?(cpearce)
(Assignee)

Comment 60

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #51)
> 
> Can we get a mochitest to cover seeking events being suppressed when coming
> out of dormant mode? We don't want to regress this.

I forgot to create a test for the above.
(Assignee)

Comment 61

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6692cf123af4
(Reporter)

Comment 62

3 years ago
Comment on attachment 8568639 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

r+ with name changes.

::: dom/media/AbstractMediaDecoder.h
@@ +34,5 @@
>  static inline bool IsCurrentThread(nsIThread* aThread) {
>    return NS_GetCurrentThread() == aThread;
>  }
>  
> +enum class MediaDecoderEventType : int8_t {

This is not a good name as it's not specific of the type of property it is describing. "Type" is a very general word. Please use another name. 

A better name is MediaDecoderEventVisibility, as this enum describes whether the state transition is visible outside of the MediaDecoder.

You could even use MediaEventVisibility to make the name shorter.

::: dom/media/MediaDecoder.cpp
@@ +987,5 @@
>    return !mIsDormant && (mPlayState == PLAY_STATE_SEEKING ||
>      (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid()));
>  }
>  
>  bool MediaDecoder::IsEnded() const

You're using MediaDecoder::IsEnded() to mean that playback has ended or the decoder has shutdown.

You're using MediaDecoder::Is_PLAY_STATE_ENDED() to mean that playback has logically ended

So please rename MediaDecoder::IsEnded() to MediaDecoder::IsEndedOrShutdown(), and rename MediaDecoder::Is_PLAY_STATE_ENDED() to MediaDecoder::IsEnded(). Then the function names match what they mean.

Be careful to rename the callers of MediaDecoder::IsEnded() in HTMLMediaElement to call MediaDecoder::IsEndedOrShutdown(), they shouldn't call what is named MediaDecoder::Is_PLAY_STATE_ENDED() in this patch.

::: dom/media/MediaDecoder.h
@@ +239,5 @@
>    {
>    }
> +  SeekTarget(int64_t aTimeUsecs,
> +             Type aType,
> +             MediaDecoderEventType aEventType =

aEventVisibility

And please change other things you named *EventType to *EventVisibility.
Attachment #8568639 - Flags: review?(cpearce) → review+
(Assignee)

Comment 63

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #62)
> Comment on attachment 8568639 [details] [diff] [review]
> Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
> 
> Review of attachment 8568639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with name changes.
> 
> ::: dom/media/AbstractMediaDecoder.h
> @@ +34,5 @@
> >  static inline bool IsCurrentThread(nsIThread* aThread) {
> >    return NS_GetCurrentThread() == aThread;
> >  }
> >  
> > +enum class MediaDecoderEventType : int8_t {
> 
> This is not a good name as it's not specific of the type of property it is
> describing. "Type" is a very general word. Please use another name. 
> 
> A better name is MediaDecoderEventVisibility, as this enum describes whether
> the state transition is visible outside of the MediaDecoder.

I will update it in a next patch.

> 
> You could even use MediaEventVisibility to make the name shorter.
> 
> ::: dom/media/MediaDecoder.cpp
> @@ +987,5 @@
> >    return !mIsDormant && (mPlayState == PLAY_STATE_SEEKING ||
> >      (mPlayState == PLAY_STATE_LOADING && mRequestedSeekTarget.IsValid()));
> >  }
> >  
> >  bool MediaDecoder::IsEnded() const
> 
> You're using MediaDecoder::IsEnded() to mean that playback has ended or the
> decoder has shutdown.
> 
> You're using MediaDecoder::Is_PLAY_STATE_ENDED() to mean that playback has
> logically ended
> 
> So please rename MediaDecoder::IsEnded() to
> MediaDecoder::IsEndedOrShutdown(), and rename
> MediaDecoder::Is_PLAY_STATE_ENDED() to MediaDecoder::IsEnded(). Then the
> function names match what they mean.

I will update it in a next patch.

> 
> Be careful to rename the callers of MediaDecoder::IsEnded() in
> HTMLMediaElement to call MediaDecoder::IsEndedOrShutdown(), they shouldn't
> call what is named MediaDecoder::Is_PLAY_STATE_ENDED() in this patch.
> 
> ::: dom/media/MediaDecoder.h
> @@ +239,5 @@
> >    {
> >    }
> > +  SeekTarget(int64_t aTimeUsecs,
> > +             Type aType,
> > +             MediaDecoderEventType aEventType =
> 
> aEventVisibility
> 
> And please change other things you named *EventType to *EventVisibility.

I will update it in a next patch.
(Assignee)

Comment 64

3 years ago
Created attachment 8568952 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Apply the comments. Carry "r=cpearce".
Attachment #8568639 - Attachment is obsolete: true
Attachment #8568952 - Flags: review+
(Assignee)

Comment 65

3 years ago
Created attachment 8569883 [details] [diff] [review]
patch 3: test for video playback with dormant

For the time being, the test is enabled only on b2g gonk. mp4 file playback's dormant is supported only on b2g gonk now.
(Assignee)

Comment 66

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #65)
> Created attachment 8569883 [details] [diff] [review]
> patch 3: test for video playback with dormant
> 
> For the time being, the test is enabled only on b2g gonk. mp4 file
> playback's dormant is supported only on b2g gonk now.

It use seeked event to trigger entering dormant, because "loadedmetadata" does not ensure that MediaDecoder is in PLAY_STATE_PAUSED state.
(Assignee)

Comment 67

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa5a572b42a7
(Assignee)

Comment 68

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #67)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa5a572b42a7

tryserver result have 2 problems. The following does not work to limit the test only to gonk.

> run-if = toolkit == 'gonk'

And test_dormant_playback.html does not pass on platform that support mp4 H.264 video playback. The test does not have a direct dependency to dormant. It should run normally on the platform.
(Assignee)

Comment 69

3 years ago
I found one problem of attachment 8568952 [details] [diff] [review]. If MediaDecoderStateMachine::SetDormant(false) is called before completing entering dormant state, the MediaDecoderStateMachine does not work correctly.
(Assignee)

Comment 70

3 years ago
And scheduling of MP4Reader::Update() also seems to have a problem.
(Assignee)

Comment 71

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #70)
> And scheduling of MP4Reader::Update() also seems to have a problem.

It was caused  by mDecodingFrozenAtStateDecoding. Need to update it.
(Assignee)

Comment 72

3 years ago
Created attachment 8570330 [details] [diff] [review]
patch 4: Change to MediaDecoderStateMachine
(Assignee)

Comment 73

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c139dc400eb6
(Assignee)

Comment 74

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #73)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c139dc400eb6

attachment 8570330 [details] [diff] [review] worked for windows 7 and windows8, but it did not work for b2g gonk.
(Assignee)

Comment 75

3 years ago
During FlushDecoding() in MediaDecoderStateMachine::RunStateMachine(), ReentrantMonitor was exited. Therefore, ReentrantMonitor lock did not work correctly in MediaDecoderStateMachine::SetDormant(), if during FlushDecoding().
(Assignee)

Comment 76

3 years ago
Created attachment 8570794 [details] [diff] [review]
patch 4: Asynchronize SetDormant()
Attachment #8570330 - Attachment is obsolete: true
(Assignee)

Comment 77

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6dc6d49628c2
(Assignee)

Comment 78

3 years ago
Pushed only test just for comparison.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17b6b4555f3
(Assignee)

Comment 79

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #78)
> Pushed only test just for comparison.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b17b6b4555f3

This result was worse than applying the patches in this bug.
(Assignee)

Comment 80

3 years ago
Created attachment 8570985 [details] [diff] [review]
patch 3: test for video playback with dormant

Update test. In previous test, video was not loaded on b2g.
Attachment #8569883 - Attachment is obsolete: true
(Assignee)

Comment 81

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6659f4d8206a
(Assignee)

Comment 82

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #81)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=6659f4d8206a

b2g's test failure was addressed.

Windows XP and android caused test failures. But they are not enabling dormant. Therefore, they are not related to this bug. It seems better to enable test only on b2g gonk and Windows 7/8.
(Assignee)

Comment 83

3 years ago
Created attachment 8571090 [details] [diff] [review]
patch 3: test for video playback with dormant

Limit test only to b2g gonk and windows 7/8.
Attachment #8570985 - Attachment is obsolete: true
(Assignee)

Comment 84

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=29424995d769
(Assignee)

Updated

3 years ago
Attachment #8570794 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8571090 - Flags: review?(cpearce)
(Reporter)

Comment 85

3 years ago
Comment on attachment 8571090 [details] [diff] [review]
patch 3: test for video playback with dormant

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

Why does android fail? Dormant mode should not be observable to content. So this test should pass on Android, even if dormant mode is not being used there.

Does this test pass on Android without your other patches? If so, then your patches regress something in Android, and you need to fix that before landing.

::: dom/media/test/mochitest.ini
@@ +363,5 @@
>  [test_defaultMuted.html]
>  [test_delay_load.html]
>  skip-if = buildapp == 'b2g' && toolkit != 'gonk' # bug 1082984
> +[test_dormant_playback.html]
> +skip-if = (os == 'win' && os_version == '5.1') || (os != 'win' && (buildapp != 'b2g' && toolkit != 'gonk'))

That second clause could be:
(os != 'win' && buildapp != 'b2g' && toolkit != 'gonk')

You've got more () than you need.
(Reporter)

Comment 86

3 years ago
Comment on attachment 8570794 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

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

I'm not finished reviewing this, but because this patch uses the new promise infrastructure bholley should also look over this.

::: dom/media/MediaDecoder.cpp
@@ +177,5 @@
>        mIsDormant = true;
>      }
>    }
>  
> +  if(prevDormant && !mIsDormant && mEnterDormantRequest.Exists()) {

if (prevDormant

space between "if" and "("

@@ +217,5 @@
> +  ReentrantMonitorAutoEnter mon(GetReentrantMonitor());
> +
> +  mEnterDormantRequest.Complete();
> +
> +  if(mReqestUpdateDormantState) {

if (mReqestUpdateDormantState) {


spaace between "if" and "("
Attachment #8570794 - Flags: review?(bobbyholley)
(Assignee)

Comment 87

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #85)
> Comment on attachment 8571090 [details] [diff] [review]
> patch 3: test for video playback with dormant
> 
> Review of attachment 8571090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why does android fail? Dormant mode should not be observable to content. So
> this test should pass on Android, even if dormant mode is not being used
> there.
> 
> Does this test pass on Android without your other patches? If so, then your
> patches regress something in Android, and you need to fix that before
> landing.


The test does not pass on android and windows XP without the patch. Therefore it is not a regression. See Comment 79.

> 
> ::: dom/media/test/mochitest.ini
> @@ +363,5 @@
> >  [test_defaultMuted.html]
> >  [test_delay_load.html]
> >  skip-if = buildapp == 'b2g' && toolkit != 'gonk' # bug 1082984
> > +[test_dormant_playback.html]
> > +skip-if = (os == 'win' && os_version == '5.1') || (os != 'win' && (buildapp != 'b2g' && toolkit != 'gonk'))
> 
> That second clause could be:
> (os != 'win' && buildapp != 'b2g' && toolkit != 'gonk')
> 
> You've got more () than you need.
(Assignee)

Comment 88

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #87)
> (In reply to Chris Pearce (:cpearce) from comment #85)
> > Comment on attachment 8571090 [details] [diff] [review]
> > patch 3: test for video playback with dormant
> > 
> > Review of attachment 8571090 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Why does android fail? Dormant mode should not be observable to content. So
> > this test should pass on Android, even if dormant mode is not being used
> > there.
> > 
> > Does this test pass on Android without your other patches? If so, then your
> > patches regress something in Android, and you need to fix that before
> > landing.
> 
> 
> The test does not pass on android and windows XP without the patch.
> Therefore it is not a regression. See Comment 79.
> 

re-check it by using latest test.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57818f88635c
(Assignee)

Comment 89

3 years ago
> > 
> > 
> > The test does not pass on android and windows XP without the patch.
> > Therefore it is not a regression. See Comment 79.
> > 
> 
> re-check it by using latest test.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=57818f88635c

Re-confirmed the test failure. Dormant mode is not supported on android yet, therefore it is not related to dormant mode.
(Assignee)

Comment 90

3 years ago
Created attachment 8571357 [details] [diff] [review]
Patch 1: Add extra dormant logging

Apply the comment. Carry "r=cpearce".
Attachment #8561710 - Attachment is obsolete: true
Attachment #8571357 - Flags: review+
(Assignee)

Comment 91

3 years ago
Created attachment 8571362 [details] [diff] [review]
patch 3: test for video playback with dormant

Apply the comment.
Attachment #8571090 - Attachment is obsolete: true
Attachment #8571090 - Flags: review?(cpearce)
Attachment #8571362 - Flags: review?(cpearce)
(Assignee)

Comment 92

3 years ago
Created attachment 8571365 [details] [diff] [review]
patch 3: test for video playback with dormant
Attachment #8571362 - Attachment is obsolete: true
Attachment #8571362 - Flags: review?(cpearce)
Attachment #8571365 - Flags: review?(cpearce)
(Assignee)

Comment 93

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #85)
> Comment on attachment 8571090 [details] [diff] [review]
> patch 3: test for video playback with dormant
> 
> Review of attachment 8571090 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why does android fail? Dormant mode should not be observable to content. So
> this test should pass on Android, even if dormant mode is not being used
> there.

I did not investigated about it. As in comment 88, android test failure is not a regression. And dormant mode is not enabled on android. The dormant mode is enabled only on gonk and windows' wmf. Therefore, it seems another problem.

> 
> Does this test pass on Android without your other patches? If so, then your
> patches regress something in Android, and you need to fix that before
> landing.
> 

As in comment 88, android test failure is not a regression.
(Assignee)

Comment 94

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #93)
> > 
> > Why does android fail? Dormant mode should not be observable to content. So
> > this test should pass on Android, even if dormant mode is not being used
> > there.
> 
> I did not investigated about it. As in comment 88, android test failure is
> not a regression. And dormant mode is not enabled on android. The dormant
> mode is enabled only on gonk and windows' wmf. Therefore, it seems another
> problem.
> 

Therefore it seems better to address the android problem in another bug.
(Reporter)

Updated

3 years ago
Attachment #8571365 - Flags: review?(cpearce) → review+
Comment on attachment 8570794 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

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

::: dom/media/MediaDecoder.h
@@ +1219,5 @@
> +
> +  MediaPromiseConsumerHolder<MediaDecoder::DormantPromise> mEnterDormantRequest;
> +
> +  // True if calling UpdateDormantState() in DormantEntered() is requested.
> +  bool mReqestUpdateDormantState;

This is misspelled.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1494,2 @@
>  {
> +   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");

Given that this returns a promise, this method should run on the state machine thread, and you can just use ProxyMediaCall to invoke it from the main thread. That way you don't need to do the stuff with SetDormantInternal below.

@@ +1494,4 @@
>  {
> +   NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
> +   AssertCurrentThreadInMonitor();
> + 

Whitespace error.

::: dom/media/MediaDecoderStateMachine.h
@@ +1167,5 @@
>    // operation soon.
>    bool mSentFirstFrameLoadedEvent;
> +
> +  MediaPromiseHolder<MediaDecoder::DormantPromise> mEnterDormantPromise;
> +  MediaPromiseHolder<MediaDecoder::DormantPromise> mExitDormantPromise;

Why do we need two promises here? Is there ever a valid reason for them both to exist?

If we're mid-way through a previous dormant=true request and now we want to set dormant=false (or vice-versa), the correct thing to do is to just do mDormantRequest.DisconnectIfExists() and issue a new SetDormant call. SetDormant should then invoke mDormantPromise.RejectifExists() before invoking mDormantPromise.Ensure(), so that issuing a new dormant request cancels the previous one.

Feel free to ping me on IRC if you have any questions about this.
Attachment #8570794 - Flags: review?(bobbyholley) → review-
(Assignee)

Comment 96

3 years ago
Created attachment 8571963 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

un-bitrot. Carry "r=cpearce".
Attachment #8568952 - Attachment is obsolete: true
(Assignee)

Comment 97

3 years ago
In attachment 8570794 [details] [diff] [review], I used MediaPromise to do throttle of calling SetDormant() in MediaDecoder side. But if we want to throttle of calling SetDormant() in MediaDecoderStateMachine side, MediaPromise seems not a good choice.
(Assignee)

Comment 98

3 years ago
Created attachment 8572150 [details] [diff] [review]
patch 4: Asynchronize SetDormant()
Attachment #8570794 - Attachment is obsolete: true
Attachment #8570794 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Attachment #8572150 - Flags: review?(cpearce)
Attachment #8572150 - Flags: review?(bobbyholley)
(Assignee)

Comment 99

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #98)
> Created attachment 8572150 [details] [diff] [review]
> patch 4: Asynchronize SetDormant()

I updated a patch as not to use MediaPromise. MediaPromise seem not fit well to this case.
Comment on attachment 8572150 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

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

Why do we need to track the runnable and cancel it? Can't we just rely on the most recent runnable putting us in the state we want?

In general, I'm trying to move us toward an architecture where we do less arms-length checking and more "forget everything before, here's what we're doing now".
(Assignee)

Comment 101

3 years ago
(In reply to Bobby Holley (:bholley) from comment #100)
> Comment on attachment 8572150 [details] [diff] [review]
> patch 4: Asynchronize SetDormant()
> 
> Review of attachment 8572150 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Why do we need to track the runnable and cancel it? Can't we just rely on
> the most recent runnable putting us in the state we want?
> 
> In general, I'm trying to move us toward an architecture where we do less
> arms-length checking and more "forget everything before, here's what we're
> doing now".

bholly, can you show the sample code that is doing it?
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 102

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #101)
> > 
> > In general, I'm trying to move us toward an architecture where we do less
> > arms-length checking and more "forget everything before, here's what we're
> > doing now".
> 
> bholly, can you show the sample code that is doing it?

I wanted to know the place in gecko media that is doing what you said. Thanks.
(In reply to Sotaro Ikeda [:sotaro] from comment #102)
> I wanted to know the place in gecko media that is doing what you said.
> Thanks.

Do you have a minute to talk on IRC right now?
We discussed this on IRC.
Flags: needinfo?(bobbyholley)
(Assignee)

Comment 105

3 years ago
Created attachment 8572276 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

Simplify implementation compared to previous one, this patch might add extra cpu in some use cases. To simplify the implementation, we keep simple approach until it causes actual harm.
Attachment #8572150 - Attachment is obsolete: true
Attachment #8572150 - Flags: review?(cpearce)
Attachment #8572150 - Flags: review?(bobbyholley)
(Assignee)

Updated

3 years ago
Attachment #8572276 - Flags: review?(cpearce)
Attachment #8572276 - Flags: review?(bobbyholley)
Comment on attachment 8572276 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

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

Per IRC discussion, let's move the runnable dispatch into MD so that MDSM methods all run on the state machine thread. :-)
Attachment #8572276 - Flags: review?(cpearce)
Attachment #8572276 - Flags: review?(bobbyholley)
Attachment #8572276 - Flags: review-
(Assignee)

Comment 107

3 years ago
Created attachment 8572284 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

Removed SetDormantInternal().
Attachment #8572276 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8572284 - Flags: review?(cpearce)
Attachment #8572284 - Flags: review?(bobbyholley)
Comment on attachment 8572284 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

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

r=me with that.

::: dom/media/MediaDecoder.cpp
@@ +190,5 @@
> +        mDecoderStateMachine,
> +        &MediaDecoderStateMachine::SetDormant,
> +        true);
> +    mDecoderStateMachine
> +    ->GetStateMachineThread()->Dispatch(event, NS_DISPATCH_NORMAL);

Please make this one line, or at least indent the -> by two spaces. Here and below.

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1499,5 @@
>  }
>  
>  void MediaDecoderStateMachine::SetDormant(bool aDormant)
>  {
> +  NS_ASSERTION(OnStateMachineThread(), "Should be on state machine thread.");

Please make this a MOZ_ASSERT.
Attachment #8572284 - Flags: review?(cpearce) → review+
(Assignee)

Comment 109

3 years ago
Created attachment 8572291 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

Apply the comment. Carry "r=bobbyholley".
Attachment #8572284 - Attachment is obsolete: true
Attachment #8572284 - Flags: review?(bobbyholley)
Attachment #8572291 - Flags: review?(cpearce)
(Assignee)

Comment 110

3 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cca3c4b7c942
Comment on attachment 8571963 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

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

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1515,5 @@
>  
>    if (aDormant) {
>      if (mState == DECODER_STATE_SEEKING && !mQueuedSeekTarget.IsValid()) {
> +      if (mQueuedSeekTarget.IsValid()) {
> +        // Keep latest seek target

Noticed while rebasing - this looks like a bug. This condition will never pass because of the !mQueuedSeekTarget.IsValid() condition on the outer conditional. I think you want to remove it on the outer conditional to avoid falling through to the else {} below and overwriting mQueuedSeekTarget.
(Assignee)

Comment 112

3 years ago
(In reply to Bobby Holley (:bholley) from comment #111)
> Comment on attachment 8571963 [details] [diff] [review]
> Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
> 
> Review of attachment 8571963 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/MediaDecoderStateMachine.cpp
> @@ +1515,5 @@
> >  
> >    if (aDormant) {
> >      if (mState == DECODER_STATE_SEEKING && !mQueuedSeekTarget.IsValid()) {
> > +      if (mQueuedSeekTarget.IsValid()) {
> > +        // Keep latest seek target
> 
> Noticed while rebasing - this looks like a bug. This condition will never
> pass because of the !mQueuedSeekTarget.IsValid() condition on the outer
> conditional. I think you want to remove it on the outer conditional to avoid
> falling through to the else {} below and overwriting mQueuedSeekTarget.

Thanks, it is a mistake of updating the patch.
(Assignee)

Comment 113

3 years ago
Created attachment 8572429 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

Fix bad if(). Carry "r=cpearce".
Attachment #8571963 - Attachment is obsolete: true
Attachment #8572429 - Flags: review+
(Reporter)

Updated

3 years ago
Attachment #8572291 - Flags: review?(cpearce) → review+
(Assignee)

Comment 114

3 years ago
 https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa85558f413
 https://hg.mozilla.org/integration/mozilla-inbound/rev/36b8feee431e
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa440c623c4d
 https://hg.mozilla.org/integration/mozilla-inbound/rev/da4499b52105
Backed out for intermittent asserts on B2G.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d89cea541e

https://treeherder.mozilla.org/logviewer.html#?job_id=7209032&repo=mozilla-inbound
(Assignee)

Comment 116

3 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #115)
> Backed out for intermittent asserts on B2G.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d89cea541e
> 
> https://treeherder.mozilla.org/logviewer.html#?job_id=7209032&repo=mozilla-
> inbound

It was caused assert of Attachment 8571357 [details] [diff].
(Assignee)

Comment 117

3 years ago
Actual assert is here, it's condition seems unavoidable, because call back is called asynchronously.

> ###!!! ASSERTION: Expected not to be dormant here: '!mIsDormant', file ../../../gecko/dom/media/MediaDecoder.cpp, line 918
(Assignee)

Comment 118

3 years ago
cpearce, can we remove this assertion from this bug and handle it in a new bug? Because it seems not a regression.
Flags: needinfo?(cpearce)
(Assignee)

Comment 119

3 years ago
Created attachment 8572862 [details] [diff] [review]
patch 4: Asynchronize SetDormant()

un-birtot.
Attachment #8572291 - Attachment is obsolete: true
Attachment #8572862 - Flags: review+
(Assignee)

Comment 120

3 years ago
(In reply to Sotaro Ikeda [:sotaro] from comment #118)
> cpearce, can we remove this assertion from this bug and handle it in a new
> bug? Because it seems not a regression.

I do not know this situation happen before apply the patches. But "patch 4: Asynchronize SetDormant()" might signifies it.
Flags: needinfo?(cpearce)
(Reporter)

Comment 121

3 years ago
I think that's a bug introduced with "patch 4: Asynchronize SetDormant(). We must be going dormant after beginning load but before a "MediaDecoder::FirstFrameLoaded" runnable from the load has run.

I think we should fix the code so this assertion doesn't fire.

Maybe we should exit inside MediaDecoder::FirstFrameLoaded() if (mShuttingDown || mIsDormant)?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 122

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #121)
> 
> Maybe we should exit inside MediaDecoder::FirstFrameLoaded() if
> (mShuttingDown || mIsDormant)?

We might need more things. MediaDecoder's state has dependency to MediaDecoderStateMachine's event callback. attachment 8572862 [details] [diff] [review] calls MediaDecoderStateMachine::SetDormant() without caring about current SetDormant() handling status. From MediaDecoder point of view, it could cause un-managed state transition.

We might need to use MediaPromise to track this situation from MediaDecoder side.
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 123

3 years ago
Created attachment 8572932 [details] [diff] [review]
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode

un-bitrot.
Attachment #8572429 - Attachment is obsolete: true
Attachment #8572932 - Flags: review+
Let's talk about this on IRC - are you around?
(In reply to Sotaro Ikeda [:sotaro] from comment #122)
> (In reply to Chris Pearce (:cpearce) from comment #121)
> > 
> > Maybe we should exit inside MediaDecoder::FirstFrameLoaded() if
> > (mShuttingDown || mIsDormant)?
> 
> We might need more things. MediaDecoder's state has dependency to
> MediaDecoderStateMachine's event callback. attachment 8572862 [details] [diff] [review]
> [diff] [review] calls MediaDecoderStateMachine::SetDormant() without caring
> about current SetDormant() handling status. From MediaDecoder point of view,
> it could cause un-managed state transition.
> 
> We might need to use MediaPromise to track this situation from MediaDecoder
> side.

Given that we need to backport this to beta and bug 1135170 is waiting on this bug, I think that we should just do the simple fix and get this in. I have some other ideas on how to improve the interaction between MD and MDSM, but we should do them as followups, and probably not backport them to beta.

Can you just repush your patches with an additional bail-out if mDormant is true in FirstFrameLoaded? I really want to land bug 1135170 tomorrow.
(Assignee)

Comment 126

3 years ago
(In reply to Chris Pearce (:cpearce) from comment #121)
> I think that's a bug introduced with "patch 4: Asynchronize SetDormant(). We
> must be going dormant after beginning load but before a
> "MediaDecoder::FirstFrameLoaded" runnable from the load has run.
> 
> I think we should fix the code so this assertion doesn't fire.

This problem could happen before applying the patches. If the runnable was already queued to the main thread and it is not dispatched yet, during this time, SetDormant() is called, this assert could hit.

> 
> Maybe we should exit inside MediaDecoder::FirstFrameLoaded() if
> (mShuttingDown || mIsDormant)?

Is could block FirstFrameLoaded event and could cause the problem.
(Assignee)

Comment 127

3 years ago
I think that most simple fix is just removing the assert. From comment 126, this assert should not exist even with the correct fix.
(Assignee)

Comment 128

3 years ago
cpearce, how do you think about comment 126 and comment 127?
Flags: needinfo?(cpearce)
(In reply to Sotaro Ikeda [:sotaro] from comment #126)
> Is could block FirstFrameLoaded event and could cause the problem.

But only in the situation where the MediaDecoder has already switched back into dormant mode and doesn't care about FirstFrameLoaded, right?
Sotaro, can you log into IRC? That would make this much easier.
(Assignee)

Comment 131

3 years ago
Created attachment 8572950 [details] [diff] [review]
Patch 1: Add extra dormant logging

Remove assert from FirstFrameLoaded(), instead add !mIsDormant check to state transition.
Attachment #8571357 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8572950 - Flags: review?(bobbyholley)
Attachment #8572950 - Flags: review?(bobbyholley) → review+
(Assignee)

Updated

3 years ago
Flags: needinfo?(cpearce)
(Assignee)

Comment 132

3 years ago
   https://hg.mozilla.org/integration/mozilla-inbound/rev/3db16602084f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/18225b2d31ba
   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b100a291614
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d78b8a0b6392
https://hg.mozilla.org/mozilla-central/rev/3db16602084f
https://hg.mozilla.org/mozilla-central/rev/18225b2d31ba
https://hg.mozilla.org/mozilla-central/rev/9b100a291614
https://hg.mozilla.org/mozilla-central/rev/d78b8a0b6392
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
37 and 38 are marked as affected. Although this is a sizable patch, because it affects YouTube, I would like to understand the risk associated with uplift before we make a decision. Please remember to request uplift once the patch is verified on m-c.
Flags: needinfo?(sotaro.ikeda.g)
I think cpearce is the one pushing to get this on ff37.
Flags: needinfo?(cpearce)
(Reporter)

Comment 136

3 years ago
(In reply to Bobby Holley (:bholley) from comment #135)
> I think cpearce is the one pushing to get this on ff37.

Anthony and I discussed this and both of us think that the regression risk here is significant enough and that we're far enough through the (short) 37 beta cycle that we should only uplift this to Aurora38, not to Beta37.
Flags: needinfo?(cpearce)
(Assignee)

Comment 137

3 years ago
Comment on attachment 8572950 [details] [diff] [review]
Patch 1: Add extra dormant logging

Approval Request Comment
[Feature/regressing bug #]: none
[User impact if declined]: Video playback(youtube) user might use more memory.
            Then, the user might face out-of-memory more than applying the patch.
[Describe test coverage new/current, TreeHerder]: One test is added in this bug.
                       Locally tested youtube video playback use cased.
[Risks and why]: enough risk when dormant mode is used.
                 This fix add dormant usage to windows7/8 during video playback.
                 But without the fix, out-of-memory risk is kept high.
[String/UUID change made/needed]: none.
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8572950 - Flags: approval-mozilla-aurora?
(In reply to Chris Pearce (:cpearce) from comment #136)
> Anthony and I discussed this and both of us think that the regression risk
> here is significant enough and that we're far enough through the (short) 37
> beta cycle that we should only uplift this to Aurora38, not to Beta37.

Thank you for the assessment. I have marked 37 as wontfix.
status-firefox37: affected → wontfix
tracking-firefox38: --- → +
Comment on attachment 8572950 [details] [diff] [review]
Patch 1: Add extra dormant logging

happy to uplift help with keeping oom lower.
Attachment #8572950 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/04b4edf606f2
https://hg.mozilla.org/releases/mozilla-aurora/rev/f89ab4b1b200
https://hg.mozilla.org/releases/mozilla-aurora/rev/23b7c077855d
https://hg.mozilla.org/releases/mozilla-aurora/rev/a211fede7312
status-firefox38: affected → fixed
Flags: in-testsuite+
Bustage fix for my rebasing error.
https://hg.mozilla.org/releases/mozilla-aurora/rev/31275b97d536
Depends on: 1142527
No longer depends on: 1142527
(Assignee)

Updated

3 years ago
Depends on: 1142527
(Assignee)

Updated

3 years ago
Depends on: 1147435
You need to log in before you can comment on or make changes to this bug.