Closed Bug 1389844 Opened 2 years ago Closed 2 years ago

[MSE] Shaka Player autoplay no longer works on ifinitytv.it and mediasetpremium.it streaming services in Firefox 55

Categories

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

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- fixed
firefox55 --- wontfix
firefox56 + fixed
firefox57 --- fixed

People

(Reporter: cpeterson, Assigned: jya)

References

Details

(Keywords: regression)

Attachments

(3 files)

Shaka Player autoplay no longer works on https://www.infinitytv.it/ and https://play.mediasetpremium.it/ streaming services.

STR:
1. Log into testmds5414.cloudapp.net.
2. Scroll down to "Max 2: White House Hero".
3. Hover over and click Max's play button.
4. The video screenshot pops up.
5. Click the "GUARDA DALL'INIZIO" button.

RESULT:
The video will buffer for a couple seconds and then either play or just keep buffering forever.

I bisected this regression to BBC autoplay bug 1382303. I had a hard time reproducing this regression reliably. I could reproduce it consistently in some Firefox user profiles but not others, even for the same Firefox version.
Jean-Yves, this autoplay regression was reported in the Shaka Player's issue tracker here:

https://github.com/google/shaka-player/issues/958
Flags: needinfo?(jyavenard)
I read the comment.

The fix in Firefox is sound.

If autoplay is active, playing event shouldn't be fired until the readyState is greater or equal than HAVE_FUTURE_DATA as per spec.

The only reason those sites can fail, is because they wait on an event that won't be fired until the readyState change. It worked before because those events (play, playing) were fired incorrectly.
Flags: needinfo?(jyavenard)
Jean-Yves, was https://github.com/google/shaka-player/issues/958#issuecomment-322014131 useful? Or should we move this to Tech Evangelism?
Flags: needinfo?(jyavenard)
I am currently investigating the issue based on a comment in the github entry, that is they had 15s of data
(https://github.com/google/shaka-player/issues/958#issuecomment-321926944)

which should have triggered the event.

However, at this stage, the new mochitest I wrote to ensure that our behaviour is indeed correct is working just fine.

The mochitest loads 10.01s of data and the events are properly fired as expected.
Flags: needinfo?(jyavenard)
Comment on attachment 8897938 [details]
Bug 1389844 - P1. Add mochitest.

https://reviewboard.mozilla.org/r/169228/#review174652
Attachment #8897938 - Flags: review?(gsquelart) → review+
Comment on attachment 8897939 [details]
Bug 1389844 - P2. Add Intervals::ContainsWithStrictEnd method.

https://reviewboard.mozilla.org/r/169230/#review174654
Attachment #8897939 - Flags: review?(gsquelart) → review+
Comment on attachment 8897940 [details]
Bug 1389844 - P3. Handle case where currentTime isn't contained in buffered range.

https://reviewboard.mozilla.org/r/169232/#review174658
Attachment #8897940 - Flags: review?(gsquelart) → review+
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5e67b69602da
P1. Add mochitest. r=gerald
https://hg.mozilla.org/integration/autoland/rev/68e2decc70c9
P2. Add Intervals::ContainsWithStrictEnd method. r=gerald
https://hg.mozilla.org/integration/autoland/rev/45a51fab5dbf
P3. Handle case where currentTime isn't contained in buffered range. r=gerald
[Tracking Requested - why for this release]: Prevent some site's using Google shaka's player to start playback.
Track 56+ as this will affect users of Google shaka player.
Blocks: 1391666
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 1391666 is required.
Attachment #8897938 - Flags: approval-mozilla-esr52?
Attachment #8897938 - Flags: approval-mozilla-beta?
Comment on attachment 8897938 [details]
Bug 1389844 - P1. Add mochitest.

Sounds like this may affect many users, let's uplift all of this for beta 6. 
I think best to leave it out of ESR though as it is not a sec issue, not fixing a top crash, and we are heading into 52.4.0esr.
Attachment #8897938 - Flags: approval-mozilla-esr52?
Attachment #8897938 - Flags: approval-mozilla-esr52-
Attachment #8897938 - Flags: approval-mozilla-beta?
Attachment #8897938 - Flags: approval-mozilla-beta+
Why the won't fix on esr52? The code that cause this regression was very recently uplifted to esr52.
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(lhenry)
Liz, I think you should reconsider, it's a very recent regression in esr52, one introduced last month.
(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.
Flags: qe-verify-
Julien, what do you think? (Since you are ESR owner for this cycle)
Flags: needinfo?(lhenry) → needinfo?(jcristau)
I think I would prefer backing out the changes in bug 1382303 to taking these new ones.  From https://bugzilla.mozilla.org/show_bug.cgi?id=1382303#c24 it sounds like the BBC had a workaround for that bug.
Fixed on ESR52 by backing out bug 1382303 at Julien's request.

https://hg.mozilla.org/releases/mozilla-esr52/rev/f01155fe4d54
Flags: needinfo?(jcristau)
You need to log in before you can comment on or make changes to this bug.