Closed Bug 1611622 Opened 6 years ago Closed 5 years ago

LEGO SPEED CHAMPIONS game is unplayable on Firefox Nightly

Tracking

()

Status:

VERIFIED FIXED

Milestone:

mozilla77

Tracking Flags:

Tracking

Status

firefox-esr68

---

wontfix

firefox74

---

wontfix

firefox75

---

wontfix

firefox76

---

wontfix

firefox77

---

fixed

People

(Reporter: alex_mayorga, Assigned: padenot)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1611622 - Add some logging to MediaBufferDecoder.cpp. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta-	Details \| Review
Bug 1611622 - Add some logging to VorbisDecoder.cpp. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta-	Details \| Review
Bug 1611622 - Don't error out if decoding fails on a block. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request		Details \| Review
Bug 1611622 - Allow nil packets in OGG demuxer. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta-	Details \| Review
Bug 1611622 - Allow testing for a decode success without checking the file. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta-	Details \| Review
Bug 1611622 - Add a decode test for an OGG Vorbis file with a nil packet at EOS. r?bryce 5 years ago Paul Adenot (:padenot) 47 bytes, text/x-phabricator-request	RyanVM : approval-mozilla-beta-	Details \| Review

alex_mayorga

Reporter

Description

•

6 years ago

¡Hola!

My son tried playing LEGO® LEGO SPEED CHAMPIONS game over at https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/index3.html

The game is unplayable on Firefox Nightly built from https://hg.mozilla.org/mozilla-central/rev/fb6b61e49217d835b2d6e435560424aab10d5475

¡Gracias!
Alex

Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)

Updated

•

6 years ago

Component: Untriaged → Performance

Product: Firefox → Core

Daniel Holbert [:dholbert]

Comment 1

•

5 years ago

Hi Alex!

Does this feel like it's the same underlying issue as bug 1611621, or are the symptoms substantially different to merit separate investigation/tracking?

(If it's the same issue, we should perhaps just mark this one as a duplicate, to avoid folks accidentally doing redundant investigation/work on the two bugs independently...)

Flags: needinfo?(alex_mayorga)

alex_mayorga

Reporter

Comment 2

•

5 years ago

¡Hola Daniel!

They're separate issues. This one won't play at all upon loading. The other one plays but the performance is really bad.

I believe them to be separate and distinct bugs.

The game plays correctly on Microsoft Edge 82.0.432.0

¡Gracias!
Alex

Flags: needinfo?(alex_mayorga)

Mike Conley (:mconley) (:⚙️)

Comment 3

•

5 years ago

Hi alex_mayorga, when you say that it's unplayable, what does it show instead? Is there an error message? Anything in the DevTools console?

Flags: needinfo?(alex_mayorga)

alex_mayorga

Reporter

Comment 4

•

5 years ago

¡Hola Mike!

Here are more detailed steps, hope these are helpful.

¡Gracias!
Alex

Steps:
Load https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/index3.html
Wait a bit for the game to load
Click on "TAP TO START"
Click the black on yellow ">" button in the "TRACK SELECT" screen
Click the black on yellow ">" button in the "VEHICLE SELECT" screen
Wait for the "3, 2, 1, GO!" countdown

Actual results:
The clock doesn't start running
The car doesn't move
Clicking on the arrow button and pause buttons have no effect

Expected results:
The game plays correctly

Please find below what the console logs.

status-firefox75: --- → affected

Flags: needinfo?(alex_mayorga)

Daniel Holbert [:dholbert]

Comment 5

•

5 years ago

Thanks for clarifying. I can reproduce those results, and in fact I see the same behavior on Microsoft Edge 18 (pre-Chromium). Haven't tested post-Chromium Edge, but that's proof enough that this isn't a firefox-specific bug.

(1) I don't think this is performance-related, so it doesn't belong in Core|Performance.
(2) I think the relevant bit of console logging is this (and, importantly, I see a version of this same error in Edge as well):

TypeError: can't access property "playbackRate", engineSFX._sound is null EasyTrack2.js:1710:9

It looks like the site's own JS is ending up with a null variable and it's not set up to handle that. Not sure why that doesn't happen in Chrome (could be any of a number of things -- using a Chrome-specific API, or a race condition in the site's code that they happen to "win" in Chrome, or UA-sniffing, or something else).

So, this seems like a bug in the site itself. Reclassifying as WebCompat.

Component: Performance → Desktop

Product: Core → Web Compatibility

Version: 74 Branch → unspecified

Daniel Holbert [:dholbert]

Comment 6

•

5 years ago

I poked around at the variable in question (engineSFX._sound) in our JS debugger, and I confirmed that it's indeed null in Firefox vs. non-null in Chrome, because of the site's phaser.js encountering a different value of the "isSoundDecoded" function.

