This bug will add the necessary plumbing to: - Store decode errors and warnings in Decoder Doctor, - Forward errors from HTMLMediaElement to Decoder Doctor, - Forward warnings (i.e., recoverable errors) from HTMLMediaElement to Decoder Doctor. The actual analysis of errors&warnings (and subsequent user notifications) will be implemented in an upcoming bug.

Comment on attachment 8841898 [details] Bug 1343161 - DecoderDoctorDiagnostics::StoreDecodeError/Warning - https://reviewboard.mozilla.org/r/115964/#review117382 ::: dom/media/DecoderDoctorDiagnostics.h:71 (Diff revision 1) > + void StoreDecodeWarning(nsIDocument* aDocument, > + const MediaResult& aWarning, > + const nsString& aSrcSENSITIVE, > + const char* aCallSite); > + > enum DiagnosticsType { WRONG brace placement ::: dom/media/DecoderDoctorDiagnostics.h:154 (Diff revision 1) > KeySystemIssue mKeySystemIssue = eUnset; > > DecoderDoctorEvent mEvent; > + > + MediaResult mDecodeIssue = NS_OK; > + nsString mDecodeIssueSrcSENSITIVE; What is sensitive about this string? it is case sensitive, or security wise? Regsrdless it is a weird name ::: dom/media/DecoderDoctorDiagnostics.cpp:468 (Diff revision 1) > default: > MOZ_ASSERT(diag.mDecoderDoctorDiagnostics.Type() > == DecoderDoctorDiagnostics::eFormatSupportCheck > || diag.mDecoderDoctorDiagnostics.Type() > - == DecoderDoctorDiagnostics::eMediaKeySystemAccessRequest); > + == DecoderDoctorDiagnostics::eMediaKeySystemAccessRequest > + || diag.mDecoderDoctorDiagnostics.Type() Isnt this unreachable code? you are in the default handling of the switch, and all those cases have been handled before. all this moz assert should be removed.

Comment on attachment 8841899 [details] Bug 1343161 - StoreDecodeError from HTMLMediaElement::DecodeError - https://reviewboard.mozilla.org/r/115966/#review117398

Comment on attachment 8841900 [details] Bug 1343161 - MediaDecoderOwner::DecodeWarning and HTMLMediaElement implementation - https://reviewboard.mozilla.org/r/115968/#review117400 TBH, i am not sure what value this is bringing to the user. it is way too technical for your average joe to care or know what to do with this. i guess maybe for telemetry reporting.. and even there, that information has no value, we have recovered from it already

Comment on attachment 8841898 [details] Bug 1343161 - DecoderDoctorDiagnostics::StoreDecodeError/Warning - https://reviewboard.mozilla.org/r/115964/#review117382 > What is sensitive about this string? > it is case sensitive, or security wise? > > Regsrdless it is a weird name It is privacy-sensitive, I just wanted to make sure that I don't expose this string carelessly, by making it obvious that it is SENSITIVE through its name (wherever it is used) rather than an easily-missed comment! After checking with sec people, I will rename to something less weird. > Isnt this unreachable code? you are in the default handling of the switch, and all those cases have been handled before. > > all this moz assert should be removed. It's code that should not be reached in normal situations, I just wanted to be explicit about which values are expected, so that they would appear in the diagnostics message attached to the assertion trigger. But it's getting too unwieldy, so I suppose I could use MOZ_ASSERT_UNREACHABLE instead.

Comment on attachment 8841900 [details] Bug 1343161 - MediaDecoderOwner::DecodeWarning and HTMLMediaElement implementation - https://reviewboard.mozilla.org/r/115968/#review117400 Good question! In this case, I intend to use it when Stagefright and Rust disagree on MP4 metadata. We still continue decoding using one of their results, but I would like to show a notification drop-down, so the user can choose to report a site issue. Telemetry will be there and tell us if users do report these warnings, or just ignore them. If after some time we think we don't get enough reports, we could change these warnings into unrecoverable errors. (I will add a pref in a later bug, so it's easy to switch between warning and error when we want to...) So in the best case we'll get some useful reports from users. In the worst case we'll still get some telemetry to help us make evidence-based decisions; and we could easily remove this extra code if we think it's really useless (based on actual usage). So I think it's worth doing this experiment.

Pushed by gsquelart@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/2d1f22488076 DecoderDoctorDiagnostics::StoreDecodeError/Warning - r=jya https://hg.mozilla.org/integration/autoland/rev/d6723e7b7126 StoreDecodeError from HTMLMediaElement::DecodeError - r=jya https://hg.mozilla.org/integration/autoland/rev/3e1458b66c24 MediaDecoderOwner::DecodeWarning and HTMLMediaElement implementation - r=jya

https://hg.mozilla.org/mozilla-central/rev/2d1f22488076 https://hg.mozilla.org/mozilla-central/rev/d6723e7b7126 https://hg.mozilla.org/mozilla-central/rev/3e1458b66c24