Closed Bug 463929 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.1

People

(Reporter: kinetik, Assigned: kinetik)

References

()

Details

(Keywords: relnote, verified1.9.1)

Attachments

(1 file, 1 obsolete file)

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."
Flags: wanted1.9.1?
Attached patch patch v0 (obsolete) — Splinter Review
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.
Attachment #347221 - Flags: superreview?(roc)
Attachment #347221 - Flags: review?(roc)
+  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.
Attached patch patch v1Splinter Review
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.
Attachment #347221 - Attachment is obsolete: true
Attachment #347234 - Flags: superreview?(roc)
Attachment #347234 - Flags: review?(roc)
Attachment #347221 - Flags: superreview?(roc)
Attachment #347221 - Flags: review?(roc)
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.
Attachment #347234 - Flags: superreview?(roc)
Attachment #347234 - Flags: superreview+
Attachment #347234 - Flags: review?(roc)
Attachment #347234 - Flags: review+
I'll make that change when I check in.
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.
(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.
Keywords: relnote
Flags: wanted1.9.1? → blocking1.9.1+
Pushed 7d06dac3fe83
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
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.
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.