Closed Bug 1382303 Opened 7 years ago Closed 7 years ago

[MSE] playing event incorrectly fired even when no data has been added

Categories

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

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(3 files)

Issue:

Create a MediaSource
Attach to HTML media element
Create a source buffer
Add Init segment.

the playing event is fired.

This is incorrect.
Per spec:
https://html.spec.whatwg.org/multipage/media.html#event-media-playing
"readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA."

here we have at best readyState = HAVE_METADATA

https://jyavenard.github.io/htmltests/tests/mse_mp4/facebook-init.html

We can see the playing event being fired right after the play event.

(reported by BBC, this is breaking their player)
https://codepen.io/anon/pen/xrojbE for a narrowed case.

It appears to be related to autoplay settings
In bug 1304134, we read "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states) 'playing' should be fired before activating autoplay"

I find no such description stating playing should be fired before autoplay

in both w3.org and whatwg.org we have:
w3.org:
"readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA. "

whatwg.org:
"readyState is newly equal to or greater than HAVE_FUTURE_DATA and paused is false, or paused is newly false and readyState is equal to or greater than HAVE_FUTURE_DATA"

now in the example above, we see that playing was fired twice. even though playback never actually started.

The only reference to the autoplaying attribute is in the section "If the new ready state is HAVE_ENOUGH_DATA"

in bug 1304134 comment 1, we read "Small correction: just noticed that the "playing" event was fired, but some 20 seconds after the video started playing."

so the issue wasn't that playing wasn't fired, but that it fired after playback had started.

bug 1304134 comment 16, there's reference that this change would be wrong with MSE.

Not even adding an init segment cause "playing" to be fired. The problem here is that we have readyState == HAVE_NOTHING

If you attempt to seek when readyState == HAVE_NOTHING then per spec, the seek is ignored.

This is likely the issue the BBC is seeing.

