timeupdate event not fired during playback position update

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Created attachment 343985 [details]
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.
(Assignee)

Updated

9 years ago
Depends on: 449154
(Assignee)

Comment 2

9 years ago
Created attachment 344025 [details] [diff] [review]
First cut at a patch
(Assignee)

Comment 3

9 years ago
Created attachment 344232 [details] [diff] [review]
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.
Attachment #344025 - Attachment is obsolete: true
Attachment #344232 - Flags: superreview?(roc)
Attachment #344232 - Flags: review?(roc)
(Assignee)

Comment 4

9 years ago
Created attachment 344233 [details] [diff] [review]
Must remember to commit changes before generating patch...
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)
(Assignee)

Comment 5

9 years ago
Created attachment 344240 [details] [diff] [review]
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.
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.
(Assignee)

Comment 7

9 years ago
Created attachment 344403 [details] [diff] [review]
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.
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+
(Assignee)

Comment 9

9 years ago
http://hg.mozilla.org/mozilla-central/rev/9f3f0b99cf37
(Assignee)

Updated

9 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 9 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.]
(Assignee)

Comment 11

9 years ago
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.