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)

50 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(5 files)

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.
Blocks: 1298606
See Also: → 1269830
Blocks: MSE
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
Assignee: nobody → jyavenard
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 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 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 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-
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 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 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+
P4 is a fix for a regression caused by bug 1258870.
Blocks: 1258870
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 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?
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)
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 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).
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.

Attachment

General

Created:
Updated:
Size: