Closed Bug 712134 Opened 12 years ago Closed 12 years ago

Log messages to console explaining media load algorithm failure

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch v1: Add logging (obsolete) — Splinter Review
Attachment #582971 - Flags: review?(roc)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
* 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!
Attachment #582971 - Attachment is obsolete: true
Attachment #582971 - Flags: review?(roc)
Attachment #583612 - Flags: review?(roc)
Attached patch Patch v3Splinter Review
Oops, forgot to remove the pref from all.js. :/
Attachment #583612 - Attachment is obsolete: true
Attachment #583612 - Flags: review?(roc)
Attachment #583613 - Flags: review?(roc)
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*
Attachment #583613 - Flags: review?(roc) → review+
https://hg.mozilla.org/mozilla-central/rev/8de05d965033
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 1322708
You need to log in before you can comment on or make changes to this bug.