The HTML 5 specification for media elements defined the 'progress' event as an HTML "Progress" event. This had extra attributes for length and current position. This is how the 'progress' event is currently implemented. This results in some duplicated code to handle and dispatch these special progress events vs 'simple' events. See bug 464398 comment 8 for example. We should change the 'progress' event to follow the spec and be simple events. We've held off from doing this as we use the additional progress event data for things like the progress bar. Bug 462957 which is close to being fixed and provides more accurate information we can use.

Requested blocking as it blocks a blocking bug 464398

I should note, because I forgot to mention it in comment 0, that the spec changed to require the "progress" events to be non Progress events which is why I'm suggesting we change to be spec compliant.

With the change to using 'buffered' instead of progress events in the video controls there were failures when running some tests. This was due to GetBuffered in the Ogg backend raising an error. The fix in the patch was discussed and worked out with Chris Pearce.

Comment on attachment 463428 [details] [diff] [review] Patch 3: Fix for GetBuffered in Ogg backend For some reason this patch to detect PAGE_SYNC_ERROR seems to cause an infinite loop in GetBuffered sometimes. It's appears when running test_play_errors. I've removed the review request and will look into it further.

Comment on attachment 463426 [details] [diff] [review] Patch 2: Change video controls to use 'buffered' instead of progress events >+++ b/toolkit/content/widgets/videocontrols.xml While we're here, init() does: 370 if (this.video.networkState == this.video.NETWORK_LOADED) 371 this.bufferBar.setAttribute("value", 100); 372 else 373 this.bufferBar.setAttribute("value", 0); Can NETWORK_LOADED doesn't even happen any more, does it? Maybe that should be something like if (this.video.readyState >= HAVE_METADATA) this.showBuffered() else this.bufferBar.setAttribute("value", 0); *sigh* We really need to get the videocontrols updated to the modern era. They're _so_ early-2010.

Ignores errors when GetBuffered is called and fixes infinite loop issue that can occur as a result.

Makes change suggested by review in comment 7. Carried forward r+.

oops, checkin comment had wrong bug number

As discussed with Chris, here's an alternative patch for the controls. Using the end time of the latest buffered range doesn't work well in a bunch of cases. For example, if the user seeks near the end of the file then seeks back to the beginning, the displayed buffer bar shows that data is buffered to the end of the old seek position even when this isn't the case. Also when seeking to the end of the file to fetch the index (in WebM) or the duration (in Ogg), it'll look like the entire file is buffered. The attached patch displays the buffered range that the current playback position is within.

My patch would work better if we fixed bug 588312.

This rolls up and rebases patch 1 and patch 3. Carrying forward r+ from roc and cpearce. I haven't included the controls changes in patch 3 - waiting for dolskes review of Matthews new patch.

Update to trunk to fix breakage from landing of Audio API patch.

Comment on attachment 466922 [details] [diff] [review] Alternative patch 2: Change video controls >- if (this.video.networkState == this.video.NETWORK_LOADED) >- this.bufferBar.setAttribute("value", 100); >+ if (this.video.readyState >= HAVE_METADATA) >+ this.showBuffered(); Presumably you mean |this.video.HAVE_METADATA|. ;) >+ showBuffered : function() { ... >+ var currentTime = this.video.currentTime; >+ var b = this.video.buffered; >+ for (var i = 0; i < b.length; ++i) { Two questions: 1. Does the media backend merge adjacent TimeRanges? EG, can you end up with ranges like [0,10)[10,20)[20,40)? 2. Is the a practical/actual limit to the number of ranges that might be iterated over here? This code might be invoked a few times every second (with XPCOM overhead for each .start() / .end() call). If something has triggered lots of skips (say, a script grabbing thumbnails at 15 second intervals), that could be a lot of timeranges... Binary search though the ranges might help too. >+ if (currentTime >= b.start(i) && currentTime < b.end(i)) { >+ this.bufferBar.max = duration; >+ this.bufferBar.value = Math.round(b.end(i) * 1000); >+ break; >+ } Nit: you've got some tabs here. Finally, would be worth setting |this.bufferBar.value = 0| when a matching timerange can't be found.

Fixed nits and changes search of 'buffered' from linear to binary.

Unfortunately the fix for the infinite loop issue in comment 8 isn't working. GetBuffered still i sometimes when running test_playback.

Same as v2, but correctly round down the probe index in bsearch.

Comment on attachment 470698 [details] [diff] [review] Fix for intermittent infinite loop Looks good. Nits: I would combine the "if (mTheoraState) {\n if (mTheoraState->Init())" into one if condition to save one line and one level of indentation. Not a big deal though. Should "possibly" in your comment have a capital 'P'?

Address review comments on the infinite loop patch. r+ from cpearce carried forward

Comment on attachment 470693 [details] [diff] [review] Alternative patch 2: Change video controls v3 >+ function bsearch(hs, n, cmp) { >+ var length = hs.length; >+ var l = 0; >+ var h = length; I was going to give a free pass for using 1-letter variable names since it's a well-known algorithm, but then I found myself looking at "var m = l + ((h - l) >> 1);" and squinting at the 1s and ls. Use a few more letters? :)

adr rvw cmts Also, I realized that we need to call showBuffered() when we seek because it's possible we're seeking into a range where we won't need to be downloading (and thus won't be firing progress events to cause the buffer display to be updated). I added a call to both seeking and seeked--once bug 588312 is fixed we can remove the calls in seeked.

Rolled up patches and rebased. This contains the 'alternative patch' for the controls and I've obsoleted the original.

Rebased on top of bug 588312. Removes bit from videocontrols.xml mentioned in Bug 588312 comment 3. Carried forward r+. Note that bug 588312 must be landed before this.

backed out due to suspect increased orange in test_videocontrols_direction http://hg.mozilla.org/mozilla-central/rev/f085aacfb4d2 http://hg.mozilla.org/mozilla-central/rev/73ab2c3c5ad9 see bug 595463.

Replaces usage of progress event data with buffered to fix video controls intermittent test failure

http://hg.mozilla.org/mozilla-central/rev/3da55bab7796 http://hg.mozilla.org/mozilla-central/rev/eea753ee0156 Sorry, got the patch credit wrong on the test fix.

From a developer's point of view, does this make any obvious difference? Doesn't look like it.

The only visible change is that the event object that the 'progress' event receives no longer has the additional members that existed in the older HTML 5 spec. If these weren't documented previously then no docs changes should be needed.

Does that mean the progress event no longer provides lengthComputable, loaded, and total attributes? How do you determine the progress details without that information?

Use the 'buffered' DOM attribute on media elements instead.