Log messages to console explaining media load algorithm failure

RESOLVED FIXED in mozilla12

Status

()

Core
Audio/Video
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug)

Trunk
mozilla12
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 582971 [details] [diff] [review]
Patch v1: Add logging
Attachment #582971 - Flags: review?(roc)
(Assignee)

Comment 2

6 years ago
Test builds at:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/cpearce@mozilla.com-fc49b30b1ffa/

Test page:
http://pearce.org.nz/video/error.html
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.
(Assignee)

Comment 4

6 years ago
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!
Attachment #582971 - Attachment is obsolete: true
Attachment #582971 - Flags: review?(roc)
Attachment #583612 - Flags: review?(roc)
(Assignee)

Comment 5

6 years ago
Created attachment 583613 [details] [diff] [review]
Patch v3

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+
(Assignee)

Comment 7

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8de05d965033
Target Milestone: --- → mozilla12

Comment 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/8de05d965033
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 1322708
You need to log in before you can comment on or make changes to this bug.