Closed Bug 460871 Opened 16 years ago Closed 16 years ago

timeupdate event not fired during playback position update

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cajbir, Assigned: cajbir)

References

()

Details

Attachments

(2 files, 4 obsolete files)

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.
Attached file 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.
Depends on: 449154
Attached patch First cut at a patch (obsolete) — Splinter Review
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.
Attachment #344025 - Attachment is obsolete: true
Attachment #344232 - Flags: superreview?(roc)
Attachment #344232 - Flags: review?(roc)
Attachment #344232 - Attachment is obsolete: true
Attachment #344233 - Flags: superreview?(roc)
Attachment #344233 - Flags: review?(roc)
Attachment #344232 - Flags: superreview?(roc)
Attachment #344232 - Flags: review?(roc)
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.
Attachment #344233 - Attachment is obsolete: true
Attachment #344240 - Flags: superreview?(roc)
Attachment #344240 - Flags: review?(roc)
Attachment #344233 - Flags: superreview?(roc)
Attachment #344233 - Flags: review?(roc)
+          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.
> 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.
Attachment #344240 - Attachment is obsolete: true
Attachment #344403 - Flags: superreview?(roc)
Attachment #344403 - Flags: review?(roc)
Attachment #344240 - Flags: superreview?(roc)
Attachment #344240 - Flags: review?(roc)
Comment on attachment 344403 [details] [diff] [review]
Address review comments

right
Attachment #344403 - Flags: superreview?(roc)
Attachment #344403 - Flags: superreview+
Attachment #344403 - Flags: review?(roc)
Attachment #344403 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
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.]
According to my understanding of the specification they need to be fired every time the playback position changes, which means every frame.
You need to log in before you can comment on or make changes to this bug.