708150 - Error text for media resource 404 is misleading

Status: RESOLVED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

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

Bug 513405 added text to the video controls when it fails to load. But the text in the case where the resource is missing is misleading. For example in this testcase:

data:text/html,<video src="invalid-url" controls>

The text "Video format of MIME type is not supported." is displayed. However I'd expect this to show a message saying "resource not found" or something like that.

I'm not sure yet if this is a bug in the controls code or the underlying media element.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Loading that data URI gives and getting the video element shows:

var v = document.getElementsByTagName("video")[0];
console.log(v.error.code); // prints 4 which is MEDIA_ERR_SRC_NOT_SUPPORTED
console.log(v.networkState); // prints 3 which is NETWORK_NO_SOURCE

Should we be prioritizing NETWORK_NO_SOURCE over MEDIA_ERR_SRC_NOT_SUPPORTED?

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 708150

This patch sets |errorNoSource| if video.error.code == MEDIA_ERR_SRC_NOT_SUPPORTED and video.networkState == NETWORK_NO_SOURCE

Comment 3

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

Comment on attachment 579914 [details] [diff] [review]
Patch for bug 708150

The test-case I filed this bug with is kinda invalid, as it's a data URI but doesn't fully-qualify the URL (i.e. it needs http://some-domain.com/path/ on the front of the URI). 

But with a fuly-qualified URL/testcase such as data:text/html,<video src="http://pearce.org.nz/invalid-url" controls> I get the same error, but that's because my server's 404 page has MIME type "text/html", so our error text kinda makes sense in that case. So I think we can not worry about this case...

But I still think we should change our error text in another case...

Take a look at this page:
http://pearce.org.nz/video/error.html

There I have a variety of cases where we should display the error text, and it shows the values of error.code and networkState. In every case where we failed to load from a <source> child we're showing the "Video not found" message, which is misleading, since we actually found a resource, we just can't play them.

So I think all we should change the string "Video not found" to "No video with supported format and MIME type found", and we'll have a catch all phrase that makes more sense here.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch for bug 708150 v2

This patch just updates the error text from "Video not found." to "No video with supported format and MIME type found."

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 579978 [details] [diff] [review]
Patch for bug 708150 v2

Dolske: What do you think about this text change? I think Chris has made some good points about changing it.

Comment 6

[Justin Dolske [:Dolske]]

cpearce: can we expose better data about the error somehow? Specifically, how can the controls determine what happened during a media source selection?

I think it's important to have clear and distinct "not found" vs "wrong bits/type" messages. Having both cases contain basically the same message makes it fairly useless for troubleshooting. (Really, I'd like to even be able to tell the difference between just a wrong mime time and invalid format... ISTR servers with misconfigured mimetypes for .webm/.ogg/.ogv was a common problem, and a clear message would help troubleshooting.)

At least, I'd like the simple cases to be accurate. I'm not sure what we should do with complex multiple-<source> cases that have varying errors. Just show whatever happened for the last <source> to be tried?

Comment 7

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

We couldn't change the existing HTMLMediaElement.error.code without violating the spec and deviating from other implementations, but we could add a new field to HTMLMediaElement (say mozLoadError) to more accurately describe the last error encountered when loading a resource, and we could define that to include the result load in the <source> children.

We could also add web console logging of what's happening during the media load algorithm, so we can explicitly report everything what we did or didn't attempt to load and why it failed. That would mostly be useful to authors when they're not using our controls. The places where we decide what to log are probably the same places where we need to set the value of mozLoadError, so we may as well do them together.

We could also push to have the spec changed to make HTMLMediaElement.error.code more meaningful. ;)

Comment 8

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

So after having thought about this more, here's what I think we should do:

* Add a new field to HTMLMediaElement.MediaError called "mozCode". This has the same codes as MediaError.code, but has additional codes to allow us to distinguish between the cases covered by MEDIA_ERR_SRC_NOT_SUPPORTED: 404, invalid MIME type HTTP header, and unsupported media format.

* Upon error, if we have an HTMLMediaElemnet.error.mozCode, display the string corresponding to error.mozCode. This will happen when loading from a HTMLMediaElement.src attribute, or when loading from a <source> child and we encounter some error after we'd decided we could play a resource.

* Upon error, if HTMLMediaElement.error is null, and networkState is 3 we should display "No video with supported format and MIME type found." I don't think we can have a non-null HTMLMediaElement.error upon while running resource selection when |!video.hasAttribute("src")|, so maybe we should just have this as a fallback/catch-all path.

* We should add console logging to describe what actions we're taking when running the media load algorithm. This will be most useful for authors when debugging errors when loading from <source> children, as it will tell the author the reason why every <source> child could not load, rather than the error from the last child <source> element. There's really no other way to efficiently communicate the cause of the error when loading from <source> children.

* [Optional] The string for the !HTMLMediaElement.error case could instead be "No video with supported format and MIME type found. See console for more details...".

Most of this needs to be done in C++, the JS is pretty simple. If you guys agree, then I'll try to tackle the C++ parts of this in the next week or so...

Comment 9

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #8)
> Most of this needs to be done in C++, the JS is pretty simple. If you guys
> agree, then I'll try to tackle the C++ parts of this in the next week or
> so...

