Last Comment Bug 623444 - (cubeb) Replace libsydneyaudio with a callback based audio library
(cubeb)
: Replace libsydneyaudio with a callback based audio library
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 5 votes (vote)
: mozilla15
Assigned To: Matthew Gregan [:kinetik]
:
Mentors:
Depends on: 783052 662417 669556 689432 723781 723793 731807 742154 742160 747793 751030 756944 757034 758198 759677 759821 788005 791112
Blocks: 542635 683263 487504 521615 582513 589595 617852 620598 640405 652485 668449 694484 698328 732046
  Show dependency treegraph
 
Reported: 2011-01-05 19:58 PST by Matthew Gregan [:kinetik]
Modified: 2012-09-13 19:21 PDT (History)
42 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
remove support for fake non-blocking writes v0 (11.94 KB, patch)
2011-07-07 18:48 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
rename samples to audio frames v0 (91.17 KB, patch)
2011-07-07 18:51 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
import libcubeb v0 (42.51 KB, patch)
2011-07-07 18:53 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
implement libcubeb based nsAudioStream v0 (11.73 KB, patch)
2011-07-07 18:59 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
patch v0 (10.51 KB, patch)
2011-08-16 18:07 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
bufferedaudiostream patch v1 (13.35 KB, patch)
2011-08-21 21:51 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Review
bufferedaudiostream patch v2 (13.47 KB, patch)
2011-09-26 20:38 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
bufferedaudiostream v3 (13.30 KB, patch)
2011-09-27 16:37 PDT, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Review
import libcubeb v1 (41.72 KB, patch)
2011-09-27 23:17 PDT, Matthew Gregan [:kinetik]
cajbir.bugzilla: review+
Details | Diff | Review
bufferedaudiostream v4 (14.60 KB, patch)
2011-09-28 22:02 PDT, Matthew Gregan [:kinetik]
kinetik: review+
Details | Diff | Review
bufferedaudiostream v5 (13.57 KB, patch)
2011-11-30 20:24 PST, Matthew Gregan [:kinetik]
kinetik: review+
Details | Diff | Review
import libcubeb v2 (55.07 KB, patch)
2011-11-30 20:30 PST, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
bufferedaudiostream v6 (15.66 KB, patch)
2012-01-12 13:23 PST, Matthew Gregan [:kinetik]
cpearce: review+
Details | Diff | Review
import libcubeb v3 win32 only (31.77 KB, patch)
2012-01-12 13:24 PST, Matthew Gregan [:kinetik]
no flags Details | Diff | Review
import libcubeb v4 win32 only (32.81 KB, patch)
2012-01-13 03:27 PST, Matthew Gregan [:kinetik]
khuey: review+
Details | Diff | Review

Description Matthew Gregan [:kinetik] 2011-01-05 19:58:44 PST
libsydneyaudio's API is not a good fit for our uses and forces us to use an unsatisfactory design for audio playback in the media playback engine.  The plan is to replace this as soon as possible after Firefox 4 is released.  I'll write some more about the plan shortly.
Comment 1 Matthew Gregan [:kinetik] 2011-01-05 20:17:27 PST
The plan is to build a small library that maps closely to modern sound APIs (PulseAudio on Linux, CoreAudio (AudioQueue) on OS X, and either DirectSound or XAudio2 on Windows).  Rather than exposing a push-to-play model like sydneyaudio (which effectively requires a separate thread per playing media element to write audio), a callback will be called when more audio is required.

It may be necessary to provide an ALSA backend in addition to the PulseAudio one, depending on the minimum Linux distro requirements post Firefox 4.

Requirements:
- remove necessity for 1-2 threads per active audio stream that we currently have (bug 592833)
- underrun behavior/handling should be the same on each platform
- low latency volume changes (bug 487504)
- sharing top-level audio resources (bug 617852)
- correct playback of sub-one-buffer length audio chunks (bug 615452)
Comment 2 Matthew Gregan [:kinetik] 2011-01-17 21:26:42 PST
Reviewing some important aspects of existing behaviour:

Playback start:
- Linux: when start threshold reached
- OS X: after first write
- Win32: when block filled

With a callback based model, this problem mostly disappears.  The library can decide what an appropriate initial buffer size is and explicitly request that the application fill it.

Underrun:
- Linux: audio clock stops ticking
- OS X: audio clock continues at normal rate (callback writes silence)
- Win32: audio clock stops ticking

It's preferable for the clock to stop ticking.  The only reason it doesn't on OS X is that I'm not aware of a way to stop it--the implementation is already using a callback model, and when a callback requests more data than is available, it's not obvious what you can do other than write silence.

API calls while underrun:
- Linux: return error, usually return useless (initial state) values after recovery
- OS X: work as there's effectively no underrun state
- Win32: return frozen state at point of underrun

Ideally the underrun recovery should be dealt with in a single place, so other API calls should try to return sensible values when playback is stopped due to underrun.

Underrun recovery:
- Linux: explicit, expensive, partial buffers lost
- OS X: N/A
- Win32: implicit (write more), cheap, no buffers lost

There's not much control available of this, it's an attribute of the OS's audio API.

Clock granularity:
- Linux: unknown (but probably discoverable), often not more than one period length
- OS X: dependent on callback buffer request size
- Win32: unknown, seems fairly high (likely sample accurate)

Clock read cost:
- Linux: medium to high, cross-DSO call, possibly involving IPC
- OS X: low, reads a local counter protected by a lock
- Win32: medium, cross-DSO call

In the current implementation, the caller may call the clock read function in a tight loop (with very short sleeps in between).  In the older Firefox media playback engine, it was assumed that the clock was fine enough granularity that individual video frames could be timed based on the audio clock updates.  Either the clock granularity must be explicit, high, and fast to read, or the API must be designed in such a way that the application can't easily make inappropriate assumptions about the clock's behaviour.
Comment 3 Shmerl 2011-03-30 10:43:17 PDT
Please don't declare PulseAudio "THE Linux audio API". OSS4 is way better. So if you design your library, please make the audio backend optional, i.e. able to support ALSA, OSS4 or whatever else. Don't forget also that other Unixes don't use PulseAudio (while OSS4 is available for example for FreeBSD and Solaris).
Comment 4 Landry Breuil (:gaston) 2011-04-10 13:14:25 PDT
Adding my voice here, pulseaudio and oss4 are not available for OpenBSD (not even speaking of alsa), so i'll need to submit a backend for our sndio API. See https://bugzilla.mozilla.org/show_bug.cgi?id=648726 for the sydneyaudio backend.
Comment 5 Matthew Gregan [:kinetik] 2011-06-06 20:01:36 PDT
OS X test builds available here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mgregan@mozilla.com-631bc0100998/try-macosx64/

Linux test builds will be available once bug 662417 is resolved.

Win32 test builds will be available once I've fixed a couple more bugs (later this week, I hope).
Comment 6 Landry Breuil (:gaston) 2011-06-07 00:30:48 PDT
And what about the others ? Is there a 'dummy fallback backend' or a way to just disable audio, until corresponding backends are written ? Is the 'new api' documented/published somewhere, so we can start working on it ?
Comment 7 Matthew Gregan [:kinetik] 2011-06-07 01:05:20 PDT
There's a build time switch to disable compilation of the new code and a run time pref (for testing purposes) to switch between the new code and the current code.  Those will both be removed at some point in the future (along with libsydneyaudio), so getting non-tier 1 platforms supported by the new code will need to happen before that removal.

Based on what we've got for libsydneyaudio, the additional backends needed are:
AIX
ALSA - I'll work on this once the initial work is done.
Android - As above, unless I can find someone else.
OpenBSD (sndio)
OS/2
OSS
Solaris (sunaudio)

API is here: http://flim.org/~kinetik/cubeb.h

If you have any questions, let me know via email or IRC.
Comment 8 Matthew Gregan [:kinetik] 2011-07-07 18:48:55 PDT
Created attachment 544676 [details] [diff] [review]
remove support for fake non-blocking writes v0
Comment 9 Matthew Gregan [:kinetik] 2011-07-07 18:51:46 PDT
Created attachment 544678 [details] [diff] [review]
rename samples to audio frames v0

Rename internal uses of "samples" to "audio frames" to make it clearer what unit is being operated on.
Comment 10 Matthew Gregan [:kinetik] 2011-07-07 18:53:01 PDT
Created attachment 544679 [details] [diff] [review]
import libcubeb v0

Import libcubeb (PulseAudio and OS X backends only, for now) and integrate with the Mozilla build system.
Comment 11 Matthew Gregan [:kinetik] 2011-07-07 18:59:18 PDT
Created attachment 544681 [details] [diff] [review]
implement libcubeb based nsAudioStream v0

A libcubeb based nsAudioStream is needed to support the current Audio Data API and remote audio code, and will be temporarily used by the builtin decoders for playback.  This allows testing libcubeb backends with minimal changes to the decoder logic, and provides a pref (media.use_cubeb) to switch between the old and new code.  The remote audio and decoder audio code will be replaced with implementations that use libcubeb directly in short order.
Comment 12 Matthew Gregan [:kinetik] 2011-07-07 21:46:39 PDT
The Windows implementation is (still) missing because there are problems with the DirectSound implementation I built that I haven't been able to solve.  The main problem is that it's impossible (as far as I can tell) to queue multiple secondary buffers to play back seamless, so I used a single DSBPLAY_LOOPING circular buffer which is refilled as soon as a quarter of it has emptied.  The problem with this approach is that if the refill thread misses the refill deadline, buffer playback wraps and it becomes impossible to accurately calculate the true playback position.  This is partially worked around by using a system timer to guess if the playback within circular buffer has looped, but the end result is not robust enough.  It also has the undesirable attribute that the user hears whatever random audio is left in the buffer upon buffer underrun.

As a short term measure, I'm building a WaveOut API version which I'll post very early next week.  After that, I'll look at building an XAudio2 implementation (which allows multiple buffers to be queued for seamless playback) and using that where possible.
Comment 13 m 2011-07-15 02:37:05 PDT
I think this also blocks bug 587465 (can't seem to update blocks/depends on).
Comment 14 Matthew Gregan [:kinetik] 2011-08-16 18:07:04 PDT
Created attachment 553645 [details] [diff] [review]
patch v0

Implement nsAudioStream interface using libcubeb.  The media.use_cubeb hidden pref (which defaults off for now) can be used to enable or disable use of the cubeb-based stream.
Comment 15 Chris Pearce (:cpearce) 2011-08-16 20:28:38 PDT
Comment on attachment 553645 [details] [diff] [review]
patch v0

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

Can you make sure http://flim.org/~kinetik/cubeb.h is up to date? That would make reviewing this easier.

::: content/media/nsAudioStream.cpp
@@ +816,5 @@
> +
> +  long DataCallback(void* aBuffer, long aFrames);
> +  int StateCallback(cubeb_state aState);
> +
> +  Monitor mMonitor;

Add a comment saying what's protected by monitor.

@@ +818,5 @@
> +  int StateCallback(cubeb_state aState);
> +
> +  Monitor mMonitor;
> +
> +  PRInt64 mLostFrames;

Definintely need a comment on mLostFrames. It's a sum of the silence cubeb injected when we underran?

@@ +833,5 @@
> +  SampleFormat mFormat;
> +  PRUint32 mBytesPerFrame;
> +
> +  PRPackedBool mPaused;
> +  PRPackedBool mRunning;

Call mRunning something different to make it obvious it's not the same as !mPaused.

@@ +900,5 @@
> +  }
> +
> +  {
> +    cubeb_stream* stream;
> +    if (cubeb_stream_init(gCubebContext, &stream, "nsBufferedAudioStream", params, params.rate / 5,

Seems the magic 5 should be a #define or constant.

@@ +911,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  mBufferLimit = aRate * mBytesPerFrame;
> +  mBufferLimit += mBytesPerFrame - (mBufferLimit % mBytesPerFrame);

Since |mBufferLimit = aRate * mBytesPerFrame|, isn't (mBufferLimit % mBytesPerFrame) always 0?

@@ +1031,5 @@
> +  }
> +
> +  uint64_t position = 0;
> +  cubeb_stream_get_position(mCubebStream, &position);
> +  PRInt64 adjusted = NS_MAX(PRInt64(0), PRInt64(position - mLostFrames));

This will be called on the state machine thread, so the read of mLostFrames needs to be synchronized by the monitor.

@@ +1081,5 @@
> +    memset(static_cast<PRUint8*>(aBuffer) + bytesToCopy, 0, framesToCopy * mBytesPerFrame);
> +    mLostFrames += framesToCopy;
> +    framesToCopy -= framesToCopy;
> +  }
> +  return aFrames - framesToCopy;

In your example in http://flim.org/~kinetik/cubeb.h you return CUBEB_OK from your data callback, why not here? Maybe you could add some appropriately named temporary variables or comments to make it more obvious what's going on here.

@@ +1085,5 @@
> +  return aFrames - framesToCopy;
> +}
> +
> +int
> +nsBufferedAudioStream::StateCallback(cubeb_state aState)

Can't this callback run after the audio thread writes data but before the audio thread calls Drain()? If that happens, won't Drain() hang forever?
Comment 16 Matthew Gregan [:kinetik] 2011-08-21 21:51:59 PDT
Created attachment 554789 [details] [diff] [review]
bufferedaudiostream patch v1

Updated patch addressing review feedback.

> Since |mBufferLimit = aRate * mBytesPerFrame|, isn't (mBufferLimit % mBytesPerFrame) always 0?
Yes, it's a leftover from when mBufferLimit was calculated as |aRate * mBytesPerFrame / 2|, which required rounding.  I'll remove this line and leave the assertion.

> In your example in http://flim.org/~kinetik/cubeb.h you return CUBEB_OK from your data callback, why not here?
Up-to-date version is here: https://github.com/kinetiknz/cubeb/blob/master/include/cubeb/cubeb.h

The API was changed so that the callee returns the number of frames copied to the provided buffer.  If the result is less than the number of frames requested, the stream enters drain state.

> Can't this callback run after the audio thread writes data but before the audio thread calls Drain()? If that happens, won't Drain() hang forever?

The stream only enters drain state once the data callback returns less than the requested number of frames.  This can't happen until Drain() sets mDraining = true.
Comment 17 Chris Pearce (:cpearce) 2011-08-22 19:58:56 PDT
Comment on attachment 554789 [details] [diff] [review]
bufferedaudiostream patch v1

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

::: content/media/nsAudioStream.cpp
@@ +1121,5 @@
> +  case FORMAT_S16_LE:
> +    SampleCopy<PRInt16>(aBuffer, mBuffer.Elements(), samplesToCopy, mVolume);
> +    break;
> +  case FORMAT_FLOAT32:
> +    SampleCopy<float>(aBuffer, mBuffer.Elements(), samplesToCopy, mVolume);

Can you also multiply mVolume by GetVolumeScale() in these SampleCopy() calls so that the media.volume_scale pref still works with your new stream?
Comment 18 Matthew Gregan [:kinetik] 2011-09-26 20:38:54 PDT
Created attachment 562652 [details] [diff] [review]
bufferedaudiostream patch v2

Two minor changes:
- avoid an unwanted lock recursion attempt in StateCallback
- draining a stream that hasn't started should complete immediately
Comment 19 Matthew Gregan [:kinetik] 2011-09-26 21:58:27 PDT
Comment on attachment 562652 [details] [diff] [review]
bufferedaudiostream patch v2

Obsoleting, because this is hitting the following assertion on tryserver:
###!!! ASSERTION: Stream pause in unexpected state.: 'mState == STARTED', file e:/builds/moz2_slave/try-w32-dbg/build/content/media/nsAudioStream.cpp, line 1033
Comment 20 Matthew Gregan [:kinetik] 2011-09-27 16:37:47 PDT
Created attachment 562907 [details] [diff] [review]
bufferedaudiostream v3

Same changes as listed for the last patch, plus this makes ::Pause and ::Resume behave the same as the existing code for unstarted streams.
Comment 21 Matthew Gregan [:kinetik] 2011-09-27 23:17:32 PDT
Created attachment 562980 [details] [diff] [review]
import libcubeb v1

Import libcubeb.  This includes working OS X (AudioQueue) and Win32 (winmm) implementations.  The build machinery is in place for a Linux (ALSA) version, but I've excluded the file from this patch until I complete some refactoring.
Comment 22 Chris Pearce (:cpearce) 2011-09-28 14:29:25 PDT
Comment on attachment 562907 [details] [diff] [review]
bufferedaudiostream v3

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

Looks good. r=cpearce assuming that nsBufferedAudioStream::mBufferLimit can never be ridiculously large (see below).

::: content/media/nsAudioStream.cpp
@@ +844,5 @@
> +  // Owning reference to a cubeb_stream.  cubeb_stream_destroy is called by
> +  // nsAutoRef's destructor.
> +  nsAutoRef<cubeb_stream> mCubebStream;
> +
> +  PRInt32 mRate;

Is it worth making mRate and mChannels PRUInt32? These are unsigned in cubeb_stream_params. r+ regardless of whether you do or not.

@@ +853,5 @@
> +  enum StreamState {
> +    INITIALIZED, // Initialized, playback has not begun.
> +    STARTED,     // Started by a call to Write() (iff INITIALIZED) or Resume().
> +    STOPPED,     // Stopped by a call to Pause().
> +    DRAINING,    // Drain requested.  DataCallback will signal end of stream

Is this comment right? Did you mean to say "StateCallback will be called to signal end of stream..." instead of "DataCallback"... ?

@@ +918,5 @@
> +    params.format = CUBEB_SAMPLE_FLOAT32LE;
> +    mBytesPerFrame = sizeof(float) * aNumChannels;
> +    break;
> +  default:
> +      return NS_ERROR_FAILURE;

Looks like indentation is off on the return here.

@@ +936,5 @@
> +
> +  // Limit mBuffer to one second of audio.  This value is arbitrary, and was
> +  // selected based on the observed behaviour of the existing nsAudioStream
> +  // implementations.
> +  mBufferLimit = aRate * mBytesPerFrame;

Am I right to assume nsBufferedAudioStream::mBufferLimit can never be ridiculously large because cubeb_stream_init() will fail if passed in ridiculous values for rate and channel? Do we need to cap this value?

@@ +1131,5 @@
> +
> +  // Remove copied data from the temporary audio buffer.
> +  mBuffer.RemoveElementsAt(0, available);
> +  NS_ABORT_IF_FALSE(mBuffer.Length() % mBytesPerFrame == 0, "Must copy complete frames");
> +

Remove extra newline here?
Comment 23 Matthew Gregan [:kinetik] 2011-09-28 22:02:47 PDT
Created attachment 563304 [details] [diff] [review]
bufferedaudiostream v4

Address review comments, carrying forward review.
Comment 24 Matthew Gregan [:kinetik] 2011-11-30 20:24:46 PST
Created attachment 578167 [details] [diff] [review]
bufferedaudiostream v5

Rebased and PR_BOOL converted.  Carrying forward review.
Comment 25 Matthew Gregan [:kinetik] 2011-11-30 20:30:05 PST
Created attachment 578169 [details] [diff] [review]
import libcubeb v2

This version includes the ALSA backend.  The only other changes are very minor:
- Added sanity checks to cubeb_stream_init.
- Removed bogus assert from wmme backend.

content/media mochitests pass locally on Linux with bare ALSA and ALSA with the PulseAudio plugin.  They also pass locally on Win32 and OS X, but there are some test timeouts on OS X on try that I'm investigating now.
Comment 26 Matthew Gregan [:kinetik] 2011-11-30 20:41:04 PST
Comment on attachment 578169 [details] [diff] [review]
import libcubeb v2

The libcubeb code corresponds to git commit ID fc6e838a0078a10ecc306f5c671c07ec2bfe7607.  README_MOZILLA has the wrong ID because I imported the code before I committed the final changes.
Comment 27 Matthew Gregan [:kinetik] 2011-12-01 17:06:20 PST
Comment on attachment 578169 [details] [diff] [review]
import libcubeb v2

Some last minute locking changes to the ALSA backend are causing problems on the try machines.  Cancelling review until I solve them.
Comment 28 Matthew Gregan [:kinetik] 2012-01-12 13:23:16 PST
Created attachment 588168 [details] [diff] [review]
bufferedaudiostream v6

Other than rebasing, the only change from the already review v5 is that I moved the media.use_cubeb pref stuff so that it shares the thread safe observing with media.volume_scale, since I was previously fetching media.use_cubeb (unsafely) off the main thread.
Comment 29 Matthew Gregan [:kinetik] 2012-01-12 13:24:34 PST
Created attachment 588170 [details] [diff] [review]
import libcubeb v3 win32 only

Import just the Win32 backend (already reviewed in v2) for now.  This just needs build system integration review before landing.
Comment 30 Chris Pearce (:cpearce) 2012-01-12 15:20:03 PST
Comment on attachment 588168 [details] [diff] [review]
bufferedaudiostream v6

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

::: content/media/nsAudioStream.cpp
@@ +343,5 @@
> +      NS_ConvertUTF16toUTF8 utf8(value);
> +      gVolumeScale = NS_MAX<double>(0, PR_strtod(utf8.get(), nsnull));
> +    }
> +  } else if (strcmp(aPref, PREF_USE_CUBEB) == 0) {
> +    bool value = Preferences::GetBool(aPref, true);

Maybe this should be GetBool(aPref, false), otherwise we'll be using cubeb by default when MOZ_CUBEB is defined? We can then enable it platform-by-platform for desktop in firefox.js, and eventually set the pref in all.js once cubeb is stable on mobile (and B2G? I'm not sure if B2G loads all.js...).

@@ +885,5 @@
> +    INITIALIZED, // Initialized, playback has not begun.
> +    STARTED,     // Started by a call to Write() (iff INITIALIZED) or Resume().
> +    STOPPED,     // Stopped by a call to Pause().
> +    DRAINING,    // Drain requested.  DataCallback will indicate end of stream
> +                 // once the remaining contents of mBuffer have requested by

s/have requested/have been requested/  ?

@@ +894,5 @@
> +
> +  StreamState mState;
> +
> +  // Arbitrary default stream latency.  The higher this value, the longer stream
> +  // volume changes will take to become audible.

The lower the value....  ?

@@ +931,5 @@
> +
> +nsresult
> +nsBufferedAudioStream::Init(PRInt32 aNumChannels, PRInt32 aRate, SampleFormat aFormat)
> +{
> +  if (!gCubebContext || aNumChannels < 0 || aRate < 0) {

Maybe <= 0 instead? Can we succeed with aNumChannels or aRate equal to 0?
Comment 31 Matthew Gregan [:kinetik] 2012-01-12 17:07:16 PST
Comment on attachment 588170 [details] [diff] [review]
import libcubeb v3 win32 only

Clearing build review for now; this needs to be updated to inclue libcubeb in the gkmedias library (bug 709721).
Comment 32 Matthew Gregan [:kinetik] 2012-01-13 03:24:57 PST
(In reply to Chris Pearce, Mozilla Corporation (:cpearce) from comment #30)
> Maybe this should be GetBool(aPref, false), otherwise we'll be using cubeb
> by default when MOZ_CUBEB is defined? We can then enable it
> platform-by-platform for desktop in firefox.js, and eventually set the pref
> in all.js once cubeb is stable on mobile (and B2G? I'm not sure if B2G loads
> all.js...).

That's intentional.  libcubeb will be enabled by default for each backend as the code is added to the tree (Win32 is being added first), and may then be entirely disabled via the pref when branching for Aurora, depending on how stable the code is.

> @@ +885,5 @@
> > +    INITIALIZED, // Initialized, playback has not begun.
> > +    STARTED,     // Started by a call to Write() (iff INITIALIZED) or Resume().
> > +    STOPPED,     // Stopped by a call to Pause().
> > +    DRAINING,    // Drain requested.  DataCallback will indicate end of stream
> > +                 // once the remaining contents of mBuffer have requested by
> 
> s/have requested/have been requested/  ?

Thanks, will fix.

> The lower the value....  ?

The higher the requested latency, the more data is buffered at the OS level after the point at which we apply the per-stream volume adjustment, so the longer it'll take for that to play out and any change in the per-stream volume to be submitted to the OS for playback.

> Maybe <= 0 instead? Can we succeed with aNumChannels or aRate equal to 0?

libcubeb/cubeb_stream_init is responsible for validating the stream parameters.  This check is only validating that the params can be converted to unsigned integers.
Comment 33 Matthew Gregan [:kinetik] 2012-01-13 03:27:19 PST
Created attachment 588363 [details] [diff] [review]
import libcubeb v4 win32 only

Previously reviewed code, just needs build system review.  This version is integrated with the gkmedias library on Win32.  Try run was as green as usual.
Comment 34 :Ms2ger 2012-01-24 03:27:14 PST
Can we please use mozilla/StdInt.h when it's available? Shouldn't be too hard to add something like http://mxr.mozilla.org/mozilla-central/source/mfbt/StdInt.h#65
Comment 35 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-29 13:21:28 PST
Comment on attachment 588363 [details] [diff] [review]
import libcubeb v4 win32 only

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

Build bits look fine.  Sorry this took so long.

::: media/libcubeb/Makefile.in
@@ +46,5 @@
> +
> +DIRS		= \
> +		include \
> +		src \
> +		$(NULL)

No tabs please.

::: media/libcubeb/src/Makefile.in
@@ +48,5 @@
> +ifeq (WINNT,$(OS_TARGET))
> +VISIBILITY_FLAGS =
> +endif
> +
> +ifneq (,$(filter WINNT WINCE,$(OS_ARCH)))

WINCE is long dead, no need for that here.

@@ +51,5 @@
> +
> +ifneq (,$(filter WINNT WINCE,$(OS_ARCH)))
> +CSRCS           = \
> +                cubeb_winmm.c \
> +                $(NULL)

No tabs please.
Comment 36 Matthew Gregan [:kinetik] 2012-02-01 18:31:19 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/71678cfd06dd
http://hg.mozilla.org/integration/mozilla-inbound/rev/7d1f570d9136

(In reply to Ms2ger from comment #34)
> Can we please use mozilla/StdInt.h when it's available? Shouldn't be too
> hard to add something like
> http://mxr.mozilla.org/mozilla-central/source/mfbt/StdInt.h#65

I changed cubeb-stdint.h to simply include mozilla/StdInt.h.
Comment 37 Ed Morley [:emorley] 2012-02-02 06:44:47 PST
https://hg.mozilla.org/mozilla-central/rev/7d1f570d9136
Comment 38 Matthew Gregan [:kinetik] 2012-02-02 12:34:51 PST
https://hg.mozilla.org/mozilla-central/rev/71678cfd06dd was pushed with bug number 677138, but actually belongs to bug 623444 (this bug).
Comment 39 Matthew Gregan [:kinetik] 2012-02-03 00:36:34 PST
This is causing a fatal assertion during tests (see bug 723781).

Disabled: http://hg.mozilla.org/integration/mozilla-inbound/rev/27f0028890e7
Comment 40 Matthew Gregan [:kinetik] 2012-06-04 18:49:09 PDT
Marking this as fixed since the bulk of the work is complete.  Everything else is in follow up bugs.
Comment 41 Grant Galitz 2012-06-25 11:48:17 PDT
What is the sample count accuracy for each OS? I see the Web Audio API for Google Chrome allows js devs to explicitly use 256 sample buffers (And I personally use the 2048 samples per buffer @ 44.1khz). mozCurrentSampleOffset relies on buffer position accuracy, so we might need charts up in the wiki. Lastly, could this buffer size (for positon accuracy) be exposed to moz audio? It seems mac really is accurate, while linux and windows suffer some loss of accuracy in position determination.

Note You need to log in before you can comment on or make changes to this bug.