1287397 - cannot play .wav file in new FF 47

Status: VERIFIED FIXED

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

Description

[K Stoilkov]

Attachment: sample audio file

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160623154057

Steps to reproduce:

We have an audio stream that comes from IBM Watson TTS. Stream is converted on a server into a .wav format audio file and then returned to the Html page (where we want to play it), as a response with a file attachment. 
Content-Disposition: attachment; filename=tts.wav
Content-Type: application/octet-stream
We use an Audio element in the html page.


Actual results:

Until now, up to Firefox 45 we can play the audio, but with version 47 and 47.0.1 we get an error: MediaError
https://gyazo.com/2f0a05af47dad2efa7f65abb7c550dff


Expected results:

Like in the older version 45, we should be able to play the audio.

Comment 1

[Andre Klapper]

> Content-Type: application/octet-stream

Have you already tried setting the correct MIME type instead of "application/octet-stream"?

Is there a minimum, self-contained, public testcase with the exact and complete code delivered to the browser?

Comment 2

[K Stoilkov]

I don't have any sample code or test cases. I did try yo set the content-type to "audio/wav", but same result. I do know that the attachment is then handled as a Blob on the front end.
If you tell what specific information you need I can try to give it to you. I have another screenshot that probably won't help at all, but just in case
http://imgur.com/a/MtLmc

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: testcase

Comment 4

[Karl Tomlinson (:karlt)]

Expect button to change to "Play" oncanplaythrough.
Actual button remains as "loading".

Comment 5

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1493196bb7a0fb7f98937c455f276098e917ad6a&tochange=8ef6378d6a59103b2dbb504c7a642a9230cb8920

Regressed by: Bug 1231793

Comment 6

[Alice0775 White]

[Tracking Requested - why for this release]: break web site.

Comment 7

[Louis Christie [:lchristie]]

The problem is that the metadata states that the data chunk length is 0xffffffff:

00000000  52 49 46 46 ff ff ff ff  57 41 56 45 66 6d 74 20  |RIFF....WAVEfmt |
00000010  10 00 00 00 01 00 01 00  22 56 00 00 44 ac 00 00  |........"V..D...|
00000020  02 00 10 00 4c 49 53 54  1a 00 00 00 49 4e 46 4f  |....LIST....INFO|
00000030  49 53 46 54 0e 00 00 00  4c 61 76 66 35 36 2e 31  |ISFT....Lavf56.1|
00000040  38 2e 31 30 30 00 64 61  74 61 ff ff ff ff 00 00  |8.100.data......|

so in dom/media/wave/WaveDemuxer.cpp:119 it overflows to read aChunkSize = 0. This causes the mDataLength to be set at 0, so the duration is set at 0 too, sending the error in metadata.

Comment 8

[Louis Christie [:lchristie]]

Attachment: bug-1287397-fix.patch

Could be fixed with something like this, although I haven't tested it on try or een run local mochitests.

Comment 9

[Marcia Knous [:marcia]]

Tracking 49/50+ for this audio regression, since it is something that used to work and now doesn't.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

We are shipping 48 with it.

Comment 11

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=738f62b8af35

Comment 12

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Bug 1287397 - Fix wave chunk size overflow;

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

Comment 13

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

Comment on attachment 8777621 [details]
Bug 1287397 - Fix wave chunk size overflow;

https://reviewboard.mozilla.org/r/69148/#review66984

::: dom/media/wave/WaveDemuxer.cpp:136
(Diff revision 1)
>        if (mFirstChunkOffset != mOffset) {
>          mFirstChunkOffset = mOffset;
>        }
>        break;
>      } else {
> -      mOffset += aChunkSize; // Skip other irrelevant chunks.
> +      mOffset += (aChunkSize + 1) & ~1; // Skip other irrelevant chunks.

It's non-obvious why we need to do this. Please add a comment to make it obvious *why* we need to do this.

Comment 14

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8777621 [details]
Bug 1287397 - Fix wave chunk size overflow;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69148/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7545a33148f1
Fix wave chunk size overflow; r=cpearce

Comment 16

[Gregory Szorc [:gps]]

I'm sorry, but I had to back this out for Valgrind test failures. e.g. https://public-artifacts.taskcluster.net/TGjgwxLFQSSDU1xSNjTLcg/0/public/logs/live_backing.log

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a3dd453166ab
Backed out changeset 7545a33148f1 for Valgrind failures

Comment 18

[Phil Ringnalda (:philor)]

That failure had nothing to do with this patch, you can just reland it.

Comment 19

[Gerry Chang [:gchang]]

Hi Anthony, this bug is assigned to nobody and it seems that you are involved in this, feel free to reassign it to someone else if you disagree.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbd63e535b53
Fix wave chunk size overflow. r=cpearce

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dbd63e535b53

Comment 22

[Gerry Chang [:gchang]]

This needs QE's help to verify.

Comment 23

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Anthony, should the fix be uplifted to Aurora50 and Beta49? It is a P1 regression.

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Comment on attachment 8777621 [details]
Bug 1287397 - Fix wave chunk size overflow;

Approval Request Comment
[Feature/regressing bug #]: 1231793
[User impact if declined]: Some WAV files will not play
[Describe test coverage new/current, TreeHerder]: No new test coverage
[Risks and why]: It may affect the way we calculate the number of samples in wrongly encoded WAV files by 1 sample. This is probably a fix rather than a bustage and probably isn't noticeable either way.
[String/UUID change made/needed]: none

Comment 25

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

Comment on attachment 8777621 [details]
Bug 1287397 - Fix wave chunk size overflow;

Fix for regression from 47. 
Assuming you want to also uplift to aurora. 
This can make the beta 8 build next Monday, I'd still like to verify it on Nightly though. Andrei can your team try it out?

Comment 26

[Andrei Vaida [:avaida]]

I've managed to reproduce this bug on on an affected build - 50.0a2
(2016-08-25), using the test case and instructions provided in Comment 3 and
Comment 4.

This is verified fixed on 51.0a1 (2016-08-25), using Windows 10 x64, Ubuntu
16.04 x64 and Mac OS X 10.9.5.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4cb3737983b1

Could we land a test that would have caught this regression?

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d1012aa4818

Comment 29

[Andrei Vaida [:avaida]]

This is verified fixed on:

   - 49.0b8-build1 (20160829102229)
   - 50.0a2 (2016-08-31)

as well, using the testcase from Comment 3 under Windows 10 x64, Mac OS X
10.10.5 and Ubuntu 14.04 x86 - the button's label now correctly states "Play".

Comment 30

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - can you write a mochitest for this?

Comment 31

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

Opened bug 1301226 to write mochitest.