Closed Bug 1248507 Opened 8 years ago Closed 8 years ago

[Decoder Doctor] Detect missing linux codecs, and notify frontend (to notify user)

Categories

(Core :: Audio/Video: Playback, defect, P2)

46 Branch
All
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Blocks 1 open bug)

Details

Attachments

(10 files, 9 obsolete files)

58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
jya
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
bzbarsky
: review+
Details
58 bytes, text/x-review-board-request
jya
: review+
Details
As part of the Decoder Doctor program, this bug will detect that FFmpeg/avcodec libraries are missing on Linux; And when they are needed for playback, an event will be sent to the Firefox front-end (so that the user may be notified about the issue -- there will be a separate bug to implement that side).
Priority: -- → P2
Boris, I'm hoping you can give some advice for an issue that makes implementation of this task a bit difficult.

The basic idea is to gather diagnostic information when first trying to play media files/streams (in this specific case checking if FFmpeg is missing when trying to play H.264 on Linux), and notify the user through web console messages, and later on a drop-down notification bar.

The issue I'm encountering is that in some situations we are getting initial diagnostics when the media element is not yet associated with a real document and/or window, and therefore any log/notification is lost. (Logs do appear in the browser console but we'd really want them to go to the web console.)

So the question is: What do you think is the best way to get our logs/notifications to the page when it is actually visible?
I'm guessing it could be a way to store notifications if needed and forward them when appropriate, either there is already such a facility, or we'd need to observe some notification? Or another way entirely?

Note: Because some web pages will try different media formats by calling a succession of JS methods (e.g. YouTube makes about twenty MediaSource.isTypeSupported calls), we will add a timer to allow for all these calls to complete before we present a synthesized log/notification. This timer in itself would probably help with the issue, because we'll be waiting before actually sending notifications. However this doesn't seem like the "right" solution, as I don't believe I can rely on a page to be presented within an arbitrary time (say 0.5s).
But let me know if you think it would actually work; it could be the basis for a polling solution (e.g., check every 0.5s whether the page is displayed, and if yes send pending notifications).

Please comment here or contact me on IRC if you need more information or would like to discuss. Thanks in advance.
Flags: needinfo?(bzbarsky)
> when the media element is not yet associated with a real document and/or window

All elements are always associated with a document.

I can believe the part about window, maybe, but I'd need to see exactly how you're doing your logging to say anything useful about that.  Normally a document that we'd actually load media for would also have a window.
Flags: needinfo?(bzbarsky) → needinfo?(gsquelart)
Thanks for your quick reply.
I most probably misspoke!
Whatever document&window are present at the time, dispatched logs do not appear in the web console.

For more context, here is a subset of a code path followed when opening an MP4 file from disk:
- nsDSURIContentListener::DoContent
  - nsDocShell::CreateContentViewer
    + nsDocShell::NewContentViewerObj
    | - nsContentDLF::CreateInstance
    |   - nsContentDLF::CreateDocument
    |     + nsCOMPtr<nsIDocument> = do_CreateInstance
    |     |   *** document created here
    |     - VideoDocument::StartDocumentLoad
    |       - VideoDocument::CreateSyntheticVideoDocument
    |         - HTMLMediaElement::LoadWithChannel
    |           - HTMLMediaElement::InitializeDecoderForChannel
    |             + DecoderTraits::CreateDecoder
    |             | - InstantiateDecoder
    |             |   - IsMP4SupportedType
    |             |       *** This is where diagnostics information is gathered
    |             - DecoderDoctorDiagnostics::AnalyzeAndNotifyUser
    |                 *** This is where the information is analyzed, and messages first logged:
    |               - LogToBrowserConsole
    |                 - nsCOMPtr<nsIConsoleService> console = do_GetService("@mozilla.org/consoleservice;1");
    |                 - console->LogStringMessage
    - nsDocShell::Embed
      - nsDocShell::SetupNewViewer
          *** Now (or maybe even later?) the document is plugged into the page?

If you'd like to see some actual code for my first attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06a88cfbd541

Sorry if I made incorrect assumptions, it's my first time diving this deep into the early page loading process.
I hope you can look past that (and correct me if you have the time), to help with what I'm trying to achieve, unless of course what I'm trying to do doesn't even make sense!

In any case it looks to me like the initial document that exists when first creating the media decoder, is not yet in its final publicly-visible location. So when trying to log something it doesn't go to the web console.
Flags: needinfo?(gsquelart) → needinfo?(bzbarsky)
Gerald, thanks.  The callstack there is very helpful, and the link to the actual patches.  The places you note as logging the information are in fact before the document has an (inner) window.  You're going to need an inner window to send things to the right console.  The good news is that this should only happen for the media document case.

What your probably want to do is add a method on nsIDocument that would allow queuing messages if nsIDocument::GetInnerWindow returns false.  You'd only want to actually queue on media documents, and only if they have never had SetScriptGlobalObject() called on them; otherwise you might end up queueing stuff after document teardown, which would have a good chance of leaking.  Then, when the SetScriptGlobalObject call happens and the window association is set up properly you'd flush out the queue and report whatever is queued up.

The other thing you will need to do is change your logging to not use LogToBrowserConsole.  That method, like it says, only logs things to the browser console.  Instead what you will want to do is put your strings into a properties file (so they can be localized properly), add that properties file to gPropertiesFiles in nsContentUtils.cpp and the corresponding enum in the header, and use nsContentUtils::ReportToConsole to report things.

I hope that helps, but please do ask if you run into trouble with that approach!
Flags: needinfo?(bzbarsky)
It sounds like the same issue as https://bugzilla.mozilla.org/show_bug.cgi?id=198301#c9. Can we have the same fix for the video document as we did for the image document?

