481040 - video controls should indicate when an error occurs

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Asa Dotzler [:asa]]

If you visit http://air.mozilla.com/ when we are not streaming, the video player attempts to connect to the stream and we see a progress indicator for that connecting but no stream is coming and I assume there's some error there that the video player isn't catching.

The spinning indicator in the video player should stop or maybe never start, if there's no content there.

Comment 1

[Justin Dolske [:Dolske]]

Yeah, Boriss and I talked discussed this during the course of bug 470983. The controls should listen for the error event (which is presumably being fired in this case, among others), and fade in the throbber overlay with the throbber replaced with an error icon.

Boriss, can you create an icon and I'll hook it up? Shouldn't be hard. [Famous last words.]

Comment 2

[Justin Dolske [:Dolske]]

Confirmed that an error event is indeed firing for Asa's specific example; generalizing bug.

Comment 3

[Matthew Gregan [:kinetik]]

You'll get an error event in Asa's case, because the media URL returns a 404. 
Most other failure cases should fire an error event, but there are two that
won't:  if the connection is interrupted after it's successfully opened, we
currently fire "load" rather than "error" in some cases due to bug 478109, and
if the decoder determines it can't decode the media after a specific decoder
backend has been selected because the MediaDecodeError event stuff is
unimplemented (I can't find the bug number for that one).

Comment 4

[Matthew Gregan [:kinetik]]

(In reply to comment #3)
> if the decoder determines it can't decode the media after a specific decoder
> backend has been selected because the MediaDecodeError event stuff is
> unimplemented (I can't find the bug number for that one).

Filed bug 481057, since there didn't seem to be anything covering this.

Comment 5

[Justin Dolske [:Dolske]]

I think we're really going to want to get at more information than MediaError.code provides. MEDIA_ERR_NETWORK, in particular, is rather broad.

Talking with Boriss, the error UI should really show both (1) an icon indicating the state and (2) some additional text specific to the problem. Sort of a lightweight, video-specific version of about:neterror. The text serves at least two major purposes:

  * Give users additional info to help with the frustration of just being told
    "something's broken, but we're not going to say what."
  * Give users something that they can Google for, ask a friend about, report
    to SUMO/tech support, etc.

I'm not sure how to deal with this in the spec, though. Enumerating every possible condition feels like fail, but perhaps with effort a reasonable set of errors types can be settled upon. (EG, some subset of error codes from HTTP + RTSP + ...). 

Another thought would be to have a MediaError.reason string that can take unspecified values. But then there are localization issues, and inevitable code will start depending on specific values/formats of the string.

It could be a Mozilla-specific extension, but the same problems remain.

Comment 6

[Justin Dolske [:Dolske]]

Also talked with Boriss about what to do with control visibility when an error occurs... Decided that if the error happens before the video has played (loaded?) anything, then we should suppress the controls. EG, if the video source is a 404, the controls are useless and just taunt the user. But if the video has been playing (and, say, hit a corrupted part of the video), we'll not do anything special, so the user can seek back to a functional part of the video or something. [We especially wouldn't want to yank away the controls after the user has usefully interacted with them, that's not nice.]

Comment 7

[Christopher Blizzard (:blizzard)]

You need an equivalent to the sad mac icon.

Comment 8

[Justin Dolske [:Dolske]]

Attachment: Patch v.1 (WIP)

WIP - Boriss is refining the icon.

Comment 9

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

Updates icon, removes controls when video is in error state.

I'm going to spin off the bit about more detailed error status code and error text into a separate bug. That's a trickier problem, and we should get this landed in the meantime.

Comment 10

[Neil Deakin]

Comment on attachment 368483 [details] [diff] [review]
Patch v.2

>-                debug : false,
>+                debug : true,

I don't think you meant to do this.


>-.throbber {
>+.statusIcon {
>+    margin-bottom: 28px; /* same height as .controlBar, to keep icon centered above it */
>+}
>+
>+.statusIcon[type="throbber"] {
>     background: url(chrome://global/skin/media/throbber.png) no-repeat center;
>+    width: 36px;
>     height: 36px;
>+}
>+
>+.statusIcon[type="error"] {
>+    background: url(chrome://global/skin/media/error.png) no-repeat center;
>     width: 36px;
>-    margin-bottom: 28px; /* same height as .controlBar, to keep throbber centered above it */
>+    height: 36px;
> }

You could just put the width and height in the .statusIcon block since they are both the same. The same with the windows version.

Comment 11

[Justin Dolske [:Dolske]]

(In reply to comment #10)
> You could just put the width and height in the .statusIcon block since they are
> both the same. The same with the windows version.

Oops, yeah, they were different sizes in the first patch, and I didn't think to collapse the rules.

Comment 12

[Justin Dolske [:Dolske]]

Pushed http://hg.mozilla.org/mozilla-central/rev/ad88f5d62c7c

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 368483 [details] [diff] [review]
Patch v.2

a191=beltzner

Comment 14

[Justin Dolske [:Dolske]]

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/33ae620b2ffa

Comment 15

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US;
rv:1.9.2a1pre) Gecko/20090413 Minefield/3.6a1pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre)
Gecko/20090413 Shiretoko/3.5b4pre