Closed
Bug 1298594
Opened 8 years ago
Closed 8 years ago
events not properly queued when readyState change as per spec
Categories
(Core :: Audio/Video: Playback, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla51
People
(Reporter: jya, Assigned: jya)
References
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(5 files)
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
jwwang
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
58 bytes,
text/x-review-board-request
|
mozbugz
:
review+
|
Details |
The non-normative section describing the waiting event indicates: https://html.spec.whatwg.org/multipage/embedded-content.html#event-media-waiting "Playback has stopped because the next frame is not available, but the user agent expects that frame to become available in due course. " However, in https://html.spec.whatwg.org/multipage/embedded-content.html#ready-states:event-media-timeupdate we read: "If the previous ready state was HAVE_FUTURE_DATA or more, and the new ready state is HAVE_CURRENT_DATA or less If the media element was potentially playing before its readyState attribute changed to a value lower than HAVE_FUTURE_DATA, and the element has not ended playback, and playback has not stopped due to errors, paused for user interaction, or paused for in-band content, the user agent must queue a task to fire a simple event named timeupdate at the element, and queue a task to fire a simple event named waiting at the element. " However, if the MediaResource::IsExpectingMoreData returns false, then the MDSM never enters the state DECODER_STATE_BUFFERING and as such HTMLMediaElement::NextFrameStatus will never return NEXT_FRAME_UNAVAILABLE_BUFFERING and the "waiting" event will never be fired. But per spec if readyState goes back to HAVE_FUTURE_DATA or less, we are to fire timeupdate and waiting.
Assignee | ||
Comment 1•8 years ago
|
||
Updating the title as the proper solution is more complicated than first thought. Currently timeupdate and waiting are fired only once nextFrameStatus = NEXT_FRAME_UNAVAILABLE_BUFFERING Before nextFrameStatus goes into that mode however, we get nextFrameStatus = NEXT_FRAME_UNAVAILABLE; this causes readyState to change to HAVE_CURRENT_DATA. At this stage no events are fired as per spec. It will takes several run of the main thread before the events are processed. The issue is that to indicate that the next frame is unavailable, we have three different states: NEXT_FRAME_UNAVAILABLE NEXT_FRAME_UNAVAILABLE_BUFFERING NEXT_FRAME_UNAVAILABLE_SEEKING and we will always go through NEXT_FRAME_UNAVAILABLE first.
Assignee: jyavenard → nobody
Summary: Media element never firing waiting when the MediaResource is ended. → events not properly queued when readyState change as per spec
Updated•8 years ago
|
Priority: -- → P3
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee: nobody → jyavenard
Comment 7•8 years ago
|
||
mozreview-review |
Comment on attachment 8785801 [details] Bug 1298594: P2. Fire waiting event when readyState move back to HAVE_CURRENT_DATA. https://reviewboard.mozilla.org/r/74860/#review72712
Attachment #8785801 -
Flags: review?(jwwang) → review+
Comment 8•8 years ago
|
||
mozreview-review |
Comment on attachment 8785800 [details] Bug 1298594: [MSE] P1. Add mochitest to verify correct behavior. https://reviewboard.mozilla.org/r/74858/#review72702 r+ with concerns that you should easily address: ::: dom/media/mediasource/test/test_WaitingOnMissingDataEnded_mp4.html:23 (Diff revision 1) > + fetchAndLoad(videosb, 'bipbop/bipbop_video', ['init'], '.mp4') > + .then(fetchAndLoad.bind(null, videosb, 'bipbop/bipbop_video', ['init'], '.mp4')) You seem to load the video init twice, is that intended? ::: dom/media/mediasource/test/test_WaitingOnMissingDataEnded_mp4.html:42 (Diff revision 1) > + // currentTime is based on the current video frame, so if the audio ends just before > + // the next video frame, currentTime can be up to 1 frame's worth earlier than end of video. > + isfuzzy(el.currentTime, videosb.buffered.end(0), 0.0465, "waiting was fired on gap"); You didn't load any audio (right?), so are that comment and fuzzing correct?
Attachment #8785800 -
Flags: review?(gsquelart) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
mozreview-review |
Comment on attachment 8785804 [details] Bug 1298594: P5. Fix mochitest. https://reviewboard.mozilla.org/r/74866/#review72716
Attachment #8785804 -
Flags: review?(gsquelart) → review+
Comment 15•8 years ago
|
||
mozreview-review |
Comment on attachment 8785802 [details] Bug 1298594: P3. Ensure currentTime is updated prior changing readyState. https://reviewboard.mozilla.org/r/74862/#review72718 ::: dom/media/MediaDecoderStateMachine.cpp:2549 (Diff revision 1) > DECODER_LOG("Changed mNextFrameStatus to %s", statusString); > + } else if (status == MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE_BUFFERING || > + status == MediaDecoderOwner::NEXT_FRAME_UNAVAILABLE) { > + // Ensure currentTime is up to date prior updating mNextFrameStatus so that > + // the MediaDecoderOwner fire events at correct currentTime. > + UpdatePlaybackPositionPeriodically(); This statement will be executed only when status == mNextFrameStatus which seems wrong to me. We should update currentTime when mNextFrameStatus is 'changed' to NEXT_FRAME_UNAVAILABLE_BUFFERING or NEXT_FRAME_UNAVAILABLE.
Attachment #8785802 -
Flags: review?(jwwang) → review-
Comment 16•8 years ago
|
||
mozreview-review |
Comment on attachment 8785803 [details] Bug 1298594: P4. Pop the frame when current time is past the end of the current frame. https://reviewboard.mozilla.org/r/74864/#review72720 ::: dom/media/MediaDecoderStateMachine.cpp:414 (Diff revision 2) > > bool MediaDecoderStateMachine::HaveNextFrameData() > { > MOZ_ASSERT(OnTaskQueue()); > return (!HasAudio() || HasFutureAudio()) && > - (!HasVideo() || VideoQueue().GetSize() > 1); > + (!HasVideo() || VideoQueue().GetSize() > 0); I don't think this is right. VideoQueue().GetSize() == 1 means we have 'current' frame only. We shouldn't return true for HaveNextFrameData() when we have current frame only. Here is the story about bug 1143575. Before the bug, we removed the frame from the video queue when it was sent to the video frame container. Therefore, queue size >= 1 means future frames. However, bug 1143575 pushes the 'current' frame back to the queue after sending a bunch of frames to the compositor. Therefore, queue size == 1 means a current frame, and > 1 means future frames.
Attachment #8785803 -
Flags: review?(jwwang) → review-
Assignee | ||
Comment 17•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8785802 [details] Bug 1298594: P3. Ensure currentTime is updated prior changing readyState. https://reviewboard.mozilla.org/r/74862/#review72718 > This statement will be executed only when status == mNextFrameStatus which seems wrong to me. > > We should update currentTime when mNextFrameStatus is 'changed' to NEXT_FRAME_UNAVAILABLE_BUFFERING or NEXT_FRAME_UNAVAILABLE. big brain fart :( I actually committed a staged change, not the final.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 23•8 years ago
|
||
mozreview-review |
Comment on attachment 8785802 [details] Bug 1298594: P3. Ensure currentTime is updated prior changing readyState. https://reviewboard.mozilla.org/r/74862/#review72730
Attachment #8785802 -
Flags: review?(jwwang) → review+
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8785803 [details] Bug 1298594: P4. Pop the frame when current time is past the end of the current frame. https://reviewboard.mozilla.org/r/74864/#review72732
Attachment #8785803 -
Flags: review?(jwwang) → review+
Assignee | ||
Comment 25•8 years ago
|
||
P4 is a fix for a regression caused by bug 1258870.
Blocks: 1258870
Comment 26•8 years ago
|
||
Pushed by jyavenard@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/fb8f6e366481 [MSE] P1. Add mochitest to verify correct behavior. r=gerald https://hg.mozilla.org/integration/autoland/rev/ce7dae2a0f06 P2. Fire waiting event when readyState move back to HAVE_CURRENT_DATA. r=jwwang https://hg.mozilla.org/integration/autoland/rev/ded582db991f P3. Ensure currentTime is updated prior changing readyState. r=jwwang https://hg.mozilla.org/integration/autoland/rev/2cdde44ebcc7 P4. Pop the frame when current time is past the end of the current frame. r=jwwang https://hg.mozilla.org/integration/autoland/rev/b1da49e041bb P5. Fix mochitest. r=gerald
Comment 27•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fb8f6e366481 https://hg.mozilla.org/mozilla-central/rev/ce7dae2a0f06 https://hg.mozilla.org/mozilla-central/rev/ded582db991f https://hg.mozilla.org/mozilla-central/rev/2cdde44ebcc7 https://hg.mozilla.org/mozilla-central/rev/b1da49e041bb
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Assignee | ||
Comment 28•8 years ago
|
||
Comment on attachment 8785803 [details] Bug 1298594: P4. Pop the frame when current time is past the end of the current frame. Approval Request Comment [Feature/regressing bug #]:1295630 [User impact if declined]: event sent one frame too early. Additionally, it appears that it changed some elemental purple particle somewhere in the universe causing a talos regression in a totally unrelated test. [Describe test coverage new/current, TreeHerder]: in central [Risks and why]: can't think of any [String/UUID change made/needed]: None
Attachment #8785803 -
Flags: approval-mozilla-aurora?
status-firefox50:
--- → affected
Hello Jean-Yves, William, if this is a fix for a regression caused by bug 1295630, I am in favor of backing out the patch from that bug instead of committing 3 additional patches (other 2 from the bug seem test only). I know that we need to make our test automation more robust but not sure if uplifting 4 patches to make that happen a week before 50 goes to Beta makes sense. I would like to deny the uplifts here and back out the patch from that bug. Or if there is a simplified version of the fix, I am open to being convinced otherwise.
Flags: needinfo?(wlachance)
Flags: needinfo?(jyavenard)
Assignee | ||
Comment 30•8 years ago
|
||
I made a mistake in the uplift request, it fixes a regression introduced by bug https://bugzilla.mozilla.org/show_bug.cgi?id=1258870 Having said, I'll be likely be requesting uplift for the remaining of those [patches in this bug, I just let it bake in central for a while. Thank you
Flags: needinfo?(wlachance)
Flags: needinfo?(jyavenard)
Comment 31•8 years ago
|
||
mozreview-review |
Comment on attachment 8785803 [details] Bug 1298594: P4. Pop the frame when current time is past the end of the current frame. https://reviewboard.mozilla.org/r/74864/#review74656 ::: dom/media/mediasink/VideoSink.cpp:400 (Diff revision 3) > NS_ASSERTION(clockTime >= 0, "Should have positive clock time."); > > // Skip frames up to the playback position. > int64_t lastDisplayedFrameEndTime = 0; > while (VideoQueue().GetSize() > mMinVideoQueueSize && > - clockTime > VideoQueue().PeekFront()->GetEndTime()) { > + clockTime >= VideoQueue().PeekFront()->GetEndTime()) { If we seek to the end of a video (i.e. v.currentTime = v.duration) does this change cause us to not render the last frame of the video?
Comment 32•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #29) > Hello Jean-Yves, William, if this is a fix for a regression caused by bug > 1295630, I am in favor of backing out the patch from that bug instead of > committing 3 additional patches (other 2 from the bug seem test only). I > know that we need to make our test automation more robust but not sure if > uplifting 4 patches to make that happen a week before 50 goes to Beta makes > sense. I would like to deny the uplifts here and back out the patch from > that bug. Or if there is a simplified version of the fix, I am open to being > convinced otherwise. So I'm not 100% sure if I understand, but the consequence of not accepting the patches from that bug (while keeping the patches here) would be no talos coverage for the basic compositor video test. I defer to you and the video experts on what is best here.
Comment 33•8 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #31) > If we seek to the end of a video (i.e. v.currentTime = v.duration) does this > change cause us to not render the last frame of the video? https://hg.mozilla.org/mozilla-central/file/tip/dom/media/MediaDecoderStateMachine.cpp#l2182 No, it doesn't. MDSM draw the 1st frame after seeking before VideoSink running any render loops (to discard late frames).
Keywords: regression
Comment on attachment 8785803 [details] Bug 1298594: P4. Pop the frame when current time is past the end of the current frame. Recent regression in 50, has stabilized in Nightly for a week, Aurora50+
Attachment #8785803 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 35•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a2f2368796f https://hg.mozilla.org/releases/mozilla-aurora/rev/824368e15957 https://hg.mozilla.org/releases/mozilla-aurora/rev/0ee7678d527b https://hg.mozilla.org/releases/mozilla-aurora/rev/f04b44d05fcc
Flags: in-testsuite+
Comment 36•8 years ago
|
||
Sorry, got a bit uplift-happy on this one. Backing out the parts that weren't actually approved to land. https://hg.mozilla.org/releases/mozilla-aurora/rev/236385abdffc
Updated•7 years ago
|
Version: unspecified → 50 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•