463968 - test_wav_ended1.html can fail on slow machines with no audio hardware

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

*** 27731 ERROR TEST-UNEXPECTED-FAIL | /tests/content/media/video/test/test_wav_ended1.html | Checking currentTime at end: 1.25

This can occur if the test machine is running slowly and it takes a long time for the nsWaveStateMachine to transition from ENDED to SHUTDOWN.  The fake audio backend's system clock is still ticking during this time, so it can report durations past the end of the media.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0

Don't let nsAudioStream::GetTime, when using the fake audio backend, report a stream time beyond the end if the samples written to the stream.

Read the final stream time when Wave playback ends, not when the decoder is shutdown.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      CloseAudioStream();
 
       if (mState != STATE_SHUTDOWN) {
         nsCOMPtr<nsIRunnable> event =
@@ -637,10 +639,6 @@
       break;
 
     case STATE_SHUTDOWN:
-      if (mAudioStream) {
-        mTimeOffset = mAudioStream->GetTime();
-      }
-      CloseAudioStream();

Hmm... Does moving CloseAudioStream here mean that we fail to close it during abnormal SHUTDOWN transitions?

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (mPaused) {
+      return mPauseTime;
+    } else {
+      float curTime = CurrentTimeInSeconds() - mStartTime;
+      float maxTime = float(mSamplesBuffered) / mRate / mChannels;
+      return NS_MIN(curTime, maxTime);
+    }

Also, don't do 'else' after 'return'. Just make the else-block follow the 'if'.

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Address review comments.

The removal of CloseAudioStream from SHUTDOWN was totally wrong, yeah.  Thanks for spotting that.  After thinking about it further, I realised that all that was necessary was to add additional GetTime/CloseAudioStream calls in the ENDED case.

Comment 5

[Matthew Gregan [:kinetik]]

This should be checked in soon to fix spurious oranges on the tinderboxes.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/8036638f1a83

Comment 7

[Tony Chung [:tchung]]

(In reply to comment #5)
> This should be checked in soon to fix spurious oranges on the tinderboxes.

Clint, since you're watching rando orange tinderboxen, can you verify if this bug is fixed?   Thanks, Tony