1391666 - [EME] media elements with autoplay attribute set will never start with EME

Status: RESOLVED FIXED

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

Description

[Jean-Yves Avenard [:jya]]

Bug 1389844 didn't fully fixed the issue.

The issue appears to be related to the dispatch of the waitingforkey event.

We enter the case:
http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/html/HTMLMediaElement.cpp#5857

mWaitingForKey is WAITING_FOR_KEY_DISPATCHED.

mWaitingForKey appears to not be reset, even after we've got the key until play() is called.

mWaitingForKey isn't reset.

Comment 1

[Jean-Yves Avenard [:jya]]

Something is a miss there:
http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/html/HTMLMediaElement.cpp#5857

mWaitingForKey will only ever go back to NOT_WAITING_FOR_KEY there:
http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/html/HTMLMediaElement.cpp#5921

But this only occurs of mPaused is false.

mPaused will only go to true later in http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/html/HTMLMediaElement.cpp#5927

By this time readyState is HAVE_FUTURE_DATA.

We will never enter again the condition:
  if (oldState < nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA &&
      mReadyState >= nsIDOMHTMLMediaElement::HAVE_FUTURE_DATA) {

And so mWaitingForKey is stuck on WAITING_FOR_KEY_DISPATCHED

Comment 2

[Jean-Yves Avenard [:jya]]

The core issue actually here is:
http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/dom/html/HTMLMediaElement.cpp#5921

    if (!mPaused) {
      mWaitingForKey = NOT_WAITING_FOR_KEY;
      NotifyAboutPlaying();
    }

mWaitingForKey will only be reset following a change of readyState, if the element isn't paused.

But this isn't the case here, playback never started, mPaused is always true

Comment 3

[Jean-Yves Avenard [:jya]]

[why tracking] This will cause a regression with any sites using EME and expecting to automatically start.

Comment 4

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1391666 - P1. Mochitest to verify correct behavior.

We load 10s of encrypted video, which should trigger readyState being HAVE_ENOUGH_DATA

Review commit: https://reviewboard.mozilla.org/r/170418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170418/

Comment 5

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1391666 - P2. Reset mWaitingForKey when we have data and autoplay attribute is set

By default, a media element that has never played is in paused mode. As such, we need to reset mWaitingForKey to NOT_WAITING_FOR_KEY otherwise, readyState will never become HAVE_ENOUGH_DATA.

Review commit: https://reviewboard.mozilla.org/r/170420/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170420/

Comment 6

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1391666 - P3. Change to HAVE_ENOUGH_DATA when possible.

In bug 1312886, we made sure that readyState would never become HAVE_ENOUGH_DATA if we were waiting for a key.
However, this is in effect useless as the next call to ChangeReadyState would have reset mWaitingForKey.

In practice, it only meant that we delayed the change from HAVE_FUTURE_DATA to HAVE_ENOUGH_DATA until the next call to UpdateReadyState.

Review commit: https://reviewboard.mozilla.org/r/170422/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170422/

Comment 7

[Jean-Yves Avenard [:jya]]

Attachment: Bug 1391666 - P4. Remove unecessary code.

It isn't possible for the End of Stream algorithm to cause an updateend event to fire. Regardless of the duration being modified.

Review commit: https://reviewboard.mozilla.org/r/170424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170424/

Comment 8

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8899092 [details]
Bug 1391666 - P1. Mochitest to verify correct behavior.

https://reviewboard.mozilla.org/r/170418/#review175596

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8899095 [details]
Bug 1391666 - P4. Remove unecessary code.

https://reviewboard.mozilla.org/r/170424/#review175598

Comment 10

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8899093 [details]
Bug 1391666 - P2. Reset mWaitingForKey when we have data and autoplay attribute is set

https://reviewboard.mozilla.org/r/170420/#review175600

It looks like I am addressing the same issue in Bug 1390443. P3.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8899093 [details]
Bug 1391666 - P2. Reset mWaitingForKey when we have data and autoplay attribute is set

https://reviewboard.mozilla.org/r/170420/#review175646

Comment 12

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8899094 [details]
Bug 1391666 - P3. Change to HAVE_ENOUGH_DATA when possible.

https://reviewboard.mozilla.org/r/170422/#review175648

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2031042cb44d
P1. Mochitest to verify correct behavior. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4c7241cf8a4f
P2. Reset mWaitingForKey when we have data and autoplay attribute is set r=cpearce
https://hg.mozilla.org/integration/autoland/rev/109370151602
P3. Change to HAVE_ENOUGH_DATA when possible. r=cpearce
https://hg.mozilla.org/integration/autoland/rev/fc9fa66d198c
P4. Remove unecessary code. r=gerald

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2031042cb44d
https://hg.mozilla.org/mozilla-central/rev/4c7241cf8a4f
https://hg.mozilla.org/mozilla-central/rev/109370151602
https://hg.mozilla.org/mozilla-central/rev/fc9fa66d198c

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Want to request uplift? Does this need manual testing/verification?

Comment 16

[Jean-Yves Avenard [:jya]]

I've already indirectly asked for uplift in bug 1389844. No need for manual testing. I've written mochi test for it, and the original person who reported the bug confirmed that the issue was fixed.



Do I need to separately ask for uplift here too?

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Yes please, it is too easy for the sheriffs (or relman) to miss that kind of indirect request.

Comment 18

[Jean-Yves Avenard [:jya]]

Comment on attachment 8899092 [details]
Bug 1391666 - P1. Mochitest to verify correct behavior.

Comment on attachment 8897938 [details]
Bug 1389844 - P1. Add mochitest.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1382303
[User impact if declined]: Playback of some video sites will never start.
[Is this code covered by automated tests?]: Yes, mochitest were written to verify coverage
[Has the fix been verified in Nightly?]: Yes, original reporter communicated to me that the issue was fixed
[Needs manual test from QE? If yes, steps to reproduce]: no, as there's mochitests in place
[List of other uplifts needed for the feature/fix]: all patches of bug 1391666
[Is the change risky?]: no
[Why is the change risky/not risky?]: We relax the behaviour for some streams whose content doesn't start exactly at 0 (which is unfortunately rather common). In the worse case we more losely say we are ready to play something when we may not, but this condition isn't fatal and will always recover.
[String changes made/needed]: none

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: see above
Fix Landed on Version: 57
Risk to taking this patch (and alternatives if risky): There are alternatives to make on the JS video player, but people are unlikely to do those.
String or UUID changes made by this patch: none

For a full fix of the problem reported, bug 1389844 is required.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8899092 [details]
Bug 1391666 - P1. Mochitest to verify correct behavior.

Let's try these patches on beta 6. Missed the build for today's beta though. 
It seems a bit risky for ESR and doesn't fit the criteria for mid-ESR uplift.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4e6b8a64e613
https://hg.mozilla.org/releases/mozilla-beta/rev/53d7d0601775
https://hg.mozilla.org/releases/mozilla-beta/rev/065ea81827ae
https://hg.mozilla.org/releases/mozilla-beta/rev/21bc77a750df

Comment 21

[Andrei Vaida [:avaida]]

(In reply to Jean-Yves Avenard [:jya] from comment #18)
> [Is this code covered by automated tests?]: Yes, mochitest were written to
> verify coverage
> [Has the fix been verified in Nightly?]: Yes, original reporter communicated
> to me that the issue was fixed
> [Needs manual test from QE? If yes, steps to reproduce]: no, as there's
> mochitests in place

Setting qe-verify- based on jya's assessment on manual testing needs and the fact that this fix has automated coverage.