881092 - Cannot decode wave file

Status: RESOLVED FIXED

(Core :: Web Audio, defect)

Description

[(no longer active)]

Chrome can decode this wave file without any problems...

Comment 1

[Ralph Giles (:rillian)]

This is a 4-channel 16-bit pcm wav file with no channel mapping.

We don't play it because we don't support multichannel output or stereo downmix for wav files.

How does chrome interpret the channels without a channel mapping?

Comment 2

[Ralph Giles (:rillian)]

http://chromium.googlecode.com/svn/trunk/samples/audio/impulse-responses/spreader50-65ms.wav looks like the same limitation.

Comment 4

[(no longer active)]

(In reply to comment #1)
> This is a 4-channel 16-bit pcm wav file with no channel mapping.
> 
> We don't play it because we don't support multichannel output or stereo downmix
> for wav files.
> 
> How does chrome interpret the channels without a channel mapping?

I'm not sure, but you get an AudioBuffer if you pass this to AudioContext.decodeAudioData/createBuffer.

For Web Audio, we should not be attempting to map channels to anything, we should just give back the raw PCM version of each channel, I think.

Comment 5

[(no longer active)]

We need to fix this before we ship Web Audio, since convolution buffers may come from wave files with more than two channels.

Ralph, do you know what's going to be involved in fixing this bug?

Comment 6

[Ralph Giles (:rillian)]

Great, bugzilla outage cleared needinfo? but lost my comment.

Ehsan, to fix this properly we need to support multichannel playback. How important is the 4-channel ConvolverNode buffer? That's a more invasive change than I'd like to make to Aurora.

Currently we downmix multichannel Opus files in the decoder and reject all other multichannel files. We'd need to move the downmix code to the audio output layer. Bug 709327 is the tracking bug; it's been low priority because game dev feedback was that actual surround output wasn't important.

Then we have the issue of channel map. This file *should* fail in normal <audio> playback because there's no channel map, thus no way to play directly. Maybe we could plumb a flag which just asks the decoder to return multichannel data regardless for webaudio purposes?

Comment 7

[(no longer active)]

(In reply to Ralph Giles (:rillian) from comment #6)
> Great, bugzilla outage cleared needinfo? but lost my comment.
> 
> Ehsan, to fix this properly we need to support multichannel playback. How
> important is the 4-channel ConvolverNode buffer? That's a more invasive
> change than I'd like to make to Aurora.
> 
> Currently we downmix multichannel Opus files in the decoder and reject all
> other multichannel files. We'd need to move the downmix code to the audio
> output layer. Bug 709327 is the tracking bug; it's been low priority because
> game dev feedback was that actual surround output wasn't important.

(Wrong bug #?)

Hmm, I was hoping to not require multi-channel playback as part of this.  Can't we just special case the decoding done by the AudioContext to decode each channel as it is, bypassing the downmix spec

> Then we have the issue of channel map. This file *should* fail in normal
> <audio> playback because there's no channel map, thus no way to play
> directly. Maybe we could plumb a flag which just asks the decoder to return
> multichannel data regardless for webaudio purposes?

Yeah, that's exactly what I would hope that we should do here.  Is that easier than the multichannel playback you mentioned above?

Comment 8

[Ralph Giles (:rillian)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #7)

> > Then we have the issue of channel map. This file *should* fail in normal
> > <audio> playback because there's no channel map, thus no way to play
> > directly. Maybe we could plumb a flag which just asks the decoder to return
> > multichannel data regardless for webaudio purposes?
> 
> Yeah, that's exactly what I would hope that we should do here.  Is that
> easier than the multichannel playback you mentioned above?

Yes, that's easier. I'd recommend that unless Paul particular wants to fix multichannel playback in the timeframe we're looking at.

Comment 9

[Paul Adenot (:padenot)]

That's the approach I'm going to take, yes. It's going to be somewhat hacky, but well...

Comment 10

[Paul Adenot (:padenot)]

Attachment: Allow decoding files we know we can't play, in the context of WebAudio. r=

Chris, could you look at this one? The idea here is that we want to be able to
decode 4 channel files for WebAudio, and the WaveReader currently errors out
while reading the metadata, when the file has four channels.

Comment 11

[Paul Adenot (:padenot)]

Attachment: Test that we can decode multichannel WAVE file only using decodeAudioData. r=

Test for multichannel file decoding.

This tests both that WebAudio can decode the file, and <audio> cannot.

Comment 12

[Ralph Giles (:rillian)]

Comment on attachment 796762 [details] [diff] [review]
Allow decoding files we know we can't play, in the context of WebAudio. r=

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

Looks good to me.

::: content/media/MediaDecoderReader.h
@@ +467,5 @@
>    // decode thread could start up and run in future.
>    virtual void OnDecodeThreadFinish() {}
>  
> +  // Tell the reader that the data decoded are not for direct playback, so it
> +  // can accept more file, in particular those which have more channels than

"...accept more files."

@@ +554,5 @@
> +
> +  // Whether we should accept media that we know we can't play
> +  // directly, because they have a number of channel higher than
> +  // what we support.
> +  bool mIgnoreAudioOutputFormat;

I would have called it mDecodeAllChannels, but that's not any shorter.

::: content/media/wave/WaveReader.cpp
@@ +61,5 @@
>  // supported by AudioStream.
>  static const uint16_t WAVE_FORMAT_ENCODING_PCM = 1;
>  
> +// Maximum number of channels supported by the audio output code. We return with
> +// an error now if we are going to play back this media directly.

"We reject files with more than this number of channels if we're decoding for playback."

@@ +436,4 @@
>    unsigned int actualFrameSize = sampleFormat == 8 ? 1 : 2 * channels;
>    if (rate < 100 || rate > 96000 ||
> +      (((channels < 1 || channels > MAX_CHANNELS) ||
> +       (frameSize != 1 && frameSize != 2 && frameSize != 4)) &&

I think the direct frameSize checks are covered by the 'frameSize != actualFrameSize' check below, and technically it's wrong if MAX_CHANNELS isn't 2, but that's a separate bug. When we add multichannel playback it will show up soon enough, so I wouldn't worry about fixing it.

Comment 13

[Ralph Giles (:rillian)]

Comment on attachment 796764 [details] [diff] [review]
Test that we can decode multichannel WAVE file only using decodeAudioData. r=

What happens if both SimpleText.finish() calls in decodeUsingAudioElement are invoked?

Comment 14

[(no longer active)]

Comment on attachment 796762 [details] [diff] [review]
Allow decoding files we know we can't play, in the context of WebAudio. r=

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

::: content/media/webaudio/MediaBufferDecoder.cpp
@@ +512,5 @@
>  
> +  // Tell the decoder reader that we are not going to play the data directly,
> +  // and that we should not reject files with more channels than the audio
> +  // bakend support.
> +  mDecoderReader->SetIgnoreAudioOutputFormat();

Shouldn't we technically call this before OnDecodeThreadStart()?

Comment 15

[(no longer active)]

Comment on attachment 796764 [details] [diff] [review]
Test that we can decode multichannel WAVE file only using decodeAudioData. r=

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

r=me sans the double finish bug that Ralph pointed out.

Comment 16

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

Comment on attachment 796762 [details] [diff] [review]
Allow decoding files we know we can't play, in the context of WebAudio. r=

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

Chris Double wrote our wave decoder, he should be the one reviewing this.

Comment 17

[cajbir (:cajbir)]

Comment on attachment 796762 [details] [diff] [review]
Allow decoding files we know we can't play, in the context of WebAudio. r=

Matthew Gregan was the original author of the wave decoder.

Comment 18

[Matthew Gregan [:kinetik]]

Comment on attachment 796762 [details] [diff] [review]
Allow decoding files we know we can't play, in the context of WebAudio. r=

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

To clarify: the Wave decoder is limited to mono or stereo because it only supports the original Wave header and has no support for WAVE_FORMAT_EXTENSIBLE.  Supporting multichannel files reliably will require extending WaveReader to support WAVE_FORMAT_EXTENSIBLE.

Comment 19

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09a51539cea1
https://hg.mozilla.org/integration/mozilla-inbound/rev/00611894f123

Comment 20

[Ed Morley [:emorley]]

Backed out for breaking the build:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27165267&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=27165241&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5e992506c2a0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ec9bf60a4b2e

Comment 21

[Paul Adenot (:padenot)]

Re-pushed with actually valid code.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1d14cd9307c3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/807e7c9465eb

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d14cd9307c3
https://hg.mozilla.org/mozilla-central/rev/807e7c9465eb

Comment 23

[Paul Adenot (:padenot)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df1504b82610
https://hg.mozilla.org/releases/mozilla-aurora/rev/1a1aeff8ab00

Comment 24

[Manuela Muntean [Away]]

Attachment: ScreenShot.png

While testing for the pre-beta sign-off of this feature, I tried to verify this bug, so here are my questions and results:

- both http://chromium.googlecode.com/svn/trunk/samples/audio/impulse-responses/spatialized5.wav and the link from comment 2, work with latest Aurora (build ID: 20130902004002) on a Mac OSX 10.9  in 32bit mode machine, but don't work with Chrome (please see the attached screenshot)

Does anyone have any suggestions/thoughts? Thanks!

Comment 25

[u279076]

Paul, can you please answer Manuela's question in comment 24?

Comment 26

[Ralph Giles (:rillian)]

I'm not Paul, but the screenshot from comment 24 shows playback in the quicktime plugin, which isn't what this bug is about. You need to verify that 4-channel wav files can be passed to the web audio API context's decodeAudioData() method.

See content/media/webaudio/test/test_decodeMultichannel.html (mochitest) for an example.

Comment 27

[u279076]

Thanks Ralph. Manuela please test using the example provided in comment 26 as a guide.

Comment 28

[u279076]

Nevermind, that would just be repeating what we already have in mochitest. Marking [qa-].