Closed Bug 611994 Opened 9 years ago Closed 9 years ago

The play/pause button doesn't switch to 'play' when the file has ended

Categories

(Core :: Audio/Video, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: spammaaja, Assigned: cpearce)

References

()

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b7) Gecko/20100101 Firefox/4.0b7
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101112 Firefox/4.0b8pre

The play/pause button stays as 'pause' when the file has ended. The 'play' button blinks for a second.

Reproducible: Always

Steps to Reproduce:
1. Play an <audio> file, e.g. on http://en.wiktionary.org/wiki/English#Pronunciation

Actual Results:  
The 'pause' button stays as 'pause' even though the file has ended.

Expected Results:  
The 'pause' button should become 'play'.
Version: unspecified → Trunk
The problem here is that we're firing a timeupdate event after the ended event has fired. This timeupdate event is being fired from a timer in nsMediaDecoder. The controls are assuming that timeupdate events are only fired while the media is playing. I think this is a reasonable assumption, and we shouldn't fire timeupdate events from that timer after we've fired the ended event (unless we start playing again of course).
Assignee: nobody → chris
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
The problem is we're receiving a timeupdate event after playback has ended from an interval timer which periodically fires timeupdate. But that timer isn't necessary, since when we change the playback position we fire a timeupdate event if we've not fired one recently anyway. That is, the timer checks periodically if we need to fire timeupdate, but we're already checking if we need to fire timeupdate when we update the currentTime on every frame anyway. The interval timer also causes us to fire the timeupdate event *after* the time has stopped updating - the cause of this bug.

This patch:
* Removes the interval timer which fires timeupdate, and moves all timeupdate firing code into nsHTMLMediaElement, so that it can record the time of specified timeupdate firings as well.
* Changes periodic timeupdates to not fire if *any* timeupdate event has fired recently, currently we don't take into account specified timeupdates, which violates the spec.
* Adds a few more timeupdate firings which were required by the spec.
* Moves the tests from test_timeupdate_seek into test_seek tests, since test_timeupdate_seek was getting confused by the extra timeupdates, and didn't really warrant being its own test once I made it non-confused.
* Removes test_timeupdate_seek.
* Test original problem (receive timeupdate after ended) in test_timeupdate_small_files.

This patch makes our timeupdate behaviour spec compliant, and ensures that our play button changes to and stays as a play button when playback ends.

This patch was green on a "try: -a" push on TryServer.
Attachment #492789 - Flags: review?(roc)
Comment on attachment 492789 [details] [diff] [review]
Patch: Don't fire timeupdate events in an interval timer

+  if (!aPeriodic ||
+      ((mTimeUpdateTime.IsNull() ||
+        now - mTimeUpdateTime >= TimeDuration::FromMilliseconds(TIMEUPDATE_MS)) &&
+        mLastCurrentTime != time)) {

Fix indent on the last line, it's confusing. It might be even better to put mLastCurrentTime != time on the second line.
Attachment #492789 - Flags: review?(roc)
Attachment #492789 - Flags: review+
Attachment #492789 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/a6d06ecdfde1
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 604410
You need to log in before you can comment on or make changes to this bug.