Steps to reproduce: * Set Flash to click-to-play * Navigate to newshub.co.nz (or other sites that use the brightcove player) * Click on a video Expected results: The browser should drop down the click to play prompt. Actual results: Click to play is only shown as an icon. It shows the message "This video is either unavailable or not supported in this browser".

Anthony, this works for various sites (like bloomberg), but not for newshub.co.nz, which doesn't send a decode doctor notification. Any idea why there's no notification to key off of there?

A better solution for us users of Firefox would be to add support for HLS as in bug577084.

Gerald - can you shed some light on this?

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #2) > [T]his works for various sites (like bloomberg), but not for > newshub.co.nz, which doesn't send a decode doctor notification. Any idea why > there's no notification to key off of there? The Decoder Doctor may not be sending the notification type you are expecting! I see in your patch that you are allowing 'MediaCannotPlayNoDecoders' through. This decoderDoctorReportId means that within a bunch of CanPlayType calls (within 1 second of each other), *none* of the requested types can be played. Looking at a newshub.co.nz video page (with MOZ_LOG=DecoderDoctor:5), it tries: - 'application/x-mpegURL' -> cannot play - 'application/vnd.apple.mpegURL' -> cannot play - 'video/mp4; codecs="avc1.42E01E, mp4a.40.2"' -> can play Based on that, DecDoc will dispatch a notification with aData.decoderDoctorReportId="MediaNoDecoders", meaning only some (not all) of the requested formats cannot be played. So I would suggest you also enable 'MediaNoDecoders'. But you may need to be a bit cleverer in your handler. E.g.: Look at aData.formats to verify that it contains something related to the plugin (like "application/x-mpegURL"?), so that you don't handle notifications that may spur from other media in the page.

That works for some videos, but I'm not seeing any decode doctor notifications for this one: http://www.newshub.co.nz/nznews/second-stripper-speaks-out-against-chiefs-2016080516#axzz4GUc7RHnU

Your link worked for me -- of course! As a proof, I added a 'dump(aData)' in your handler after 'if (this.haveShownNotification [...]) {', and I got two hits: > {"decoderDoctorReportId":"MediaNoDecoders", > "formats":"application/x-mpegURL, application/vnd.apple.mpegURL","isSolved":false, > "type":"can-play-but-some-missing-decoders"} and > {"decoderDoctorReportId":"MediaCannotPlayNoDecoders","formats":"application/x-mpegURL","isSolved":false,"type":"cannot-play"} Could you please add the 'dump(aData)' and then try again with MOZ_LOG=DecoderDoctor:5. When you encounter the problem, send me the log or post it here, thank you.

we seem to have a Heisenbug here; it works with the logging enabled, doesn't work without it.

here's a screencast to show I'm not crazy

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #9) > I'm not crazy You are not crazy! I finally found what the difference was between us: "media.decoder-doctor.verbose" was 'true' in my profile. It is false by default (probably your case), which blocks 'MediaNoDecoders', because I thought this would produce too many console logs for perfectly-normal situations. So now we would want to dispatch the notification, but still block the console message. For that, in DecoderDoctorDiagnostics.cpp you'll need to: - remove the 'if (Preferences::GetBool("media.decoder-doctor.verbose", false))' test from line 584, so we always call ReportAnalysis(). - Change the test in ReportAnalysis at line 327 to something like: 'if (!aIsSolved && (&aNotification != &sMediaNoDecoders || Preferences::GetBool("media.decoder-doctor.verbose", false)))', this should block the console message unless verbose is set. Good luck!

Comment on attachment 8779182 [details] [diff] [review] hls_notificaiton.patch Review of attachment 8779182 [details] [diff] [review]: ----------------------------------------------------------------- I'm inclined to land this and file a follow up for the media team to sort out how to get better decode doctor notifications.

Comment on attachment 8779182 [details] [diff] [review] hls_notificaiton.patch Review of attachment 8779182 [details] [diff] [review]: ----------------------------------------------------------------- Hey blassey, I'm not 100% certain this is the right approach. I'm not too familiar with Decoder Doctor, but from a quick poke around, it looks like most of its notifications are handled in browser-media.js (http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/browser/base/content/browser-media.js). I suspect we should probably put all of the handling in the same place. Or is there a reason to put this handling into PluginContent.jsm that I'm not understanding? ::: browser/modules/PluginContent.jsm @@ +131,5 @@ > + observe: function observe(aSubject, aTopic, aData) { > + switch (aTopic) { > + case "decoder-doctor-notification": > + let data = JSON.parse(aData); > + if (this.haveShownNotification && Out of curiosity, why is it necessary that we have shown the notification before doing this? @@ +801,5 @@ > // If there are no instances of the plugin on the page any more, what the > // user probably needs is for us to allow and then refresh or if we have > // a placeholder, the page needs to be refreshed to insert its plugins > + if (newState != "block" && > + (!pluginFound || placeHolderFound || contentWindow.pluginRequiresReload)) { This placeHolderFound variable is being added in bug 1289802, which hasn't landed yet. This should be rebased on central. @@ +863,5 @@ > > this.pluginData.set(pluginInfo.permissionString, pluginInfo); > } > > + this.haveShownNotification = true; This state is going to exist in between page loads in the same tab. If this state is necessary only for a particular page, we need to clear it when the page unloads.

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #12) > Comment on attachment 8779182 [details] [diff] [review] > ::: browser/modules/PluginContent.jsm > @@ +131,5 @@ > > + observe: function observe(aSubject, aTopic, aData) { > > + switch (aTopic) { > > + case "decoder-doctor-notification": > > + let data = JSON.parse(aData); > > + if (this.haveShownNotification && > > Out of curiosity, why is it necessary that we have shown the notification > before doing this? This is only relevant if the site has tested for flash and fallen back to HLS. If they just got for HLS strait off and there's no plugin content then there's no reason to prompt the user.

Comment on attachment 8789802 [details] [diff] [review] hls_notificaiton.patch Review of attachment 8789802 [details] [diff] [review]: ----------------------------------------------------------------- Thanks blassey. Can you also provide a test for this? ::: browser/modules/PluginContent.jsm @@ +139,5 @@ > + let principal = this.content.document.nodePrincipal; > + let location = this.content.document.location.href; > + this.global.content.pluginRequiresReload = true; > + this.global.sendAsyncMessage("PluginContent:ShowClickToPlayNotification", { > + plugins: [... this.pluginData.values()], Nit: Busted indentation from lines 143-146 @@ +800,5 @@ > // If there are no instances of the plugin on the page any more, what the > // user probably needs is for us to allow and then refresh or if we have > // a placeholder, the page needs to be refreshed to insert its plugins > + if (newState != "block" && > + (!pluginFound || placeHolderFound || contentWindow.pluginRequiresReload)) { Can you please add a comment about where pluginRequiresReload comes from?

Pushed by blassey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0df9c1afedc2 Make Flash click to play icon into a drop down when HLS is used r=mconley

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d065b725b7f Backed out changeset 0df9c1afedc2

sorry had to back this out because this conflicted with another backout in bug 1263665

Pushed by blassey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad3ce51b1324 Make Flash click to play icon into a drop down when HLS is used r=mconley

Please pref off or back out this feature due to serious regression bug 1308677.

Patch in bug 1308677 to address it.

I have a second lead cooking in bug 1308677 that should help address this regression.

This feature seems to be enabled in 52, per status flag. Brad, could you please confirm?

held to nightly