463929 - Wave backend for <audio> can't play back 8-bit samples

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

There are a lot of 8-bit samples on the web, and people are surprised when they don't play.  We probably also need this for spec compliance, since the spec says:

"User agents must support the WAVE container format with audio encoded using the PCM format."

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Modify nsAudioStream to take a sample format parameter on initialization.  Remove nsAudioStream::Write(const float*, PRUint32) overload and modify remaining Write method to take a const void* and perform the appropriate sample conversion by switching on the sample format specified at initialization time.  Modify callers in nsWaveDecoder.cpp and nsOggDecoder.cpp.  Fix a bug in nsWaveStateMachine::LoadFormatChunk when dealing with extended format chunks.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  NS_ABORT_IF_FALSE(aCount % mChannels == 0,
+                    "Buffer size must be divisible by sample size times channel count");

This message is confusing since you're not actually checking the sample size and buffer size, just the sample count.

+      const PRUint8* buf = reinterpret_cast<const PRUint8*>(aBuf);

These can be static_casts, I believe.

+      const PRUint8* buf = reinterpret_cast<const PRUint8*>(aBuf);
+      for (PRUint32 i = 0; i < aCount; ++i) {
+        s_data[i] = ((buf[i] - 128) << 8) * mVolume;
+      }

Maybe it's not strictly necessary, but it'd be clearer if we cast buf[i] to PRInt32 before using it. Also on Windows this is probably causing warnings in the implicit float (or is it double?) to short conversion. How about just doing the obvious fixed-point equivalent?

+        s_data[i] = buf[i] * mVolume;

Fixed-point would be nice here too.

Comment 3

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Address review comments.  I'm not entirely sure what you meant by "obvious fixed-point equivalent", so my change to that code may not reflect your intention.  I've added a cast to short during the volume/sample conversion, even with the fixed-point version we'd get an int->short warning here.  I also converted all of the mAudioHandle casts to static_casts (except the sa_stream_create_pcm call, which is a sa_stream_t** cast), since they were unnecessarily reinterpret_casts.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 347234 [details] [diff] [review]
patch v1

That's good, except instead of using PR_INT16_MAX as the scale factor, I'd use 2^16 and make 'volume' a PRInt32. That means we can use a right shift instead of a division.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'll make that change when I check in.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er, but this isn't really needed for beta2, so I guess you can make that change and someone can check it in after beta2.

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #6)
> Er, but this isn't really needed for beta2

From a technical standpoint, I agree, but from a social standpoint, are you sure about that?  I wouldn't be surprised to see a decent number of dups filed on this by people expecting that because WAV is said to work, all WAV does work.  (And who reads the relnote for this anyway?  I doubt it's that effective in preventing dups.)  Given that, I'd say land it to prevent them.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 7d06dac3fe83

Comment 9

[Jeff Walden [:Waldo]]

This isn't quite working for me; downloading rheet.wav from the URL to disk and loading it directly plays the file (twice, in fact, will look for an already-filed bug in the morning since that's probably a separate issue), albeit with the assertion:

###!!! ASSERTION: OnDataAvailable implementation consumed no data: 'Error', file /Users/jwalden/moz/2/netwerk/base/src/nsInputStreamPump.cpp, line 529

However, loading a sibling HTML file whose contents are the single line:

<audio controls src="rheet.wav" autoplay></audio>

does nothing.  (I assume this problem is also what keeps the two URLs in bug 463999 from playing.)  Reopen, file a new bug, something else?  Comment and I'll do whatever in the morning, or preempt me if you feel like it.

Comment 10

[Matthew Gregan [:kinetik]]

I tested rheet.wav by loading the mozilla.org FTP URL directly and didn't hear it play twice.  Please go ahead and file a new bug (cc me) with any relevant details.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20081221 Minefield/3.2a1pre ID:20081221222459

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20081224 Minefield/3.2a1pre ID:20081224034752

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081225 Shiretoko/3.1b3pre ID:20081225020447

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081225 Shiretoko/3.1b3pre ID:20081225034145