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

Status: RESOLVED FIXED

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

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

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".

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: hls_notificaiton.patch

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 3

[6lobe]

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

Comment 4

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Gerald - can you shed some light on this?

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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.

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: hls_notificaiton.patch

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

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: no_doctor.mov

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

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(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 11

[Brad Lassey [:blassey] (use needinfo?)]

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 12

[Mike Conley (: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.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

(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 14

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: hls_notificaiton.patch

Comment 15

[Mike Conley (: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?

Comment 16

[Pulsebot]

Pushed by [e-mail]:
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

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d065b725b7f
Backed out changeset 0df9c1afedc2

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Pulsebot]

Pushed by [e-mail]:
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

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ad3ce51b1324

Comment 21

[Benjamin Smedberg]

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

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Patch in bug 1308677 to address it.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

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

Comment 24

[Andrei Vaida [:avaida]]

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

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

held to nightly