Closed Bug 1292317 Opened 4 years ago Closed 4 years ago

Make Flash click to play icon into a drop down when HLS is used

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: ajones, Assigned: blassey)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

Attachments

(2 files, 2 obsolete files)

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".
Attached patch hls_notificaiton.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
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?
Flags: needinfo?(ajones)
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?
Flags: needinfo?(ajones) → needinfo?(gsquelart)
(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.
Flags: needinfo?(gsquelart)
Attached patch hls_notificaiton.patch (obsolete) — Splinter Review
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
Attachment #8778499 - Attachment is obsolete: true
Flags: needinfo?(gsquelart)
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.
Flags: needinfo?(gsquelart)
we seem to have a Heisenbug here; it works with the logging enabled, doesn't work without it.
Attached video no_doctor.mov
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!
Priority: -- → P3
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.
Attachment #8779182 - Flags: review?(mconley)
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.
Attachment #8779182 - Flags: review?(mconley) → review-
(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.
Attachment #8779182 - Attachment is obsolete: true
Attachment #8789802 - Flags: review?(mconley)
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?
Attachment #8789802 - Flags: review?(mconley) → review+
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
sorry had to back this out because this conflicted with another backout in bug 1263665
Flags: needinfo?(blassey.bugs)
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
https://hg.mozilla.org/mozilla-central/rev/ad3ce51b1324
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: needinfo?(blassey.bugs)
Depends on: 1308677
Blocks: 1277346
Please pref off or back out this feature due to serious regression bug 1308677.
Flags: needinfo?(mconley)
Flags: needinfo?(blassey.bugs)
Patch in bug 1308677 to address it.
Flags: needinfo?(blassey.bugs)
I have a second lead cooking in bug 1308677 that should help address this regression.
Flags: needinfo?(mconley)
This feature seems to be enabled in 52, per status flag. Brad, could you please confirm?
Flags: qe-verify?
Flags: needinfo?(blassey.bugs)
held to nightly
Flags: needinfo?(blassey.bugs)
Depends on: 1348810
You need to log in before you can comment on or make changes to this bug.