607838 - When playing a sound with play(), calling currentTime = 0 doesn't rewind the sound.

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[t4ils]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre
Build Identifier: 4.0b8pre

When I play a sound in javascript, let's say mySound.play(), It seems that mySound.currentTime = 0 doesn't rewind the sound.
So when I'm trying to stop a sound in order to play it from start right after stoppage, it doesn't work.



Reproducible: Always

Steps to Reproduce:
1. Load a sound : var a = new Audio("someSound.ogg");
2. Call play() method to start the sound : a.play();
3. Put the currentTime property back to 0 to stop it : a.currentTime = 0;
4. Play the sound again : a.play();
Actual Results:  
The second call to play() won't start until the first call is over.
It creates a delay, or worst, if play() calls are very close, it skips some sounds

Expected Results:  
The sound stops and starts right back with no delay or wait

Works on Firefox 3.6

Comment 1

[Boris Zbarsky [:bzbarsky]]

Nominating for blocking given it's a regression.  I certainly see this on Mac.

Comment 2

[cajbir (:cajbir)]

Nightly build for 2010-06-22 exhibits the problem. Nightly build for 2010-06-21 does not. The following landings seem to be in the regression range:

b793c7686b4f	Bug 572299 - Stuttering audio playback with some Vorbis files (ffmpeg muxed).
5e852b461c06	Bug 555121 - Update in-tree libvorbis to 1.3.1.

Comment 3

[cajbir (:cajbir)]

Looks like the regression is in:

5e852b461c06    Bug 555121 - Update in-tree libvorbis to 1.3.1.

Comment 4

[cajbir (:cajbir)]

Bug 600776 contains the fix for this issue. Applying the patch for that bug makes the test case run fine.

Comment 5

[Matthew Gregan [:kinetik]]

Bug 600776 changes the behaviour, but I don't think it contains the actual fix.  I realized today that the fix in Vorbis 1.3.2 that I was thinking of probably doesn't affect us, and verified in the debugger that we never hit it... so I couldn't come up with an explanation of how it would fix this bug.  I tested it and noticed it's better, but there are still sometimes long delays before the sound starts playing again.

Comment 6

[cajbir (:cajbir)]

Looks like I got the wrong regression commit. I did a bisect again and it appears to be:

b793c7686b4f    Bug 572299 - Stuttering audio playback with some Vorbis files
(ffmpeg muxed).

Comment 7

[Matthew Gregan [:kinetik]]

We might be entering buffering state where we weren't before, if that changeset caused this.  No wonder it's tricky to track down--that logic has changed quite a bit.  As it stands right now, mBufferExhausted has been completely removed (bug 603226).

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #7)
> We might be entering buffering state where we weren't before, if that changeset
> caused this.

I don't see any transition to buffering state in the debug logging when reproducing this bug, so I don't think it can be related to buffering. This file is small; it should be entirely cached anyway.

It looks to me like the problem is that the second play request comes in while the nsAudioStream is in a Drain() call, and that blocks until the audio finishes playing. The audio thread holds the audio monitor while it does this, so we can't take the monitor in order to stop and destroy the audio stream at the start of the SEEKING case in nsBuiltinDecoderStateMachine::Run().

I'm not sure how we could fix this... Make Drain() interruptable?

Comment 9

[Matthew Gregan [:kinetik]]

Are you seeing that on Windows?  It's probably worth mentioning bug 582513, where drain is blocking for up to 2 seconds on Linux/PulseAudio systems, which is going to conflate issues when investigating this bug.

Comment 10

[cajbir (:cajbir)]

As Matthew noted in comment 7, the code for that changeset has changed quite a bit. It looks like another change down the line also broke things.

I did another bisect starting from that changeset where I reverted that change allowing me to find the other changeset that introduced the issue. That came up with:

Bug 589626 - Make video buffing logic consistent. 
http://hg.mozilla.org/mozilla-central/rev/d43718e5b27a

Comment 11

[cajbir (:cajbir)]

The offending change in that changeset is:

-      while (!mStopDecodeThreads &&
-             mBufferExhausted &&
-             mState != DECODER_STATE_SHUTDOWN)
-      {
-        mon.Wait();
-      }

It's the removal of the mon.Wait() call. Possibly for this smallish audio file the DecodeLoop spends almost all of it's time locked in the monitor which impedes the ability of the other thread(s) to handle the seek request.

Comment 12

[cajbir (:cajbir)]

Attachment: make drain interruptible

Attempts to implement Chris Pearce's idea in comment 8 to make Drain interruptible. Instead of draining I loop, Waiting for short periods, and check for seeking or shutdown in the loop. When the audio position no longer changes or we are at the end I then Drain().

This fixes the issue reported in the bug. It's not a perfect solution but I'm wondering if it'll be ok until 'the great audio rewrite' occurs post FF 4.

Comment 13

[cajbir (:cajbir)]

Attachment: HTML file I used for testing

Put a file, sound.ogg, in the same directory as the HTML file. Hit space bar many times.

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 507336 [details] [diff] [review]
make drain interruptible

* nsBuiltinDecoderStateMachine::Wait(PRUint32) accepts times in ms, so no need to call PR_MillisecondsToInterval().
* mAudioStream could be destroyed if the state machine is shutdown while the thread is sleeping, so derefs of mAudioStream need guards.
* mAudioEndTime is the media time at the end of the audio pushed to hardware, e.g. start_time_of_first_sample + duration_of_audio. If someone has a media with non-zero start time, this loop would end up waiting forever.
* You can remove the need to hold the audio monitor (and instead hold the decoder monitor most of the time and use the state data directly), if you calculate the time remaining as |mAudioEndTime - GetMediaTime()|. Then you can loop while |(mAudioEndTime - GetMediaTime()) > 0| and while we're not seeking and not shutdown. Then you only need to release the decoder monitor and take the audio monitor when you drain after the waiting loop exits. That should make the code a lot simpler.
* Should we still drain the event manager if playback has stopped while the audio thread slept? We call mEventManager->Clear() when we call nsBuiltinDecoderStateMachine::StopPlayback(AUDIO_SHUTDOWN), and then set mAudioStream=nsnull, so only drain the mEventManager while holding the audio monitor and when mAudioStream is non-null?

Comment 15

[cajbir (:cajbir)]

Attachment: make drain interruptible

Address review comments

Comment 16

[cajbir (:cajbir)]

Attachment: make drain interruptible

Change style of 'while' loop to match usage in file.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2/2: Don't drain audio stream when paused

Patch to resolve additional problem detected on Windows, where drain does not return if called on a paused audio stream. Stream can become paused while we wait in the "wait while we still have audio left to play" loop.

To reproduce the problem this fixes, apply doublec's patch, and close the browser while spamming keypresses on http://t4ils.free.fr/audioBug/ . I think this is Windows only.

Comment 18

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/ee00364d3092
http://hg.mozilla.org/mozilla-central/rev/59bcb91cc348

Comment 19

[Aaron Train [:aaronmt]]

Verified fixed on 

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110203 Firefox/4.0b12pre

20110204030345
http://hg.mozilla.org/mozilla-central/rev/847a825087f2