events not properly queued when readyState change as per spec

RESOLVED FIXED in Firefox 50

Status

()

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jya, Assigned: jya)

Tracking

(Blocks: 1 bug, {regression})

50 Branch
mozilla51
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox50 fixed, firefox51 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

3 years ago
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)

Updated

3 years ago
Blocks: 1298606
(Assignee)

Updated

3 years ago
See Also: → bug 1269830
(Assignee)

Updated

3 years ago
Blocks: 778617
(Assignee)

Comment 1

3 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
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → jyavenard

Comment 7

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 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

3 years ago
P4 is a fix for a regression caused by bug 1258870.
Blocks: 1258870

Comment 26

3 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
(Assignee)

Comment 28

3 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

3 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

3 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?
(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.
(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+
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
Version: unspecified → 50 Branch
You need to log in before you can comment on or make changes to this bug.