Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2660003003:
Add MediaError.message (Closed)
|
Created:
3 years, 10 months ago by wolenetz Modified:
3 years, 8 months ago Reviewers:
jochen (gone - plz use gerrit), dglazkov, sandersd (OOO until July 31), mlamouri (slow - plz ping), emircan, mcasas, DaleCurtis, foolip CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, posciak+watch_chromium.org, Srirama Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
Review-Url: https://codereview.chromium.org/2660003003
Cr-Commit-Position: refs/heads/master@{#466359}
Committed: https://chromium.googlesource.com/chromium/src/+/ed8e7091b5b0a51a8c0781f95813a5929c1f8110
Patch Set 1 #Patch Set 2 : Major rework to expand utility of message and include tests #Patch Set 3 : --flakiness by using std::map, update virtual/stable/[win,mac] global interface listing too #
Total comments: 17
Patch Set 4 : Rework to single human-readable, newline-delimited, message string #Patch Set 5 : Rebase and fix layout test failure #Patch Set 6 : Simplify to just 1 string with no newlines: [status plus first MEDIA_ERROR_LOG_ENTRY], if any #
Total comments: 4
Patch Set 7 : Just rebase. Still working on updating per c#64 #Patch Set 8 : Update message format to be code: detail #
Total comments: 7
Patch Set 9 : Address dalecurtis@'s comment: Use std::stringbuilder in RenderMediaLog::GetErrorMessage() #Patch Set 10 : Address dalecurtis@'s comment: Document MediaLog::GetErrorMessage() potential for partial msg #
Total comments: 2
Patch Set 11 : Address dalecurtis@'s comment: undef STRINGIFY_STATUS_CASE #
Total comments: 3
Messages
Total messages: 94 (44 generated)
Patchset #2 (id:20001) has been deleted
Description was changed from
==========
WIP add MediaError.message
Prototype -- do not land
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage() to getErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage() to getErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer getErrorMessage() to
getErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer getErrorMessage() to
getErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086, 710617
==========
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
Intent to implement and ship is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
wolenetz@chromium.org changed reviewers: + jochen@chromium.org, mcasas@chromium.org, mlamouri@chromium.org, sandersd@chromium.org
Please review: sandersd@: Everything, especially //media/* and //content/renderer/media/* mlamouri@: //third_party/WebKit/Source/core/html/* and the media/video-error-does-not-exist.html layout test jochen@: //third_party/WebKit/public/platform/*, third_party/WebKit/Source/platform/*, and API_OWNERS review of the updated global-interface-listing-expected.txt mcasas@: //content/renderer/media_capture_from_element/*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Review for everything outside of third_party/. https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:114: void TestErrorMessage(const std::string& tag, Rename as RunErrorMessageTest to match other variants? https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:298: "supported streams\"]}", This format seems overly complex and exposes rather more internal detail than I was hoping. The object structure here is: { "MEDIA_ERROR_LOG_ENTRY": [ "{\"error\":\"FFmpegDemuxer: no supported streams\"}" ], "PIPELINE_ERROR": [ "demuxer: no supported streams" ] } I would propose: { "cause": "demuxer: no supported streams", "error_log": [ "FFmpegDemuxer: no supported streams" ] } https://codereview.chromium.org/2660003003/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.h (right): https://codereview.chromium.org/2660003003/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.h:79: std::deque<std::unique_ptr<media::MediaLogEvent>> Perhaps std::vector would be a better choice, since MediaLogEvents are small? https://codereview.chromium.org/2660003003/diff/60001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/2660003003/diff/60001/media/base/media_log.cc... media/base/media_log.cc:212: bool include_type) { Not a fan of this parameter, it's not clear from context what it does at the call sites. This is a case where two methods makes more sense.
jochen@chromium.org changed reviewers: + dglazkov@chromium.org
Dimitri, who's the right person these days to decide what types to expose in the public api? https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:780: std::map<std::string, std::vector<std::string>> hum, it looks like you really want a base::Value or something here, no?
https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:74: std::string MediaBrowserTest::EncodeErrorMessage( Where is this called? https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() const { return message_; } That's quite unexpected: why are we passing a JSONObject? Usually error messages are simple strings. Returning a serialised JSON object seems like slightly working around the spec. Could we have a `messages` IDL instead or simply return the most recent error? How likely it is that the most recent error would be noise?
mlamouri@chromium.org changed reviewers: + foolip@chromium.org
+foolip@ for an opinion on the comment above.
https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() const { return message_; } On 2017/04/12 15:56:41, mlamouri wrote: > That's quite unexpected: why are we passing a JSONObject? Usually error messages > are simple strings. Returning a serialised JSON object seems like slightly > working around the spec. Could we have a `messages` IDL instead or simply return > the most recent error? How likely it is that the most recent error would be > noise? Yes, if we usually return a string that can be passed to JSON.parse(), then people may start doing just that. If exceptions are thrown in other browsers as a result, there would be frowning. How much structure is there to this error message, is it feasible to just format the JSON into something more human readable and less machine readable? If returning an arbitrary object is very valuable, then we should revisit the spec and have this return a dictionary or similar instead.
Just a reply for now while I work on addressing sandersd@'s comments and await Dmitri/Jochen's further replies. https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() const { return message_; } On 2017/04/12 15:56:41, mlamouri wrote: > That's quite unexpected: why are we passing a JSONObject? Usually error messages > are simple strings. Returning a serialised JSON object seems like slightly > working around the spec. Could we have a `messages` IDL instead or simply return > the most recent error? How likely it is that the most recent error would be > noise? The format and content of the "message" attribute is implementation-specific. Considering the current state of our media-internals error logging mechanisms, and allowing for future extensibility/modification and easy parsing by web apps of Chrome's message details, I chose a serialized JSON object rather than just some weird concatenation of strings. In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR (at most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason for the latter is that, for example, an MSE stream parser might first detect "parse failure" with some detailed MEDIA_LOG(ERROR) emission describing the details of the parse failure, and when this failure bubbles up into the demuxer, the pipeline, and eventually the webmediaplayer implementation, additional higher level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are typically more general, so preserving the series of logs allows the greatest exposure of error logging detail to help identify the root cause. None of this was an attempt to force a message structure or content into the spec or other implementations, rather to expose the information that we may have to assist debugging errors that occur when using our implementation. It may well result that other implementations (like Edge) might have completely different, though just as valid for this use case (identifying implementation-specific error details to assist media error debugging), format and content of their errors (such as perhaps just stringifying the msExtendedCode value that they already have in Edge). See also related discussion on the spec issue (https://github.com/whatwg/html/issues/2085)
https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() const { return message_; } On 2017/04/12 17:29:57, wolenetz wrote: > On 2017/04/12 15:56:41, mlamouri wrote: > > That's quite unexpected: why are we passing a JSONObject? Usually error > messages > > are simple strings. Returning a serialised JSON object seems like slightly > > working around the spec. Could we have a `messages` IDL instead or simply > return > > the most recent error? How likely it is that the most recent error would be > > noise? > > The format and content of the "message" attribute is implementation-specific. > Considering the current state of our media-internals error logging mechanisms, > and allowing for future extensibility/modification and easy parsing by web apps > of Chrome's message details, I chose a serialized JSON object rather than just > some weird concatenation of strings. > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR (at > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason for the > latter is that, for example, an MSE stream parser might first detect "parse > failure" with some detailed MEDIA_LOG(ERROR) emission describing the details of > the parse failure, and when this failure bubbles up into the demuxer, the > pipeline, and eventually the webmediaplayer implementation, additional higher > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > typically more general, so preserving the series of logs allows the greatest > exposure of error logging detail to help identify the root cause. > > None of this was an attempt to force a message structure or content into the > spec or other implementations, rather to expose the information that we may have > to assist debugging errors that occur when using our implementation. It may well > result that other implementations (like Edge) might have completely different, > though just as valid for this use case (identifying implementation-specific > error details to assist media error debugging), format and content of their > errors (such as perhaps just stringifying the msExtendedCode value that they > already have in Edge). > > See also related discussion on the spec issue > (https://github.com/whatwg/html/issues/2085) Can you share an example of what one of the more verbose error messages will look like? Is it just a list of messages concatenated, or is there real structure/nesting to the serialized object?
On 2017/04/12 17:21:52, foolip_UTC7 wrote: > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/media/MediaError.h (right): > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() > const { return message_; } > On 2017/04/12 15:56:41, mlamouri wrote: > > That's quite unexpected: why are we passing a JSONObject? Usually error > messages > > are simple strings. Returning a serialised JSON object seems like slightly > > working around the spec. Could we have a `messages` IDL instead or simply > return > > the most recent error? How likely it is that the most recent error would be > > noise? > > Yes, if we usually return a string that can be passed to JSON.parse(), then > people may start doing just that. If exceptions are thrown in other browsers as > a result, there would be frowning. > > How much structure is there to this error message, is it feasible to just format > the JSON into something more human readable and less machine readable? > > If returning an arbitrary object is very valuable, then we should revisit the > spec and have this return a dictionary or similar instead. I see the concern better now. Thank you. I'm less certain about the value of changing the spec to do something like return a dictionary: the route I chose in Chrom* implementation was due to the existing infrastructure for logging details about errors that media-internals (and HTMLMediaElement itself) as the transition of the element to error state was determined. To alleviate the interop concern that using JSON serialization might become a non-interoperable but de-facto standard, I could change it to not be directly parseable by a JSON parser by just concatenating the strings and using more human-readable labels. Proposed example (see sandersd@'s earlier comment): Instead of: { "cause": "demuxer: no supported streams", "error_log": [ "FFmpegDemuxer: no supported streams" ] } How about: [optional, only if there is a description available:] Error description: demuxer: no supported streams <newline> [optional, only if there are details available:] Error Details: <newline> FFmpegDemuxer: no supported streams
On 2017/04/12 17:40:40, foolip_UTC7 wrote: > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/media/MediaError.h (right): > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() > const { return message_; } > On 2017/04/12 17:29:57, wolenetz wrote: > > On 2017/04/12 15:56:41, mlamouri wrote: > > > That's quite unexpected: why are we passing a JSONObject? Usually error > > messages > > > are simple strings. Returning a serialised JSON object seems like slightly > > > working around the spec. Could we have a `messages` IDL instead or simply > > return > > > the most recent error? How likely it is that the most recent error would be > > > noise? > > > > The format and content of the "message" attribute is implementation-specific. > > Considering the current state of our media-internals error logging mechanisms, > > and allowing for future extensibility/modification and easy parsing by web > apps > > of Chrome's message details, I chose a serialized JSON object rather than just > > some weird concatenation of strings. > > > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR (at > > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason for > the > > latter is that, for example, an MSE stream parser might first detect "parse > > failure" with some detailed MEDIA_LOG(ERROR) emission describing the details > of > > the parse failure, and when this failure bubbles up into the demuxer, the > > pipeline, and eventually the webmediaplayer implementation, additional higher > > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > > typically more general, so preserving the series of logs allows the greatest > > exposure of error logging detail to help identify the root cause. > > > > None of this was an attempt to force a message structure or content into the > > spec or other implementations, rather to expose the information that we may > have > > to assist debugging errors that occur when using our implementation. It may > well > > result that other implementations (like Edge) might have completely different, > > though just as valid for this use case (identifying implementation-specific > > error details to assist media error debugging), format and content of their > > errors (such as perhaps just stringifying the msExtendedCode value that they > > already have in Edge). > > > > See also related discussion on the spec issue > > (https://github.com/whatwg/html/issues/2085) > > Can you share an example of what one of the more verbose error messages will > look like? Is it just a list of messages concatenated, or is there real > structure/nesting to the serialized object? A more complex set of errors would result from something like an attempted playback of a file using MSE that hits the error condition at https://cs.chromium.org/chromium/src/media/formats/mp4/aac.cc?rcl=91b3f7e7b1f..., resulting in a message that contains (format under discussion here, still): A pipeline error message (like "chunk demuxer: append failed"), and multiple MEDIA_LOG(ERROR) emissions like: "Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO 14496-3:2009 Table 1.18 for supported Sampling Frequencies." and (emitted from https://cs.chromium.org/chromium/src/media/filters/source_buffer_state.cc?typ...) "SourceBufferState::Append: stream parsing failed. Data size=<some number> append_window_start=<some_number> append_window_end=<some_number>" Note that the latter error log entry contains details related to the context of the append, where the earlier one contains the root reason why the append failed, and the pipeline error message is a top-level description of the error that is just a little more detailed than the standardized MediaError.code enum. In the serialized JSON object, it would have two entries in the "error_log" array, along with one "cause". In the proposed in #26 version of the format, it would be more human readable but contain all the same basic content.
On 2017/04/12 18:01:45, wolenetz wrote: > On 2017/04/12 17:40:40, foolip_UTC7 wrote: > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/html/media/MediaError.h (right): > > > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() > > const { return message_; } > > On 2017/04/12 17:29:57, wolenetz wrote: > > > On 2017/04/12 15:56:41, mlamouri wrote: > > > > That's quite unexpected: why are we passing a JSONObject? Usually error > > > messages > > > > are simple strings. Returning a serialised JSON object seems like slightly > > > > working around the spec. Could we have a `messages` IDL instead or simply > > > return > > > > the most recent error? How likely it is that the most recent error would > be > > > > noise? > > > > > > The format and content of the "message" attribute is > implementation-specific. > > > Considering the current state of our media-internals error logging > mechanisms, > > > and allowing for future extensibility/modification and easy parsing by web > > apps > > > of Chrome's message details, I chose a serialized JSON object rather than > just > > > some weird concatenation of strings. > > > > > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR (at > > > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason for > > the > > > latter is that, for example, an MSE stream parser might first detect "parse > > > failure" with some detailed MEDIA_LOG(ERROR) emission describing the details > > of > > > the parse failure, and when this failure bubbles up into the demuxer, the > > > pipeline, and eventually the webmediaplayer implementation, additional > higher > > > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > > > typically more general, so preserving the series of logs allows the greatest > > > exposure of error logging detail to help identify the root cause. > > > > > > None of this was an attempt to force a message structure or content into the > > > spec or other implementations, rather to expose the information that we may > > have > > > to assist debugging errors that occur when using our implementation. It may > > well > > > result that other implementations (like Edge) might have completely > different, > > > though just as valid for this use case (identifying implementation-specific > > > error details to assist media error debugging), format and content of their > > > errors (such as perhaps just stringifying the msExtendedCode value that they > > > already have in Edge). > > > > > > See also related discussion on the spec issue > > > (https://github.com/whatwg/html/issues/2085) > > > > Can you share an example of what one of the more verbose error messages will > > look like? Is it just a list of messages concatenated, or is there real > > structure/nesting to the serialized object? > > A more complex set of errors would result from something like an attempted > playback of a file using MSE that hits the error condition at > https://cs.chromium.org/chromium/src/media/formats/mp4/aac.cc?rcl=91b3f7e7b1f..., > resulting in a message that contains (format under discussion here, still): > > A pipeline error message (like "chunk demuxer: append failed"), and multiple > MEDIA_LOG(ERROR) emissions like: > "Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO > 14496-3:2009 Table 1.18 for supported Sampling Frequencies." > and (emitted from > https://cs.chromium.org/chromium/src/media/filters/source_buffer_state.cc?typ...) > "SourceBufferState::Append: stream parsing failed. Data size=<some number> > append_window_start=<some_number> append_window_end=<some_number>" > > Note that the latter error log entry contains details related to the context of > the append, where the earlier one contains the root reason why the append > failed, and the pipeline error message is a top-level description of the error > that is just a little more detailed than the standardized MediaError.code enum. > > In the serialized JSON object, it would have two entries in the "error_log" > array, along with one "cause". In the proposed in #26 version of the format, it > would be more human readable but contain all the same basic content. After chatting at length with sandersd@, we also agreed that reversing the sequence of queued media_error_event_log entries when emitting them from render_media_log.cc will allow us to maintain a more human-readable convention of a list of log description and entries starting at the most general and ending with the most specific available. Something like: Error description:\n demuxer: no supported streams\n Error details:\n SourceBufferState::Append: stream parsing failed. Data size=<some number> append_window_start=<some_number> append_window_end=<some_number>\n Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO 14496-3:2009 Table 1.18 for supported Sampling Frequencies.\n Or another example for a case where the WebMediaPlayer doesn't exist, but HTMLMediaElement has some info to report: Error description:\n media element error: format error\n We may also modify in this or a later CL the MEDIA_LOG macro to require a third parameter, "tag", which will help identify the module that produced the error event log entry in the eventual message's Error details list.
On 2017/04/12 19:37:22, wolenetz wrote: > On 2017/04/12 18:01:45, wolenetz wrote: > > On 2017/04/12 17:40:40, foolip_UTC7 wrote: > > > > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/html/media/MediaError.h (right): > > > > > > > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() > > > const { return message_; } > > > On 2017/04/12 17:29:57, wolenetz wrote: > > > > On 2017/04/12 15:56:41, mlamouri wrote: > > > > > That's quite unexpected: why are we passing a JSONObject? Usually error > > > > messages > > > > > are simple strings. Returning a serialised JSON object seems like > slightly > > > > > working around the spec. Could we have a `messages` IDL instead or > simply > > > > return > > > > > the most recent error? How likely it is that the most recent error would > > be > > > > > noise? > > > > > > > > The format and content of the "message" attribute is > > implementation-specific. > > > > Considering the current state of our media-internals error logging > > mechanisms, > > > > and allowing for future extensibility/modification and easy parsing by web > > > apps > > > > of Chrome's message details, I chose a serialized JSON object rather than > > just > > > > some weird concatenation of strings. > > > > > > > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR > (at > > > > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason > for > > > the > > > > latter is that, for example, an MSE stream parser might first detect > "parse > > > > failure" with some detailed MEDIA_LOG(ERROR) emission describing the > details > > > of > > > > the parse failure, and when this failure bubbles up into the demuxer, the > > > > pipeline, and eventually the webmediaplayer implementation, additional > > higher > > > > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > > > > typically more general, so preserving the series of logs allows the > greatest > > > > exposure of error logging detail to help identify the root cause. > > > > > > > > None of this was an attempt to force a message structure or content into > the > > > > spec or other implementations, rather to expose the information that we > may > > > have > > > > to assist debugging errors that occur when using our implementation. It > may > > > well > > > > result that other implementations (like Edge) might have completely > > different, > > > > though just as valid for this use case (identifying > implementation-specific > > > > error details to assist media error debugging), format and content of > their > > > > errors (such as perhaps just stringifying the msExtendedCode value that > they > > > > already have in Edge). > > > > > > > > See also related discussion on the spec issue > > > > (https://github.com/whatwg/html/issues/2085) > > > > > > Can you share an example of what one of the more verbose error messages will > > > look like? Is it just a list of messages concatenated, or is there real > > > structure/nesting to the serialized object? > > > > A more complex set of errors would result from something like an attempted > > playback of a file using MSE that hits the error condition at > > > https://cs.chromium.org/chromium/src/media/formats/mp4/aac.cc?rcl=91b3f7e7b1f..., > > resulting in a message that contains (format under discussion here, still): > > > > A pipeline error message (like "chunk demuxer: append failed"), and multiple > > MEDIA_LOG(ERROR) emissions like: > > "Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO > > 14496-3:2009 Table 1.18 for supported Sampling Frequencies." > > and (emitted from > > > https://cs.chromium.org/chromium/src/media/filters/source_buffer_state.cc?typ...) > > "SourceBufferState::Append: stream parsing failed. Data size=<some number> > > append_window_start=<some_number> append_window_end=<some_number>" > > > > Note that the latter error log entry contains details related to the context > of > > the append, where the earlier one contains the root reason why the append > > failed, and the pipeline error message is a top-level description of the error > > that is just a little more detailed than the standardized MediaError.code > enum. > > > > In the serialized JSON object, it would have two entries in the "error_log" > > array, along with one "cause". In the proposed in #26 version of the format, > it > > would be more human readable but contain all the same basic content. > > After chatting at length with sandersd@, we also agreed that reversing the > sequence of queued media_error_event_log entries when emitting them from > render_media_log.cc will allow us to maintain a more human-readable convention > of a list of log description and entries starting at the most general and ending > with the most specific available. > > Something like: > Error description:\n > demuxer: no supported streams\n > Error details:\n > SourceBufferState::Append: stream parsing failed. Data size=<some number> > append_window_start=<some_number> append_window_end=<some_number>\n > Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO > 14496-3:2009 Table 1.18 for supported Sampling Frequencies.\n > > Or another example for a case where the WebMediaPlayer doesn't exist, but > HTMLMediaElement has some info to report: > Error description:\n > media element error: format error\n > > We may also modify in this or a later CL the MEDIA_LOG macro to require a third > parameter, "tag", which will help identify the module that produced the error > event log entry in the eventual message's Error details list. That newline-separated and more human readable format looks good, thanks! I realize that it's unusual to ask for things to be *less* machine readable, but if we find requests for more structure in the error logs, we should probably start up a new spec-side conversation about how best to represent that.
On 2017/04/13 at 06:38:28, foolip wrote: > On 2017/04/12 19:37:22, wolenetz wrote: > > On 2017/04/12 18:01:45, wolenetz wrote: > > > On 2017/04/12 17:40:40, foolip_UTC7 wrote: > > > > > > > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/core/html/media/MediaError.h (right): > > > > > > > > > > > > > https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() > > > > const { return message_; } > > > > On 2017/04/12 17:29:57, wolenetz wrote: > > > > > On 2017/04/12 15:56:41, mlamouri wrote: > > > > > > That's quite unexpected: why are we passing a JSONObject? Usually error > > > > > messages > > > > > > are simple strings. Returning a serialised JSON object seems like > > slightly > > > > > > working around the spec. Could we have a `messages` IDL instead or > > simply > > > > > return > > > > > > the most recent error? How likely it is that the most recent error would > > > be > > > > > > noise? > > > > > > > > > > The format and content of the "message" attribute is > > > implementation-specific. > > > > > Considering the current state of our media-internals error logging > > > mechanisms, > > > > > and allowing for future extensibility/modification and easy parsing by web > > > > apps > > > > > of Chrome's message details, I chose a serialized JSON object rather than > > > just > > > > > some weird concatenation of strings. > > > > > > > > > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR > > (at > > > > > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason > > for > > > > the > > > > > latter is that, for example, an MSE stream parser might first detect > > "parse > > > > > failure" with some detailed MEDIA_LOG(ERROR) emission describing the > > details > > > > of > > > > > the parse failure, and when this failure bubbles up into the demuxer, the > > > > > pipeline, and eventually the webmediaplayer implementation, additional > > > higher > > > > > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > > > > > typically more general, so preserving the series of logs allows the > > greatest > > > > > exposure of error logging detail to help identify the root cause. > > > > > > > > > > None of this was an attempt to force a message structure or content into > > the > > > > > spec or other implementations, rather to expose the information that we > > may > > > > have > > > > > to assist debugging errors that occur when using our implementation. It > > may > > > > well > > > > > result that other implementations (like Edge) might have completely > > > different, > > > > > though just as valid for this use case (identifying > > implementation-specific > > > > > error details to assist media error debugging), format and content of > > their > > > > > errors (such as perhaps just stringifying the msExtendedCode value that > > they > > > > > already have in Edge). > > > > > > > > > > See also related discussion on the spec issue > > > > > (https://github.com/whatwg/html/issues/2085) > > > > > > > > Can you share an example of what one of the more verbose error messages will > > > > look like? Is it just a list of messages concatenated, or is there real > > > > structure/nesting to the serialized object? > > > > > > A more complex set of errors would result from something like an attempted > > > playback of a file using MSE that hits the error condition at > > > > > https://cs.chromium.org/chromium/src/media/formats/mp4/aac.cc?rcl=91b3f7e7b1f..., > > > resulting in a message that contains (format under discussion here, still): > > > > > > A pipeline error message (like "chunk demuxer: append failed"), and multiple > > > MEDIA_LOG(ERROR) emissions like: > > > "Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO > > > 14496-3:2009 Table 1.18 for supported Sampling Frequencies." > > > and (emitted from > > > > > https://cs.chromium.org/chromium/src/media/filters/source_buffer_state.cc?typ...) > > > "SourceBufferState::Append: stream parsing failed. Data size=<some number> > > > append_window_start=<some_number> append_window_end=<some_number>" > > > > > > Note that the latter error log entry contains details related to the context > > of > > > the append, where the earlier one contains the root reason why the append > > > failed, and the pipeline error message is a top-level description of the error > > > that is just a little more detailed than the standardized MediaError.code > > enum. > > > > > > In the serialized JSON object, it would have two entries in the "error_log" > > > array, along with one "cause". In the proposed in #26 version of the format, > > it > > > would be more human readable but contain all the same basic content. > > > > After chatting at length with sandersd@, we also agreed that reversing the > > sequence of queued media_error_event_log entries when emitting them from > > render_media_log.cc will allow us to maintain a more human-readable convention > > of a list of log description and entries starting at the most general and ending > > with the most specific available. > > > > Something like: > > Error description:\n > > demuxer: no supported streams\n > > Error details:\n > > SourceBufferState::Append: stream parsing failed. Data size=<some number> > > append_window_start=<some_number> append_window_end=<some_number>\n > > Sampling Frequency Index(0x<some_hex_value) is not supported. Please see ISO > > 14496-3:2009 Table 1.18 for supported Sampling Frequencies.\n > > > > Or another example for a case where the WebMediaPlayer doesn't exist, but > > HTMLMediaElement has some info to report: > > Error description:\n > > media element error: format error\n > > > > We may also modify in this or a later CL the MEDIA_LOG macro to require a third > > parameter, "tag", which will help identify the module that produced the error > > event log entry in the eventual message's Error details list. > > That newline-separated and more human readable format looks good, thanks! I realize that it's unusual to ask for things to be *less* machine readable, but if we find requests for more structure in the error logs, we should probably start up a new spec-side conversation about how best to represent that. Would it make sense to at least acknowledge the multiple messages by having `message` be renamed to `messages` and return a `sequence<DOMString>`? Websites will try to parse those messages for debug purposes and if we have a clear structure, it is very likely that they will use it and require all other browsers to imitate it. Also, I think it would be valuable to define in spec languages what messages would end up in this list. Even if not a requirement, it would slightly help interop.
On 2017/04/13 10:09:39, mlamouri wrote: > On 2017/04/13 at 06:38:28, foolip wrote: > > That newline-separated and more human readable format looks good, thanks! I > realize that it's unusual to ask for things to be *less* machine readable, but > if we find requests for more structure in the error logs, we should probably > start up a new spec-side conversation about how best to represent that. > > Would it make sense to at least acknowledge the multiple messages by having > `message` be renamed to `messages` and return a `sequence<DOMString>`? Websites > will try to parse those messages for debug purposes and if we have a clear > structure, it is very likely that they will use it and require all other > browsers to imitate it. Also, I think it would be valuable to define in spec > languages what messages would end up in this list. Even if not a requirement, it > would slightly help interop. Yes, we still have enough structure that attempts to parse it isn't implausible, just a bit more cumbersome than JSON. Something more structured like sequence<DOMString> would be a better API. Now the trouble is that Firefox has already shipped this as a DOMString, so either we'd end up with two APIs, or having to convince them to change. A bit of work, but not impossible.
On 2017/04/13 10:17:09, foolip_UTC7 wrote: > On 2017/04/13 10:09:39, mlamouri wrote: > > On 2017/04/13 at 06:38:28, foolip wrote: > > > That newline-separated and more human readable format looks good, thanks! I > > realize that it's unusual to ask for things to be *less* machine readable, but > > if we find requests for more structure in the error logs, we should probably > > start up a new spec-side conversation about how best to represent that. > > > > Would it make sense to at least acknowledge the multiple messages by having > > `message` be renamed to `messages` and return a `sequence<DOMString>`? > Websites > > will try to parse those messages for debug purposes and if we have a clear > > structure, it is very likely that they will use it and require all other > > browsers to imitate it. Also, I think it would be valuable to define in spec > > languages what messages would end up in this list. Even if not a requirement, > it > > would slightly help interop. > > Yes, we still have enough structure that attempts to parse it isn't implausible, > just a bit more cumbersome than JSON. Something more structured like > sequence<DOMString> would be a better API. Now the trouble is that Firefox has > already shipped this as a DOMString, so either we'd end up with two APIs, or > having to convince them to change. A bit of work, but not impossible. I'll reach out to FF today. I'd still like to get this into M59, even if merged back, since it is a high priority request from web authors to get at least some better error message info than just MediaError.code.
On 2017/04/12 09:16:42, jochen wrote: > Dimitri, who's the right person these days to decide what types to expose in the > public api? > > https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... > File media/blink/webmediaplayer_impl.cc (right): > > https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... > media/blink/webmediaplayer_impl.cc:780: std::map<std::string, > std::vector<std::string>> > hum, it looks like you really want a base::Value or something here, no? The API shape might be changing to a FrozenArray<DOMString>, and to limit the machine-parseability of the internals of a DOMString in such an array to prevent de-facto String structure standard from creating interop issues. As a result, the WebMediaPlayer API would probably just change to a vector of strings. If that API shape results, would you recommend we use a std::vector<std::string> or a WebVector<WebString> in the WebMediaPlayer API? I'm fairly confident that the latter conforms to existing code and would be the safest way to proceed; please let me know if this assumption is incorrect.
On 2017/04/13 19:01:20, wolenetz wrote: > On 2017/04/13 10:17:09, foolip_UTC7 wrote: > > On 2017/04/13 10:09:39, mlamouri wrote: > > > On 2017/04/13 at 06:38:28, foolip wrote: > > > > That newline-separated and more human readable format looks good, thanks! > I > > > realize that it's unusual to ask for things to be *less* machine readable, > but > > > if we find requests for more structure in the error logs, we should probably > > > start up a new spec-side conversation about how best to represent that. > > > > > > Would it make sense to at least acknowledge the multiple messages by having > > > `message` be renamed to `messages` and return a `sequence<DOMString>`? > > Websites > > > will try to parse those messages for debug purposes and if we have a clear > > > structure, it is very likely that they will use it and require all other > > > browsers to imitate it. Also, I think it would be valuable to define in spec > > > languages what messages would end up in this list. Even if not a > requirement, > > it > > > would slightly help interop. > > > > Yes, we still have enough structure that attempts to parse it isn't > implausible, > > just a bit more cumbersome than JSON. Something more structured like > > sequence<DOMString> would be a better API. Now the trouble is that Firefox has > > already shipped this as a DOMString, so either we'd end up with two APIs, or > > having to convince them to change. A bit of work, but not impossible. > > I'll reach out to FF today. I'd still like to get this into M59, even if merged > back, since it is a high priority request from web authors to get at least some > better error message info than just MediaError.code. I reached out using a new WHATWG spec bug (https://github.com/whatwg/html/issues/2531), and FF's initial response isn't positive (https://github.com/whatwg/html/issues/2531#issuecomment-294002913). There are limits to how much we could/should standardize the format/content of error messages. Implementations like FF produce only a single error message; Chromium could potentially aggregate errors from fallback attempts and at different points in the media engine, resulting in a set of strings. I'm not sure that a newline-delimited human-readable single string would trigger bad expectations or interop in either case. Even if we had a set of such strings, we then need to possible consider standardizing their order (granularity? chronological? severity of causing the eventual fatal error?) Again, implementations outside of Chromium might not emit more than just 1 "message", and if they did, they might choose different ordering that is more natural to express from their implementation. And the format of even a single string varies across implementations (and is allowed to per spec): FF (per https://github.com/whatwg/html/issues/2085#issuecomment-262634835): "The content of message itself shouldn't be subjected to a defined behaviour I believe. For Firefox it contains an extended code error, followed by a human readable and meaningful error message, followed by the location in the code that produced the error)" tl;dr: Though not implausible that web apps might parse a newline-delimited human-readable message string, if there are no newlines, what is the problem?
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog and WebMediaPlayer GetErrorMessage() to
GetErrorMessages():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* Includes the most recent pipeline error.
* Returns a map to let the caller handle serialization. Blink (see
next bullet) also generates messages, so the final serialization
from a JSONObject is done in Blink.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
To improve the usability of the messages, they are in JSON format,
though this is not required by the standard. The format may change in
the future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
Intent to implement and ship is
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future. This CL produces
messages in the following newline-delimited human readable format,
assuming error information is available:
Error description:
<some pipeline error description, or "Media element error">
Error Details:
<reverse chronological list of up to 20 most recent MEDIA_LOG(ERROR)
events at time of MediaError construction>
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086, 710617
==========
I've reworked this to be a single, human readable, newline-delimited message string (see updated CL description). Discussion of perhaps changing the spec to be a FrozenArray<DOMString> is happening in https://github.com/whatwg/html/issues/2531, though it has received frowns from Firefox who have already implemented the single string version. IMHO, it's probably best at this time to proceed with this single-string version. Please review (same reviewers/scope as before, plus foolip@ also for //third_party/WebKit/Source/core/html/*). https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:74: std::string MediaBrowserTest::EncodeErrorMessage( On 2017/04/12 15:56:41, mlamouri wrote: > Where is this called? Oops! :) Especially with newlines in the message, I'll correctly call this now from ::RunErrorMessageTest() like I was originally intending. Done. https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:114: void TestErrorMessage(const std::string& tag, On 2017/04/12 01:09:37, sandersd wrote: > Rename as RunErrorMessageTest to match other variants? Done. https://codereview.chromium.org/2660003003/diff/60001/content/browser/media/m... content/browser/media/media_browsertest.cc:298: "supported streams\"]}", On 2017/04/12 01:09:37, sandersd wrote: > This format seems overly complex and exposes rather more internal detail than I > was hoping. The object structure here is: > > { > "MEDIA_ERROR_LOG_ENTRY": [ "{\"error\":\"FFmpegDemuxer: no supported > streams\"}" ], > "PIPELINE_ERROR": [ "demuxer: no supported streams" ] > } > > I would propose: > > { > "cause": "demuxer: no supported streams", > "error_log": [ "FFmpegDemuxer: no supported streams" ] > } Done. See related discussion thread https://codereview.chromium.org/2660003003/#msg28 https://codereview.chromium.org/2660003003/diff/60001/content/renderer/media/... File content/renderer/media/render_media_log.h (right): https://codereview.chromium.org/2660003003/diff/60001/content/renderer/media/... content/renderer/media/render_media_log.h:79: std::deque<std::unique_ptr<media::MediaLogEvent>> On 2017/04/12 01:09:38, sandersd wrote: > Perhaps std::vector would be a better choice, since MediaLogEvents are small? And std::unique_ptrs to them are even smaller ;) Done. https://codereview.chromium.org/2660003003/diff/60001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/2660003003/diff/60001/media/base/media_log.cc... media/base/media_log.cc:212: bool include_type) { On 2017/04/12 01:09:38, sandersd wrote: > Not a fan of this parameter, it's not clear from context what it does at the > call sites. This is a case where two methods makes more sense. Done. https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... File media/blink/webmediaplayer_impl.cc (right): https://codereview.chromium.org/2660003003/diff/60001/media/blink/webmediapla... media/blink/webmediaplayer_impl.cc:780: std::map<std::string, std::vector<std::string>> On 2017/04/12 09:16:42, jochen wrote: > hum, it looks like you really want a base::Value or something here, no? I switched back to just blink::WebString. JSON-encoding the error message was contentious (see other threads on this review). https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/media/MediaError.h:60: String message() const { return message_; } On 2017/04/12 17:40:40, foolip_UTC7 wrote: > On 2017/04/12 17:29:57, wolenetz wrote: > > On 2017/04/12 15:56:41, mlamouri wrote: > > > That's quite unexpected: why are we passing a JSONObject? Usually error > > messages > > > are simple strings. Returning a serialised JSON object seems like slightly > > > working around the spec. Could we have a `messages` IDL instead or simply > > return > > > the most recent error? How likely it is that the most recent error would be > > > noise? > > > > The format and content of the "message" attribute is implementation-specific. > > Considering the current state of our media-internals error logging mechanisms, > > and allowing for future extensibility/modification and easy parsing by web > apps > > of Chrome's message details, I chose a serialized JSON object rather than just > > some weird concatenation of strings. > > > > In the Chromium-side implementation, we hold onto just 1 PIPELINE_ERROR (at > > most), and potentially multiple MEDIA_LOG(ERROR) event logs. The reason for > the > > latter is that, for example, an MSE stream parser might first detect "parse > > failure" with some detailed MEDIA_LOG(ERROR) emission describing the details > of > > the parse failure, and when this failure bubbles up into the demuxer, the > > pipeline, and eventually the webmediaplayer implementation, additional higher > > level MEDIA_LOG(ERROR) emissions may also occur. These later emissions are > > typically more general, so preserving the series of logs allows the greatest > > exposure of error logging detail to help identify the root cause. > > > > None of this was an attempt to force a message structure or content into the > > spec or other implementations, rather to expose the information that we may > have > > to assist debugging errors that occur when using our implementation. It may > well > > result that other implementations (like Edge) might have completely different, > > though just as valid for this use case (identifying implementation-specific > > error details to assist media error debugging), format and content of their > > errors (such as perhaps just stringifying the msExtendedCode value that they > > already have in Edge). > > > > See also related discussion on the spec issue > > (https://github.com/whatwg/html/issues/2085) > > Can you share an example of what one of the more verbose error messages will > look like? Is it just a list of messages concatenated, or is there real > structure/nesting to the serialized object? Per discussion threads, redone as a human-readable, newline-delimited single string. There's also spec discussion to, instead of newline-delimited single string, use an array of strings (https://github.com/whatwg/html/issues/2531), though that is getting push-back from at least Firefox initially.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Discussion on the spec issue (https://github.com/whatwg/html/issues/2531#issuecomment-294213577) is proceeding, but slowly. In the meantime, it looks strongly likely that a single string 'message' attribute will remain, and the discussion is around the internal format of that message. To speed getting a solution (hopefully) into M59, I'm going to adjust this CL back to a simpler version which reports a single (new newline) string, with a concatenation of up to max 1 each of : 1) Pipeline status error message, or media element error if no pipeline error, and 2) First cached MEDIA_LOG(ERROR) message for the player (this has best chance of being the most specific cause of the error). Subsequent changes might re-introduce a "multiple" MEDIA_LOG(ERROR) capability to this field (or maybe some other 'messages' field) depending on how the spec discussion goes, and orthogonal changes will likely improve the variety, quality, and usability of the specific error messages as needed. I'll have an updated CL shortly.
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please take a look at patch set 6. Same reviewer coverage as before.
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future. This CL produces
messages in the following newline-delimited human readable format,
assuming error information is available:
Error description:
<some pipeline error description, or "Media element error">
Error Details:
<reverse chronological list of up to 20 most recent MEDIA_LOG(ERROR)
events at time of MediaError construction>
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future. This CL produces
messages in the following newline-delimited human readable format,
assuming error information is available:
Error description:
<some pipeline error description, or "Media element error">
Error Details:
<reverse chronological list of up to 20 most recent MEDIA_LOG(ERROR)
events at time of MediaError construction>
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
* Includes (up to a capped number) multiple recent
MEDIA_ERROR_LOG_ENTRY because such errors are frequently reported in
multiple places with varying detail for the same underlying root
cause.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future. This CL produces
messages in the following newline-delimited human readable format,
assuming error information is available:
Error description:
<some pipeline error description, or "Media element error">
Error Details:
<reverse chronological list of up to 20 most recent MEDIA_LOG(ERROR)
events at time of MediaError construction>
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
Includes only the first MEDIA_ERROR_LOG_ENTRY from the log, if any,
since that is most likely to contain the most precise error
information, and the structure of any aggregation of multiple error
events is still under discussion in the spec [4].
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). This CL produces messages in the following human
readable format, with no newlines, assuming error information is
available: a concatenation of up to max 1 each of:
1) Pipeline status error message, or media element error if no pipeline
error, and
2) First cached MEDIA_LOG(ERROR) message for the player (this has best
chance of being the most specific cause of the error).
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
lgtm. We should follow up by auditing all of the error message paths to improve the formatting, perhaps as a fixit task.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/04/14 20:44:45, wolenetz wrote: > Discussion on the spec issue > (https://github.com/whatwg/html/issues/2531#issuecomment-294213577) is > proceeding, but slowly. In the meantime, it looks strongly likely that a single > string 'message' attribute will remain, and the discussion is around the > internal format of that message. To speed getting a solution (hopefully) into > M59, I'm going to adjust this CL back to a simpler version which reports a > single (new newline) string, with a concatenation of up to max 1 each of : Was this a typo, do you mean no newlines in the string? That SGTM if it's still possible to get across the important information to diagnose the issue. > 1) Pipeline status error message, or media element error if no pipeline error, > and > 2) First cached MEDIA_LOG(ERROR) message for the player (this has best chance of > being the most specific cause of the error). > > Subsequent changes might re-introduce a "multiple" MEDIA_LOG(ERROR) capability > to this field (or maybe some other 'messages' field) depending on how the spec > discussion goes, and orthogonal changes will likely improve the variety, > quality, and usability of the specific error messages as needed. > > I'll have an updated CL shortly. Explicitly not reviewing any code since I wasn't asked to initially, please poke me if needed.
On 2017/04/17 08:49:01, foolip_UTC7 wrote: > On 2017/04/14 20:44:45, wolenetz wrote: > > Discussion on the spec issue > > (https://github.com/whatwg/html/issues/2531#issuecomment-294213577) is > > proceeding, but slowly. In the meantime, it looks strongly likely that a > single > > string 'message' attribute will remain, and the discussion is around the > > internal format of that message. To speed getting a solution (hopefully) into > > M59, I'm going to adjust this CL back to a simpler version which reports a > > single (new newline) string, with a concatenation of up to max 1 each of : > > Was this a typo, do you mean no newlines in the string? That SGTM if it's still > possible to get across the important information to diagnose the issue. Yes, typo. I meant to say "(no newline)" in that comment. > > 1) Pipeline status error message, or media element error if no pipeline error, > > and > > 2) First cached MEDIA_LOG(ERROR) message for the player (this has best chance > of > > being the most specific cause of the error). > > > > Subsequent changes might re-introduce a "multiple" MEDIA_LOG(ERROR) capability > > to this field (or maybe some other 'messages' field) depending on how the spec > > discussion goes, and orthogonal changes will likely improve the variety, > > quality, and usability of the specific error messages as needed. > > > > I'll have an updated CL shortly. > > Explicitly not reviewing any code since I wasn't asked to initially, please poke > me if needed. OK. mlamouri@, jochen@ and mcasas@, please take a look. Thanks!
lgtm assuming the spec discussion concludes https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... content/browser/media/media_browsertest.cc:296: GetParam()); Should there be a test for multi-lines errors? https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... File content/browser/media/media_browsertest.h (right): https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... content/browser/media/media_browsertest.h:45: std::string EncodeErrorMessage(const std::string& original_message); It doesn't sound like you need this to be part of the class but instead could be part of the anonymous namespace. Any specific reason why it's part of the header?
On 2017/04/18 12:37:59, mlamouri wrote: > lgtm assuming the spec discussion concludes > > https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... > File content/browser/media/media_browsertest.cc (right): > > https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... > content/browser/media/media_browsertest.cc:296: GetParam()); > Should there be a test for multi-lines errors? > > https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... > File content/browser/media/media_browsertest.h (right): > > https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... > content/browser/media/media_browsertest.h:45: std::string > EncodeErrorMessage(const std::string& original_message); > It doesn't sound like you need this to be part of the class but instead could be > part of the anonymous namespace. Any specific reason why it's part of the > header? Thanks for the review, Mounir. Please take a look at jyavenard@'s earlier response in the spec discussion (https://github.com/whatwg/html/issues/2531#issuecomment-294133130), which strongly implies the 'message' API will remain as-is. Possibly we could standardize later some 'messages' API, but that would be for a different CL.
https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... File content/browser/media/media_browsertest.cc (right): https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... content/browser/media/media_browsertest.cc:296: GetParam()); On 2017/04/18 12:37:58, mlamouri wrote: > Should there be a test for multi-lines errors? The spec discussion (and this patch set) leans towards no multi-line errors. This CL combines the pipeline_status message ("demuxer: no supported streams") with the underlying first-logged MEDIA_LOG(ERROR) message ("FFmpegDemuxer: no supported streams") to produce a single output message string that does not contain any newlines. Of course, this particular example error case shows there is room for improving the content of the messages going forwards, but it shows the format does not contain newlines. https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... File content/browser/media/media_browsertest.h (right): https://codereview.chromium.org/2660003003/diff/120001/content/browser/media/... content/browser/media/media_browsertest.h:45: std::string EncodeErrorMessage(const std::string& original_message); On 2017/04/18 12:37:59, mlamouri wrote: > It doesn't sound like you need this to be part of the class but instead could be > part of the anonymous namespace. Any specific reason why it's part of the > header? In this particular patch set, you're correct: this method is only locally used and could be moved to anonymous namespace within the impl (media_browsertest.cc). However, I foresee we may want to have similar error message testing later on in other kinds of MediaBrowserTest, so put it here to make it more obvious that this method is here and help reduce possibility of eventual duplication of this functionality. No other strong feeling here, so if you feel strongly, I'll move it to the impl.
jochen@ and mcasas@, friendly ping to please review this: jochen@: //third_party/WebKit/public/platform/*, third_party/WebKit/Source/platform/*, and API_OWNERS review of the various updated global-interface-listing-expected.txt mcasas@: //content/renderer/media_capture_from_element/*
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Have you thought about how mega-sites like YouTube, Vimeo, Facebook will be able to tokenize and aggregate this data in reports to us? Aggregate error messages are a problem in this case since they result in a combinatorial explosion. I'm a bit concerned about stuffing multiple error messages into a single one with no obvious token scheme for them to split on.
On 2017/04/18 at 18:25:11, DaleCurtis wrote: > Have you thought about how mega-sites like YouTube, Vimeo, Facebook will be able to tokenize and aggregate this data in reports to us? Aggregate error messages are a problem in this case since they result in a combinatorial explosion. I'm a bit concerned about stuffing multiple error messages into a single one with no obvious token scheme for them to split on. As noted offline, it's a hard requirement that this be machine parseable. We should work with other browsers to find the most compatible approach, but by definition this field is defined as platform specific so I don't buy the arguments that by making our field machine parseable other browsers will suffer. Especially since the users of this will be highly incentivized to do whatever is appropriate per-platform.
On 2017/04/18 19:07:25, DaleCurtis wrote: > On 2017/04/18 at 18:25:11, DaleCurtis wrote: > > Have you thought about how mega-sites like YouTube, Vimeo, Facebook will be > able to tokenize and aggregate this data in reports to us? Aggregate error > messages are a problem in this case since they result in a combinatorial > explosion. I'm a bit concerned about stuffing multiple error messages into a > single one with no obvious token scheme for them to split on. > > As noted offline, it's a hard requirement that this be machine parseable. We > should work with other browsers to find the most compatible approach, but by > definition this field is defined as platform specific so I don't buy the > arguments that by making our field machine parseable other browsers will suffer. > Especially since the users of this will be highly incentivized to do whatever is > appropriate per-platform. Your concern is valid; it led to my original approach (JSON-object with clearly identified key/values), which led to concerns of the message format itself becoming a de-facto standard which Firefox's implementation (already shipped) would not conform to. For now, we might already have de-facto standard for a parseable format from Firefox's shipped implementation, which appears to be: Either "", or: <all_caps_eror_code_identifier> (<0x32-bit hex error code)[ : optional additional message containing things like function signature followed by a readable error message that may contain like track numbers] So the non-noisy "code" part of their message is prior to the first ':', if any. Example: "NS_ERROR_DOM_MEDIA_DEMUXER_ERR (0x806e000c) : virtual RefPtrMP4Demuxer::InitPromise mozilla::MP4Demuxer::Init(): No MP4 audio () or video () tracks" Now, their codes are more differentiated than our PipelineStatus codes, but we could do something that fits in the same tokenization for the code as Firefox like: Either "", or: <PipelineStatus enum identifier or "HTML_MEDIA_ELEMENT_ERROR"> [perhaps add more module-specific code here] [ : optional additional message containing detail, format and content to be clarified / improved over time] Example: "PIPELINE_ERROR_NO_SUPPORTED_STREAMS : FFmpegDemuxer: no supported streams" The concern would be the limited variation currently in our PipelineStatus codes would not afford clear variation/understanding of the specific reason for the failure (without aggregating the (potentially variable) optional additional message; though that concern is shared with Firefox's implementation currently. A solution would be to introduce greater variety of implementation-specific codes. WDYT? If this appears workable to you, I'll reach out to Firefox, WHATWG and W3C with such a proposal.
> On 2017/04/18 at 18:25:11, DaleCurtis wrote: .. > I'm a bit concerned about stuffing multiple error messages into a > single one with no obvious token scheme for them to split on. .. Also, we've already rejected the immediate need for stuffing multiple error messages into the 'message' field. The earliest MEDIA_LOG(ERROR) should be the most specific cause of the error in the majority of cases. That log (if any), combined with the PipelineStatus (if any pipeline, or an HTMLMediaElement internal error code) code is the current proposal; we're just trying to figure out the best format for parseability and interop of this code+optional detail at the moment.
On 2017/04/18 at 20:34:27, wolenetz wrote: > > On 2017/04/18 at 18:25:11, DaleCurtis wrote: > .. > > I'm a bit concerned about stuffing multiple error messages into a > > single one with no obvious token scheme for them to split on. > .. > > Also, we've already rejected the immediate need for stuffing multiple error messages into the 'message' field. The earliest MEDIA_LOG(ERROR) should be the most specific cause of the error in the majority of cases. That log (if any), combined with the PipelineStatus (if any pipeline, or an HTMLMediaElement internal error code) code is the current proposal; we're just trying to figure out the best format for parseability and interop of this code+optional detail at the moment. "PIPELINE_ERROR_XXX (##) : <message>" sgtm if we're not sticking multiple messages in there. I don't like ":" as a separator given how frequently it ends up in messages, but so long as we're guaranteed there will be no ":" prior to the actual message that sgtm.
On 2017/04/18 21:19:04, DaleCurtis wrote: > On 2017/04/18 at 20:34:27, wolenetz wrote: > > > On 2017/04/18 at 18:25:11, DaleCurtis wrote: > > .. > > > I'm a bit concerned about stuffing multiple error messages into a > > > single one with no obvious token scheme for them to split on. > > .. > > > > Also, we've already rejected the immediate need for stuffing multiple error > messages into the 'message' field. The earliest MEDIA_LOG(ERROR) should be the > most specific cause of the error in the majority of cases. That log (if any), > combined with the PipelineStatus (if any pipeline, or an HTMLMediaElement > internal error code) code is the current proposal; we're just trying to figure > out the best format for parseability and interop of this code+optional detail at > the moment. > > "PIPELINE_ERROR_XXX (##) : <message>" sgtm if we're not sticking multiple > messages in there. I don't like ":" as a separator given how frequently it ends > up in messages, but so long as we're guaranteed there will be no ":" prior to > the actual message that sgtm. OK. It seems there's currently no objection to that format on the spec discussion either, just requests for further work on perhaps an additional UA-specific-code field (and also, IMHO a less motivated/needed, multiple-string `messages` API). I'll update this CL to conform to this new format.
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog getErrorMessage():
Includes only the first MEDIA_ERROR_LOG_ENTRY from the log, if any,
since that is most likely to contain the most precise error
information, and the structure of any aggregation of multiple error
events is still under discussion in the spec [4].
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information. This should assist
edge case differentiation.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). This CL produces messages in the following human
readable format, with no newlines, assuming error information is
available: a concatenation of up to max 1 each of:
1) Pipeline status error message, or media element error if no pipeline
error, and
2) First cached MEDIA_LOG(ERROR) message for the player (this has best
chance of being the most specific cause of the error).
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
BUG=601086, 710617
==========
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Please review patch set 8: dalecurtis@: w.r.t. format of message change to [[code][: detail message]] sandersd@: Everything, especially //media/* and //content/renderer/media/* mlamouri@: //third_party/WebKit/Source/core/html/* and the media/video-error-does-not-exist.html layout test jochen@: //third_party/WebKit/public/platform/*, third_party/WebKit/Source/platform/*, and API_OWNERS review of the updated global-interface-listing-expected.txt mcasas@: //content/renderer/media_capture_from_element/*
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:95: // Hold onto the most recent PIPELINE_ERROR and the first, if any, I thought we wanted the earliest one ? https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:144: std::string result = ""; Why did you drop the std::stringstream? Generally you want to avoid extraneous string allocations. Given the asynchronous nature of the errors reported, this sometimes generates messages like: ": blah" "PIPELINE_ERROR_READ" Versus always generating something of the format "<a> (<#>): <c>" which is what we discussed earlier. Between two subsequent calls to this function you're going to end up with incomplete messages. I.e ERROR_READ on one call, then "ERROR_READ: extra error" If this all happens before we surface an error to client we're probably okay, but otherwise telemetry may end up pulling partially completed messages. Can you talk a bit more about this and why / why not it's an issue?
https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:95: // Hold onto the most recent PIPELINE_ERROR and the first, if any, On 2017/04/20 20:57:13, DaleCurtis wrote: > I thought we wanted the earliest one ? Interesting. I hadn't considered that we might report a sequence of different pipeline errors, too, for a single eventual MediaError instance. Like the first MEDIA_ERROR_LOG_ENTRY, would the first PIPELINE_ERROR event also be the most specific? https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:144: std::string result = ""; On 2017/04/20 20:57:13, DaleCurtis wrote: > Why did you drop the std::stringstream? Generally you want to avoid extraneous > string allocations. Given the asynchronous nature of the errors reported, this > sometimes generates messages like: > > ": blah" > "PIPELINE_ERROR_READ" > > Versus always generating something of the format "<a> (<#>): <c>" which is what > we discussed earlier. Between two subsequent calls to this function you're going > to end up with incomplete messages. I.e > > ERROR_READ on one call, then "ERROR_READ: extra error" > > If this all happens before we surface an error to client we're probably okay, > but otherwise telemetry may end up pulling partially completed messages. Can you > talk a bit more about this and why / why not it's an issue? Before a MediaError is constructed by the HTMLMediaElement (due to top-level fatal media error being determined), yes the pipeline_status events do not occur at the same time as other events like MEDIA_LOG_ERROR_ENTRY events. However, once that fatal media error is reported up to the media element, the *render* media log should contain this state. If there are paths that, say, log pertinent error information *after* reporting the error condition to WebMediaPlayerClient, then that error info would not be included in the MediaError.message. tl;dr: any events pertinent to the top-level error must be added to the RenderMediaLog prior to signalling the top-level error, otherwise yes there would be partial error messages produced. I think we're ok.
On 2017/04/20 21:09:03, wolenetz wrote: > https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... > File content/renderer/media/render_media_log.cc (right): > > https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... > content/renderer/media/render_media_log.cc:95: // Hold onto the most recent > PIPELINE_ERROR and the first, if any, > On 2017/04/20 20:57:13, DaleCurtis wrote: > > I thought we wanted the earliest one ? > > Interesting. I hadn't considered that we might report a sequence of different > pipeline errors, too, for a single eventual MediaError instance. Like the first > MEDIA_ERROR_LOG_ENTRY, would the first PIPELINE_ERROR event also be the most > specific? > > https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... > content/renderer/media/render_media_log.cc:144: std::string result = ""; > On 2017/04/20 20:57:13, DaleCurtis wrote: > > Why did you drop the std::stringstream? Generally you want to avoid extraneous > > string allocations. Given the asynchronous nature of the errors reported, this > > sometimes generates messages like: > > > > ": blah" > > "PIPELINE_ERROR_READ" > > > > Versus always generating something of the format "<a> (<#>): <c>" which is > what > > we discussed earlier. Between two subsequent calls to this function you're > going > > to end up with incomplete messages. I.e > > > > ERROR_READ on one call, then "ERROR_READ: extra error" > > > > If this all happens before we surface an error to client we're probably okay, > > but otherwise telemetry may end up pulling partially completed messages. Can > you > > talk a bit more about this and why / why not it's an issue? > > Before a MediaError is constructed by the HTMLMediaElement (due to top-level > fatal media error being determined), yes the pipeline_status events do not occur > at the same time as other events like MEDIA_LOG_ERROR_ENTRY events. However, > once that fatal media error is reported up to the media element, the *render* > media log should contain this state. If there are paths that, say, log pertinent > error information *after* reporting the error condition to WebMediaPlayerClient, > then that error info would not be included in the MediaError.message. > > tl;dr: any events pertinent to the top-level error must be added to the > RenderMediaLog prior to signalling the top-level error, otherwise yes there > would be partial error messages produced. I think we're ok. Also, our format in this CL is less specific than "<a> (<#>): <c>" It's just "code: detail" (we can always add stuff to the code portion, like (<#>) later. Dan and I were thinking of possibly including module tags or greater pipelinestatus variety.)
https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:95: // Hold onto the most recent PIPELINE_ERROR and the first, if any, On 2017/04/20 21:09:03, wolenetz wrote: > On 2017/04/20 20:57:13, DaleCurtis wrote: > > I thought we wanted the earliest one ? > > Interesting. I hadn't considered that we might report a sequence of different > pipeline errors, too, for a single eventual MediaError instance. Like the first > MEDIA_ERROR_LOG_ENTRY, would the first PIPELINE_ERROR event also be the most > specific? I dug a little further: it looks like we only report at most one PIPELINE_ERROR event currently before top-level error is determined, causing MediaError.message creation from the result of this method. That PIPELINE_ERROR event originates only in WMPI at https://cs.chromium.org/chromium/src/media/blink/webmediaplayer_impl.cc?rcl=a... which is also the location from where WebMediaPlayerClient is signalled that there is a top-level error. So I think we're ok here. The discussion we previously had about multiple error messages was specific (I thought) to MEDIA_LOG_ERROR_ENTRY events, where the first of those would be the most precise. It appears that remains correct. And even if there were multiple PIPELINE_ERRORs, the one that would be most current/relevant to the top-level error would be that determined right before signalling the top-level error.
https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:144: std::string result = ""; On 2017/04/20 21:09:03, wolenetz wrote: > On 2017/04/20 20:57:13, DaleCurtis wrote: > > Why did you drop the std::stringstream? Generally you want to avoid extraneous w.r.t. my not using std::stringstream here: Oops! That crept in during an interim patch set. I just now checked with a modified media_perftest, and stringstream operator<< and str() usage in similar case is about 20% faster than std::string operator+= on linux Z620 Debug build. Fixed in patch set 9.
https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... File content/renderer/media/render_media_log.cc (right): https://codereview.chromium.org/2660003003/diff/160001/content/renderer/media... content/renderer/media/render_media_log.cc:144: std::string result = ""; On 2017/04/20 21:09:03, wolenetz wrote: > On 2017/04/20 20:57:13, DaleCurtis wrote: > > Why did you drop the std::stringstream? Generally you want to avoid extraneous > > string allocations. Given the asynchronous nature of the errors reported, this > > sometimes generates messages like: > > > > ": blah" > > "PIPELINE_ERROR_READ" > > > > Versus always generating something of the format "<a> (<#>): <c>" which is > what > > we discussed earlier. Between two subsequent calls to this function you're > going > > to end up with incomplete messages. I.e > > > > ERROR_READ on one call, then "ERROR_READ: extra error" > > > > If this all happens before we surface an error to client we're probably okay, > > but otherwise telemetry may end up pulling partially completed messages. Can > you > > talk a bit more about this and why / why not it's an issue? > > Before a MediaError is constructed by the HTMLMediaElement (due to top-level > fatal media error being determined), yes the pipeline_status events do not occur > at the same time as other events like MEDIA_LOG_ERROR_ENTRY events. However, > once that fatal media error is reported up to the media element, the *render* > media log should contain this state. If there are paths that, say, log pertinent > error information *after* reporting the error condition to WebMediaPlayerClient, > then that error info would not be included in the MediaError.message. > > tl;dr: any events pertinent to the top-level error must be added to the > RenderMediaLog prior to signalling the top-level error, otherwise yes there > would be partial error messages produced. I think we're ok. In patch set 10, I've added a comment to media_log.h's declaration of this virtual method to help clarify in the code.
lgtm.
media log format lgtm, defer to sandersd@ for rest of review. Thanks Matt! https://codereview.chromium.org/2660003003/diff/200001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/2660003003/diff/200001/media/base/media_log.c... media/base/media_log.cc:194: } #undef macro after usage.
https://codereview.chromium.org/2660003003/diff/200001/media/base/media_log.cc File media/base/media_log.cc (right): https://codereview.chromium.org/2660003003/diff/200001/media/base/media_log.c... media/base/media_log.cc:194: } On 2017/04/20 22:37:30, DaleCurtis wrote: > #undef macro after usage. Done in patch set 11 (and I moved the #define to within the method, too).
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
lgtm
lgtm with the Finalized questions answered. https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:224: String BuildElementErrorMessage(const String& error) { In the future, we might want to move this to MediaError. https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/MediaError.h:37: : public GarbageCollectedFinalized<MediaError>, Why Finalized? Finalized adds an overhead for the GC so we should avoid this unless there is a reason for it.
Thanks for review, Mounir. mcasas@, I believe your review is all that's left pending at the moment. https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/media/MediaError.h (right): https://codereview.chromium.org/2660003003/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/media/MediaError.h:37: : public GarbageCollectedFinalized<MediaError>, On 2017/04/21 10:26:24, mlamouri wrote: > Why Finalized? Finalized adds an overhead for the GC so we should avoid this > unless there is a reason for it. WTFString has a RefPtr private member, which IIUC requires that classes that have a WTFString themselves require finalization (which the message_ added in this CL to MediaError triggers). See https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... Also, when attempting to build with just GarbageCollected, the following error occurred: In file included from gen/blink/bindings/core/v8/V8MediaError.cpp:12: In file included from gen/blink/bindings/core/v8/V8MediaError.h:23: ../../third_party/WebKit/Source/core/html/media/MediaError.h:36:1: error: [blink-gc] Class 'MediaError' requires finalization. tl;dr: As we chatted about just now, I think GarbageCollectedFinalized is needed here.
wolenetz@chromium.org changed reviewers: + emircan@chromium.org
+emircan@ for reviewing media/media_capture_from_element/* Please take a look. Thanks!
On 2017/04/21 10:57:56, wolenetz wrote: > +emircan@ for reviewing media/media_capture_from_element/* > Please take a look. Thanks! capture_from_element changes necessary? Looks like a text reflow but don't let me stop this large CL, lgtm
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sandersd@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2660003003/#ps220001 (title: "Address dalecurtis@'s comment: undef STRINGIFY_STATUS_CASE")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492791803910390,
"parent_rev": "c0b2b8b1b919b0fcda51e691406d50a62f89771d", "commit_rev":
"ed8e7091b5b0a51a8c0781f95813a5929c1f8110"}
Message was sent while issue was closed.
Description was changed from
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
==========
to
==========
Add MediaError.message
This improves the level of information available for web authors to use
when debugging media playback errors.
* Adds support for the MediaError.message field [1] [2]
* Changes MediaLog::PipelineStatusToString() to return just a
stringified version of the enum identifier, suitable for use as a
UA-specific-code when included in MediaError.message.
* Changes MediaLog::GetErrorMessage() to produce messages in the form:
[[Stringified pipeline error code, if any, without any colon][: The
first MEDIA_ERROR_LOG_ENTRY from the log, if any, since that is most
likely to contain the most precise error information]]
The produced message does not contain any newlines.
See the updated media_browsertest.cc for basic examples.
* When the message would otherwise be empty for various cases in
HTMLMediaElement, and more meaningful information beyond the basic
standard MediaError codes and HTMLMediaElement networkStates is
available, reports basic additional information (prefixed by error
code "MEDIA_ELEMENT_ERROR: ".) This should assist edge case
differentiation. See the updated media_browsertest.cc for basic
examples.
* Test coverage:
* Adds basic layout test coverage test similar to that added in [3].
* Adds basic content browser test coverage for a small variety of
errors, since these messages' format and content are specific to
Chrom*.
The format of the message string may change in future (for example, see
discussion in [4]). Also, the variety of error codes in the message
prior to the first ':' will likely change in future, and the descriptive
error message detail following the ':' will likely change to include
more cases or more consistent format in future.
[1] - https://github.com/whatwg/html/issues/2085
[2] - https://github.com/whatwg/html/pull/2086
[3] - https://github.com/w3c/web-platform-tests/pull/4638
[4] - https://github.com/whatwg/html/issues/2531
[Intent to implement and ship] -
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yDIyWozG1xk
BUG=601086, 710617
Review-Url: https://codereview.chromium.org/2660003003
Cr-Commit-Position: refs/heads/master@{#466359}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed8e7091b5b0a51a8c0781f95813...
==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001) as https://chromium.googlesource.com/chromium/src/+/ed8e7091b5b0a51a8c0781f95813... |
