whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.09k stars 2.66k forks source link

Change the "message" field in MediaError object to be a "messages" FrozenArray<DOMString> field #2531

Open wolenetz opened 7 years ago

wolenetz commented 7 years ago

(See https://github.com/whatwg/html/issues/2085 for the introduction of the current 'message' field that this issue seeks to change.)

It's become apparent during the code review (https://codereview.chromium.org/2660003003/#msg31) that the shape of the new MediaError.message API may be better if standardized as a sequence rather than just a single DOMString.

Why?

Implementations may have multiple items of error message information available, all related to the same error condition. Indeed, in Chromium, we have precisely this situation (a top-level media engine status string that is slightly more detailed than just the MediaError.code enum, as well as potentially multiple, varying in granularity/specificity, error messages from things like the media parser portion of the media engine.

Attempts to relay this information in an interoperable way to web apps using just a single DOMString result in either:

  1. A readily machine-parsable, but with unstandardized structure, aggregation of such strings (such as a serialized JSONObject, or a newline-delimited and more human-readable listing of the strings, or
  2. A reduction in the granularity and detail of potentially critical error message information produced by the implementation to fit the message within a single string.

Option 1 results in a de-facto standard if web apps begin to expect to be able to use a JSON parser, but not all implementations would have this nor would there be any official standardization of the format or structure. Option 2 limits the utility of this feature in general. The whole point is to provide additional error detail to enable web apps to aggregate and debug common, sometimes hard-to-repro, error conditions in media playback.

Potential solution:

A reasonable mitigation of these two options would be to simply change the type (and name) of the field to a sequence<DOMString> messages field, empty if there is no additional info beyond that available in MediaError.code, otherwise one or more informative strings in a sequence.

domenic commented 7 years ago

Well, you can't have attributes that are sequences. But this would probably not change after initialization of the MediaError object, so FrozenArray<DOMString> messages would work.

@jyavenard, this would require a breaking change from Mozilla. Thoughts?

wolenetz commented 7 years ago

@foolip, @mounirlamouri, @jyavenard, @jdsmith3000, @domenic FYI

Also, would it make sense to provide a non-normative expectation that the messages in a sequence of multiple strings in this proposed messages field start as a general message and later ones are more specific? This is how I was planning to implement the original #2085 in Chromium.

Edited: Actually, even that might not be a good idea. Chromium's implementation might better be described as reverse-chronological, with more specific underlying causes of the eventually fatal error (if available) shown later in the message(s) than the more recently-determined higher-level summary of the error. And if there was a sequence of non-fatal errors that eventually led to a fatal error, the earlier errors would be listed later. Such an ordering may not be reasonable to impose on all implementations, and rather be left to implementations to decide.

wolenetz commented 7 years ago

I'll update the title per https://github.com/whatwg/html/issues/2531#issuecomment-293997100

jyavenard commented 7 years ago

I'm no fan of the idea.

Most media error being fatal, as such, when the first error occurs, parsing or playback stop, there's only a single error to report.

I'm personally yet to encounter a time where we would have multiple errors to return in gecko.

If one wants to provide more error, there's nothing preventing the message string to be formatted in such a way presenting all the errors.

We've just got message through only to break that API for a new one??

I will discuss with our team during our next meeting, but I have to say that I'm less than enthousiatjc about it.

wolenetz commented 7 years ago

@jyavenard, I agree that many fatal errors might be representable in a single string. However, consider a more complex case such as: 1) Hardware media decode attempted, succeeds for a while, then fails. Implementation heuristic falls back to attempting software decode. 3) Then software decode fails. Implementation heuristic gives up and issues fatal decode error.

Information about why the two distinct decode attempts failed could be presented either as a concatenated string or in an array of strings. My attempts to use a concatenated string in Chromium implementation (https://codereview.chromium.org/2660003003/#msg31) led to concerns that web apps might attempt to parse out distinct portions of these messages and increase concern that however Chromium encodes multiple strings might become a de-facto standard leading potentially to interop issues.

wolenetz commented 7 years ago

I've also notified www-archive@w3.org of this potential change to the API and requested feedback at https://lists.w3.org/Archives/Public/www-archive/2017Apr/0000.html

foolip commented 7 years ago

In addition to the hardware-to-software fallback case, are there other cases where an initially non-fatal error is relevant to understanding the problem? Perhaps the demuxer made some guesses about an incorrectly muxed file, that then turns into a decoding error? Maybe an example media file where Chrome could report multiple error reasons would be helpful.

A somewhat ugly compromise would be to say that the string must not begin or end with a newline, and that newlines are used to separate distinct messages. And probably that they should be chronological. This way, even if some engine always returns a newline-free message, as long as others sometimes do, then like message.split('\n') and the following array handling is very likely to work in all browsers.

With Firefox just having shipped message as a plain string, I think that the preferences of Gecko devs like @jyavenard should be given extra weight here, so let's wait until after next week's meeting.

jyavenard commented 7 years ago

@wolenetz: It comes down then about who is the intended target of this information. Is it the user-agent or the web developer?

I had always seen message as something useful primarily for the web developer. It lets him know why a video failed to play, which would allow to correct the problem and improve the user experience. I doubt the information that somehow the GPU decoding failed and then it felt back to software gives any value to the web dev, there's little they can do about it.

We're also starting the tread on the very fine line of user's privacy. Gecko's message was strictly reviewed such that no user information was revealed nor anything that could uniquely identified the user: no hardware details, no file path.

In the upcoming Firefox 53, when a file fails to play, there's a little banner showed at the top inviting the user to reports the issue to webcompat using a url like: https://webcompat.com/issues/new?url=http%3A%2F%2Fdash.edgesuite.net%2Fenvivio%2FEnvivio-dash2%2Fv4_258-Header.m4s&problem_type=video_bug&src=media-decode-error&details=Technical+Information%3A%0AError+Code%3A+NS_ERROR_DOM_MEDIA_DEMUXER_ERR+%280x806e000c%29%0ADetails%3A+virtual+RefPtr%3CMP4Demuxer%3A%3AInitPromise%3E+mozilla%3A%3AMP4Demuxer%3A%3AInit%28%29%3A+No+MP4+audio+%28%29+or+video+%28%29+tracks%0AResource%3A+http%3A%2F%2Fdash.edgesuite.net%2Fenvivio%2FEnvivio-dash2%2Fv4_258-Header.m4s

that pre-fills the form on what was wrong here, using the content of message: "Error Code: NS_ERROR_DOM_MEDIA_DEMUXER_ERR (0x806e000c) Details: virtual RefPtr mozilla::MP4Demuxer::Init(): No MP4 audio () or video () tracks Resource: http://dash.edgesuite.net/envivio/Envivio-dash2/v4_258-Header.m4s"

Now, I understand this may see this as irrelevant as to how the message attribute should be defined as. But we've worked hard with some partners to get them to use the message attribute to identify their problems and allow them to report it to us if we know have to redo it all over again with some UA not having any attributes, some having message, and now messages...

I just think that the effort isn't worth it for what I believe to be little gain.

wolenetz commented 7 years ago

Edit - I hadn't seen the two responses yet when I just posted this. I'll reply to them shortly. @jyavenard (https://github.com/whatwg/html/issues/2531#issuecomment-294002913)

If one wants to provide more error, there's nothing preventing the message string to be formatted in such a way presenting all the errors.

I actually tend to agree with you. I filed this issue on concerns that arose during early code review of my attempt to land a JSON-serialized version of a message in Chromium. While JSON-serialization affords easy parsing, it might become a de-facto standard/expectation. So I've updated my Chromium change to be a newline delimited, human readable version, and that is in a bit of a code review limbo pending discussion here on this issue.

@ All: Any further support for or against changing the API ASAP would be welcome, as web authors really need some better differentiation/debug support of media errors ASAP.

wolenetz commented 7 years ago

@foolip (https://github.com/whatwg/html/issues/2531#issuecomment-294126524): I would be fine with that. Relative to my current change, this would change it to be something more like:

[If any MEDIA_LOG(ERROR) events were reported, enumerate them chronologically with a header like "Error details:\n"] (This would likely result in the most specific root cause leading to the error being earlier in the enumeration, though recovery from earlier non-fatal errors would list details of what led to such earlier non-fatal errors first, if available).

[Then, report the non-OK PIPELINE_STATUS string or general media element error, if any.]

Regarding actual sequences of MEDIA_LOG(ERROR), we currently have one primary case of which I'm aware, and that is MSE parse errors reporting error at different levels in the stack (though we could collapse these/change it to be just 1 error in this common case). We currently log (only to media-internals, not to this message API, and invisible to web apps) additional DEBUG and INFO log messages, which are involved in scenarios like you mention where there may be supplemental information warned about earlier in a parse that led to an eventual decode error. However, decoder fallback recovery is another path being developed currently in Chromium, for which there are definitely pieces of error information occurring in a sequence over time. In that case, we could, if this forum requires, restrict the error message to just the most critical error that resulted in the fatal error, but I was hoping not to restrict user agents on the format of this message too much.

@jyavenard : The primary focus of this feature is the web developer. They have the burden currently of not having enough information to diagnose hard-to-repro media playback failures without this feature. There are of course benefits to user agents in that they can target fixes or investigations more precisely based on data provided by web developers that use this feature (in ways like you describe with a feedback form or other channels specific to the user agent).

mounirlamouri commented 7 years ago

@jyavenard when did Mozilla ship the message property? Would it be possible for you all to switch to messages? Alternatively, what would you all think of having message and messages with the former returning the last meaningful messages and the latter all the relevant ones.

annevk commented 7 years ago

@mounirlamouri it seems like you haven't read through https://github.com/whatwg/html/issues/2531#issuecomment-294133130.

wolenetz commented 7 years ago

Since it seems there is opposition to changing 'message' API that was standardized in #2085 (and shipped by Firefox already) with extra opposition to changing it in multiple comments from Firefox, especially https://github.com/whatwg/html/issues/2531#issuecomment-294133130:

I believe the existing 'message' API should be retained and this issue should either be closed or renamed to "Add a 'messages' field...". Any preference from the community?

-minor edit for readability

wolenetz commented 7 years ago

@foolip (https://github.com/whatwg/html/issues/2531#issuecomment-294213577): I've updated my Chromium change to not contain any newlines, and to give what we believe to be the most actionable error message information in a single string. I believe this satisfies known concerns with the existing 'message' API.

dalecurtis commented 7 years ago

This message must be machine parseable; i.e. multiple errors need to be delimited in some way.

The intended user is mega-video sites like Facebook, Vimeo, YouTube, Netflix, etc who have vast telemetry arrays for reporting failures. Aside from Edge, there's already other mechanisms for single-users to find error details (chrome://media-internals, etc).

wolenetz commented 7 years ago

@dalecurtis (https://github.com/whatwg/html/issues/2531#issuecomment-294951278) - the format I was considering for the message field (given what Firefox has shipped already, and what information is most likely important to pinpoint the source of the error) is no longer a combination of multiple error messages; rather just some kind of "code string" plus an optional details string. Following on the format of Firefox's current MediaError.message strings, any "optional details string" would begin at the first ":" in the message. The "code string" is akin to an implementation-specific enum (without any ":" in the code, of course), and the "optional details string" can contain code-specific details like timestamps, track IDs, or even function signatures, along with explanatory wording. If indeed multiple error messages are desired, that would probably need to be a new API, like maybe a messages field. Please see my responses on the Chromium CL (https://codereview.chromium.org/2660003003/#msg61 and https://codereview.chromium.org/2660003003/#msg62) for the direction I'm considering, and please provide comment.

mounirlamouri commented 7 years ago

@wolenetz @dalecurtis in general, I would be more comfortable if we had tried to discuss different "code string" instead of having all implementations define their own and ask websites to parse the message to figure out what type of error they are hitting knowing that this code will be, by definition UA specific. Though, it seems that this ship has sailed with Mozilla doing it.

Can we work on a better API that will allow for minimal UA specific code required and hopefully multiple messages?

To be clear, I understand that these messages are platform specifics but they should only be platform specific as far as the message is an opaque string to the UA. In other words, because the UA knows the type of error, it should tag it instead of adding the type in the string that is sent back.

wolenetz commented 7 years ago

@mounirlamouri Would you be averse to Chrome shipping the existing (single) message API that's currently in the standard, and with a parseable UA-specific-code followed by an optional " : " + UA-specific-message as described in https://github.com/whatwg/html/issues/2531#issuecomment-294986407? I agree that having this UA-specific-code also available without parsing in some new field would reinforce the interop of large players aggregating per-UA-codes, leaving the format of the existing message API as-is in the spec. Discussion of possibly including multiple "messages" in some portion of the API can also proceed separately, but not block the UA-specific-code field API nor implementations (like Chrome and FF) shipping (single) message.

mounirlamouri commented 7 years ago

@wolenetz I guess that question would be more for the blink-dev thread. I'm no Blink API Owner so my opinion doesn't matter much. This said, because Mozilla already shipped something that assumes browser parsable strings and @jyavenard seems to be happy with it and not super inclined to change, I would not mind us doing something similar. I don't think that what you propose would make the situation worse and we clearly can't imitate the string that Firefox will return. However, I would really like us to work towards something better.

wolenetz commented 7 years ago

SGTM @mounirlamouri thank you for your input.

foolip commented 7 years ago

What is the kind of substructure of a single message that could be useful to expose to avoid parsing of the message? Most of this discussion has been about multiple messages, which could solved by either a new FrozenArray<DOMString> message or explicitly allowing message to contain newlines, but that's different to the ":"-separated structure discussed in https://github.com/whatwg/html/issues/2531#issuecomment-294986407 and beyond.

wolenetz commented 7 years ago

Summary of my understanding of MediaError.message(s) discussion on this issue so far: This issue was originally filed to discuss perhaps API change of message to messages. The discussion, IIUC, has pointed to strong preference (especially since Firefox has shipped message already) of retaining message, along with some further discussion of how the internal formatting of message could be made both human readable and still valuable for web authors (especially large sites like YT, FB) who could most benefit from: a parseable simple "error code" that is UA-specific, and an optional UA-specific additional descriptive message (possibly containing variable details like timestamps or track identifiers, etc), separated in some (currently unspecified) internal format.

It's becoming clear to me that:

  1. We need both of these (UA-specific error code, optional descriptive error message) more clearly specified, since it is already becoming a defacto standard (FF has shipped this, Chrome is intending to).

  2. Further, the API could benefit from exposing the "UA-specific code" by itself in some separate portion of the API besides the single DOMString message field, to allow more interoperable collection and aggregation of UA-specific error codes by users of the API, without having to rely on parsing some under-specified internal format of message to retrieve it.

  3. It is less clear that there is support or need for an API to retrieve multiple messages, especially once 1-2 are done (and 1-2 are more clearly motivated/needed IMHO). Any additional informational message describing the error could be included, perhaps, at the tail end of the optional descriptive string in message. The original motivation for multiple error messages to describe a single resulting MediaError was, to be honest, a little lacking. I found only a couple cases: A) UA logged details of the same resulting error at differing granularities (e.g., in the parser, then in the parser's host, then in the media element, as the error bubbled up through the implementation's execution stack). In such a scenario, the best information is at the original detection of the error condition, and any other information is additive and could be included in the same single descriptive error message if the UA decides it is warranted. B) A non-fatal initial error might occur with UA deciding to attempt some recovery(ies) that eventual fail with a MediaError: in this case, the first error is still likely to be the most informative, and any additional descriptive error information could still be added to the message if the UA warrants. In both A and B, the UA can modify their choice of the "UA-specific error code" to help identify the scenario (was it just a parse error on appendBuffer (an example of "A"), or was it a hardware-decode failure followed by software decode recovery attempt that eventually failed (an example of "B").

I'm proceeding with (1) in the Chrome implementation, since that appears to have to most interop and support, and will file a spec issue for (2), since that warrants additional discussion. Let's use #2531 to continue to discuss (3).

(@foolip)

wolenetz commented 7 years ago

What is the kind of substructure of a single message that could be useful to expose to avoid parsing of the message?

A different field. See item 2 in https://github.com/whatwg/html/issues/2531#issuecomment-295834441. The message field already contains variable information (e.g. timestamps or track ids) in shipping implementation (Firefox, and Chrome soon)

Most of this discussion has been about multiple messages, which could solved by either a new FrozenArray message or explicitly allowing message to contain newlines, but that's different to the ":"-separated structure discussed in #2531 (comment) and beyond.

Correct. FrozenArray messages (item 3 in https://github.com/whatwg/html/issues/2531#issuecomment-295834441) is still the ongoing focus of this issue, but I suspect it has much less motivation to become standardized now.

Would it help if I also filed a spec issue to, say, add a non-normative note to the spec for message that provides informative examples of implementations that may assist developers' parsing the UA-specific code, at least until option 2 from https://github.com/whatwg/html/issues/2531#issuecomment-295834441 (or similar) is standardized?

(@foolip)

(edited formatting)

foolip commented 7 years ago

Saying something normative about the structure of the message with examples sounds great. It doesn't have to be a real example, something made up could be just as clear.

foolip commented 7 years ago

Both the normative bits and examples would probably best be discussed in the form of a PR reviewed by at least @jyavenard, and possible with that the tests in web-platform-tests could actually require something beyond the message being a string.