Closed Bug 1128357 Opened 10 years ago Closed 10 years ago

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

Categories

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

Product:
Core
Component:
Audio/Video
Platform:
x86_64
Windows Vista
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox37 + wontfix
firefox38 + fixed
firefox39 --- fixed

People

(Reporter: cpearce, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 28 obsolete files)

crash log of Comment 17
1.68 KB, text/plain
Details
patch 3: test for video playback with dormant
3.03 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
patch 4: Asynchronize SetDormant()
4.06 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Patch 2: Don't dispatch seeking/seeked events when coming out of dormant mode
50.62 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
Patch 1: Add extra dormant logging
4.67 KB, patch
bholley
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
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.
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
We should probably just disable heuristic dormant on beta 36, so we don't break non-MSE video.
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.
This patch makes YouTube not clear its speed dial. But the seek back to previous playback position when coming out of dormant still fails.
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.
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)
(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.
Priority: -- → P1
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)
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)
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.
(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)
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
(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: nobody → sotaro.ikeda.g
Blocks: 1050031
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.
(In reply to Sotaro Ikeda [:sotaro] from comment #18) > Created attachment 8563989 [details] > crash log of Comment 17 Correction: stack dump.
Update comment.
Attachment #8563929 - Attachment is obsolete: true
Fix a mistake.
Attachment #8564070 - Attachment is obsolete: true
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.
attachment 8564470 [details] [diff] [review] fixed the different problem than this bug. It seems better to split to a different bug.
Blocks: 1133167
No longer blocks: 1133167
Depends on: 1133167
Attachment #8564470 - Attachment is obsolete: true
Tracking all MSE P1 bugs for Firefox 37.
status-firefox37: --- → affected
status-firefox38: --- → affected
tracking-firefox37: --- → +
Sotaro - is this likely to make the uplift?
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8561710 - Attachment description: WIP Patch 1: Add extra dormant logging. → Patch 1: Add extra dormant logging.
Flags: needinfo?(sotaro.ikeda.g)
Some code clean up.
Attachment #8565551 - Attachment is obsolete: true
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.
(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.
(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.
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.
And HTMLMediaElement::SeekStarted() is also used to fix Bug 1048171. Therefore, it seems better not to skip to call HTMLMediaElement::SeekStarted().
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.
Blocks: 1134523
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.
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
(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 :-)
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.
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.
(In reply to Sotaro Ikeda [:sotaro] from comment #43) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d68186ea671 some test failures exist.
(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.
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()]
Comment 46 is different problem from this bug. I am going to create a different bug.
Blocks: 1135304
No longer blocks: 1135304
Depends on: 1135304
un-bitrot.
Attachment #8566624 - Attachment is obsolete: true
Attachment #8561710 - Flags: review?(cpearce)
Attachment #8567931 - Flags: review?(cpearce)
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+
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-
(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+
(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)
(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.
(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()?
(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.
> 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?
Apply the comments.
Attachment #8567931 - Attachment is obsolete: true
No longer blocks: 1134523
Attachment #8568639 - Flags: review?(cpearce)
(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.
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+
(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.
Apply the comments. Carry "r=cpearce".
Attachment #8568639 - Attachment is obsolete: true
Attachment #8568952 - Flags: review+
For the time being, the test is enabled only on b2g gonk. mp4 file playback's dormant is supported only on b2g gonk now.
(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.
(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.
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.
And scheduling of MP4Reader::Update() also seems to have a problem.
(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.
(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.
During FlushDecoding() in MediaDecoderStateMachine::RunStateMachine(), ReentrantMonitor was exited. Therefore, ReentrantMonitor lock did not work correctly in MediaDecoderStateMachine::SetDormant(), if during FlushDecoding().
Attachment #8570330 - Attachment is obsolete: true
(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.
Update test. In previous test, video was not loaded on b2g.
Attachment #8569883 - Attachment is obsolete: true
(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.
Limit test only to b2g gonk and windows 7/8.
Attachment #8570985 - Attachment is obsolete: true
Attachment #8570794 - Flags: review?(cpearce)
Attachment #8571090 - Flags: review?(cpearce)
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.
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)
(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.
(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
> > > > > > 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.
Apply the comment. Carry "r=cpearce".
Attachment #8561710 - Attachment is obsolete: true
Attachment #8571357 - Flags: review+
Apply the comment.
Attachment #8571090 - Attachment is obsolete: true
Attachment #8571090 - Flags: review?(cpearce)
Attachment #8571362 - Flags: review?(cpearce)
Attachment #8571362 - Attachment is obsolete: true
Attachment #8571362 - Flags: review?(cpearce)
Attachment #8571365 - Flags: review?(cpearce)
(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.
(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.
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-
un-bitrot. Carry "r=cpearce".
Attachment #8568952 - Attachment is obsolete: true
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.
Attachment #8570794 - Attachment is obsolete: true
Attachment #8570794 - Flags: review?(cpearce)
Attachment #8572150 - Flags: review?(cpearce)
Attachment #8572150 - Flags: review?(bobbyholley)
(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".
(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)
(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)
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)
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-
Removed SetDormantInternal().
Attachment #8572276 - Attachment is obsolete: true
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+
Apply the comment. Carry "r=bobbyholley".
Attachment #8572284 - Attachment is obsolete: true
Attachment #8572284 - Flags: review?(bobbyholley)
Attachment #8572291 - Flags: review?(cpearce)
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.
(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.
Fix bad if(). Carry "r=cpearce".
Attachment #8571963 - Attachment is obsolete: true
Attachment #8572429 - Flags: review+
Attachment #8572291 - Flags: review?(cpearce) → review+
(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].
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
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)
un-birtot.
Attachment #8572291 - Attachment is obsolete: true
Attachment #8572862 - Flags: review+
(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)
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)
(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)
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.
(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.
I think that most simple fix is just removing the assert. From comment 126, this assert should not exist even with the correct fix.
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.
Remove assert from FirstFrameLoaded(), instead add !mIsDormant check to state transition.
Attachment #8571357 - Attachment is obsolete: true
Attachment #8572950 - Flags: review?(bobbyholley)
Attachment #8572950 - Flags: review?(bobbyholley) → review+
Flags: needinfo?(cpearce)
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)
(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)
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+
Depends on: 1142527
No longer depends on: 1142527
Depends on: 1142527
Depends on: 1147435
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: