The default bug view has changed. See this FAQ.

Make media "progress" events be 'simple' Events, not 'progress' Events

RESOLVED FIXED

Status

()

Core
Audio/Video
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: cajbir, Assigned: cajbir)

Tracking

({dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 14 obsolete attachments)

23.93 KB, patch
cajbir
: review+
Details | Diff | Splinter Review
1.11 KB, patch
kinetik
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

7 years ago
Requested blocking as it blocks a blocking bug 464398
Blocks: 464398
blocking2.0: --- → ?
Depends on: 462957
Summary: Made media "progress" events be 'simple' Event's, not 'progress' Events → Make media "progress" events be 'simple' Event's, not 'progress' Events
(Assignee)

Comment 2

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

Updated

7 years ago
Assignee: nobody → chris.double
Status: NEW → ASSIGNED
(Assignee)

Comment 3

7 years ago
Created attachment 463425 [details] [diff] [review]
Patch 1: Changes media progress events to be normal events
Attachment #463425 - Flags: review?(roc)
(Assignee)

Comment 4

7 years ago
Created attachment 463426 [details] [diff] [review]
Patch 2: Change video controls to use 'buffered' instead of progress events
Attachment #463426 - Flags: review?(dolske)
(Assignee)

Comment 5

7 years ago
Created attachment 463428 [details] [diff] [review]
Patch 3: Fix for GetBuffered in Ogg backend

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.
Attachment #463428 - Flags: review?(chris)
(Assignee)

Comment 6

7 years ago
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.
Attachment #463428 - Flags: review?(chris)
Attachment #463425 - Flags: review?(roc) → review+
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.
Attachment #463426 - Flags: review?(dolske) → review+
(Assignee)

Comment 8

7 years ago
Created attachment 464304 [details] [diff] [review]
Patch 3: Fix for GetBuffered in Ogg backend

Ignores errors when GetBuffered is called and fixes infinite loop issue that can occur as a result.
Attachment #463428 - Attachment is obsolete: true
Attachment #464304 - Flags: review?(chris)
(Assignee)

Comment 9

7 years ago
Created attachment 464313 [details] [diff] [review]
Patch 2: Change video controls to use 'buffered' instead of progress events

Makes change suggested by review in comment 7. Carried forward r+.
Attachment #463426 - Attachment is obsolete: true
Attachment #464313 - Flags: review+
Attachment #464304 - Flags: review?(chris) → review+
(Assignee)

Updated

7 years ago
Blocks: 586924
Summary: Make media "progress" events be 'simple' Event's, not 'progress' Events → Make media "progress" events be 'simple' Events, not 'progress' Events

Comment 10

7 years ago
http://hg.mozilla.org/mozilla-central/rev/4bea4e06aa3f
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 11

7 years ago
oops, checkin comment had wrong bug number
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Created attachment 466922 [details] [diff] [review]
Alternative patch 2: Change video controls

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.
Attachment #466922 - Flags: review?(dolske)
Attachment #466922 - Attachment is patch: true
Attachment #466922 - Attachment mime type: application/octet-stream → text/plain
My patch would work better if we fixed bug 588312.
Depends on: 588312
Keywords: dev-doc-needed
(Assignee)

Comment 14

7 years ago
Created attachment 468935 [details] [diff] [review]
Rolled up and rebased patch

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.
Attachment #463425 - Attachment is obsolete: true
Attachment #464313 - Attachment is obsolete: true
Attachment #468935 - Flags: review+
(Assignee)

Updated

7 years ago
Attachment #464313 - Attachment is obsolete: false
(Assignee)

Updated

7 years ago
Attachment #464304 - Attachment is obsolete: true
(Assignee)

Comment 15

7 years ago
Created attachment 469714 [details] [diff] [review]
Rolled up and rebased patch

Update to trunk to fix breakage from landing of Audio API patch.
Attachment #468935 - Attachment is obsolete: true
Attachment #469714 - Flags: review+
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.
Attachment #466922 - Flags: review?(dolske) → review+
Blocks: 591790
Created attachment 470640 [details] [diff] [review]
Alternative patch 2: Change video controls v2

Fixed nits and changes search of 'buffered' from linear to binary.
Attachment #466922 - Attachment is obsolete: true
Attachment #470640 - Flags: review?(dolske)
(Assignee)

Comment 18

7 years ago
Unfortunately the fix for the infinite loop issue in comment 8 isn't working. GetBuffered still i sometimes when running test_playback.
Created attachment 470693 [details] [diff] [review]
Alternative patch 2: Change video controls v3

Same as v2, but correctly round down the probe index in bsearch.
Attachment #470640 - Attachment is obsolete: true
Attachment #470693 - Flags: review?(dolske)
Attachment #470640 - Flags: review?(dolske)
(Assignee)

Comment 20

7 years ago
Created attachment 470698 [details] [diff] [review]
Fix for intermittent infinite loop
Attachment #470698 - Flags: review?(chris)
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'?
Attachment #470698 - Flags: review?(chris) → review+
(Assignee)

Comment 22

7 years ago
Created attachment 470713 [details] [diff] [review]
Fix for intermittent infinite loop

Address review comments on the infinite loop patch. r+ from cpearce carried forward
Attachment #470698 - Attachment is obsolete: true
Attachment #470713 - Flags: review+
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? :)
Attachment #470693 - Flags: review?(dolske) → review+
Created attachment 471360 [details] [diff] [review]
Alternative patch 2: Change video controls v4

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.
Attachment #470693 - Attachment is obsolete: true
Attachment #471360 - Flags: review+
blocking2.0: ? → betaN+
(Assignee)

Comment 25

7 years ago
Created attachment 472321 [details] [diff] [review]
Rolled up and rebased patch

Rolled up patches and rebased. This contains the 'alternative patch' for the controls and I've obsoleted the original.
Attachment #464313 - Attachment is obsolete: true
Attachment #469714 - Attachment is obsolete: true
Attachment #470713 - Attachment is obsolete: true
Attachment #471360 - Attachment is obsolete: true
Attachment #472321 - Flags: review+
(Assignee)

Comment 26

7 years ago
Created attachment 473911 [details] [diff] [review]
Rolled up and rebased patch

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.
Attachment #472321 - Attachment is obsolete: true
Attachment #473911 - Flags: review+
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Updated

7 years ago
Blocks: 571822
(Assignee)

Comment 27

7 years ago
http://hg.mozilla.org/mozilla-central/rev/96b74fec2915
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Blocks: 595463
(Assignee)

Comment 29

7 years ago
Created attachment 474858 [details] [diff] [review]
fix for video controls direction test failure

Replaces usage of progress event data with buffered to fix video controls intermittent test failure
Attachment #474858 - Flags: review?(kinetik)
Attachment #474858 - Flags: review?(kinetik) → review+
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.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
From a developer's point of view, does this make any obvious difference? Doesn't look like it.
(Assignee)

Comment 32

7 years ago
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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 778105
You need to log in before you can comment on or make changes to this bug.