The current playback loop keeps the backend audio buffers full, but these buffers can be many seconds long. Because of this, changes to the volume take quite a long time to propagate from the user changing the volume into the actual sound being played. The solution is to rewrite the playback loop to only have a few hundred milliseconds of audio buffered. The Ogg backend only queues a few frames worth of audio at a time, so it does not run in to this problem. Rewriting the playback loop also allows me to fix an issue with EOF handling. At present, a flag on the state machine (mExpectMoreData) is used to determine if we can expect any more data than what Available() reports from the media stream. This flag is set to false when ResourceLoaded fires. This ends up being wrong in some cases, e.g. if the resource completes loading, but then we seek backwards in the stream--in this case, mExpectMoreData is now false, but we may have seeked back to the start of the stream. The reliable way to detect EOF is to try to read some data, and when Read returns 0, we know the stream has finished. Because I only have a few hundred milliseconds of data queued, I can drop the Pause/Resume code from nsAudioStream (again). Pause stops feeding data to the audio backend, like the Ogg decoder does. The last small amount of already buffered audio plays, then the pause is in effect. (Note that Pause/Resume aren't implemented in the ALSA backend, so it's good to avoid trying to use it anyway.) I've also dropped my use of GetTime, since this can be wrong in a lot of causes due to the audio backends often using a wallclock to report this time. Instead, I track the current time within the audio stream by hand--each time we queue a small amount of audio data, advance the currentTime by the same amount of time.

This is the basic idea. Needs a little more work: * the delta time calculation needs to cope with Read blocking * I think the buffer size should be more like 200ms with a 100ms sleep (rather than 100/90) to deal with slow machines more gracefully * need to verify that all backends can buffer at least AUDIO_BUFFER_LENGTH bytes (or make AUDIO_BUFFER_LENGTH adapt to the backend behaviour)

Might want to look at bug 463627 as that makes changes to nsAudioStream too.

Comment on attachment 352677 [details] [diff] [review] patch v0 These pass on cpearce's XP machine, which seems to be good at finding test problems. Also pass on my OS X machine. The changed tests are intended to still check the same assertions, so requesting review to make sure I didn't screw up.

Comment on attachment 352677 [details] [diff] [review] patch v0 Oops, wrong bug!

Same as the earlier patch, but with my todo list addressed and rebased on top of the large changes made to the media code recently. Includes support for firing timeupdate events (which will fix bug 477214), as doing this is trivial with the new code. Change the RIFF/WAVE parser to handle files where the format chunk does not immediately follow the RIFF header (commonly this will be a list chunk containing metadata). Fixes a bug where we can write a partial sample to the backend, causing corrupt audio playback. Also made an effort to drop the state machine lock whenever calling into nsMediaStream to avoid potential deadlocks (per bug 476176 for the Ogg decoder).

What does this need before review?

I noticed that I should refactor the RIFF chunk scanning loop into a new function, since it's now called in two places. Also needed to rebase against the trunk now that bug 462455 landed. Mochitests pass on OS X and Linux (waiting for Win32 to finish building, but the patch prior to rebasing passed on all platforms).

Win32 passes too, FWIW.

+ // Playback position (in bytes), updated as the playback loop runs and + // upon seeking. + PRUint32 mTimeOffset; PRInt64? + if (mMetadataValid) { + return BytesToTime(mTimeOffset); Fix indent + mCurrentTime = mPlaybackStateMachine->GetCurrentTime(); + mPlaybackStateMachine->ClearPositionChangeFlag(); I think getting the current time and clearing the flag needs to be done atomically. Otherwise you run the risk of getting the current time, and then the current time being changed in the decoder thread but a FirePositionChange is not fired because the flag is set, then we clear the flag on the main thread and an event is sent with the old time and no new event is sent. + monitor.Exit(); + PRUint32 available = mStream->Available(); + monitor.Enter(); Can you avoid calling available by just tracking the current download position and current playback position? That's what I did in the Ogg decoder, and it would be nice to remove the Available() API. If you do it this way, you probably need to check for SHUTDOWN at least after reacquiring the monitor, before going to sleep. + PRIntervalTime lastWakeup = PR_MillisecondsToInterval(PR_IntervalToMilliseconds(PR_IntervalNow()) - + AUDIO_BUFFER_LENGTH); lines too long Why do we need IsValidStateTransition? Can't we just make it debug-only or something?

I'll change it to a PRInt64, and make a note to review the rest of the code for 32 vs 64 bit issues, but note that Wave data can't be longer than uint32 anyway because the size stored in the file header is uint32. I think this is still true for WAVE_EXTENSIBLE (which we don't support right now); I'll double check. I'm not sure it matters that the GetCurrentTime/ClearFlag sequence is not atomic, since the user can't expect to see a particular number of timeupdates fired, and any critical positionchange are fired ignoring the position change flag (so that they're not dropped because a non-critical positionchange is pending). But it's easy to make that atomic, so I can go ahead and change that for clarity. IsValidStateTransition is a type of assertion, I'll hide it in DEBUG defines when I update the patch. New patching coming up shortly.

Using mDownloadPosition/mPlaybackPosition to calculate available data. Removed Available() method from nsMediaStream since nsWaveDecoder was the last consumer. Make sure nsHttpStreamStrategy updates the decoder's playback position after a read-forward seek. Clean up a bunch of 32 vs 64 bit stuff dealing with data length. Address other review comments.

Comment on attachment 362671 [details] [diff] [review] patch v1.2 I think you can make IsValidStateTransition just a static local function instead of a member.

Move the State enum to file scope, and make IsValidStateTransition a static local function.

Some minor extra 32/64-bit cleanup that I accidentally left out of the earlier patch due to a mixup with my patch queues. I checked with roc that I could carry review forward on this.

Pushed http://hg.mozilla.org/mozilla-central/rev/d7b1a95416b6