864780 - decodeAudioData fails to decode a wave file

Status: RESOLVED FIXED

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

Description

[(no longer active)]

Joe sent me a test case, and I can reproduce locally.  Need to investigate what's going on...

Comment 1

[(no longer active)]

Test case: http://people.mozilla.org/~eakhgari/webaudio/decode-864780.html

Comment 2

[(no longer active)]

So what happens here is that we fail to read chunks out of the wave file here: <http://mxr.mozilla.org/mozilla-central/source/content/media/wave/WaveReader.cpp#406>.  Here, aChunkSize is 20 and extra is 0.  It seems like this wave file is kind of invalid, but I don't know enough about this code to see how we can work around it.

Note that decoding this wave file through an <audio> element will result in the exact same failure.

Comment 3

[joe]

FYI this file comes from a commercial music sample provider and can be read into various popular audio applications successfully (Audacity, Kontakt Player, Cubase, etc.). I don't know what program was used to create it originally.

Comment 4

[(no longer active)]

Ralph, could you please look into this as well?  Thanks!

Comment 5

[(no longer active)]

Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.

Comment 6

[Maire Reavy [:mreavy]]

Ralph -- Have you had a chance to look into this?  I wonder how common this failure is.  (If it's common, I'd want to make this a P1.)  Thanks!

Comment 7

[Ralph Giles (:rillian)]

I haven't sorry. I don't think it can be common, but we should fix it. I'll take a look after the break.

Comment 8

[Ralph Giles (:rillian)]

Sorry I haven't been able to get to this. Best to give it to someone else.

Comment 9

[Jesse]

I ran into this problem with a wav file I was trying to use and determined that the issue was that the "fmt" chunk was larger than 16 bytes.  Here is the discussion on SO:
http://stackoverflow.com/questions/26169678/why-certain-wav-files-cannot-be-decoded-in-firefox

Here is my solution on jsfiddle:
http://jsfiddle.net/2hav4yLc/11/

Comment 10

[Cristiano Belloni]

Hello, if you need to have some wav file that won't decode in Firefox (but will anywhere else, included Chrome & Audacity), I have plenty.

Comment 11

[Cristiano Belloni]

Also, https://wav.hya.io links at it.

Comment 12

[Chinmay Pendharkar]

(In reply to Cristiano Belloni from comment #10)
> Hello, if you need to have some wav file that won't decode in Firefox (but
> will anywhere else, included Chrome & Audacity), I have plenty.

Christiano: Can I have such files to add them to my decoder testing tool? http://chinpen.net/decodethis

Comment 13

[Maire Reavy [:mreavy]]

Hey Anthony -- This bug (and the linked bug 1074141) are both issues with our wav reader not handling wav files that various tools generate, and most audio tools can read with no problems.  For example, https://wav.hya.io tells people basically to use chrome.  I'm moving this to playback as it fails when these files are fed directly to an <audio> element.

Note also the stackoverflow page linked to from the other bug.

Comment 14

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

Anyone want to attach an example WAV file to this bug.

Comment 15

[Chinmay Pendharkar]

Hello. I have a online suite of files which I use to check for browser codec compatibility. It uses the decodeAudioData in WebAudio. http://chinpen.net/decodethis/ 

All the audio files are here: https://github.com/notthetup/decodethis/tree/gh-pages/public/audio 

