460871 - timeupdate event not fired during playback position update

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[cajbir (:cajbir)]

Section 4.8.10.8 of the spec in the url states:

"If the time was reached through the usual monotonic increase of the current playback position during normal playback, the user agent must then queue a task to fire a simple event called timeupdate at the element. (In the other cases, such as explicit seeks, relevant events get fired as part of the overall process of changing the current playback position.)"

Although this is hidden away in the cue ranges section, it needs to happen on normal playback without cue ranges.

Comment 1

[cajbir (:cajbir)]

Attachment: testcase

The current time on this page should update on every frame of the video. Currently it only updates when seeking, or some other special change of the current time occurs.

Comment 2

[cajbir (:cajbir)]

Attachment: First cut at a patch

Comment 3

[cajbir (:cajbir)]

Attachment: Merges Invalidate and timeupdate events

This patch does not dispatch an event if one is already outstanding and it processes the invalidate and the timeupdate in a single posted event rather than two individual events.

Comment 4

[cajbir (:cajbir)]

Attachment: Must remember to commit changes before generating patch...

Comment 5

[cajbir (:cajbir)]

Attachment: Adds a couple of tests and fixes for issues raised by those tests

This adds two extra tests. One to check that the video has the correct size on the first timeupdate. This exposed a timing issue where the media size was updated after the first frame was displayed, so the first frame had the incorrect size.

The second is that after the seek the timeupdate event is raised. I moved the responsibility for this from nsHTMLMediaElement to the actual backend.

Comment 6

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

+          if (mState != DECODER_STATE_SHUTDOWN) {
+            // Wait for the time of one frame so we don't tight loop
+            // and we need to release the monitor so timeupdate and
+            // invalidate's on the main thread can occur.
+            mon.Wait(PR_MillisecondsToInterval(PRInt64(mCallbackPeriod*1000)));

Why is Wait() (no timeout) insufficient?

+  if (mElement && lastTime != mCurrentTime) {

Accessing mCurrentTime here without the monitor held seems bad.

+  if (mDecodeStateMachine) {
+    mDecodeStateMachine->ClearPositionChangeFlag();
+  }

This should happen before you drop the lock and fire the event. Otherwise we've got a race where events can be lost, I think.

Comment 7

[cajbir (:cajbir)]

Attachment: Address review comments

> Why is Wait() (no timeout) insufficient?

This is happending during the COMPLETED state. There are no NotifyAll's occurring from the main thread except when a state change occurs to SHUTDOWN. The loop needs to play the remaining buffered frames. A Wait with no timeout would display one frame and then wait for the SHUTDOWN.

> Accessing mCurrentTime here without the monitor held seems bad.

mCurrentTime is only read and written from the main thread, and this function is only run from within the main thread. It should be safe to access without the monitor?

> This should happen before you drop the lock and fire the event. 
> Otherwise we've got a race where events can be lost, I think.

Good point, fixed.

Comment 8

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

Comment on attachment 344403 [details] [diff] [review]
Address review comments

right

Comment 9

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/9f3f0b99cf37

Comment 10

[Justin Dolske [:Dolske]]

Do these events really need to be fired every frame? That results in dozens of events per second, when seems inefficient. At least for the purposes of playback controls, receiving an event every 250ms (or so) would be sufficient. [I can do filtering in the event handler, but was thinking it would be better to avoid getting them in the first place.]

Comment 11

[cajbir (:cajbir)]

According to my understanding of the specification they need to be fired every time the playback position changes, which means every frame.