This happens when EasyTrack2.js calls engineSFX.loopFull(0.5). That gets us to line 76303 in the phaser.js script, which is the "isSoundDecoded" check here, with this.key = carloop.

        if (this.usingWebAudio)
        {
            //  Does the sound need decoding?
            if (this.game.cache.isSoundDecoded(this.key))
            {
                this._sound = this.context.createBufferSource();

In chrome, isSoundDecoded returns true here. In Firefox, it returns false. This means the _sound member gets assigned in Chrome but never gets assigned (to anything non-null) in Firefox (and presumably Edge18 hits the same issue).

And this probably all traces back to this earlier decoding error from Alex Mayorga's above-pasted console output:

The buffer passed to decodeAudioData contains invalid content which cannot be decoded successfully.

Mike Taylor [:miketaylr]

Comment 7

•

5 years ago

If I break inside the callback to decodeAudioData:

try {
  this.context.decodeAudioData(soundData, function (buffer) {

  if (buffer)
    {
     _this.game.cache.decodedSound(key, buffer);
     _this.onSoundDecode.dispatch(key, sound);
    }
  });
}
  catch (e) {}
}

The AudioBuffer object looks like this:

duration: 0.9547083333333334
length: 45826
numberOfChannels: 2
sampleRate: 48000

We also get the following console error when breaking EncodingError: The given encoding is not supported..

Daniel Holbert [:dholbert]

Comment 8

•

5 years ago

ni'ing ehsan since there's WebAudio stuff going on here... (i.e. is this something we should be successfully decoding, or is Chrome doing something shady by decoding it properly, or are both behaviors "acceptable" and the site is just being overly strict in its assumptions of decodability?)

ehsan, do you know if we can find out if there is in fact corrupt data here, and if we should in fact be decoding it properly?

(Feel free to redirect the needinfo if there's someone else who could also reason about WebAudio stuff, too.)

Flags: needinfo?(ehsan)

Daniel Holbert [:dholbert]

Comment 9

•

5 years ago

•

Edited

For anyone investigating this -- the JS source here is pretty inspectable / debuggable, fortunately. The relevant load is kicked off in phaser.js, line 70681, when key is "carloop" https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/lib/phaser/build/phaser.js

And I think the resource in question seems to be https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/assets/Sfx/CarLoop.ogg which we seem to be able to play just fine, if I download it and load it in Firefox. (Though maybe there's other external audio/encoding data as well which is causing the problem...)

Andrea Marchesini [:baku]

Comment 10

•

5 years ago

Ehsan is on PTO. Maybe Paul can help here.

Flags: needinfo?(ehsan) → needinfo?(padenot)

Paul Adenot (:padenot)

Assignee

Comment 11

•

5 years ago

This errors out in vorbis_synthesis here: https://searchfox.org/mozilla-central/source/dom/media/platforms/agnostic/VorbisDecoder.cpp#161 on seemingly the very last packet. I haven't dug into this function to understand why it errors out.

The reason why this site is broken here is because in Firefox, a decodeAudioData call succeeds or fail, there is no notion of "partial success". It does the same error when played back with an HTMLMediaElement, but unlike in the Web Audio API case, every successfully decoded packet is queued and played back, even if there is an error down the line.

It sounds to me that we should proceed in the same way, this vorbis file might be invalid, but is played correctly by all software I've tried. I don't know if this is something that would be easy to do in practice. I don't know what to do with the error.

Flags: needinfo?(padenot)

Paul Adenot (:padenot)

Assignee

Comment 12

•

5 years ago

Attached file Bug 1611622 - Add some logging to MediaBufferDecoder.cpp. r?bryce — Details

Phabricator Automation

Updated

•

5 years ago

Assignee: nobody → padenot

Status: NEW → ASSIGNED

Paul Adenot (:padenot)

Assignee

Comment 13

•

5 years ago

Attached file Bug 1611622 - Add some logging to VorbisDecoder.cpp. r?bryce — Details

Paul Adenot (:padenot)

Assignee

Comment 14

•

5 years ago

Attached file Bug 1611622 - Don't error out if decoding fails on a block. r?bryce (obsolete) — Details

Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail

Comment 15

•

5 years ago

There's a few weird things happening when I test this.

We trigger an assertion by allocating a 0 size shmem.
The data we get in the decode function is NULL, so the vorbis decoder returns -135/OV_ENOTAUDIO because we've given it nothing.
This appears to be an old regression in the range https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=286f71223256cbb3a769432fd860f563c4886e81&tochange=bdb2387396b4a74dfefb7c983733eed3625e906a

IMO this is an issue earlier in the pipeline, we shouldn't be passing NULL to a decoder, and should locate the earlier code that is doing so and see if we can remedy the issue there.

Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail

Comment 16

•

5 years ago

After investigating a bit more.

The OggDemuxer is outputting a sample with an empty buffer.
As Paul indicated, you can see this if you playback the file in question (https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/assets/Sfx/CarLoop.ogg) in an HTMLMediaElement.
Dumping that file with oggz tools via oggz-dump shows

<snip>

00:00:02.238: serialno 0435311790, granulepos 98707, packetno 113 *** eos: 0 bytes

i.e. the last packet in the file is empty.

I don't know the ogg format that well. My thought would be that when getting samples from our packets we can discard empty ones and return null (somewhere around https://searchfox.org/mozilla-central/rev/72e3388f74458d369af4f6cdbaeaacb719523b8c/dom/media/ogg/OggDemuxer.cpp#1236). Paul, kinetik, do you know the container? Does that sound sane? It appears to fix this issue.

Flags: needinfo?(kinetik)

Matthew Gregan [:kinetik]

Updated

•

5 years ago

Comment 17

•

5 years ago

I believe the file is valid. Per RFC3533: "Eos pages may be 'nil' pages, that is, pages containing no content but simply a page header with position information and the eos flag set in the page header." In this case the EOS flag could be set on the previous packet IIUC, but I can imagine it's simpler to write a muxer that emits a zero-byte EOS packet after it's told the stream has ended.

From memory, this used to work correctly (but apparently isn't covered by tests) so knowing which change regressed this would be helpful. It might be as simple as special casing MediaRawData.mEOS == true && MediaRawData->Size() == 0 to signal decoding is complete with no error.

Matthew Gregan [:kinetik]

Updated

•

5 years ago

Flags: needinfo?(kinetik)

Phabricator Automation

Updated

•

5 years ago

Attachment #9140491 - Attachment is obsolete: true

Paul Adenot (:padenot)

Assignee

Comment 18

•

5 years ago

Attached file Bug 1611622 - Allow nil packets in OGG demuxer. r?bryce — Details

Paul Adenot (:padenot)

Assignee

Comment 19

•

5 years ago

Attached file Bug 1611622 - Allow testing for a decode success without checking the file. r?bryce — Details

Useful to test the demuxer in the web audio case.

Paul Adenot (:padenot)

Assignee

Comment 20

•

5 years ago

Attached file Bug 1611622 - Add a decode test for an OGG Vorbis file with a nil packet at EOS. r?bryce — Details

The test file was created using VLC, that has a muxer that does this.

Pulsebot

Comment 21

•

5 years ago

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1db1f7da1215 Add some logging to MediaBufferDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/7f14a871f69e Add some logging to VorbisDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/ca1198a5d65c Allow nil packets in OGG demuxer. r=bryce https://hg.mozilla.org/integration/autoland/rev/e6286802e8b5 Allow testing for a decode success without checking the file. r=bryce https://hg.mozilla.org/integration/autoland/rev/8be308a0ff7d Add a decode test for an OGG Vorbis file with a nil packet at EOS. r=bryce

Alexandru Michis [:malexandru]

Comment 22

•

5 years ago

Backed out 5 changesets (Bug 1611622) for causing bustages.

Backout link: https://hg.mozilla.org/integration/autoland/rev/def5136392242e98b4b58b42d6b444d78a04bffc

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&tochange=def5136392242e98b4b58b42d6b444d78a04bffc&fromchange=8be308a0ff7d405d879d015142acffcd06b99e3d

Failure logs:

Flags: needinfo?(padenot)

Pulsebot

Comment 23

•

5 years ago

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/adab60e7e9c5 Add some logging to MediaBufferDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/daf821043996 Add some logging to VorbisDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/1177ae3a0ef3 Allow nil packets in OGG demuxer. r=bryce https://hg.mozilla.org/integration/autoland/rev/87254fc7b777 Allow testing for a decode success without checking the file. r=bryce https://hg.mozilla.org/integration/autoland/rev/f4a83f3ac965 Add a decode test for an OGG Vorbis file with a nil packet at EOS. r=bryce

Noemi Erli[:noemi_erli]

Comment 24

•

5 years ago

Backed out 5 changesets (Bug 1611622) for causing failures in test_mediaDecoding.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=298485341&resultStatus=testfailed%2Cbusted%2Cexception&revision=f4a83f3ac965ef8b8090f617e8032b2bfa916b31

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=298485329&repo=autoland&lineNumber=1130

Backout: https://hg.mozilla.org/integration/autoland/rev/a5ccdb585a8edfc8302dc83d495904693fcf3ea4

Paul Adenot (:padenot)

Assignee

Comment 25

•

5 years ago

Hrm so apparently phabsend failed to send the binary file.

Flags: needinfo?(padenot)

Paul Adenot (:padenot)

Assignee

Comment 26

•

5 years ago

Ok, green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe15aa79934f6c8cb12251795d4b21d36be23ee9

Pulsebot

Comment 27

•

5 years ago

Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d0829114d218 Add some logging to MediaBufferDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/4209bc1f48f6 Add some logging to VorbisDecoder.cpp. r=bryce https://hg.mozilla.org/integration/autoland/rev/0e25f0be7897 Allow nil packets in OGG demuxer. r=bryce https://hg.mozilla.org/integration/autoland/rev/aeef8770688c Allow testing for a decode success without checking the file. r=bryce https://hg.mozilla.org/integration/autoland/rev/917b18a41085 Add a decode test for an OGG Vorbis file with a nil packet at EOS. r=bryce

Arthur Iakab [arthur_iakab]

Comment 28

•

5 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/d0829114d218
https://hg.mozilla.org/mozilla-central/rev/4209bc1f48f6
https://hg.mozilla.org/mozilla-central/rev/0e25f0be7897
https://hg.mozilla.org/mozilla-central/rev/aeef8770688c
https://hg.mozilla.org/mozilla-central/rev/917b18a41085

Status: ASSIGNED → RESOLVED

Closed: 5 years ago

status-firefox77: --- → fixed

Resolution: --- → FIXED

Paul Adenot (:padenot)

Assignee

Comment 29

•

5 years ago

Comment on attachment 9141300 [details]
Bug 1611622 - Allow nil packets in OGG demuxer. r?bryce

Beta/Release Uplift Approval Request

User impact if declined: Firefox rejects legitimate OGG files in use on the web, and sites appear to not work. Unfortunately files that have this characteristic (a nil packet at the end) are what VLC outputs by default, and it's not a particularly obscure software.
Is this code covered by automated tests?: Yes
Has the fix been verified in Nightly?: Yes
Needs manual test from QE?: No
If yes, steps to reproduce: Very easy to repro, but also covered by a test now: load https://www.lego.com/assets/FranchiseSites/SpeedChampions/html5game/v10/index3.html, open the console, try to play: it doesn't work without this patch, and there is an error in the console. It works well with this patch
List of other uplifts needed: None
Risk to taking this patch: Low
Why is the change risky/not risky? (and alternatives if risky): Well covered by tests (I created a special test file to catch the regression), the OGG spec clearly states that the very specific case we are adding here is legal (and we used to erroneously reject files like this).

This just adds a test in an if, and is basically a translation of the OGG spec in C++. This new handling only affects those kind of files.

The other patches in this bug are more logs and testing, which don't affect the final binary, and aren't needed to fix this, and this patch can be landed cleanly without the others.

String changes made/needed:

Attachment #9141300 - Flags: approval-mozilla-beta?

Paul Adenot (:padenot)

Assignee

Updated

•

5 years ago

Attachment #9140488 - Flags: approval-mozilla-beta?

Attachment #9140489 - Flags: approval-mozilla-beta?

Attachment #9141301 - Flags: approval-mozilla-beta?

Attachment #9141302 - Flags: approval-mozilla-beta?

Ryan VanderMeulen [:RyanVM]

Comment 30

•

5 years ago

Comment on attachment 9140488 [details]
Bug 1611622 - Add some logging to MediaBufferDecoder.cpp. r?bryce

With 76 going to RC on Monday, I think this is too much to take this late in the Beta cycle, especially since this isn't a new issue. Let's let this ride 77 to release so it can get more bake time before shipping.

Attachment #9140488 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Attachment #9140489 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Attachment #9141300 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Attachment #9141301 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Attachment #9141302 - Flags: approval-mozilla-beta? → approval-mozilla-beta-

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox74: affected → wontfix

status-firefox75: affected → wontfix

status-firefox76: --- → wontfix

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

status-firefox-esr68: --- → wontfix

Component: Desktop → Web Audio

Product: Web Compatibility → Core

Target Milestone: --- → mozilla77

Ryan VanderMeulen [:RyanVM]

Updated

•

5 years ago

Flags: in-testsuite+

alex_mayorga

Reporter

Comment 32

•

5 years ago

¡Hola Paul!

Thanks for fixing this.

I'm happy to report that my son was able to play this game now on Firefox Nightly built from https://hg.mozilla.org/mozilla-central/rev/e803948bb3cd52dae5566bfc517e3d04f06252e7

¡Gracias!
Alex

Status: RESOLVED → VERIFIED

You need to log in before you can comment on or make changes to this bug.