571822 - Fire timeupdate event less frequently than once per frame

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

The HTML 5 spec says:

When the earliest possible position changes, then: if the current playback position is before the earliest possible position, the user agent must seek to the earliest possible position; otherwise, if the user agent has not fired a timeupdate event at the element in the past 15 to 250ms and is not still running event handlers for such an event, then the user agent must queue a task to fire a simple event named timeupdate at the element.

Firefox fires the timeupdate event once per frame.  Safari 5 and Chrome 6 fire every 250ms.  Opera 10.50 fires every 200ms.

I propose that we fire it every 250ms as allowed by the spec.  This will bring our behaviour into line with other browsers that implement <video> and means that web developers developing on Firefox won't have unexpected behaviour with other browsers (and vice versa) when using timeupdate.  There are also potential performance benefits to firing this event less often.

The downside is that some of our video/canvas demos that grab video frames will slow down dramatically as they're driven off of timeupdate.  My proposed solution is to change the demos that need faster updates to use a timer.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I thought the whole point of firing once a frame was to allow scripts to do per-frame processing (e.g. drawing into a canvas).  Would doing that off a timer really work well?

Comment 2

[cajbir (:cajbir)]

The problem is that the other browsers all seem to use the 250ms time interval. So anything built for Firefox that relies on the fact we fire at a faster rate is non-portable. And as a bonus for being non-portable we have a higher CPU usage than the other browsers.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Right, but have we tried for pushing in the direction of them firing more often?  There was certainly discussion about that on the mailing list at some point; not sure it went anywhere.

Comment 4

[Boris Zbarsky [:bzbarsky]]

One other question is whether it would make sense to treat timeupdate the way we do mutation events in terms of skipping firing if there are no listeners, if we do decide to fire it often.

Comment 5

[cajbir (:cajbir)]

We could but we'll always have listeners if controls are enabled since our own controls use it.

Comment 6

[Biju]

(In reply to comment #0)
> slow down dramatically as they're driven off of timeupdate.  My proposed
> solution is to change the demos that need faster updates to use a timer.

In that case some times we may be processing frame more than one time or not processing some frames. Which will be bad for some cool video processing web application in future.
So alternatively, why cant we have a read/write boolean property on video element like 
  video.mozTimeUpdateOnFrames
by default it should be false, and if programmer wants faster updates he can set it true.

Comment 7

[Matthew Gregan [:kinetik]]

(In reply to comment #5)
> We could but we'll always have listeners if controls are enabled since our own
> controls use it.

Right, and that listener already contains code to ignore timeupdates within a 333ms window.

(In reply to comment #6)
> In that case some times we may be processing frame more than one time or not
> processing some frames.

The latter can happen with timeupdate anyway, because there's some processing/event loop delay between timeupdate being raised and the listener script actually running.  You could deal with the former when using timers by checking the value of currentTime to see if it has advanced since the last time a frame was processed.

It's worth noting that the current spec wording prohibits firing timeupdate for every frame if the video's frame rate is greater than 66 frames per second.  Our current implementation doesn't follow this and will fire an timeupdate for every frame no matter how high the frame rate of the video.

Comment 8

[cajbir (:cajbir)]

Attachment: Fire timeupdate every 250ms instead of every frame

Change to stop firing timeupdate on every frame and instead fire it every 250ms if the value of currentTime has changed, driven by a timer in the same manner as progress events.

This is built on top of bug 584615. Note that test_playback hangs due to what seems to be a bug in GetBuffered in the ogg backend with chained files that bug 584615 triggers.

Comment 9

[Matthew Gregan [:kinetik]]

The Wave decoder needs to call Start/StopTimeupdate() directly because it doesn't inherit from nsBuiltinDecoder (unfortunately!).  Other than that the patch looks good.

Comment 10

[cajbir (:cajbir)]

Attachment: Fire timeupdate every 250ms instead of every frame

Address review comment by adding start/start of the timeupdate timer for the wave backend.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 470702 [details] [diff] [review]
Fire timeupdate every 250ms instead of every frame

> nsMediaDecoder::nsMediaDecoder() :
>   mProgressTime(),
>+  mTimeUpdateTime(),
>   mDataTime(),

This isn't necessary, the default ctor will be called anyway.  I don't know why the other two already do this, they're just wrong--please kill them all.

Comment 12

[cajbir (:cajbir)]

Attachment: Fire timeupdate every 250ms instead of every frame

Kill the default constructors as per review comment. r+ from kinetik carried forward.

Comment 13

[cajbir (:cajbir)]

It would be good to get this into FF 4 so we are work the same as the other browsers and authors don't depend on our behaviour for the lifetime of FF 4.

Comment 14

[cajbir (:cajbir)]

Attachment: Fire timeupdate every 250ms instead of every frame

Rebased on top of bug 584615 ready for landing. r+ carried forward.

Comment 15

[Biju]

Comment on attachment 473951 [details] [diff] [review]
Fire timeupdate every 250ms instead of every frame

>+// Number of milliseconds between timeupdate events as defined by spec
>+#define TIMEUPDATE_MS 250

Why cant TIMEUPDATE_MS be a property property of video like mozMinTimeupdate which can set by javascript developer.

Comment 16

[cajbir (:cajbir)]

I don't think it's worth it if none of the other browser developers are willing to do it. Perhaps you could ask in the HTML mailing list(s) (WHATWG or W3C). We're basically doing this to align with what the other browsers do.

Comment 17

[cajbir (:cajbir)]

http://hg.mozilla.org/mozilla-central/rev/3d42767d964d

Comment 18

[Marco Bonardo [:mak]]

backed out due to suspect increased orange in test_videocontrols_direction
http://hg.mozilla.org/mozilla-central/rev/e29874ead8b3
http://hg.mozilla.org/mozilla-central/rev/cc4df3b40018

see bug 595463.

Comment 19

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/7aab25634114

Comment 20

[Boris Zbarsky [:bzbarsky]]

Fwiw, this "breaks" the fps meter at http://people.mozilla.com/~vladimir/demos/jsfft/jsfft.html...

Comment 21

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Can we at least get an event added that notifies when a frame is available?  Otherwise code has no way to do per-frame processing; doing it off a timer would almost certainly mean both skipping frames and double-processing some frames.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #21)
> Can we at least get an event added that notifies when a frame is available? 
> Otherwise code has no way to do per-frame processing; doing it off a timer
> would almost certainly mean both skipping frames and double-processing some
> frames.

Apple vetoed it, so it wouldn't be standardized:
http://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-September/028483.html

We're working on adding (Mozilla specific) video frame painted statistics in bug 580531, would that be sufficient for an fps meter?

Comment 23

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

Well, Simon did say "I don't support the general use of a 'newFrame' callback except in the context of video processing via canvas" so maybe there's a possibility there.

Personally I think the "right" way to do video processing with canvas would be to allow both canvases and video elements to be hooked up to a Worker. The canvas would delegate control to the Worker, and the video decoder would send frames to the Worker for processing.