I wonder how many authors are going to assume that Mozilla doesn't follow the spec here since this would sound so similar to the spec. Maybe call it "mozDescriptiveError", "mozAdditionalError", or "mozDetailedError".

Would there be a way for authors to turn off the console logging when they are finished with debugging?

I might be OK with clicking on the error message causing the console to open ("View details" link), but generally I believe the masses will not understand what we mean when we say "See console for more details...".

Other than those changes, I think this is something we should do.

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

s/moz*Error/moz*Code in my previous comment.

The debugging output to the console could be triggered by the presence of a "mozDebugOutput" attribute on the media element. That way authors can remove the attribute when they push to a production server.

Comment 11

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

(In reply to Jared Wein [:jwein and :jaws] from comment #9)
> I wonder how many authors are going to assume that Mozilla doesn't follow
> the spec here since this would sound so similar to the spec. Maybe call it
> "mozDescriptiveError", "mozAdditionalError", or "mozDetailedError".

We'd still leave HTMLMediaElement.error.code unchanged, so we'd still be spec compliant in that regards. Naming things is hard. ;)

> Would there be a way for authors to turn off the console logging when they
> are finished with debugging?

The console logging only appears when the web console is open. We could define a pref to disable it, I'd rather do that than define another content attribute.

> I might be OK with clicking on the error message causing the console to open
> ("View details" link), but generally I believe the masses will not
> understand what we mean when we say "See console for more details...".

Yeah, I wasn't sure if we should do this...

Comment 12

[Justin Dolske [:Dolske]]

(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #8)

> * Upon error, if HTMLMediaElement.error is null, and networkState is 3 we
> should display "No video with supported format and MIME type found." I don't
> think we can have a non-null HTMLMediaElement.error upon while running
> resource selection when |!video.hasAttribute("src")|, so maybe we should
> just have this as a fallback/catch-all path.

Not sure I'm following the conditions here. Are you just saying this case shouldn't happen, and so it's a just-in-case "unexpected error" kind of thing?

> [...]
> There's really no other way to
> efficiently communicate the cause of the error when loading from <source>
> children.

Hmm, I wonder if the spec should be extended to have a .error (or whatever) on each source?


(In reply to Jared Wein [:jwein and :jaws] from comment #9)

> I might be OK with clicking on the error message causing the console to open
> ("View details" link), but generally I believe the masses will not
> understand what we mean when we say "See console for more details...".

Yeah. I think we can get away with just not calling it out, it's already a place webdevs will/should be looking when things are not working.

Why only log when the console is open? Concern about spew? You could buffer the string internally, and only log it if the media selection finishes in an error state...

Comment 13

[Justin Dolske [:Dolske]]

Comment on attachment 579978 [details] [diff] [review]
Patch for bug 708150 v2

Review of attachment 579978 [details] [diff] [review]:
-----------------------------------------------------------------

Let's go ahead and land this string change while we work towards a better solution. Having fuzzy error messages isn't great, but having crisply misleading error messages is worst.

However, you need to change the entity name for this, as with all L10N string changes. :/ Mechanical search-and-replace, so r+ with that.

Comment 14

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

(In reply to Justin Dolske [:Dolske] from comment #12)
> (In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #8)
> 
> > * Upon error, if HTMLMediaElement.error is null, and networkState is 3 we
> > should display "No video with supported format and MIME type found." I don't
> > think we can have a non-null HTMLMediaElement.error upon while running
> > resource selection when |!video.hasAttribute("src")|, so maybe we should
> > just have this as a fallback/catch-all path.
> 
> Not sure I'm following the conditions here. Are you just saying this case
> shouldn't happen, and so it's a just-in-case "unexpected error" kind of
> thing?

I'm saying:
* In the general case where decide we can't play a resource from a <source> child we'll get HTMLMediaElement.error==null and networkState==3, and from script we can't distinguish why any of the children failed to load given the current spec.
* When loading from <source> children we'll only get error!=null when networkState != 3, and we'll only get this if we thought we could play a resource and discover later that we can't.
* So when !video.hasAttribute("src") (when we're loading from <source> children) and when HTMLMediaElement.error==null we should just display "No video with supported format and MIME type found." since we have no idea why the load failed. We don't need to check the value of networkState can be since it (should) always be 3 when error==null.

 
> Hmm, I wonder if the spec should be extended to have a .error (or whatever)
> on each source?

Not a bad idea.

> Why only log when the console is open? Concern about spew? You could buffer
> the string internally, and only log it if the media selection finishes in an
> error state...

That's just the behaviour I observed when using nsContentUtils::ReportToConsole(). Maybe I'm just using it wrong...

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/cc283f4ffdec

Comment 16

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/mozilla-central/rev/cc283f4ffdec

Comment 17

[[:Aleksej]]

This bug is reproducible (by opening “data:text/html,<video src="invalid-url" type="video/webm" controls>”) with
* Aurora Mozilla/5.0 (X11; Linux x86_64; rv:12.0a2) Gecko/20120302 Firefox/12.0a2
* Firefox 11.0 beta 4

Is it supposed to be fixed there?