Btw, it will also fix bug 608634 which I failed to fix due to lack of decent knowledge about page/document load.
(In reply to JW Wang [:jwwang] from comment #5)
> It sounds like the same issue as
> https://bugzilla.mozilla.org/show_bug.cgi?id=198301#c9. Can we have the same
> fix for the video document as we did for the image document?

13 years ago!?! :-O

> Btw, it will also fix bug 608634 which I failed to fix due to lack of decent
> knowledge about page/document load.

Thank you JW, I'll keep these in mind.
NI:bz, feel free to skip to the bottom of this comment for the main feedback request, if you don't have the time to go through the rest.

(In reply to Boris Zbarsky [:bz] from comment #4)
> What you probably want to do is add a method on nsIDocument that would
> allow queuing messages if nsIDocument::GetInnerWindow returns false.  You'd
> only want to actually queue on media documents, and only if they have never
> had SetScriptGlobalObject() called on them; otherwise you might end up
> queueing stuff after document teardown, which would have a good chance of
> leaking.  Then, when the SetScriptGlobalObject call happens and the window
> association is set up properly you'd flush out the queue and report whatever
> is queued up.

Thank you for the tip. I've instrumented all SetScriptGlobalObject() methods and when opening a media file it is indeed called on the VideoDocument (which calls it on the MediaDocument superclass, which in turn calls it on the nsDocument superclass), so it seems like a good place to work from. But...

> [...] You're going to need an inner
> window to send things to the right console.  The good news is that this
> should only happen for the media document case.

Unfortunately, in other cases like YouTube, js calls to MediaSource.IsTypeSupported() happen in a non-media document, and I only see calls to nsDocument::SetScriptGlobalObject(), i.e., not its subclasses.
(No stack trace for this case, it's harder for me to follow it.)

So I think I may need to work directly inside nsDocument. (Or not, more below...)


> The other thing you will need to do is change your logging to not use
> LogToBrowserConsole.  That method, like it says, only logs things to the
> browser console.  Instead what you will want to do is put your strings into
> a properties file (so they can be localized properly), add that properties
> file to gPropertiesFiles in nsContentUtils.cpp and the corresponding enum in
> the header, and use nsContentUtils::ReportToConsole to report things.

Yes, it was definitely planned to use localized strings, I was just using LogToBrowserConsole as a proof of concept for now. But of course that explained why I was never seeing anything in the browser console, duh!
Using ReportToConsole after SetScriptGlobalObject() works fine.


NI:bz, starting here for the main question:

Looking at nsDocument::SetScriptGlobalObject(), after mWindow is set, a couple of interesting actions happen:
- Queued CSP errors are flushed to the console -- Looks like I'm not the first one to have to deal with this!
- Some observers are notified about the potential visibility change.

This last one seems like the ideal point to latch onto; It is already used by some media code (e.g., to know when to actually start decoding, only when the page is actually visible).
I think it would make sense for the DecoderDoctor to perform its user-visible actions (console logging, and soon a drop-down notification bar) when the document becomes visible (if ever).
It also means I'd just have to call nsIDocument::RegisterActivityObserver and deal with callbacks, instead of modifying nsDocument itself, which was a bit scary to me!

So which option would you recommend?
A. Using the ActivityObserver mechanism (my personal preference), or
B. Implement separate handling closer to your initial suggestion in comment 4? (And probably similar to the CSP case)
C. Something else?
Flags: needinfo?(bzbarsky)
(In reply to JW Wang [:jwwang] from comment #5)
> It sounds like the same issue as
> https://bugzilla.mozilla.org/show_bug.cgi?id=198301#c9. Can we have the same
> fix for the video document as we did for the image document?
> 
> Btw, it will also fix bug 608634 which I failed to fix due to lack of decent
> knowledge about page/document load.

I had another look, and I don't think that solution would work in all cases here.
I.e.: It could work for video documents. But it won't help with the Youtube case, where I'm getting information from a script that's not directly related to any obviously-media element/document.

It might still be useful to port that fix from bug 198301 for video documents as you suggest, if only to fix bug 608634.
However I would argue that it'd better be handled separately, e.g. in bug 608634 directly!
> Unfortunately, in other cases like YouTube, js calls to MediaSource.IsTypeSupported() happen in a non-media document

Sure.  The media document case is only interesting because some stuff happens before there is a window, which is not typical.  So that stuff would need to queue up messages until such a time as there _is_ a window.  Other cases would just log immediately.

> I think it would make sense for the DecoderDoctor to perform its user-visible actions (console logging, and soon a drop-down
> notification bar) when the document becomes visible (if ever).

That seems fine in general, but...  if we ever take account of things like display:none iframes in the visibility state, would you really not want to do console messages for the display:none iframes?  That seems a bit odd.

> So which option would you recommend?

What are our actual goals here?  What feedback are we trying to provide to whom, why, and when?  What action do we expect them to take?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #9)
> > Unfortunately, in other cases like YouTube, js calls to MediaSource.IsTypeSupported() happen in a non-media document
> 
> Sure.  The media document case is only interesting because some stuff
> happens before there is a window, which is not typical.  So that stuff would
> need to queue up messages until such a time as there _is_ a window.  Other
> cases would just log immediately.

Ah yep, you're right -- of course!
Sorry for the confusion.

> > I think it would make sense for the DecoderDoctor to perform its user-visible actions (console logging, and soon a drop-down
> > notification bar) when the document becomes visible (if ever).
> 
> That seems fine in general, but...  if we ever take account of things like
> display:none iframes in the visibility state, would you really not want to
> do console messages for the display:none iframes?  That seems a bit odd.
> 
> > So which option would you recommend?
> 
> What are our actual goals here?  What feedback are we trying to provide to
> whom, why, and when?  What action do we expect them to take?

The goal is to inform the user that something on their system is preventing playback (when a page tries to play something), or not getting the best results.
For example, if the user goes to a web page that tries to play H.264 (whether a straight file or something like YouTube) but they don't have a decoder library we need, we want to display something like "The video on this page can't be played. Your system may not have the required video codecs." with a link to SUMO for further help.
More details there: https://docs.google.com/document/d/1BUHV-kQ3Kf9fC614CTjPxz_PaNerlNJerMQ8-t3e3lM

About display:none iframes, I suppose we should display messages, you're right.

So I'll go ahead with your original suggestion in comment 4. Thank you for your help so far, I'll contact you when I've got something to show...
Well, since whatever I said after comment 4 was crap, I revisited JW's suggestion from comment 5 yet again (which references related work by bz in the ImageDocument case), and early tests show it might actually work! And it would save us from hacking ns*Document's to artificially queue pre-window messages.
WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf95e36e2562
Pass declared-but-yet-undefined DecoderDoctorDiagnostics pointer to various
routines that contribute to deciding if a media format can be played, and
those that create decoders.

Points where a DecoderDoctorDiagnostics can be injected are currently marked
with "/* DecoderDoctorDiagnostics* */ nullptr", and some will be used in
following patches.
Attachment #8739951 - Flags: review?(cpearce)
Attachment #8739949 - Attachment is obsolete: true
Minimal DecoderDoctorDiagnostics interface and skeleton implementation.
Attachment #8739952 - Flags: review?(cpearce)
DecoderDoctorDiagnostics are now used at places where Firefox Chrome and/or
websites checks whether some media formats may be played:
- audio|video.canPlayType()
- audio|video resource loader
- MediaSource.IsTypeSupported()
- MediaSource.AddSourceBuffer()
Attachment #8739953 - Flags: review?(cpearce)
Minimal implementation of DecoderDoctorDiagnostics.

If the Decoder Doctor analysis needs to report something, a notification is
logged to the web console, with the media format(s) that cannot be decoded.

In this patch, there are only two notification types: "Cannot play" when no
decoders are found for any of the requested formats), or "Can play" (if pref
"media.decoderdoctor.verbose" is true) when decoders are missing for only
some of the requested formats.

MozReview-Commit-ID: 4QkiVvcNSU3
Attachment #8739954 - Flags: review?(cpearce)
Until bug 1261536 lands, it is possible for a synthetic video
document to be created and to start loading (triggering an analysis)
*before* that document is attached to a window.
So for now we're allowing analyses to be gathered for a document with no
window.
Best case: The window is attached shortly after, and when the synthesis
routine runs 1 second later, it will be able to send its notifications.
Worst case: The analyses will linger around for a could of seconds, but
nothing more will happen.
Attachment #8739955 - Flags: review?(cpearce)
If the Decoder Doctor analysis needs to report something, a notification
is sent to listeners of "decoder-doctor-notification", with data identifying
the type of notification along with the media format(s) that could not be
decoded.

In this patch, there are only two notification types: "cannot-play", or
"can-play-but-some-missing-decoders" (if pref "media.decoderdoctor.verbose" is
true).

In a future bug, the Firefox front-end will handle this notification and then
optionally display a user notification.

Note: "can-play-but-some-missing-decoders" should be useful to help implement
the front-end side (as sites like YouTube will probably have some formats we
don't handle); it may be removed later on if it has no further use.
Attachment #8739956 - Flags: review?(cpearce)
If the FFmpeg decoder module cannot be started, the failure is recorded in the
DecoderDoctorDiagnostics structure.
In this case, on Linux if there are no suitable decoders for any requested
format, a "platform decoder not found" notification is sent to Chrome (a
separate bug will implement the actual front-end notification), and logged to
the web console.
Attachment #8739957 - Flags: review?(cpearce)
Comment on attachment 8739951 [details] [diff] [review]
p1. Pass DecoderDoctorDiagnostics to PDMs&more -

Chris is too busy right now, passing reviews to Jean-Yves.
Note: I will update the 'r=' line in all patches before pushing.
Attachment #8739951 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739952 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739953 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739954 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739955 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739956 - Flags: review?(cpearce) → review?(jyavenard)
Attachment #8739957 - Flags: review?(cpearce) → review?(jyavenard)
Pass declared-but-yet-undefined DecoderDoctorDiagnostics pointer to various
routines that contribute to deciding if a media format can be played, and
those that create decoders.

Points where a DecoderDoctorDiagnostics can be injected are currently marked
with "/* DecoderDoctorDiagnostics* */ nullptr", and some will be used in
following patches.

Review commit: https://reviewboard.mozilla.org/r/45689/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45689/
Attachment #8740270 - Flags: review?(jyavenard)
Attachment #8740271 - Flags: review?(jyavenard)
Attachment #8740272 - Flags: review?(jyavenard)
Attachment #8740273 - Flags: review?(jyavenard)
Attachment #8740274 - Flags: review?(jyavenard)
Attachment #8740275 - Flags: review?(jyavenard)
Attachment #8740276 - Flags: review?(jyavenard)
DecoderDoctorDiagnostics are now used at places where Firefox Chrome and/or
websites checks whether some media formats may be played:
- audio|video.canPlayType()
- audio|video resource loader
- MediaSource.IsTypeSupported()
- MediaSource.AddSourceBuffer()

Review commit: https://reviewboard.mozilla.org/r/45693/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45693/
Minimal implementation of DecoderDoctorDiagnostics.

If the Decoder Doctor analysis needs to report something, a notification is
logged to the web console, with the media format(s) that cannot be decoded.

In this patch, there are only two notification types: "Cannot play" when no
decoders are found for any of the requested formats), or "Can play" (if pref
"media.decoderdoctor.verbose" is true) when decoders are missing for only
some of the requested formats.

Review commit: https://reviewboard.mozilla.org/r/45695/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45695/
Until bug 1261536 lands, it is possible for a synthetic video
document to be created and to start loading (triggering an analysis)
*before* that document is attached to a window.
So for now we're allowing analyses to be gathered for a document with no
window.
Best case: The window is attached shortly after, and when the synthesis
routine runs 1 second later, it will be able to send its notifications.
Worst case: The analyses will linger around for a could of seconds, but
nothing more will happen.

Review commit: https://reviewboard.mozilla.org/r/45697/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45697/
If the Decoder Doctor analysis needs to report something, a notification
is sent to listeners of "decoder-doctor-notification", with data identifying
the type of notification along with the media format(s) that could not be
decoded.

In this patch, there are only two notification types: "cannot-play", or
"can-play-but-some-missing-decoders" (if pref "media.decoderdoctor.verbose" is
true).

In a future bug, the Firefox front-end will handle this notification and then
optionally display a user notification.

the front-end side (as sites like YouTube will probably have some formats we
don't handle); it may be removed later on if it has no further use.

Review commit: https://reviewboard.mozilla.org/r/45699/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45699/
If the FFmpeg decoder module cannot be started, the failure is recorded in the
DecoderDoctorDiagnostics structure.
In this case, on Linux if there are no suitable decoders for any requested
format, a "platform decoder not found" notification is sent to Chrome (a
separate bug will implement the actual front-end notification), and logged to
the web console.

Review commit: https://reviewboard.mozilla.org/r/45701/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45701/
Attachment #8739951 - Attachment is obsolete: true
Attachment #8739951 - Flags: review?(jyavenard)
Attachment #8739952 - Attachment is obsolete: true
Attachment #8739952 - Flags: review?(jyavenard)
Attachment #8739953 - Attachment is obsolete: true
Attachment #8739953 - Flags: review?(jyavenard)
Attachment #8739954 - Attachment is obsolete: true
Attachment #8739954 - Flags: review?(jyavenard)
Attachment #8739955 - Attachment is obsolete: true
Attachment #8739955 - Flags: review?(jyavenard)
Attachment #8739956 - Attachment is obsolete: true
Attachment #8739956 - Flags: review?(jyavenard)
Attachment #8739957 - Attachment is obsolete: true
Attachment #8739957 - Flags: review?(jyavenard)
Attachment #8740270 - Flags: review?(jyavenard) → review+
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

https://reviewboard.mozilla.org/r/45689/#review42213

This is starting to look very messy and exacerbate and existing problem when it comes to passing arguments to either the DecoderTraits utility or creating a MediaDataDecoder.

We have two cases:
1- Passing the mimetype as used with canPlayType which is an extended mimetype for the container along side optional codecs.
2- Passing the mimetype to create the decoder. This mimetype is different to the one above in that it refers to the codec itself.

Now depending on the method, sometimes it take a char*, sometimes a nsACString&, something a nsCString&, sometimes an optional string.

And now we're adding one extra parameter.

It would be much better to create a new parameter type that could be extended in the future.
This parameter would allow to pass either type of mimetype, a pointer to the diagnostic structure etc.

It would be complete enough to handle all the current uses and could be universally shared between the media client (either the HTMLMediaElement or the MediaFormatReader) and the end MediaDataDecoder.

I understand that you probably don't want to do that now because all the remaining patches depend on it, so I'll r+ on the provisio that there will be a follow up bug to simplify all of this.

Thank you !

::: dom/media/platforms/PDMFactory.h:15
(Diff revision 1)
>  #include "PlatformDecoderModule.h"
>  
>  class CDMProxy;
>  
>  namespace mozilla {
> +  

whitespaces
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

https://reviewboard.mozilla.org/r/45691/#review42215

Assuming there's more to come
Attachment #8740271 - Flags: review?(jyavenard) → review+
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

https://reviewboard.mozilla.org/r/45693/#review42217

You should get someone who knows mow about things like: do_QueryInterface(aOwner.GetAsSupports());
I've had issues in the past with e10s calling this function.

LGTM otherwise, but probably want someone with more dom experience to review that.

::: dom/media/mediasource/MediaSource.cpp:239
(Diff revision 1)
> -                                         /* DecoderDoctorDiagnostics* */ nullptr);
> +  nsresult rv = mozilla::IsTypeSupported(aType, &diagnostics);
> +  if (NS_SUCCEEDED(rv)) {
> +    diagnostics.SetCanPlay();
> +  }
> +  if (GetOwner()) {
> +    diagnostics.AnalyzeAndNotifyUser(GetOwner()->GetExtantDoc(), aType, "AddSourceBuffer with GetOwner()->GetExtantDoc()");

80 columns formatting, looks too wide at a glance

::: dom/media/mediasource/MediaSource.cpp:240
(Diff revision 1)
> +  if (NS_SUCCEEDED(rv)) {
> +    diagnostics.SetCanPlay();
> +  }
> +  if (GetOwner()) {
> +    diagnostics.AnalyzeAndNotifyUser(GetOwner()->GetExtantDoc(), aType, "AddSourceBuffer with GetOwner()->GetExtantDoc()");
> +  } else {

if GetOwner() is null, then it would fail at the test below if (mReadyState != MediaSourceReadyState::Open) {

shouldn't you report that too ? (that the script was screwed and attempted to create a source buffer on a non attached media source element?

::: dom/media/mediasource/MediaSource.cpp:372
(Diff revision 1)
> +  if (doc) {
> +    diagnostics.AnalyzeAndNotifyUser(doc, aType, "IsTypeSupported with aOwner.GetAsSupports()<nsIDocument>");
> +  } else {
> +    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(aOwner.GetAsSupports());
> +    if (window) {
> +      diagnostics.AnalyzeAndNotifyUser(window->GetExtantDoc(), aType, "IsTypeSupported with aOwner.GetAsSupports()<nsPIDOMWindowInner>->GetExtantDoc()");

80 columns formatting
Attachment #8740272 - Flags: review?(jyavenard)
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

https://reviewboard.mozilla.org/r/45695/#review42221

::: dom/media/DecoderDoctorDiagnostics.cpp:24
(Diff revision 1)
>  #define DD_WARN(arg, ...) DD_LOG(mozilla::LogLevel::Warning, arg, ##__VA_ARGS__)
>  
>  namespace mozilla
>  {
>  
> +namespace decoderdoctor

do you must have a new namespace?

I thought this was to be limited to the bare minimum and only if we couldn't do without...

::: dom/media/DecoderDoctorDiagnostics.cpp:56
(Diff revision 1)
> +  void ReportAnalysis(const char* aReportStringId,
> +                      const nsAString& aFormats);
> +
> +  void SynthesizeAnalysis();
> +
> +  nsIDocument* mDocument;

What is the lifetime of mDocument.

Seeing that it's a raw pointer, how do you know it won't be destroyed underneath you.

Who owns the DocumentWatcher ?

::: dom/media/DecoderDoctorDiagnostics.cpp:98
(Diff revision 1)
> +                   const DecoderDoctorDiagnostics& aDiagnostics);
> +
> +  void DeregisterDocumentWatcher(nsIDocument* aDocument);
> +
> +private:
> +  std::map<nsIDocument*, RefPtr<DocumentWatcher>> mWatchers;

nsCOMPtr ??

::: dom/media/DecoderDoctorDiagnostics.cpp:130
(Diff revision 1)
> +DocumentWatcher::StopWatching()
> +{
> +  MOZ_ASSERT(!!mDocument);
> +
> +  // Keep 'this' alive in this method and the dispatched task.
> +  RefPtr<DocumentWatcher> self = this;

I believe nsI* derivative needs to go in nsCOMPtr (just like you do with mTimer)


You need a dom peer to review that.
Attachment #8740273 - Flags: review?(jyavenard)
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

https://reviewboard.mozilla.org/r/45697/#review42225
Attachment #8740274 - Flags: review?(jyavenard) → review+
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

https://reviewboard.mozilla.org/r/45701/#review42231

with fix addressed

::: dom/media/DecoderDoctorDiagnostics.cpp:239
(Diff revision 1)
>    nsAutoString formats;
>    for (auto& diag : mDiagnosticsSequence) {
>      if (diag.mDecoderDoctorDiagnostics.CanPlay()) {
>        canPlay = true;
>      } else {
> +#if defined(XP_LINUX)

FFmpeg isn't used just on linux.

Use #if defined(MOZ_FFMPEG)

you'll likely also want to handle ffvpx too which would follow a similar code path
Attachment #8740276 - Flags: review?(jyavenard) → review+
Comment on attachment 8740275 [details]
MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r=jya

https://reviewboard.mozilla.org/r/45699/#review42227

::: dom/media/DecoderDoctorDiagnostics.cpp:185
(Diff revision 1)
>      }
>    }
>  }
>  
> +static
>  void

static would look better on the same line
Attachment #8740275 - Flags: review?(jyavenard) → review+
https://reviewboard.mozilla.org/r/45693/#review42217

Thank you, I'll get someone else to double-check...

> if GetOwner() is null, then it would fail at the test below if (mReadyState != MediaSourceReadyState::Open) {
> 
> shouldn't you report that too ? (that the script was screwed and attempted to create a source buffer on a non attached media source element?

The nullptr-document case is handled in DecoderDoctorDiagnostics::AnalyzeAndNotifyUser().
Currently it just logs a warning to the output console, but in the future it could do more, like notify the user about the issue.
So I think we should keep this code as I propose for now; in the worst case it's one useless check.
https://reviewboard.mozilla.org/r/45695/#review42221

> do you must have a new namespace?
> 
> I thought this was to be limited to the bare minimum and only if we couldn't do without...

Indeed I see that the coding style prefers no namespaces (other than 'mozilla'), except when common names may clash.
I'm just worried that `DocumentWatcher` and `Service` are a bit generic, so I will rename them to avoid clashes.

> What is the lifetime of mDocument.
> 
> Seeing that it's a raw pointer, how do you know it won't be destroyed underneath you.
> 
> Who owns the DocumentWatcher ?

Added comments explaining mDocument lifetime (mainly: we get notified before the document is destroyed), and DocumentWatcher ownership.

> nsCOMPtr ??

It is only used to hold concrete DocumentWatcher's, there is no type-casting between XPCOM interfaces, so RefPtr is sufficient.

> I believe nsI* derivative needs to go in nsCOMPtr (just like you do with mTimer)
> 
> 
> You need a dom peer to review that.

It is only used to temporarily hold a concrete DocumentWatcher, there is no type-casting between XPCOM interfaces, so RefPtr is sufficient.
https://reviewboard.mozilla.org/r/45701/#review42231

> FFmpeg isn't used just on linux.
> 
> Use #if defined(MOZ_FFMPEG)
> 
> you'll likely also want to handle ffvpx too which would follow a similar code path

This bug intentionally only implements the FFMpeg/Linux scenario, as per https://docs.google.com/document/d/1BUHV-kQ3Kf9fC614CTjPxz_PaNerlNJerMQ8-t3e3lM/
But good point, thank you, I will follow up with Chris Peterson for other similar situations.
Boris, thank you again for your help so far.

No good deed goes unpunished, so once again I would like request your help, this time to review a few webidl's, notification dispatches, nsIDocument and nsWeakPtr things (mostly).
I'll push them here shortly, but please let me know if you're too busy for this now, and/or who else could help.
Flags: needinfo?(bzbarsky)
If the Decoder Doctor analysis needs to report something, a notification
is sent to listeners of "decoder-doctor-notification", with data identifying
the type of notification along with the media format(s) that could not be
decoded.

In this patch, there are only two notification types: "cannot-play", or
"can-play-but-some-missing-decoders" (if pref "media.decoderdoctor.verbose" is
true).

In a future bug, the Firefox front-end will handle this notification and then
optionally display a user notification.

the front-end side (as sites like YouTube will probably have some formats we
don't handle); it may be removed later on if it has no further use.

Review commit: https://reviewboard.mozilla.org/r/45907/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45907/
Attachment #8740272 - Attachment description: MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r?jya → MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r?jya,bz
Attachment #8740274 - Attachment description: MozReview Request: Bug 1248507 - p5. Disable window check until bug 1261536 is resolved - r?jya → MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r?bz
Attachment #8740273 - Attachment description: MozReview Request: Bug 1248507 - p4. DecoderDoctorDiagnostics implementation - r?jya → MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r?jya,bz
Attachment #8740275 - Attachment description: MozReview Request: Bug 1248507 - p6. Notify decoder-doctor-notification listeners - r?jya → MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r?jya
Attachment #8740276 - Attachment description: MozReview Request: Bug 1248507 - p7. Detect and report when FFMpeg/Linux fails to load - r?jya → MozReview Request: Bug 1248507 - p7. Minimal notification definition - r?bz
Attachment #8740686 - Flags: review?(jyavenard)
Attachment #8740686 - Flags: review?(bzbarsky)
Attachment #8740687 - Flags: review?(bzbarsky)
Attachment #8740688 - Flags: review?(bzbarsky)
Attachment #8740689 - Flags: review?(jyavenard)
Attachment #8740272 - Flags: review?(jyavenard)
Attachment #8740272 - Flags: review?(bzbarsky)
Attachment #8740274 - Flags: review?(bzbarsky)
Attachment #8740273 - Flags: review?(jyavenard)
Attachment #8740273 - Flags: review?(bzbarsky)
Attachment #8740276 - Flags: review?(bzbarsky)
If the FFmpeg decoder module cannot be started, the failure is recorded in the
DecoderDoctorDiagnostics structure.
In this case, on Linux if there are no suitable decoders for any requested
format, a "platform decoder not found" notification is sent to Chrome (a
separate bug will implement the actual front-end notification), and logged to
the web console.

Review commit: https://reviewboard.mozilla.org/r/45913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45913/
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/1-2/
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/1-2/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/1-2/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/1-2/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/1-2/
Comment on attachment 8740275 [details]
MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45699/diff/1-2/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/1-2/
Attachment #8740272 - Flags: review?(jyavenard) → review+
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

https://reviewboard.mozilla.org/r/45693/#review42491
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

https://reviewboard.mozilla.org/r/45695/#review42493

r=me with nit.

I would also add strong assertion that all methods run on the main threads.

::: dom/media/DecoderDoctorDiagnostics.cpp:331
(Diff revision 2)
> +    // No (more) inner window -> Don't watch. We shouldn't be watching anyway.
> +    MOZ_ASSERT(!mWatchers[aDocument]);
> +    return;
> +  }
> +
> +  if (!NS_IsMainThread()) {

The DecoderDoctorDiagnostic is only ever used on the main thread (all functions of HTMLMediaElement and MediaSource are only ever called on the main thread and strongly assert that they do).

So this code serve will never be called.

I would remove this and strongly assert that you are on the main thread.
Attachment #8740273 - Flags: review?(jyavenard) → review+
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

https://reviewboard.mozilla.org/r/45907/#review42497
Attachment #8740686 - Flags: review?(jyavenard) → review+
https://reviewboard.mozilla.org/r/45695/#review42493

I did that for all public methods. But I'll add more assertions, just to be sure and explicit.

> The DecoderDoctorDiagnostic is only ever used on the main thread (all functions of HTMLMediaElement and MediaSource are only ever called on the main thread and strongly assert that they do).
> 
> So this code serve will never be called.
> 
> I would remove this and strongly assert that you are on the main thread.

Agreed.
Attachment #8740689 - Flags: review?(jyavenard) → review+
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

https://reviewboard.mozilla.org/r/45913/#review42499

::: dom/media/DecoderDoctorDiagnostics.cpp:246
(Diff revision 1)
>  DecoderDoctorDocumentWatcher::SynthesizeAnalysis()
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
>    bool canPlay = false;
> +#if defined(XP_LINUX)

still think that should be MOZ_FFMPEG.

while it's enabled on linux, you have the *BSD that while not tier1 would benefit from it.

MOZ_FFMPEG will be defined on all those platforms, XP_LINUX is well, just linux

::: dom/media/DecoderDoctorDiagnostics.cpp:254
(Diff revision 1)
>    nsAutoString formats;
>    for (auto& diag : mDiagnosticsSequence) {
>      if (diag.mDecoderDoctorDiagnostics.CanPlay()) {
>        canPlay = true;
>      } else {
> +#if defined(XP_LINUX)

I can't see this code evolving nicely in time.. will quickly grow in a bit of a mess if you have methods for every single case that needs handling
(In reply to Jean-Yves Avenard [:jya] from comment #56)
> Comment on attachment 8740689 [details]
> MozReview Request: Bug 1248507 - p11. Detect and report when FFMpeg/Linux
> fails to load - r?jya
> 
> https://reviewboard.mozilla.org/r/45913/#review42499
> 
> > +#if defined(XP_LINUX)
> 
> still think that should be MOZ_FFMPEG.
> 
> while it's enabled on linux, you have the *BSD that while not tier1 would
> benefit from it.
> 
> MOZ_FFMPEG will be defined on all those platforms, XP_LINUX is well, just
> linux

As I wrote in comment 39, and as the bug title implies, this bug focuses on the Linux case.

But I'll defer to Chris to decide between XP_LINUX vs a wider MOZ_FFMPEG.


> I can't see this code evolving nicely in time.. will quickly grow in a bit
> of a mess if you have methods for every single case that needs handling

Yes, we currently need to handle specific cases, as they are tied to corresponding SUMO help pages. Of course if there are ways to combine them into more generic checks in the future, it would be nicer.
Flags: needinfo?(cpeterson)
(In reply to Gerald Squelart [:gerald] from comment #57)
> > MOZ_FFMPEG will be defined on all those platforms, XP_LINUX is well, just
> > linux
…
>
> But I'll defer to Chris to decide between XP_LINUX vs a wider MOZ_FFMPEG.

MOZ_FFMPEG makes sense. The name is more descriptive and platform independent. This code could work on BSD, for example. You'll probably need to change all the XP_LINUX checks in DecoderDoctorDiagnostics.cpp to MOZ_FFMPEG.
Flags: needinfo?(cpeterson)
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

https://reviewboard.mozilla.org/r/45693/#review42825

::: dom/media/mediasource/MediaSource.cpp:239
(Diff revision 2)
> -                                         /* DecoderDoctorDiagnostics* */ nullptr);
> +  nsresult rv = mozilla::IsTypeSupported(aType, &diagnostics);
> +  if (NS_SUCCEEDED(rv)) {
> +    diagnostics.SetCanPlay();
> +  }
> +  if (GetOwner()) {
> +    diagnostics.AnalyzeAndNotifyUser(GetOwner()->GetExtantDoc(), aType,

GetExtandDoc() can return null, but I guess the callee can handle that, given the else branch here....

::: dom/media/mediasource/MediaSource.cpp:368
(Diff revision 2)
> -  nsresult rv = mozilla::IsTypeSupported(aType,
> -                                         /* DecoderDoctorDiagnostics* */ nullptr);
> +  DecoderDoctorDiagnostics diagnostics;
> +  nsresult rv = mozilla::IsTypeSupported(aType, &diagnostics);
> +  if (NS_SUCCEEDED(rv)) {
> +    diagnostics.SetCanPlay();
> +  }
> +  nsCOMPtr<nsIDocument> doc = do_QueryInterface(aOwner.GetAsSupports());

This is always going to be null.  The nsISupports in a GlobalObject will be the nsISupports for the nsIGlobalObject involved.  That will never be a document.  Just take this whole branch out, please.

r=me
Attachment #8740272 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

https://reviewboard.mozilla.org/r/45697/#review42827

r=me, but this didn't really need a review from me, I suspect...
Attachment #8740274 - Flags: review?(bzbarsky) → review+
Attachment #8740273 - Flags: review?(bzbarsky)
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

https://reviewboard.mozilla.org/r/45695/#review42829

::: dom/media/DecoderDoctorDiagnostics.cpp:60
(Diff revision 2)
> +
> +  // Raw pointer to an nsIDocument.
> +  // Must be non-null during construction.
> +  // Nulled when we want to stop watching, because:
> +  // 1. As nsIDocumentActivity we have received a
> +  //    NotifyOwnerDocumentActivityChanged where the inner window was null,

What guarantees that such a notification is sent?  Certainly nothing in nsIDocumentActivity.h documents that such a thing will happen, and my quick skim of the callsites in nsDocument.cpp does not make it obvious that such a call will take place.

Please just use an nsWeakPtr here unless the uses of mDocument are performance sensitive.

I guess that still leaves you with the problem of how to remove yourself from mWatchers properly...

::: dom/media/DecoderDoctorDiagnostics.cpp:73
(Diff revision 2)
> +  struct Diagnostics
> +  {
> +    Diagnostics(const DecoderDoctorDiagnostics& aDiagnostics,
> +                const nsAString& aFormat,
> +                const char* aCallSite)
> +      : mDecoderDoctorDiagnostics(aDiagnostics)

This is going to copy the DecoderDoctorDiagnostics struct.  Is that OK?  So far it's just the one boolean, but is that expected to continue?

::: dom/media/DecoderDoctorDiagnostics.cpp:112
(Diff revision 2)
> +private:
> +  // Main owning references to watchers.
> +  // New watchers are created when a first analysis arrives for a document.
> +  // Watchers deregisters themselves later on, which will probably destroy them
> +  // when short-lived self-references die too.
> +  std::map<nsIDocument*, RefPtr<DecoderDoctorDocumentWatcher>> mWatchers;

What ensures this key pointer won't start pointing to a new document?

::: dom/media/DecoderDoctorDiagnostics.cpp:115
(Diff revision 2)
> +  // Watchers deregisters themselves later on, which will probably destroy them
> +  // when short-lived self-references die too.
> +  std::map<nsIDocument*, RefPtr<DecoderDoctorDocumentWatcher>> mWatchers;
> +};
> +
> +static DecoderDoctorService sDecoderDoctorService;

This adds a static constructor, right?  Expect the folks who are measuring startup time to complain...

::: dom/media/DecoderDoctorDiagnostics.cpp:124
(Diff revision 2)
> +                  nsIDocumentActivity, nsITimerCallback)
> +
> +DecoderDoctorDocumentWatcher::DecoderDoctorDocumentWatcher(nsIDocument* aDocument)
> +  : mDocument(aDocument)
> +{
> +  MOZ_ASSERT(!!aDocument);

Drop the !! bit.  Just MOZ_ASSERT(aDocument).

::: dom/media/DecoderDoctorDiagnostics.cpp:133
(Diff revision 2)
> +    NS_ISUPPORTS_CAST(nsIDocumentActivity*, this));
> +}
> +
> +DecoderDoctorDocumentWatcher::~DecoderDoctorDocumentWatcher()
> +{
> +  DD_LOG(mDocument ? mozilla::LogLevel::Warning : mozilla::LogLevel::Debug,

It's weird to have a null-check here given the following assert...  If we think !mDocument can be asserted, why are we bothering to check it?

::: dom/media/DecoderDoctorDiagnostics.cpp:143
(Diff revision 2)
> +}
> +
> +void
> +DecoderDoctorDocumentWatcher::StopWatching()
> +{
> +  MOZ_ASSERT(!!mDocument);

Again, drop the !! bit.  I'm going to stop commenting on this, but there are more instances in this diff.

::: dom/media/DecoderDoctorDiagnostics.cpp:153
(Diff revision 2)
> +  // Deregister from the Decoder Doctor service now, so we don't receive more
> +  // analysis data. (A new one might get started, that's okay.)
> +  sDecoderDoctorService.DeregisterDocumentWatcher(mDocument);
> +
> +  // Keep weak reference to the document, for the dispatched task.
> +  nsWeakPtr documentWeakPtr = do_GetWeakReference(mDocument);

So here you'd swap() mDocument into documentWeakPtr, if mDocument were an nsWeakPtr to start with.

::: dom/media/DecoderDoctorDiagnostics.cpp:288
(Diff revision 2)
> +  if (!mDocument) {
> +    return NS_OK;

This is assuming that the timer drops its ref to us, right?  Might be good to drop the ref to mTimer too.

::: dom/media/DecoderDoctorDiagnostics.cpp:292
(Diff revision 2)
> +  // Keep timer alive until we exit this method (because when it dies, this
> +  // DecoderDoctorDocumentWatcher may be destroyed with it).

Doesn't the timer keep itself alive while notifying?  I would be quite surprised if it does not, given that it passes itself as an argument!  Please double-check.

::: dom/media/DecoderDoctorDiagnostics.cpp:331
(Diff revision 2)
> +  if (!NS_IsMainThread()) {
> +    nsWeakPtr documentWeakPtr = do_GetWeakReference(aDocument);

Uh... no.  You can't do that.  You can't get a weak reference to a document off the main thread.  You _definitely_ can't call GetInnerWindow() off the main thread (something you did earlier in this function).

Of course you shouldn't have any AddAnalysis calls passing an nsIDocument off the main thread to start with, I would think, because no one should be holding nsIDocument pointers off the main thread!

Anyway, something here is fishy and this code is either nonsensical or totally unsafe.
Attachment #8740276 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

https://reviewboard.mozilla.org/r/45701/#review42831

r=me
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

https://reviewboard.mozilla.org/r/45907/#review42833

::: dom/media/DecoderDoctorDiagnostics.cpp:207
(Diff revision 1)
> +                     dom::DecoderDoctorNotificationType aNotificationType,
> +                     const nsAString& aFormats)
> +{
> +  dom::DecoderDoctorNotification data;
> +  data.mType = aNotificationType;
> +  data.mFormats.Construct(aFormats);

If you always construct it anyway, why isn't it required in the dictionary?

::: dom/media/DecoderDoctorDiagnostics.cpp:209
(Diff revision 1)
> +{
> +  dom::DecoderDoctorNotification data;
> +  data.mType = aNotificationType;
> +  data.mFormats.Construct(aFormats);
> +  nsAutoString json;
> +  data.ToJSON(json);

ToJSON can fail (e.g. on OOM).  This at least deserves a comment about why notifying with an empty string is better than not notifying at all if that happens.

::: dom/media/DecoderDoctorDiagnostics.cpp:237
(Diff revision 1)
>                                    aReportStringId,
>                                    params,
>                                    ArrayLength(params));
> +
> +  DispatchNotification(
> +    mDocument->GetInnerWindow(), aNotificationType, aFormats);

Is there a reason we're using the window, not the document, as the notification subject?

And in particular, are we 100% sure that we can't end up sending these sorts of notifications for documents that are no longer the current ones in their browsing context?
Attachment #8740686 - Flags: review?(bzbarsky)
Flags: needinfo?(bzbarsky)
Attachment #8740687 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

https://reviewboard.mozilla.org/r/45909/#review42835

r=me
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

https://reviewboard.mozilla.org/r/45911/#review42837

r=me
Attachment #8740688 - Flags: review?(bzbarsky) → review+
Oh, and in the future, no need to needinfo me just to send me mail telling me there is more mail with review requests coming.  ;)
https://reviewboard.mozilla.org/r/45695/#review42829

> This is going to copy the DecoderDoctorDiagnostics struct.  Is that OK?  So far it's just the one boolean, but is that expected to continue?

I was OK with that (I don't expect this struct to grow that much). But the way it's uses, it's just as easy to Move() it, so I've done just that.

> What ensures this key pointer won't start pointing to a new document?

Good catch. Worst case wouldn't have been that bad, but better get it right.
Fixed by checking that the watcher found in the map actually has its weak-pointed-at document still alive.

> This adds a static constructor, right?  Expect the folks who are measuring startup time to complain...

Implemented on-demand service, that kills itself when there are no more watchers.

> It's weird to have a null-check here given the following assert...  If we think !mDocument can be asserted, why are we bothering to check it?

This was just to output a warning in case of a null document, but it's true that it is duplicating the assert -- removed.

> So here you'd swap() mDocument into documentWeakPtr, if mDocument were an nsWeakPtr to start with.

Yep, makes sense, thank you.

> This is assuming that the timer drops its ref to us, right?  Might be good to drop the ref to mTimer too.

Right. Nulling mTimer first now.

> Doesn't the timer keep itself alive while notifying?  I would be quite surprised if it does not, given that it passes itself as an argument!  Please double-check.

Yes, I've checked and it does the right think (keeping itself and the task alive while running). Removed unnecessary 'kungfudeathgrip'.

> Uh... no.  You can't do that.  You can't get a weak reference to a document off the main thread.  You _definitely_ can't call GetInnerWindow() off the main thread (something you did earlier in this function).
> 
> Of course you shouldn't have any AddAnalysis calls passing an nsIDocument off the main thread to start with, I would think, because no one should be holding nsIDocument pointers off the main thread!
> 
> Anyway, something here is fishy and this code is either nonsensical or totally unsafe.

jya already mentionned that everything should happen on the main thread anyway, so I've removed this dispatch, and added asserts. No more cross-thread weak pointers.
https://reviewboard.mozilla.org/r/45907/#review42833

> If you always construct it anyway, why isn't it required in the dictionary?

Currently all notification types do carry formats, but in the future I can envisage some notifications that may not.
To make it clearer and be ready for the future, I've made this construction contigent on having a non-empty aFormats string.

> ToJSON can fail (e.g. on OOM).  This at least deserves a comment about why notifying with an empty string is better than not notifying at all if that happens.

In fact I should have checked the return value, so I don't dispatch an empty string.

> Is there a reason we're using the window, not the document, as the notification subject?
> 
> And in particular, are we 100% sure that we can't end up sending these sorts of notifications for documents that are no longer the current ones in their browsing context?

I assumed the document's window would be the natural candidate, but if the document itself makes more sense, I'll make the change.
I admit I don't know enough to properly answer the second question! I'll contact you/someone offline...
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/2-3/
Attachment #8740270 - Attachment description: MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r?jya → MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya
Attachment #8740271 - Attachment description: MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r?jya → MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya
Attachment #8740272 - Attachment description: MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r?jya,bz → MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz
Attachment #8740274 - Attachment description: MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r?bz → MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz
Attachment #8740275 - Attachment description: MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r?jya → MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r=jya
Attachment #8740276 - Attachment description: MozReview Request: Bug 1248507 - p7. Minimal notification definition - r?bz → MozReview Request: Bug 1248507 - p7. Minimal notification definition - r=bz
Attachment #8740687 - Attachment description: MozReview Request: Bug 1248507 - p9. FFMpeg checks: Console message - r?bz → MozReview Request: Bug 1248507 - p9. FFMpeg checks: Console message - r=bz
Attachment #8740688 - Attachment description: MozReview Request: Bug 1248507 - p10. FFMpeg checks: Notification definition - r?bz → MozReview Request: Bug 1248507 - p10. FFMpeg checks: Notification definition - r=bz
Attachment #8740689 - Attachment description: MozReview Request: Bug 1248507 - p11. Detect and report when FFMpeg/Linux fails to load - r?jya → MozReview Request: Bug 1248507 - p11. Detect and report when FFMpeg/Linux fails to load - r=jya
Attachment #8740273 - Flags: review?(bzbarsky)
Attachment #8740686 - Flags: review?(bzbarsky)
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/2-3/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/2-3/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/2-3/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/2-3/
Comment on attachment 8740275 [details]
MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45699/diff/2-3/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/2-3/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/1-2/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/1-2/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/1-2/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/1-2/
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/3-4/
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/3-4/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/3-4/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/3-4/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/3-4/
Comment on attachment 8740275 [details]
MozReview Request: Bug 1248507 - p6. Disable window check until bug 1261536 is resolved - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45699/diff/3-4/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/3-4/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/2-3/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/2-3/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/2-3/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/2-3/
Attachment #8740273 - Flags: review?(bzbarsky)
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

https://reviewboard.mozilla.org/r/45695/#review43063

::: dom/media/DecoderDoctorDiagnostics.cpp:73
(Diff revisions 2 - 4)
>    //    which indicates that the document has been detached from its window
>    //    and will (probably) get destroyed soon.
> -  //    Note that we expect the document to stay alive at least until then.
>    // 2. We have not received new diagnostic information within a short time
>    //    period, so we just stop watching.
> -  nsIDocument* mDocument;
> +  nsWeakPtr mDocumentWeakPtr;

So here's what I don't quite understand.  If the document can die before we've removed ourselves from the hashtable... how do we go about removing ourselves from the hashtable?  Do we basically leak?

If that _can't_ happen, I would like to understand why not.

::: dom/media/DecoderDoctorDiagnostics.cpp:172
(Diff revisions 2 - 4)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +
> +  nsCOMPtr<nsIDocument> document;
> +  if (mDocumentWeakPtr) {
> +    document = do_QueryReferent(mDocumentWeakPtr);

do_QueryReferent is null-safe, so no need to null-check mDocumentWeakPtr.

::: dom/media/DecoderDoctorDiagnostics.cpp:194
(Diff revisions 2 - 4)
>    RefPtr<DecoderDoctorDocumentWatcher> self = this;
>  
>    // Deregister from the Decoder Doctor service now, so we don't receive more
>    // analysis data. (A new one might get started, that's okay.)
> -  sDecoderDoctorService.DeregisterDocumentWatcher(mDocument);
> +  DecoderDoctorService::GetService().DeregisterDocumentWatcher(
> +    reinterpret_cast<nsIDocument*>(mDocumentOriginalPtr));

Er, no, mDocumentWeakPtr might not mean the thing it used to mean anymore, if the document is dead, right?

In particular, what ensures there isn't a _new_ value in the map for that key?

So thinking about this some more, why is maintaining this side table the right way to go?  Why can't we just store the object we care about on the document (via nsINode::SetProperty, which will ensure the property destructor is in fact called if the document dies, in case we care about that)?
Attachment #8740686 - Flags: review?(bzbarsky)
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

https://reviewboard.mozilla.org/r/45907/#review43067

::: dom/media/DecoderDoctorDiagnostics.cpp:298
(Diff revision 3)
>                                    nsContentUtils::eDOM_PROPERTIES,
>                                    aReportStringId,
>                                    params,
>                                    ArrayLength(params));
> +
> +  DispatchNotification(document, aNotificationType, aFormats);

That's an odd thing to pass for an aWindow argument... ;)

On a more serious note, whether to use document or window here depends on the consumers and what they plan to do with the value.  What _do_ they plan to do with the value?
https://reviewboard.mozilla.org/r/45695/#review43063

> So here's what I don't quite understand.  If the document can die before we've removed ourselves from the hashtable... how do we go about removing ourselves from the hashtable?  Do we basically leak?
> 
> If that _can't_ happen, I would like to understand why not.

Either we'll get a nsIDocumentActivity::NotifyOwnerDocumentActivityChanged in which we see that the document's inner window is null, or the timer will lapse and we'll see that the document is now dead.
Note that there is always a timer running while a watcher is alive: It is started on the first AddAnalysis, and continually restarted if another AddAnalysis arrived in the meantime, and finally if nothing happened the watcher will be killed.
I was hoping the comments just above that line would be sufficient. Would you like more details there?
(But this may become moot based on your last comment below.)

> do_QueryReferent is null-safe, so no need to null-check mDocumentWeakPtr.

Actually I do want that test there, because I do something different depending on the weak pointer value itself. Basically there are 3 states following this sequence:
1. The weak pointer is not null, and the referent is not null -> The document is alive.
2. The weak pointer is not null, but the referent is null -> The document has just died, and we want to do some special work, including nulling the weak pointer itself!
3. The weak pointer is already null -> The document had already died and we knew that, we just don't want to do anything more.

> Er, no, mDocumentWeakPtr might not mean the thing it used to mean anymore, if the document is dead, right?
> 
> In particular, what ensures there isn't a _new_ value in the map for that key?

In this case I don't actually dereference that pointer (notice it's 'mDocumentOriginalPtr', not the weak pointer); this original pointer is only used as the key into the Service map in order to remove the entry.
If you look at DecoderDoctorService::AddAnalysis, it specifically handles this case where we receive something for a new document at the same address: The old watcher will be removed first, before we create&add a new watcher for the more recent document.

Ah yes, if there is a way to make a document own the DecoderDoctorDocumentWatcher, it would make life much simpler!
I will look into this now...
https://reviewboard.mozilla.org/r/45695/#review43063

> Either we'll get a nsIDocumentActivity::NotifyOwnerDocumentActivityChanged in which we see that the document's inner window is null, or the timer will lapse and we'll see that the document is now dead.
> Note that there is always a timer running while a watcher is alive: It is started on the first AddAnalysis, and continually restarted if another AddAnalysis arrived in the meantime, and finally if nothing happened the watcher will be killed.
> I was hoping the comments just above that line would be sufficient. Would you like more details there?
> (But this may become moot based on your last comment below.)

> or the timer will lapse and we'll see that the document is now dead.

Yes, but in that case how do we remove ourselves from the std::map?  We no longer have the key we used to add ourselves, in a useful way (in that that key may now map to another value).

> In this case I don't actually dereference that pointer (notice it's 'mDocumentOriginalPtr', not the weak pointer); this original pointer is only used as the key into the Service map in order to remove the entry.
> If you look at DecoderDoctorService::AddAnalysis, it specifically handles this case where we receive something for a new document at the same address: The old watcher will be removed first, before we create&add a new watcher for the more recent document.

Ah, I see.  This could all use some comments describing the lifetime management, but OK, I agree that this works.

Either way.  I'm now convinced that what you have works; just needs documentation somewhere describing how the various parts interact.
https://reviewboard.mozilla.org/r/45695/#review43063

> > or the timer will lapse and we'll see that the document is now dead.
> 
> Yes, but in that case how do we remove ourselves from the std::map?  We no longer have the key we used to add ourselves, in a useful way (in that that key may now map to another value).

We do have the key in 'mDocumentOriginalPtr'. (But I think you figured that out below.)

I will first look into nsINode::SetProperty, which would be much cleaner I think.
Otherwise I'll document this convoluted lifecycle better.
Jared: This is about the notification that my code sends to your code, to know whether I should target the window (as per my original code), or the document?

(Boris Zbarsky [:bz] from comment #63)
> > +  DispatchNotification(
> > +    mDocument->GetInnerWindow(), aNotificationType, aFormats);
> 
> Is there a reason we're using the window, not the document, as the
> notification subject?
> 
> And in particular, are we 100% sure that we can't end up sending these sorts
> of notifications for documents that are no longer the current ones in their
> browsing context?

I blindly changed the target to be the document, but then:

(Boris Zbarsky [:bz] from comment #92)
> > +  DispatchNotification(document, aNotificationType, aFormats);
> 
> [...] whether to use document or window here depends on
> the consumers and what they plan to do with the value.  What _do_ they plan
> to do with the value?

Jared, I think you're "the consumers / they" in this last comment!
So, what do you think?
Window or document, or even something else?
Suggestions about Boris' point regarding non-current documents?
Flags: needinfo?(jaws)
We are only concerned with the top window. If the document is sent, we can use document.defaultView to get the document's window, and then window.top to get the top window. So it doesn't make much difference either way. Sending the document is fine.
Flags: needinfo?(jaws)
If you're going to try to get a window at any point, we might as well send the window to start with.

I hope you're just using that window to decide which console to show things in or whatnot, and not for anything like security checks or anything that depends on origins.
I should note that for consistency sake, we should go with 'window'. The "mediakeys-request" uses window, http://mxr.mozilla.org/mozilla-central/source/dom/media/eme/MediaKeySystemAccess.cpp#592

We don't do any security checks with this. It's just used to decide which browser should get the notification bar.
https://reviewboard.mozilla.org/r/45907/#review43067

> That's an odd thing to pass for an aWindow argument... ;)
> 
> On a more serious note, whether to use document or window here depends on the consumers and what they plan to do with the value.  What _do_ they plan to do with the value?

Back to window, after feedback from :jaws.
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/4-5/
Attachment #8740276 - Attachment description: MozReview Request: Bug 1248507 - p7. Minimal notification definition - r=bz → MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz
Attachment #8740686 - Attachment description: MozReview Request: Bug 1248507 - p8. Notify decoder-doctor-notification listeners - r?jya,bz → MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r?jya,bz
Attachment #8740687 - Attachment description: MozReview Request: Bug 1248507 - p9. FFMpeg checks: Console message - r=bz → MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz
Attachment #8740688 - Attachment description: MozReview Request: Bug 1248507 - p10. FFMpeg checks: Notification definition - r=bz → MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz
Attachment #8740689 - Attachment description: MozReview Request: Bug 1248507 - p11. Detect and report when FFMpeg/Linux fails to load - r=jya → MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya
Attachment #8740273 - Flags: review?(bzbarsky)
Attachment #8740686 - Flags: review?(bzbarsky)
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/4-5/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/4-5/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/4-5/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/4-5/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/4-5/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/3-4/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/3-4/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/3-4/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/3-4/
Attachment #8740275 - Attachment is obsolete: true
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/5-6/
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/5-6/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/5-6/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/5-6/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/5-6/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/5-6/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/4-5/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/4-5/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/4-5/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/4-5/
Rebasing (especially to include Widevine work in following patches), and fixed a small correctness issue in p3:
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/6-7/
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/6-7/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/6-7/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/6-7/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/6-7/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/6-7/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/5-6/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/5-6/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/5-6/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/5-6/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

https://reviewboard.mozilla.org/r/45695/#review43865

::: dom/media/DecoderDoctorDiagnostics.cpp:113
(Diff revisions 4 - 7)
> -
> -// Class that keeps track of all DecoderDoctorDocumentWatcher's.
> -class DecoderDoctorService
> +// Object that will be attached to a document as "decoder-doctor" property.
> +// Its purpose is to:
> +// - Associate a DecoderDoctorDocumentWatcher with a document,
> +// - Notify the DecoderDoctorDocumentWatcher when the document is being
> +//   destroyed.
> +class DecoderDoctorDocumentProperty

Why is this extra class needed?  Why can't we just store the pointer to DecoderDoctorDocumentWatcher (addrefed) in the document property, and have the destroy callback NS_RELEASE it?

::: dom/media/DecoderDoctorDiagnostics.cpp:215
(Diff revisions 4 - 7)
> +  DD_DEBUG("DecoderDoctorDocumentProperty[%p, watcher=%p]::DestroyProperty()\n",
> +           property, property->mWatcher.get());
> +  // StopWatching will call RemovePropertyFromDocument, so we need to flag that
> +  // we are being destroyed to avoid an extra UnsetProperty&delete.
> +  property->mBeingDestroyed = true;
> +  property->mWatcher->StopWatching();

So here we could have a watcher->StopWatching() with an argument to tell it not to try to remove itself from the document (hence no need for the mBeingDestroyed bit), and then NS_RELEASE(watcher).

::: dom/media/DecoderDoctorDiagnostics.cpp:247
(Diff revisions 4 - 7)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    // StopWatching() shouldn't be called twice.
> -  MOZ_ASSERT(mDocumentWeakPtr);
> +  MOZ_ASSERT(mDocument);
> +
> +  RefPtr<DecoderDoctorDocumentWatcher> kungfudeathgrip = this;

Who's calling StopWatching without holding a strong ref?

If we do need the extra class, please add documentation explaining why, and document why the kungfudeathgrip is needed.  r=me with that done, or those bits removed.
Attachment #8740273 - Flags: review?(bzbarsky) → review+
Attachment #8740686 - Flags: review?(bzbarsky) → review+
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

https://reviewboard.mozilla.org/r/45907/#review43867

r=me
(In reply to Boris Zbarsky [:bz] from comment #133)
> Why is this extra class needed?  Why can't we just store the pointer to
> DecoderDoctorDocumentWatcher (addrefed) in the document property, and have
> the destroy callback NS_RELEASE it?

Because I was intent on keeping things artificially complicated!

Thank you for this last tip, I'll implement it, test, and land...
Comment on attachment 8740270 [details]
MozReview Request: Bug 1248507 - p1. Pass DecoderDoctorDiagnostics to PDMs&more - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45689/diff/7-8/
Attachment #8740273 - Attachment description: MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r?jya,bz → MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz
Attachment #8740686 - Attachment description: MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r?jya,bz → MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz
Comment on attachment 8740271 [details]
MozReview Request: Bug 1248507 - p2. DecoderDoctorDiagnostics boilerplate - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45691/diff/7-8/
Comment on attachment 8740272 [details]
MozReview Request: Bug 1248507 - p3. Use DecoderDoctorDiagnostics - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45693/diff/7-8/
Comment on attachment 8740274 [details]
MozReview Request: Bug 1248507 - p4. DecoderDoctor base console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45697/diff/7-8/
Comment on attachment 8740273 [details]
MozReview Request: Bug 1248507 - p5. DecoderDoctorDiagnostics implementation - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45695/diff/7-8/
Comment on attachment 8740276 [details]
MozReview Request: Bug 1248507 - p6. Minimal notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45701/diff/7-8/
Comment on attachment 8740686 [details]
MozReview Request: Bug 1248507 - p7. Notify decoder-doctor-notification listeners - r=jya,bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45907/diff/6-7/
Comment on attachment 8740687 [details]
MozReview Request: Bug 1248507 - p8. FFMpeg checks: Console message - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45909/diff/6-7/
Comment on attachment 8740688 [details]
MozReview Request: Bug 1248507 - p9. FFMpeg checks: Notification definition - r=bz

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45911/diff/6-7/
Comment on attachment 8740689 [details]
MozReview Request: Bug 1248507 - p10. Detect and report when FFMpeg/Linux fails to load - r=jya

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45913/diff/6-7/
One thing I apparently missed in the reviews above:

>+RefPtr<DecoderDoctorDocumentWatcher>
>+DecoderDoctorDocumentWatcher::RetrieveOrCreate(nsIDocument* aDocument)

The normal way to do this is to have the return type be already_AddRefed<DecoderDoctorDocumentWatcher> and in the method do:

  return watcher.forget();

when doing the return from a RefPtr.
(In reply to Boris Zbarsky [:bz] from comment #147)
> One thing I apparently missed in the reviews above:

I think I added that as a fix to your last r+ review, that's why you didn't see it.
(I probably thought it was fine&simpler-looking in that case, as I controlled the only use of it lower in that file, and it was doing a RefPtr assignement, which I assumed would trigger an RVO).

> >+RefPtr<DecoderDoctorDocumentWatcher>
> >+DecoderDoctorDocumentWatcher::RetrieveOrCreate(nsIDocument* aDocument)
> 
> The normal way to do this is to have the return type be
> already_AddRefed<DecoderDoctorDocumentWatcher> and in the method do:
>   return watcher.forget();
> when doing the return from a RefPtr.

Better use the normal way, if only for consistency, so I'll add that right now in follow-up bug 848994.
Thank you for checking.
> I think I added that as a fix to your last r+ review, that's why you didn't see it.

Nah, I checked and it was present in https://reviewboard.mozilla.org/r/45695/diff/7/ which is the last thing I reviewed.  So it's totally my fault.  ;)
You need to log in before you can comment on or make changes to this bug.