Closed Bug 449315 Opened 16 years ago Closed 16 years ago

Support WAV format in <audio> element

Categories

(Core :: Audio/Video, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: kinetik)

References

Details

Attachments

(3 files, 10 obsolete files)

The HTML5 spec says we should support WAV, and it makes sense. We need an internal decoder backend for WAV. This should actually be slightly higher priority than the platform framework backends.
Severity: normal → enhancement
Assignee: nobody → kinetik
Attached patch patch v0 (obsolete) — Splinter Review
Preliminary patch for early review.
Attachment #343507 - Flags: review?(roc)
 #ifdef MOZ_OGG
 #include "nsOggDecoder.h"
+#include "nsWaveDecoder.h"
 #endif

This should not be inside MOZ_OGG, should it?

nsWaveDecoder could use a lot of comments. Also, the methods that override nsMediaDecoder should be marked "virtual".

Maybe we should make the buffering parameters preferences and share them between backends as much as possible? Or do we need different parameter values for different backends?

nsWaveStateMachine needs lots of comments about the fields.

+      monitor.Wait();
+    }

This naked Wait in STATE_END is probably wrong. You'll want a do { Wait } while (state != SHUTDOWN), I think.

+      }
+    }
+      break;

Weird indentation.

+  nsresult rv = mStream->Read(buf, sizeof(buf), &got);
+  if (NS_FAILED(rv) || got != sizeof(buf)) {
+    NS_WARNING("Stream read failed");
+    return PR_FALSE;

So we don't have to worry about short reads? is that documented somewhere?

Could simplify things a bit by making Uint16/32LE take &p and automatically advance p.

Basically looks good. We should think about what we could do to increase code sharing here with Ogg, but we have to be careful not to share too much or too little.
Attached patch patch v1 (obsolete) — Splinter Review
Addresses review comments except for code sharing with Ogg backend and prefs for buffering wait values.  I agree that we should do at least the first, and probably the second.
Attachment #343507 - Attachment is obsolete: true
Attachment #345969 - Flags: review?(roc)
Attachment #343507 - Flags: review?(roc)
+#if !defined(nsAudioStream_h___)
+#define nsAudioStream_h___

At most one trailing underscore.

You need to update your changes to nsAudioStream to account for the fact that nsAudioStream is now responsible for scaling sample values according to mVolume.

There are also a few little changes due to the landing of standalone video/audio documents.

+#include "algorithm"
+#include "limits"

Get rid of these.

+  // Our audio backend.  Created on demand when entering playback state.  It
+  // is destroyed when seeking begins and will not be reinitialized until
+  // playback resumes, so it is possible for this to be null.
+  nsAutoPtr<nsAudioStream> mAudioStream;

I think "Our audio stream" would be more accurate?

+      if (LoadRIFFChunk() && LoadFormatChunk() && FindDataOffset()) {
+        nsCOMPtr<nsIRunnable> event =
+          NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, MetadataLoaded);
+        NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

Shouldn't we check for SHUTDOWN state before firing this event?

+      if (mState != STATE_BUFFERING) {
+        nsCOMPtr<nsIRunnable> event =
+          NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, BufferingStopped);
+        NS_DispatchToMainThread(event, NS_DISPATCH_NORMAL);

Ditto. (Maybe after the Wait(1000) we should check for SHUTDOWN and bail out there if necessary.)

+      if (mExpectMoreData == PR_FALSE && mStream->Available() < mSampleSize) {

if (!mExpectMoreData

+            if (RoundDownToSample(len) != len) {
+              NS_WARNING("PCM data does not end with complete sample");
+            }

Shouldn't we set len to RoundDownToSample(len) here to avoid sending garbage to the audio device?

+        // Sleep until approximately half of the currently buffered audio has
+        // been consumed by the audio hardware.

What's the rationale for this?

+        nsCOMPtr<nsIRunnable> startEvent =
+          NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, SeekingStarted);
+        NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);

Need to check SHUTDOWN state after this.

+      if (mAudioStream) {
+        mAudioStream->Drain();
+      }

We should not be holding the monitor while we do this (as discussed F2F). We should also check for SHUTDOWN state before firing the following event.

+    case STATE_ERROR:
+      do {
+        monitor.Wait();
+      } while (mState != STATE_SHUTDOWN);
+      break;

This could be simplified to just a single monitor.Wait().

+      return NS_OK;
+      break;

No need for break here.

+static PRUint32
+ReadChunkName(const char** aBuffer)
+{
+  PRUint32 result =
+    PRUint8((*aBuffer)[0]) << 24 |
+    PRUint8((*aBuffer)[1]) << 16 |
+    PRUint8((*aBuffer)[2]) << 8 |
+    PRUint8((*aBuffer)[3]);
+  *aBuffer += sizeof(PRUint32);
+  return result;
+}

Can't we call this ReadUint32BE?

+  do {
+    PRUint32 read = 0;
+    nsresult rv = mStream->Read(buf + got, sizeof(buf) - got, &read);
+    if (NS_FAILED(rv)) {
+      NS_WARNING("Stream read failed");
+      return PR_FALSE;
+    }
+    got += read;
+  } while (got != sizeof(buf));

Factor this into some kind of ReadAll helper and use it from a few places.
> +#include "limits"
> 
> Get rid of these.

I guess we need to keep this one.
Attached patch patch v2 (obsolete) — Splinter Review
Here's the latest version.  Changes:

- update to tip (adjusted nsAudioStream changes for volume handling, reworked changes to nsHTMLMediaElement for media selection and loading with existing channels)
- address review comments
- resurrect nsAudioStream::{Pause,Resume,GetTime} (recently removed by doublec when he stopped using them)
- add a test (more coming, will submit them separately)
- fixed some bugs
- inverted logic in configure.in so Wave is enabled by default

This has been built and tested on OS X, Linux, and Windows.
Attachment #345969 - Attachment is obsolete: true
Attachment #346188 - Flags: superreview?(roc)
Attachment #346188 - Flags: review?(roc)
Attachment #345969 - Flags: review?(roc)
Attached patch patch v2.1 (obsolete) — Splinter Review
Missed a line when inverting the configure logic for Wave.
Attachment #346188 - Attachment is obsolete: true
Attachment #346191 - Flags: superreview?(roc)
Attachment #346191 - Flags: review?(roc)
Attachment #346188 - Flags: superreview?(roc)
Attachment #346188 - Flags: review?(roc)
+      monitor.Exit();
+
+      {
+        nsCOMPtr<nsIRunnable> startEvent =
+          NS_NEW_RUNNABLE_METHOD(nsWaveDecoder, mDecoder, SeekingStarted);
+        NS_DispatchToMainThread(startEvent, NS_DISPATCH_SYNC);
+
+        if (mState == STATE_SHUTDOWN) {
+          break;
+        }

You can't check mState without holding the monitor...
You should probably check mState after seeking and before firing SeekingStopped, too.
Attached patch patch v2.2 (obsolete) — Splinter Review
Nice catch.  Latest patch fixes those two issues.
Attachment #346191 - Attachment is obsolete: true
Attachment #346191 - Flags: superreview?(roc)
Attachment #346191 - Flags: review?(roc)
Attachment #346200 - Flags: superreview?(roc)
Attachment #346200 - Flags: review?(roc)
Comment on attachment 346200 [details] [diff] [review]
patch v2.2

The 'break's inside {} can just stand alone. Whoever  checks this in can fix that, if necessary.

Why are you removing test_start.html from the Makefile?
Attachment #346200 - Flags: superreview?(roc)
Attachment #346200 - Flags: superreview+
Attachment #346200 - Flags: review?(roc)
Attachment #346200 - Flags: review+
Attached patch patch v2.3 (obsolete) — Splinter Review
Sorry, removing test_start.html was not intentional, it was accidentally picked up in a qrefresh while poking at some other tests.

I didn't remove the braces around the breaks because it'd make the rest of the file inconsistent--I included braces around every single statement conditional/loop.
Attachment #346200 - Attachment is obsolete: true
Backed out:

http://hg.mozilla.org/mozilla-central/rev/18403769ec19

/builds/moz2_slave/mozilla-central-linux-debug/build/content/html/content/src/nsHTMLMediaElement.cpp: In member function ‘PRBool nsHTMLMediaElement::CreateDecoder(const nsACString_internal&)’:
/builds/moz2_slave/mozilla-central-linux-debug/build/content/html/content/src/nsHTMLMediaElement.cpp:702: error: cannot allocate an object of abstract type ‘nsWaveDecoder’
../../../../dist/include/content/nsWaveDecoder.h:141: note:   because the following virtual functions are pure within ‘nsWaveDecoder’:
../../../../dist/include/content/nsMediaDecoder.h:149: note: 	virtual void nsMediaDecoder::SetSeekable(PRBool)
../../../../dist/include/content/nsMediaDecoder.h:152: note: 	virtual PRBool nsMediaDecoder::GetSeekable()
gmake[7]: *** [nsHTMLMediaElement.o] Error 1
This is a pretty big patch to take with no tests. Shouldn't it be able to play rheeet at the very least?
There is a test:
+++ b/content/media/video/test/test_wav_ended1.html

and there are some more tests coming (WAV versions of most of the Ogg tests).
Attached patch patch v2.4 (obsolete) — Splinter Review
Rebase against current tip--the duration changes for the Ogg backend that landed shortly before this patch added a pure virtual to the nsMediaDecoder base class.  Fix is to add a trivial implementation to nsWaveDecoder, so no additional review required.
Attachment #346220 - Attachment is obsolete: true
Sadly we can't play ftp://ftp.mozilla.org/pub/mozilla.org/mozilla/libraries/bonus-tracks/rheet.wav because the audio backends only support 16-bit samples for now.
The library checks in configure.in broke --disable-compile-environment. We don't need audio libs without compiling, which is what we're doing for l10n builds.

See http://l10n.mozilla.org/buildbot/builders/hg-langpack/builds/5115/steps/configure/logs/stdio for a sample failure log.

There are a bunch of examples in configure.in on how to wrap your code such that it's not run when COMPILE_ENVIRONMENT is off.
Attached patch patch v2.5 (obsolete) — Splinter Review
Address Pike's issue, rebase against trunk (no change, but diff hunks moved around).  This includes the changes required for the patch in bug 449307 already (and will also build without that patch), so the two patches can be applied in any order.
Attachment #346228 - Attachment is obsolete: true
Attached patch patch v2.6 (obsolete) — Splinter Review
Remove setTimeout from test_wav_ended1.html like cpearce is doing for the existing media tests in bug 462933.
Attachment #346328 - Attachment is obsolete: true
Attached audio test wave file
patch v2.6 includes a binary file (a test wave) encoded in extended hg/git patch format, which means the patch must be imported using hg import or the binary file will be ignored.

I've had a report from cpearce that hg import crashes on Win32 for him, so I'm attaching the file separately for anyone who runs into the same problem or wants to apply the patch to their tree with patch rather than hg import.
This was landed, and subsequently backed out due to unit test timeouts.
moz2-win32-slave07 logged:

*** 27746 INFO Running /tests/content/media/video/test/test_wav_ended1.html...
[WAVE API] System driver not present error returned while executing opening audio device for playback
[WAVE API] Invalid device handle error returned while executing resetting audio device

This is easy to reproduce, just disable the sound card in device manager.  Because the audio backend fails to intialize, nsAudioStream::Available will always return 0.  This means the playback loop in nsWaveStateMachine::Run will never make progress, and thus we'll never transition to STATE_ENDED and fire the playbackEnded event.

My guess is that moz2-linux-slave08 timed out for the same reason.
If the audio backend fails to open, we won't be able report useful times for currentTime, so tests depending on that are going to fail too.
I talked about this with Chris D. This is annoying, but we do have to handle this case ... even though audio's not playing, we still want to behave roughly as if we had a real audio device. Otherwise Web pages using these features could break in strange ways.

Maybe you can fix this at the nsAudioStream level, by detecting no audio device and faking it.
Attached patch fake audio backend (obsolete) — Splinter Review
Modify nsAudioStream to behave as if it is playing audio, even if the underlying backend failed to initialize.  This is necessary to provide correct behaviour on systems where the backend failed to initialize, such as machines without sound hardware/drivers.
Attachment #346607 - Flags: superreview?(roc)
Attachment #346607 - Flags: review?(roc)
Attachment #346607 - Flags: superreview?(roc)
Attachment #346607 - Flags: superreview+
Attachment #346607 - Flags: review?(roc)
Attachment #346607 - Flags: review+
Comment on attachment 346607 [details] [diff] [review]
fake audio backend

Please don't overload mStartTime to be both a wall-clock time and a stream elapsed time --- use a separate variable for one of them. Otherwise looks good. Please do run the full set of Ogg and WAV tests with and without sound enabled, then attach a complete patch. Thanks!
Address review comments.  Attaching for interdiff purposes only.  Rolled up patching coming up.
Attachment #346607 - Attachment is obsolete: true
Attached patch patch v2.7Splinter Review
Includes nsAudioStream changes reviewed separately in the fake audio backend patch.  Built and tested all content/media/video/test tests on Mac with working and fake audio backends.  Also tested test_wav_ended1.html with the fake audio backend on Win32 and the sound card disabled.
Attachment #346329 - Attachment is obsolete: true
Keywords: checkin-needed
this patch causes,

c++ -o nsOggDecoder.o -c -I../../../../dist/include/system_wrappers -include ../../../../config/gcc_hidden.h -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_COM_OBSOLETE -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES  -DZLIB_INTERNAL -DOSTYPE=\"Linux2.6\" -DOSARCH=Linux  -I. -I. -I../../../../dist/include/xpcom -I../../../../dist/include/string -I../../../../dist/include/gfx -I../../../../dist/include/content -I../../../../dist/include/thebes -I../../../../dist/include/layout -I../../../../dist/include/widget -I../../../../dist/include/dom -I../../../../dist/include/js -I../../../../dist/include/locale -I../../../../dist/include/unicharutil -I../../../../dist/include/webshell -I../../../../dist/include/uriloader -I../../../../dist/include/htmlparser -I../../../../dist/include/necko -I../../../../dist/include/view -I../../../../dist/include/pref -I../../../../dist/include/docshell -I../../../../dist/include/xpconnect -I../../../../dist/include/xuldoc -I../../../../dist/include/caps -I../../../../dist/include/editor -I../../../../dist/include/imglib2 -I../../../../dist/include/mimetype -I../../../../dist/include/exthandler -I../../../../dist/include/uconv -I../../../../dist/include/intl -I../../../../dist/include/plugin -I../../../../dist/include/cairo -I../../../../dist/include/libpixman -I../../../../dist/include   -I../../../../dist/include/content -I/home/mdew/ff/mozilla-central/dist/include/nspr     -I/home/mdew/ff/mozilla-central/dist/sdk/include -I./../../../base/src -I./../../../html/content/src     -fPIC   -fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -fno-strict-aliasing -fshort-wchar -pthread -pipe  -DNDEBUG -DTRIMMED -O3 -msse2 -mmmx   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h -Wp,-MD,.deps/nsOggDecoder.pp nsOggDecoder.cpp
nsOggDecoder.cpp:45:27: error: nsAudioStream.h: No such file or directory
nsOggDecoder.cpp: In member function ‘PRBool nsOggDecodeStateMachine::PlayAudio(nsOggDecodeStateMachine::FrameData*)’:
nsOggDecoder.cpp:664: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp:667: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp: In member function ‘void nsOggDecodeStateMachine::OpenAudioStream()’:
nsOggDecoder.cpp:676: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp:681: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp:682: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp: In member function ‘void nsOggDecodeStateMachine::CloseAudioStream()’:
nsOggDecoder.cpp:690: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
nsOggDecoder.cpp: In member function ‘void nsOggDecodeStateMachine::SetVolume(float)’:
nsOggDecoder.cpp:764: error: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: error: forward declaration of ‘struct nsAudioStream’
../../../../dist/include/xpcom/nsAutoPtr.h: In destructor ‘nsAutoPtr<T>::~nsAutoPtr() [with T = nsAudioStream]’:
nsOggDecoder.cpp:458:   instantiated from here
../../../../dist/include/xpcom/nsAutoPtr.h:104: warning: possible problem detected in invocation of delete operator:
../../../../dist/include/xpcom/nsAutoPtr.h:104: warning: invalid use of incomplete type ‘struct nsAudioStream’
../../../../dist/include/content/nsOggDecoder.h:269: warning: forward declaration of ‘struct nsAudioStream’
../../../../dist/include/xpcom/nsAutoPtr.h:104: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.
../../../../dist/include/xpcom/nsAutoPtr.h: In member function ‘void nsAutoPtr<T>::assign(T*) [with T = nsAudioStream]’:
../../../../dist/include/xpcom/nsAutoPtr.h:134:   instantiated from ‘nsAutoPtr<T>& nsAutoPtr<T>::operator=(T*) [with T = nsAudioStream]’
nsOggDecoder.cpp:691:   instantiated from here
../../../../dist/include/xpcom/nsAutoPtr.h:71: warning: possible problem detected in invocation of delete operator:
../../../../dist/include/xpcom/nsAutoPtr.h:69: warning: ‘oldPtr’ has incomplete type
../../../../dist/include/content/nsOggDecoder.h:269: warning: forward declaration of ‘struct nsAudioStream’
../../../../dist/include/xpcom/nsAutoPtr.h:71: note: neither the destructor nor the class-specific operator delete will be called, even if they are declared when the class is defined.
make[6]: *** [nsOggDecoder.o] Error 1
make[6]: Leaving directory `/home/mdew/ff/mozilla-central/content/media/video/src'
make[5]: *** [src_libs] Error 2
make[5]: Leaving directory `/home/mdew/ff/mozilla-central/content/media/video'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/home/mdew/ff/mozilla-central/content/media'
make[3]: *** [media_libs] Error 2
make[3]: Leaving directory `/home/mdew/ff/mozilla-central/content'
make[2]: *** [libs_tier_gecko] Error 2
make[2]: Leaving directory `/home/mdew/ff/mozilla-central'
make[1]: *** [tier_gecko] Error 2
make[1]: Leaving directory `/home/mdew/ff/mozilla-central'
make: *** [default] Error 2
(In reply to comment #31)
> this patch causes,
> 
> -O3 -msse2 -mmmx   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h
> -Wp,-MD,.deps/nsOggDecoder.pp nsOggDecoder.cpp
> nsOggDecoder.cpp:45:27: error: nsAudioStream.h: No such file or directory

You need to regenerate configure and rerun it.
(In reply to comment #32)
> (In reply to comment #31)
> > this patch causes,
> > 
> > -O3 -msse2 -mmmx   -DMOZILLA_CLIENT -include ../../../../mozilla-config.h
> > -Wp,-MD,.deps/nsOggDecoder.pp nsOggDecoder.cpp
> > nsOggDecoder.cpp:45:27: error: nsAudioStream.h: No such file or directory
> 
> You need to regenerate configure and rerun it.

confirmed, thanks :)
http://hg.mozilla.org/mozilla-central/rev/3dea3415b003
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 463537
Is this now working in current trunk build? Can the checkin-needed keyword be removed?
I tried to get this working, using: http://martijn.martijn.googlepages.com/test_wav.html
But I don't get to hear any sound, am I doing something wrong?
Yes, thanks for the reminder.  As for your testcase, audio/video elements don't play by default, so you need to either set the autoplay attribute, or call the play method on the element.  In this case, block.wav is 8-bit, so we can't play it back anyway because the audio backend (libsydneyaudio) doesn't support playback of 8-bit samples.
Keywords: checkin-needed
Ok, thanks, do you know of an example .wav file where I do get sound with then? I tried the one from http://mxr.mozilla.org/mozilla-central/source/content/media/video/test/r11025_s16_c1.wav but that doesn't seem to generate sound either?
I guess the 8-bit .wav no sound thing is a bug, right?
Seems like there's quite a bit of 8-bit WAV around, perhaps we should implement that for 3.1.
Blocks: 463929
r11025_s16_c1.wav is a silent test file, so at most you'll hear a click as it
plays.

We should treat the lack of 8-bit sample support as a bug.  I've opened bug
463929 for this.
Depends on: 465165
You need to log in before you can comment on or make changes to this bug.