One of the wav files that fails for sure (it's 24bit) is https://raw.githubusercontent.com/notthetup/decodethis/gh-pages/public/audio/tom-1.wav

Comment 16

[Louis Christie [:lchristie]]

Attachment: Added support for 24 bit wave files

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f084eea28b9

Comment 18

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

Comment on attachment 8689844 [details] [diff] [review]
Added support for 24 bit wave files

Review of attachment 8689844 [details] [diff] [review]:
-----------------------------------------------------------------

Please encode a 24bit wav file, place it in dom/media/test/, add it to dom/media/test/mochitest.ini, and add it to gPlayTests in dom/media/manifest.js so that when you run `./mach mochitest dom/media/test/test_playback.html` we also hit your code. You need to `hg add $path/to/your/new/file` in order to include it in your patch.

Also, Ehsan's original testcase that this bug was reported for still doesn't play. Can you figure that out?
http://people.mozilla.org/~eakhgari/webaudio/decode-864780.html

::: dom/media/wave/WaveReader.cpp
@@ +200,5 @@
> +
> +template <> inline int16_t
> +Signed24bIntToAudioSample<int16_t>(int32_t aValue)
> +{
> +  return aValue * 0x7fff / 0x800000;

I'm not sure how this works. How does it work?

You can download the Firefox for Android build on your try push and install that on your Android phone to test that 24bit wave files plays when this code is being used.

Comment 19

[Louis Christie [:lchristie]]

The original test fails because of an unexpected case in the metadata.

Test Case:
00000000  52 49 46 46  0e 7e 01 00  57 41 56 45  66 6d 74 20  |RIFF.~..WAVEfmt |
00000010  14 00 00 00  01 00 01 00  44 ac 00 00  88 58 01 00  |........D....X..|
00000020  02 00 10 00 >00 00 00 00< 64 61 74 61  a2 7d 01 00  |........data.}..|
00000030  00 00 fe ff  fd ff fa ff  f7 ff f6 ff  f4 ff f7 ff  |................|

Expected Metadata Example:
00000000  52 49 46 46  46 56 00 00  57 41 56 45  66 6d 74 20  |RIFFFV..WAVEfmt |
00000010  10 00 00 00  01 00 01 00  11 2b 00 00  22 56 00 00  |.........+.."V..|
00000020  02 00 10 00  64 61 74 61  22 56 00 00  ff 7f 00 80  |....data"V......|
00000030  00 00 00 80  00 80 00 80  00 80 00 80  00 80 00 80  |................|

The decoder expects the first two bytes to tell it how far it should skip, so the bytes within > < would read 02 00 so that it would skip the other two bytes. This is not a problem with 24bit PCM, just with a specific format chunk.

Comment 20

[Louis Christie [:lchristie]]

Attachment: bug-864780-fix.patch

Comment 21

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

Comment on attachment 8690605 [details] [diff] [review]
bug-864780-fix.patch

Review of attachment 8690605 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/wave/WaveReader.cpp
@@ +431,5 @@
>      extra += extra % 2;
>  
> +    static_assert(UINT16_MAX + (UINT16_MAX % 2) < UINT_MAX / sizeof(char),
> +                  "chunkExtension array too large for iterator.");
> +    nsAutoArrayPtr<char> chunkExtension(new char[extra]);

You're allocating memory based on an unsanitized value read from a file. In this case the allocation is it's limited to 2^16 bytes, which isn't big enough to be too dangerous, but it's still bad practice as a maliciously crafted file could exploit this.

Since we're not doing anything with the data, there seems little point in reading/copying it. So instead of allocating memory and reading into it, you should skip over the data so that the next read start after it. Do this by adding:

if (NS_FAILED(mResource.Seek(nsISeekableStream::NS_SEEK_CUR, extra))) {
  return false;
}

This seeks the read cursor forward "extra" bytes ahead of the current read cursor position, and returns false (i.e. indicates failure) if the seek fails.

Comment 22

[Louis Christie [:lchristie]]

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

Comment 23

[Louis Christie [:lchristie]]

Attachment: bug-864780-fix.patch

Comment 24

[Louis Christie [:lchristie]]

Attachment: bug-864780-fix.patch

Comment 25

[Carsten Book [:Tomcat]]

sorry failed to apply:

applying bug-864780-fix.patch
patching file dom/media/test/manifest.js
Hunk #1 FAILED at 148
1 out of 1 hunks FAILED -- saving rejects to file dom/media/test/manifest.js.rej
patching file dom/media/wave/WaveReader.cpp
Hunk #1 FAILED at 421
1 out of 1 hunks FAILED -- saving rejects to file dom/media/wave/WaveReader.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-864780-fix.patch

Comment 26

[Louis Christie [:lchristie]]

Attachment: bug-864780-fix.patch

Comment 27

[Carsten Book [:Tomcat]]

Hi, this seems to have still problems - patching file dom/media/test/manifest.js
Hunk #1 FAILED at 148
1 out of 1 hunks FAILED -- saving rejects to file dom/media/test/manifest.js.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-864780-fix.patch

Comment 28

[Carsten Book [:Tomcat]]

(In reply to Carsten Book [:Tomcat] from comment #27)
> Hi, this seems to have still problems - patching file
> dom/media/test/manifest.js
> Hunk #1 FAILED at 148
> 1 out of 1 hunks FAILED -- saving rejects to file
> dom/media/test/manifest.js.rej
> patch failed, unable to continue (try -v)
> patch failed, rejects left in working directory
> errors during apply, please fix and qrefresh bug-864780-fix.patch

oh never mind, this depened on the other bug

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2dba896ba45b

Comment 30

[Carsten Book [:Tomcat]]

sorry had to back this out seems this or the other change caused test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18535643&repo=mozilla-inbound

Comment 31

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/16563b744913

Comment 32

[Louis Christie [:lchristie]]

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

Comment 33

[Louis Christie [:lchristie]]

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

Comment 34

[Louis Christie [:lchristie]]

Attachment: bug-864780-fix.patch

Comment 35

[Louis Christie [:lchristie]]

This patch needs to be applied after the patch uploaded to https://bugzilla.mozilla.org/show_bug.cgi?id=524109

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/27def71e2d0b

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/27def71e2d0b