Closed Bug 470983 Opened 11 years ago Closed 11 years ago

Video controls should ...buffering... indicate when ...buffering... playback is stalled

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

(Keywords: verified1.9.1)

Attachments

(2 files, 3 obsolete files)

The video controls should provide some kind of UI feedback when the video playback halts because it's run out of available data to play (ie, it's expecting more, but the network/server is slow).

RealPlayer's infamous "buffering" message is one example of this. YouTube just pauses with a centered throbber (which is usually hard to see against a busy background). The BBC News' media player, OTOH, has a pleasing effect for this state: the video fades to a darker color over 1-2 seconds, and a centered throbber is shown. A fade is great and should be simple to implement, the throbber might be trickier. [If it's centered in the video frame, it probably needs to scale with video size. Perhaps we could place a fixed-size throbber on the controls or in the corner, but I'm not sure how that would look. Or maybe we can find a fixed size that doesn't look enormous on small videos, yet doesn't get lost inside a giant video.]

The HTML5 spec has a "stalled" event that should trigger this. Looks like this isn't currently supported, but will be added in the patch to bug 464376.
Talked with Boriss about this for a bit. Let's try fixed-size throbber + fading to see if it works well enough. If that's insufficient, we can can look at scaling/location alternatives.

[An in-between idea that occurred to me was to generate a large throbber, say 128x128, and downscale that with CSS as needed.]
Summary of some more discussion, in the context of loading an autoplay video. The basic desired steps are this:

0) Video element created
1) Load begins, <video> content area is empty (controls not addressed here)
2) Fade in  a throbber + dim video (w/semitransparent black overlay) when the
   first of these happens:
     - |loadeddata| fires (1st frame is visible)
     - about 2 seconds elapse
3) ...waiting for enough data to buffer before starting autoplay...
4) If we stop receiving data (|stalled| event fires), additionally add some text (eg "Stalled...") below the throbber. Remove text when we start getting data again.
5) When playback beings (|playing| fires), remove the throbber + undim video.

Key points of note are:

* Don't show a throbber/dimming right away, since usually you'll get the 1st frame quickly. Having a single "1st frame + throbber" transition is cleaner than blinking both in at slightly different times. But don't wait too long, or the user won't know what's happening with the video.

* The addition of the text (@ #4) is to help mark it as a different state. We considered just rolling these states together (since for the user it's still "video isn't playing"), but thought the distinction was useful... One state means playback should start soon (at least ideally) so stay tuned, while the other state means you can divert your attention elsewhere because things are not going so well.

We should also do the same thing (wait ~2s, then throbber+dim) when seeking, especially since it's so slow with Ogg.
Assignee: nobody → dolske
Target Milestone: --- → mozilla1.9.2a1
Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
Most of this is refactoring fadeControls() and part of onMouseInOut() so that the fading login is reusuable. This gets reused to fade in the "busy" UI, and will also be used later when we add a time indicator that fades in over the scrubber thumb.

Right now this is only hooked up to seeking, so that long seeks trigger a throbber. The delay and throbber are just placeholders at the moment.

I next need to look at exactly what events to watch for the other times we want to show the throbber UI, and if the video backend is correcting supporting those yet.
Flags: wanted1.9.1?
Blocks: TrackAVUI
Depends on: 476811
Depends on: 476813
Attached patch Patch v.2 (WIP) (obsolete) — Splinter Review
This is a mostly-complete implementation, but only hooked up for slow seeking. Waiting on a new throbber design from Boriss, and we need to figure out if we can get away with a single throbber size or if we'll need small/medium/large versions for various video sizes.

Eventually this UI needs to be hooked up to the other "busy" states (see comment 0 and comment 2), but since those seem to need some more backend work, it might make sense to land this as-is (ie, slow-seeking feedback only), and hook up the other bits later.
Attachment #360279 - Attachment is obsolete: true
Flags: wanted1.9.1? → wanted1.9.1+
Attached image events flowchart
New patch coming up. Here's a crude flowchart with the relevant events, which kind of gives a high-level view of what the code is trying to do.
Attached patch Patch v.3 (obsolete) — Splinter Review
Some of these events are tricky to catch in normal usage, so it's not really possible to write automated tests for this (yet?). I've written a PHP script to control the data transfer, so that it's easy to simulate a slow or stalled network... See: http://dolske.net/mozilla/tests/video/tests.html

A fair chunk of the patch is refactoring the code which drives the control fading, so that both the controls and throbber can reuse the same code. Basically, the state for each is moved into an object (controlFader, throbberFader), and the fading code is told which one it should use. This unfortunately churns some code for the diff, but it's largely just renaming variables, and not a change in behavior.
Attachment #360648 - Attachment is obsolete: true
Attachment #363813 - Flags: review?(enndeakin)
Comment on attachment 363813 [details] [diff] [review]
Patch v.3

>+            <vbox class="throbberOverlay">
>+                <box class="throbber" flex="1"/>
>+                <spacer class="controlsSpacer"/>
>+            </vbox>

You could just use a single element here and just use a 28 pixel bottom margin.

Also, <image> could be used which should layout to the size of the actual image rather than hardcoding a size.


>+                        case "canplaythrough":
>+                            this.startFadeOut(this.throbberFader);
>+                            break;

You could combine this case with the 'playing' event.
Attachment #363813 - Flags: review?(enndeakin) → review+
Comment on attachment 363813 [details] [diff] [review]
Patch v.3

Marking UI+ on throbber
Attachment #363813 - Flags: ui-review+
Attached patch Patch v.4Splinter Review
Updated with nits. Also, Boriss wanted a slightly smaller throbber than was in the last patch.
Attachment #363813 - Attachment is obsolete: true
Oops, forgot to reply to this bit:

(In reply to comment #7)
> Also, <image> could be used which should layout to the size of the actual image
> rather than hardcoding a size.

We can't use <image>s in the controls, because they cause |load| events to be fired when the image loads, and could cause a load-listener on the video element to become confused.
Pushed http://hg.mozilla.org/mozilla-central/rev/c0c058f2f503
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Does this need 1.9.1 landing?
Comment on attachment 364197 [details] [diff] [review]
Patch v.4

a191=beltzner
Attachment #364197 - Flags: approval1.9.1+
Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090527 Shiretoko/3.5pre Ubiquity/0.1.5 and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090527 Minefield/3.6a1pre
Status: RESOLVED → VERIFIED
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.