Closed Bug 463968 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: kinetik, Assigned: kinetik)

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

*** 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.
Attached patch patch v0 (obsolete) — Splinter Review
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.
Attachment #347229 - Flags: superreview?(roc)
Attachment #347229 - Flags: review?(roc)
+      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?
+    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'.
Attached patch patch v1Splinter Review
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.
Attachment #347229 - Attachment is obsolete: true
Attachment #347257 - Flags: superreview?(roc)
Attachment #347257 - Flags: review?(roc)
Attachment #347229 - Flags: superreview?(roc)
Attachment #347229 - Flags: review?(roc)
Attachment #347257 - Flags: superreview?(roc)
Attachment #347257 - Flags: superreview+
Attachment #347257 - Flags: review?(roc)
Attachment #347257 - Flags: review+
This should be checked in soon to fix spurious oranges on the tinderboxes.
Keywords: checkin-needed
Attachment #347257 - Flags: approval1.9.1b2?
Target Milestone: --- → mozilla1.9.1b2
Attachment #347257 - Flags: approval1.9.1b2?
http://hg.mozilla.org/mozilla-central/rev/8036638f1a83
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
(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
You need to log in before you can comment on or make changes to this bug.