decodeAudioData fails to decode a wave file

RESOLVED FIXED in Firefox 46

Status

()

P2
normal
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: lchristie)

Tracking

Trunk
mozilla46
x86
macOS
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Joe sent me a test case, and I can reproduce locally.  Need to investigate what's going on...
(Reporter)

Comment 2

6 years ago
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

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Ralph, could you please look into this as well?  Thanks!
Assignee: ehsan → giles
(Reporter)

Comment 5

6 years ago
Mass moving Web Audio bugs to the Web Audio component.  Filter on duckityduck.
Component: Video/Audio → Web Audio
(Reporter)

Updated

6 years ago
Whiteboard: [blocking-webaudio-]
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!
Flags: needinfo?(giles)
Priority: -- → P2
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.
Sorry I haven't been able to get to this. Best to give it to someone else.
Assignee: giles → nobody
Flags: needinfo?(giles)

Comment 9

5 years ago
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/
Blocks: 1074141
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.
Also, https://wav.hya.io links at it.
(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
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.
Component: Web Audio → Audio/Video: Playback
Flags: needinfo?(ajones)
Whiteboard: [blocking-webaudio-]
No longer blocks: 779297
Anyone want to attach an example WAV file to this bug.
Flags: needinfo?(ajones)
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
Summary: decodeAudioData fails to decode a wave file → decodeAudioData fails to decode a wave file (24 bit PCM)
(Assignee)

Updated

3 years ago
Assignee: nobody → lchristie
(Assignee)

Updated

3 years ago
Attachment #8689844 - Flags: review?(cpearce)
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.
Attachment #8689844 - Flags: review?(cpearce) → review-
(Assignee)

Comment 19

3 years ago
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.
(Assignee)

Comment 20

3 years ago
Posted patch bug-864780-fix.patch (obsolete) — Splinter Review
Attachment #8689844 - Attachment is obsolete: true
Attachment #8690605 - Flags: review?(cpearce)
(Assignee)

Updated

3 years ago
Summary: decodeAudioData fails to decode a wave file (24 bit PCM) → decodeAudioData fails to decode a wave file
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.
Attachment #8690605 - Flags: review?(cpearce) → review-
(Assignee)

Comment 23

3 years ago
Posted patch bug-864780-fix.patch (obsolete) — Splinter Review
Attachment #8690605 - Attachment is obsolete: true
Attachment #8697352 - Flags: review?(cpearce)
(Assignee)

Comment 24

3 years ago
Posted patch bug-864780-fix.patch (obsolete) — Splinter Review
Attachment #8697352 - Attachment is obsolete: true
Attachment #8697352 - Flags: review?(cpearce)
Attachment #8697358 - Flags: review?(cpearce)
Attachment #8697358 - Flags: review?(cpearce) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
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
Flags: needinfo?(lchristie)
Keywords: checkin-needed
(Assignee)

Comment 26

3 years ago
Posted patch bug-864780-fix.patch (obsolete) — Splinter Review
Attachment #8697358 - Attachment is obsolete: true
Flags: needinfo?(lchristie)
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
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
Flags: needinfo?(lchristie)
Keywords: checkin-needed
(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
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
(Assignee)

Comment 34

3 years ago
Attachment #8697906 - Attachment is obsolete: true
Flags: needinfo?(lchristie)
(Assignee)

Comment 35

3 years ago
This patch needs to be applied after the patch uploaded to https://bugzilla.mozilla.org/show_bug.cgi?id=524109
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 37

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/27def71e2d0b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.