Last Comment Bug 712134 - Log messages to console explaining media load algorithm failure
: Log messages to console explaining media load algorithm failure
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Chris Pearce (:cpearce)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-19 14:40 PST by Chris Pearce (:cpearce)
Modified: 2011-12-22 03:48 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1: Add logging (12.43 KB, patch)
2011-12-19 14:54 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch v2 (11.12 KB, patch)
2011-12-21 13:49 PST, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review
Patch v3 (10.31 KB, patch)
2011-12-21 13:51 PST, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-12-19 14:40:27 PST
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) 2011-12-19 14:54:26 PST
Created attachment 582971 [details] [diff] [review]
Patch v1: Add logging
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-19 18:07:26 PST
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) 2011-12-21 13:49:20 PST
Created attachment 583612 [details] [diff] [review]
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) 2011-12-21 13:51:06 PST
Created attachment 583613 [details] [diff] [review]
Patch v3

Oops, forgot to remove the pref from all.js. :/
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-12-21 14:25:16 PST
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 8 Ed Morley [:emorley] 2011-12-22 03:48:17 PST
https://hg.mozilla.org/mozilla-central/rev/8de05d965033

Note You need to log in before you can comment on or make changes to this bug.