712134 - Log messages to console explaining media load algorithm failure

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

Pursuant to our conversation in bug 708150, we should log messages to the webconsole explaining why media resource loads are failing to make debugging load failures when loading media from <source> children easier.

Comment 1

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

Attachment: Patch v1: Add logging

Comment 2

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

Test builds at:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fc49b30b1ffa/

Test page:
http://pearce.org.nz/video/error.html

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 582971 [details] [diff] [review]
Patch v1: Add logging

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

I think a helper function is worth having here, say nsHTMLMediaElement::ReportLoadError(char* aMsg, const PRUnichar* aParams = nsnull, PRUint32 aParamCount = 0).

Let me bikeshed the messages a bit...

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +313,5 @@
> +  nsCOMPtr<nsIHttpChannel> hc = do_QueryInterface(aRequest);
> +  if (hc) {
> +    PRUint32 responseStatus = 0;
> +    hc->GetResponseStatus(&responseStatus);
> +    if (responseStatus == 404) {

Call GetRequestSucceeded instead of checking 404.

@@ +318,5 @@
> +      element->NotifyLoadError();
> +      if (Preferences::GetBool("media.log_load_failures")) {
> +        nsCAutoString src;
> +        element->GetCurrentSpec(src);
> +        NS_ConvertUTF8toUTF16 srcUTF16(src);

Just call element->GetCurrentSrc().

@@ +325,5 @@
> +                                        "DOM",
> +                                        element->OwnerDoc(),
> +                                        nsContentUtils::eDOM_PROPERTIES,
> +                                        "MediaLoad404",
> +                                        params, ArrayLength(params));

Pass the response status as an extra parameter

@@ +766,5 @@
>  
>      // Must have src attribute.
>      nsAutoString src;
>      if (!child->GetAttr(kNameSpaceID_None, nsGkAtoms::src, src)) {
>        DispatchAsyncSourceError(child);

Why not add a message here? "<source> element missing src attribute" or something like that.

@@ +841,5 @@
> +                                    "DOM",
> +                                    OwnerDoc(),
> +                                    nsContentUtils::eDOM_PROPERTIES,
> +                                    "MediaLoadSuspendedPreloadNone",
> +                                    params, ArrayLength(params));

I don't think preload="none" needs a message.

@@ +2022,5 @@
>    if (!decoder) {
> +    if (Preferences::GetBool("media.log_load_failures")) {
> +      nsCAutoString src;
> +      GetCurrentSpec(src);
> +      NS_ConvertUTF8toUTF16 srcUTF16(src);

GetCurrentSrc

@@ +2204,5 @@
>  {
> +  if (Preferences::GetBool("media.log_load_failures")) {
> +    nsCAutoString src;
> +    GetCurrentSpec(src);
> +    NS_ConvertUTF8toUTF16 srcUTF16(src);

GetCurrentSrc

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +137,5 @@
>  WithCredentialsSyncXHRWarning=Use of XMLHttpRequest's withCredentials attribute is no longer supported in the synchronous mode in window context.
>  JSONCharsetWarning=An attempt was made to declare a non-UTF-8 encoding for JSON retrieved using XMLHttpRequest. Only UTF-8 is supported for decoding JSON.
> +MediaLoadExhaustedCandidates=Loading of a media element paused because all candidate resources failed to load.
> +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load.
> +MediaLoad404=Media load of %S failed, file not found.

This should handle any HTTP failure, so needs to change. e.g.
 Media resource %1$S failed to load (HTTP load failed with status %2$d).

@@ +139,5 @@
> +MediaLoadExhaustedCandidates=Loading of a media element paused because all candidate resources failed to load.
> +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load.
> +MediaLoad404=Media load of %S failed, file not found.
> +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load.
> +MediaLoadInvalidURI=Media load of %S failed, not a valid URI.

Media resource %S failed to load (invalid URI).

@@ +141,5 @@
> +MediaLoad404=Media load of %S failed, file not found.
> +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load.
> +MediaLoadInvalidURI=Media load of %S failed, not a valid URI.
> +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the resource's format/codec type (basically the file type, e.g. MP4,AVI,WMV,MOV etc).
> +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S.

Media resource %1$S was not loaded (specified type "%2$S" is not supported).

@@ +143,5 @@
> +MediaLoadInvalidURI=Media load of %S failed, not a valid URI.
> +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the resource's format/codec type (basically the file type, e.g. MP4,AVI,WMV,MOV etc).
> +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S.
> +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the MIME type being sent by the web server.
> +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S".

Media resource %1$S failed to load (HTTP Content-Type "%2$S" is not supported).

@@ +145,5 @@
> +MediaLoadUnsupportedType=Media load of %1$S failed, resource has an unsupported "type" attribute of %2$S.
> +# LOCALIZATION NOTE: %1$S is the URL of the media resource which failed to load, %2$S is the MIME type being sent by the web server.
> +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S".
> +# LOCALIZATION NOTE: %S is the URL of the media resource whose load is being suspended.
> +MediaLoadSuspendedPreloadNone=Media load of %S suspended because element is preload="none".

Remove

@@ +147,5 @@
> +MediaLoadUnsupportedMimeType=Media load of %1$S failed, resource is being served with unsupported "Content-Type" HTTP header of "%2$S".
> +# LOCALIZATION NOTE: %S is the URL of the media resource whose load is being suspended.
> +MediaLoadSuspendedPreloadNone=Media load of %S suspended because element is preload="none".
> +# LOCALIZATION NOTE: %S is the URL of the media resource which failed to load because of error in decoding.
> +MediaLoadDecodeError=Media load of %S failed due to error decoding resource.

Media resource %S could not be decoded.

::: modules/libpref/src/init/all.js
@@ +195,5 @@
>  // Whether to autostart a media element with an |autoplay| attribute
>  pref("media.autoplay.enabled", true);
>  
> +// Log failures in the media load algorithm to the web console.
> +pref("media.log_load_failures", true);

Let's just use a new category. If it produces too much spew then the Web Console people can filter it out.

Comment 4

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

Attachment: Patch v2

* Removed pref.
* Removed preload="none" message.
* Moved messages to "Media" category.
* Added helper nsHTMLMediaElement::ReportLoadError().
* Report HTTP status code on request failure.
* Added warning for missing @src on <source>.
* Took some of the bikeshed text changes suggestions on board, but I switched to using two sentences (instead of 1 + $specific_error_message in parentheses) and inverted the clause order. i.e. "$specific_failure. Load of media resource $src failed". This seems to be the convention used by existing warnings (at least in layout anyway), and I think it's weird to have parentheses in the error messages as suggested. The $specific_failure text is what's important/most useful!

Comment 5

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

Attachment: Patch v3

Oops, forgot to remove the pref from all.js. :/

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 583613 [details] [diff] [review]
Patch v3

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

::: content/html/content/public/nsHTMLMediaElement.h
@@ +360,5 @@
> +   * aMsg is the localized message identifier, aParams is the parameters to
> +   * be substituted into the localized message, and aParamCount is the number
> +   * of parameters in aParams.
> +   */
> +  void ReportLoadError(char* aMsg,

const char*

Comment 7

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8de05d965033

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8de05d965033