IMHO bug 1304134 should be reversed, or modified such as with MSE that incorrect behaviour doesn't apply.
Blocks: 1304134
Flags: needinfo?(jwwang)
(In reply to Jean-Yves Avenard [:jya] from comment #1)
> In bug 1304134, we read
> "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states)
> 'playing' should be fired before activating autoplay"
> 
> I find no such description stating playing should be fired before autoplay

https://html.spec.whatwg.org/multipage/media.html#ready-states

If the new ready state is HAVE_ENOUGH_DATA

    If the element is not eligible for autoplay, then the user agent must abort these substeps.
    The user agent may run the following substeps:

      Set the paused attribute to false.
      If the element's show poster flag is true, set it to false and run the time marches on steps.
      Queue a task to fire an event named play at the element.
      Notify about playing for the element. <-- fire 'playing'.
Flags: needinfo?(jwwang)
(In reply to Jean-Yves Avenard [:jya] from comment #1)
> in bug 1304134 comment 1, we read "Small correction: just noticed that the
> "playing" event was fired, but some 20 seconds after the video started
> playing."
> 
> so the issue wasn't that playing wasn't fired, but that it fired after
> playback had started.

There are in fact 2 issues:
1. comment 0 which I was able to reproduce.
2. comment 1 which was reproducible by the reporter.

Issue 1 is fixed and issue 2 is not bug at all.
(In reply to Jean-Yves Avenard [:jya] from comment #1)
> bug 1304134 comment 16, there's reference that this change would be wrong
> with MSE.
> 
> Not even adding an init segment cause "playing" to be fired. The problem
> here is that we have readyState == HAVE_NOTHING
> 
> If you attempt to seek when readyState == HAVE_NOTHING then per spec, the
> seek is ignored.
> 
> This is likely the issue the BBC is seeing.
> 
> IMHO bug 1304134 should be reversed, or modified such as with MSE that
> incorrect behaviour doesn't apply.

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/html/HTMLMediaElement.cpp#5989-5990
We should remove the special logic about MediaStream and MediaSource. However, it will break some sites or tests if I remember correctly.

A quick workaround would be check readyState in CheckAutoplayDataReady() again before firing 'playing' to make MSE autoplay less broken.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #2)
> (In reply to Jean-Yves Avenard [:jya] from comment #1)
> > In bug 1304134, we read
> > "(https://dev.w3.org/html5/spec-preview/media-elements.html#ready-states)y
> > 'playing' should be fired before activating autoplay"
> > 
> > I find no such description stating playing should be fired before autoplay
> 
> https://html.spec.whatwg.org/multipage/media.html#ready-states
> 
> If the new ready state is HAVE_ENOUGH_DATA
> 
>     If the element is not eligible for autoplay, then the user agent must
> abort these substeps.
>     The user agent may run the following substeps:
> 
>       Set the paused attribute to false.
>       If the element's show poster flag is true, set it to false and run the
> time marches on steps.
>       Queue a task to fire an event named play at the element.
>       Notify about playing for the element. <-- fire 'playing'.

The key point to get there is “if the new ready state is HAVE_ENOUGH_DATA”, at no stage did we reach readyState having that value.
I’m sorry, but the reason behind bug 1304134 is incorrect.

This change should be backed out

There’s nothing in the spec that states playing should be fired before readyState having the right value.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> (In reply to Jean-Yves Avenard [:jya] from comment #1)
> > in bug 1304134 comment 1, we read "Small correction: just noticed that the
> > "playing" event was fired, but some 20 seconds after the video started
> > playing."
> > 
> > so the issue wasn't that playing wasn't fired, but that it fired after
> > playback had started.
> 
> There are in fact 2 issues:
> 1. comment 0 which I was able to reproduce.
> 2. comment 1 which was reproducible by the reporter.
> 
> Issue 1 is fixed and issue 2 is not bug at all.

Where/when was 1 fixed?

Tests were done using today’s nightly
(In reply to Jean-Yves Avenard [:jya] from comment #6)
> Where/when was 1 fixed?
> Tests were done using today’s nightly

I mean bug 1304134 comment 0 and bug 1304134 comment 1 respectively.
https://hg.mozilla.org/mozilla-central/rev/3891407491fb

We should revert this change so autoplay is activated only when readyState is HAVE_ENOUGH_DATA.
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

https://reviewboard.mozilla.org/r/159440/#review164948

::: dom/media/mediasource/test/test_PlayEventsAutoPlaying.html:64
(Diff revision 1)
> +       promises.push(once(el, 'playing'));
> +       promises.push(once(el, 'ended'));
> +       // We're only adding 1.6s worth of data, not enough for readyState to change to HAVE_ENOUGH_DATA
> +       // So we end the media source so that all the playable data is available.
> +       promises.push(fetchAndLoad(videosb, 'bipbop/bipbop_video', range(1, 3), '.m4s')
> +                     .then(() => ms.endOfStream()));

We should have ENOUGH_DATA after calling endOfStream() since:
1. playback hasn't reached the end.
2. we have all the data needed to play till the end without stopping.

You can fix it in a new bug.
Attachment #8888483 - Flags: review?(jwwang) → review+
Comment on attachment 8888484 [details]
Bug 1382303 - P2. Do not activate autoplay early.

https://reviewboard.mozilla.org/r/159442/#review164952

::: commit-message-64659:3
(Diff revision 1)
> +Bug 1382303 - P2. Do not activate autoplay early. r?jwwang
> +
> +Per spec, autoplay should only gets triggered once readyState is equal or greater than HAVE_ENOUGH_DATA

It is impossible for readyState to be greater than HAVE_ENOUGH_DATA.
Attachment #8888484 - Flags: review?(jwwang) → review+
Comment on attachment 8888485 [details]
Bug 1382303 - P3. Reduce enough data threadhold to 10s.

https://reviewboard.mozilla.org/r/159444/#review164956

::: dom/media/mediasource/MediaSourceDecoder.cpp:316
(Diff revision 1)
> -  // If we have data up to the mediasource's duration or 30s ahead, we can
> +  // If we have data up to the mediasource's duration or 10s ahead, we can
>    // assume that we can play without interruption.
>    TimeIntervals buffered = GetBuffered();
>    buffered.SetFuzz(MediaSourceDemuxer::EOS_FUZZ / 2);
>    TimeUnit timeAhead =
> -    std::min(duration, currentPosition + TimeUnit::FromSeconds(30));
> +    std::min(duration, currentPosition + TimeUnit::FromSeconds(10));

It is more flexible to read the value from MediaPrefs.
Attachment #8888485 - Flags: review?(jwwang) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6568f892a5c4
P1. Add mochitest. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/0c6a7835fef4
P2. Do not activate autoplay early. r=jwwang
https://hg.mozilla.org/integration/autoland/rev/5e11375d63c7
P3. Reduce enough data threadhold to 10s. r=jwwang
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

https://reviewboard.mozilla.org/r/159440/#review164948

> We should have ENOUGH_DATA after calling endOfStream() since:
> 1. playback hasn't reached the end.
> 2. we have all the data needed to play till the end without stopping.
> 
> You can fix it in a new bug.

not sure I understand your comment here. It's exactly why we call endOfStream

The reason we call endOfStream is precisely to move to HAVE_ENOUGH_DATA , otherwise readyState is HAVE_FUTURE_DATA

And we do want to play something

What is there to fix?
Comment on attachment 8888485 [details]
Bug 1382303 - P3. Reduce enough data threadhold to 10s.

https://reviewboard.mozilla.org/r/159444/#review164956

> It is more flexible to read the value from MediaPrefs.

yes, will open a new bug for that.
Breaking the BBC player sounds bad. Please request Beta/ESR52 approval on this when you get a chance.
Assignee: nobody → jyavenard
Flags: needinfo?(jyavenard)
Flags: in-testsuite+
Version: 54 Branch → 51 Branch
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

https://reviewboard.mozilla.org/r/159440/#review164948

> not sure I understand your comment here. It's exactly why we call endOfStream
> 
> The reason we call endOfStream is precisely to move to HAVE_ENOUGH_DATA , otherwise readyState is HAVE_FUTURE_DATA
> 
> And we do want to play something
> 
> What is there to fix?

Forget it. I just misread the comments at #61-62.
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1304134
[User impact if declined]: Broken video playback on an unknown number of sites (the issue was originally reported to us by the BBC)
[Is this code covered by automated tests?]: Yes, a new test was added.
[Has the fix been verified in Nightly?]: No, but per jya, the BBC already worked around our bug with a change on their end, so verifying could be difficult at this point.
[Needs manual test from QE? If yes, steps to reproduce]: No, the new test covers the spec compliance bug that was fixed here.
[List of other uplifts needed for the feature/fix]: All 3 patches from this bug.
[Is the change risky?]: Not risky.
[Why is the change risky/not risky?]: Gets us spec compliant where we previously were not. New test added to cover this scenario.
[String changes made/needed]: None.
Flags: needinfo?(jyavenard)
Attachment #8888483 - Flags: approval-mozilla-esr52?
Attachment #8888483 - Flags: approval-mozilla-beta?
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

mse fix, beta55+ (all three patches)
Attachment #8888483 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> [Is this code covered by automated tests?]: Yes, a new test was added.
> [Has the fix been verified in Nightly?]: No, but per jya, the BBC already
> worked around our bug with a change on their end, so verifying could be
> difficult at this point.
> [Needs manual test from QE? If yes, steps to reproduce]: No, the new test
> covers the spec compliance bug that was fixed here.

Setting qe-verify- based on Ryan's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
Comment on attachment 8888483 [details]
Bug 1382303 - P1. Add mochitest.

MSE fix. Uplift it to ESR52.3.
Attachment #8888483 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Depends on: 1389844
You need to log in before you can comment on or make changes to this bug.