1167690 - Support Flash for the audio channel notifications indicating when audio playback starts/stops

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

Flash player 18 implements new NPAPI APIs that will allow us to receive notifications when a plugin instance receives a notification.  We should integrate them with the APIs implemented in bug 923247.

Comment 1

[(no longer active)]

Attachment: Draft of the NPAPI API implemented by Flash

Please note that this is an early draft, and subject to change from Adobe.  We should start implementing by this, and change our implementation if the Flash team decides to change things on their side.

Comment 2

[Benoit Girard (:BenWa)]

Tentatively interested in doing this.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 0: npapi.h change (don't land but merge from upstream)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

I assume this would work on Windows and OSX only?

Comment 5

[Benoit Girard (:BenWa)]

Attachment: Testing: noisecontrol.xpi repack to not special case plugins

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8609567 [details]
Testing: noisecontrol.xpi repack to not special case plugins

nvm, this addon doesn't use the API I though it did.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: Part 1: Implement NPPVpluginIsPlayingAudio

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Olli Pettay [:smaug] (review overload. No new review requests before May 24, please) from comment #4)
> I assume this would work on Windows and OSX only?

Best way to find out is to build with part 0 + part 1 and look for the 'set audio 0x1' printf.

Comment 9

[Benoit Girard (:BenWa)]

:baku I implemented part 1 similarly to how you did bug 923247. I guess we likely want to use a different event for plugin playback notification?

Comment 10

[(no longer active)]

(In reply to Benoit Girard (On Leave Until June 1st!) from comment #9)
> :baku I implemented part 1 similarly to how you did bug 923247. I guess we
> likely want to use a different event for plugin playback notification?

It would be better if we hook both to the same event, IMO.  That way, in the front-end code, we can just deal with one event, since we won't care about whether audio is coming from Flash or Gecko there...

Comment 11

[Benoit Girard (:BenWa)]

Cool, in that case we should probably document how it works when there's videos and plug-ins changing their audio state independently. Should the plugin and audio code coordinate together such that there's a single media-playback active/inactive state, or should the plugin/audio code publish their state independently and let the consumer count how many active/inactive calls have gone by.

Also we will want to deal with the case where the plug-in crashes which my patch does not yet do.

Comment 12

[(no longer active)]

(In reply to Benoit Girard (On Leave Until June 1st!) from comment #11)
> Cool, in that case we should probably document how it works when there's
> videos and plug-ins changing their audio state independently. Should the
> plugin and audio code coordinate together such that there's a single
> media-playback active/inactive state, or should the plugin/audio code
> publish their state independently and let the consumer count how many
> active/inactive calls have gone by.

That's a good question.  Ideally we would tie them to the audio channel similarly to the way that HTML audio is, but Andrea is much more familiar with that stuff, so I'll let him answer this.  :-)

> Also we will want to deal with the case where the plug-in crashes which my
> patch does not yet do.

Oh, yes, I assume we already get notifications for that, right?  We can probably tie those to the audio channel as well, and consider a crash as, well, stopping to play audio if Flash audio was already being played!

Comment 13

[Andrea Marchesini [:baku]]

Comment on attachment 8609571 [details] [diff] [review]
Part 1: Implement NPPVpluginIsPlayingAudio

Review of attachment 8609571 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +2431,5 @@
> +          services::GetObserverService();
> +        if (observerService) {
> +          // XXX THIS NEEDS A BETTER API
> +          observerService->NotifyObservers(ToSupports(domwindow),
> +                                           "media-playback",

Instead doing this, would be nice to send this information to AudioChannelService and let it do the notification.
In this way, if we have a mediaelement playing and a flash plugin getting active, we don't send the notification twice.

The main problem I see is this: AudioChannelService has been fully rewritten recently and It's going to land as soon as the gaia team implements something on their side. bug 1113086.

I would like to see this code built on top of that bug otherwise we do the work twice and I think bug 1113086 will land before the next cycle.

What I would like to see here is: This code should use an nsIAudioChannelAgent and register it to the AudioChannelService.
Then, when you receive a notification by the service, mute the server.
If you want I can review this part or implement it, if you prefer.

Comment 14

[Benoit Girard (:BenWa)]

You're more familiar with that code so it should be easier for you to implement it. Go ahead.

Comment 15

[Andrea Marchesini [:baku]]

I'm not able to receive any notification from flash plugin. Do you?
I'm using the last flash plugin from the adobe labs on windows.

Comment 16

[Benoit Girard (:BenWa)]

Yes, we'd have to see if Flash has it only implemented on mac.

This is moving to the accepted status on npapi.h so we can land this properly shortly.

Comment 17

[Benoit Girard (:BenWa)]

Attachment: https://github.com/mozilla/npapi-sdk/pull/26

Comment 18

[Andrea Marchesini [:baku]]

benWa, I don't have mac environment. What about if I write some code and then you test/finish it?
I'll send you a patch for the next week.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: Part 1: Implement NPPVpluginIsPlayingAudio

Can you please test this patch?

Comment 20

[Benoit Girard (:BenWa)]

I'm not sure what additional information I can provide by testing this patch.

I've already confirmed that the flash events are being received on OSX.

Comment 21

[Andrea Marchesini [:baku]]

Attachment: Part 1: Implement NPPVpluginIsPlayingAudio

I tested this patch on windows and I confirm that the window receives the media-playback notifications. If we want to mute the flash player when the AudioChannelService wants, we should implement the SetIsMuted() method.

Comment 22

[(no longer active)]

Comment on attachment 8623024 [details] [diff] [review]
Part 1: Implement NPPVpluginIsPlayingAudio

Review of attachment 8623024 [details] [diff] [review]:
-----------------------------------------------------------------

Josh, do you mind reviewing the plugin related parts of this patch?  Hopefully they are easy to review, since the changes seem pretty similar to the surrounding stuff.

Thanks!

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +212,5 @@
> +private:
> +  ~EventProxyHandler()
> +  {}
> +
> +  nsNPAPIPluginInstance* mInstance;

Please make this an nsRefPtr.

::: dom/plugins/base/nsNPAPIPluginInstance.h
@@ +121,5 @@
>    nsPluginInstanceOwner* GetOwner();
>    void SetOwner(nsPluginInstanceOwner *aOwner);
>    nsresult ShowStatus(const char* message);
>  
> +  bool HasAudioChannelAgent()

Nit: const

Comment 23

[Andrea Marchesini [:baku]]

> ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> @@ +212,5 @@
> > +private:
> > +  ~EventProxyHandler()
> > +  {}
> > +
> > +  nsNPAPIPluginInstance* mInstance;
> 
> Please make this an nsRefPtr.

this EventProxyHandler is kept alive by nsNPAPIPluginInstance. Maybe a comment is enough to explain the raw pointer?

Comment 24

[(no longer active)]

(In reply to Andrea Marchesini (:baku) from comment #23)
> > ::: dom/plugins/base/nsNPAPIPluginInstance.cpp
> > @@ +212,5 @@
> > > +private:
> > > +  ~EventProxyHandler()
> > > +  {}
> > > +
> > > +  nsNPAPIPluginInstance* mInstance;
> > 
> > Please make this an nsRefPtr.
> 
> this EventProxyHandler is kept alive by nsNPAPIPluginInstance. Maybe a
> comment is enough to explain the raw pointer?

No (see bug 1114683 please!)

To avoid a reference cycle, you can clear out the pointer when the nsNPAPIPluginInstance object is about to be desoryed.

Comment 25

[Josh Aas]

Comment on attachment 8623024 [details] [diff] [review]
Part 1: Implement NPPVpluginIsPlayingAudio

Review of attachment 8623024 [details] [diff] [review]:
-----------------------------------------------------------------

Need to see the conflict with the spec resolved, could use some logic flow cleanup.

::: dom/plugins/base/nsNPAPIPlugin.cpp
@@ +2421,5 @@
>        return inst->SetUsesDOMForCursor(useDOMForCursor);
>      }
>  
> +    case NPPVpluginIsPlayingAudio: {
> +      bool isPlaying = !!result;

This code is interpreting the pointer value itself as a bool. This is not what the spec currently says should happen:

https://wiki.mozilla.org/NPAPI:AudioControl

"This NPPVariable, when set via NPN_SetValue, will let the plugin instance signal via a pointer to a BOOL if the current instance is currently playing audio."

If this patch implements existing behavior in a shipping Flash player, then we should change the spec to reflect reality. We shouldn't really be stuffing non-pointer values into pointers though. It's a bad habit from the early days of NPAPI.

@@ +2426,5 @@
> +
> +      nsNPAPIPluginInstance* inst = (nsNPAPIPluginInstance*) npp->ndata;
> +      MOZ_ASSERT(inst);
> +
> +      if (!isPlaying && !inst->HasAudioChannelAgent()) {

Every time we access isPlaying we negate it (again below). Why not just have a "muted" variable and get rid of three negations in this patch?

Also, do we even need this if block? It seems like an unnecessary optimization. The code below should behave just fine without it.

@@ +2439,5 @@
> +
> +      MOZ_ASSERT(agent);
> +
> +      if (!isPlaying) {
> +        rv = agent->StopPlaying();

My understanding is that StopPlaying() does not actually stop anything from playing, it just notifies that something did stop playing. This is a poorly named method, though it's out of the scope of this patch. Would be nice to rename to "NotifyStoppedPlaying()".

@@ +2441,5 @@
> +
> +      if (!isPlaying) {
> +        rv = agent->StopPlaying();
> +        if (NS_WARN_IF(NS_FAILED(rv))) {
> +          return NPERR_GENERIC_ERROR;

This is telling the plugin that there was an error on our side, but in this case the error is irrelevant to the plugin. We just didn't do our own notification. Fine to bail, but no need to raise the alarm to the plugin. That might execute error paths in the plugin that we don't want running.

@@ +2447,5 @@
> +
> +        return NPERR_NO_ERROR;
> +      }
> +
> +      int32_t canPlay;

All of this code is better organized as an "else" block for the if (!isPlaying) block above.

@@ +2448,5 @@
> +        return NPERR_NO_ERROR;
> +      }
> +
> +      int32_t canPlay;
> +      rv = agent->StartPlaying(&canPlay);

Same comment here as about the naming of StopPlaying above. Poorly named.

@@ +2450,5 @@
> +
> +      int32_t canPlay;
> +      rv = agent->StartPlaying(&canPlay);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return NPERR_GENERIC_ERROR;

Same comment as above about the error here being irrelevant to the plugin. Fine to bail, but don't raise an error with the plugin.

@@ +2455,5 @@
> +      }
> +
> +      rv = inst->CanPlayChanged(canPlay);
> +      if (NS_WARN_IF(NS_FAILED(rv))) {
> +        return NPERR_GENERIC_ERROR;

Same comment as above about the error here being irrelevant to the plugin. Fine to bail, but don't raise an error with the plugin.

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +326,5 @@
>  #endif
> +
> +  if (mEventProxyHelper) {
> +    nsCOMPtr<nsPIDOMWindow> window = GetDOMWindow();
> +    if (window) {

You can avoid some unnecessary nesting here by just returning. Same for if (targe) below.

@@ +1943,5 @@
> +{
> +  nsresult rv = SetMuted(canPlay == nsIAudioChannelAgent::AUDIO_AGENT_STATE_MUTED);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

Why don't you just return rv from this method? Why the if block followed immediately by "return NS_OK"?

Comment 26

[Andrea Marchesini [:baku]]

> https://wiki.mozilla.org/NPAPI:AudioControl

Basically... there is not relationship between this proposal and what flash plugin is actually implementing.
That documentation has to be fully updated.

> If this patch implements existing behavior in a shipping Flash player, then
> we should change the spec to reflect reality. We shouldn't really be
> stuffing non-pointer values into pointers though. It's a bad habit from the
> early days of NPAPI.

Would be nice to have some from MDN involved in this documentation.

> > +      if (!isPlaying && !inst->HasAudioChannelAgent()) {

> Also, do we even need this if block? It seems like an unnecessary
> optimization. The code below should behave just fine without it.

I disagree. I don't think we should create a AudioChannelAgent if it doesn't exist.

> My understanding is that StopPlaying() does not actually stop anything from
> playing, it just notifies that something did stop playing. This is a poorly
> named method, though it's out of the scope of this patch. Would be nice to
> rename to "NotifyStoppedPlaying()".

Good point. But I'm not going to do it in this patch. It will touch way to much code and I prefer to do it in a follow up.

> Why don't you just return rv from this method? Why the if block followed
> immediately by "return NS_OK"?

for NS_WARN_IF. Debugging messages are nice to have :)
In dom/* we do this quite a lot in new code, but I'm fine to remove it.

Comment 27

[Andrea Marchesini [:baku]]

Attachment: Part 1: Implement NPPVpluginIsPlayingAudio

Comment 28

[Josh Aas]

(In reply to Andrea Marchesini (:baku) from comment #26)
> > https://wiki.mozilla.org/NPAPI:AudioControl
> 
> Basically... there is not relationship between this proposal and what flash
> plugin is actually implementing.
> That documentation has to be fully updated.

OK, if this patch is implementing what the plugin is actually doing then we need to update the spec and notify plugin-futures. We cannot implement new features without an accurate spec. I can do this if you want.

> > My understanding is that StopPlaying() does not actually stop anything from
> > playing, it just notifies that something did stop playing. This is a poorly
> > named method, though it's out of the scope of this patch. Would be nice to
> > rename to "NotifyStoppedPlaying()".
> 
> Good point. But I'm not going to do it in this patch. It will touch way to
> much code and I prefer to do it in a follow up.

Understandable, no problem

> > Why don't you just return rv from this method? Why the if block followed
> > immediately by "return NS_OK"?
> 
> for NS_WARN_IF. Debugging messages are nice to have :)
> In dom/* we do this quite a lot in new code, but I'm fine to remove it.

NS_WARN_IF is fine, but it doesn't need to be wrapped in that if statement, does it?

Comment 29

[Andrea Marchesini [:baku]]

> OK, if this patch is implementing what the plugin is actually doing then we
> need to update the spec and notify plugin-futures. We cannot implement new
> features without an accurate spec. I can do this if you want.

Thanks! Would be great.

> NS_WARN_IF is fine, but it doesn't need to be wrapped in that if statement,
> does it?

I'll replace it with:

NS_WARN_IF(NS_FAILED(rv));
return rv;

Are you ok if I don't upload a new patch just for this?

Comment 30

[Josh Aas]

(In reply to Andrea Marchesini (:baku) from comment #29)

> Are you ok if I don't upload a new patch just for this?

No problem.

Comment 31

[Josh Aas]

Comment on attachment 8625930 [details] [diff] [review]
Part 1: Implement NPPVpluginIsPlayingAudio

Review of attachment 8625930 [details] [diff] [review]:
-----------------------------------------------------------------

I have updated the spec and emailed the list.

Comment 32

[Andrea Marchesini [:baku]]

BenWa, can you show me how to use the Mute flag? Better if you can implement something on top of my last patch instead the TODO I left in the code. Thanks!

Comment 33

[Benoit Girard (:BenWa)]

Just give me a bit more time, I'll get to it!

Comment 34

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f8eb8ae05e6

Comment 35

[Benoit Girard (:BenWa)]

Attachment: Part 1: Implement NPPVpluginIsPlayingAudio

Here I wasn't sure how to test the audio channel stuff.

Comment 36

[Andrea Marchesini [:baku]]

> Here I wasn't sure how to test the audio channel stuff.

We should implement a test plugin that works as flash plugin does. Can you submit an interdiff of your patch vs mine?
Or tell what is changed. Thanks.

Comment 37

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5f8eb8ae05e6

Comment 38

[Caspy7]

I don't know if the current code accounts for this, but one thing I have noticed when using the Noise Control addon is that the audio indicator icon stays present even when a tab has become unloaded. I use an addon to unload unused tabs.

(Apologies if this issue is not directly relevant to this bug or belongs in another place.)

Comment 39

[(no longer active)]

For a slightly easier way to test this, you can apply my patches in bug 1180421 and bug 486262, and watch the tab bar.  :-)

Comment 40

[Benoit Girard (:BenWa)]

(In reply to Andrea Marchesini (:baku) from comment #36)
> > Here I wasn't sure how to test the audio channel stuff.
> 
> We should implement a test plugin that works as flash plugin does. Can you
> submit an interdiff of your patch vs mine?
> Or tell what is changed. Thanks.

I don't have anything handy to generate interdiff. Ideally you would of done your edit using another part instead of lumping it in my patch which was originally an independent logical change.

Anyways I implemented the plugin stuff and finished 'nsNPAPIPluginInstance::SetMuted'.

Comment 41

[Benoit Girard (:BenWa)]

(In reply to Caspy7 from comment #38)
> (Apologies if this issue is not directly relevant to this bug or belongs in
> another place.)

I believe it is since this is for an entirely new feature to know accurately when flash is playing audio. Might be worth filling a new issue if it's a platform bug if you want this to get attention.

Comment 42

[Andrea Marchesini [:baku]]

OK. Thanks. When bug 1113086 lands, we can land this second patch. Otherwise we mute the flash plugin all the time we change tab.

Comment 43

[Andrea Marchesini [:baku]]

Attachment: Part 2: AudioChannel integration

rebasing the patch I preferred to split it in 3 independent parts as suggested by comment 36.

Comment 44

[Andrea Marchesini [:baku]]

Attachment: Part 3: correct #ifdef in npapi.h

Comment 45

[Andrea Marchesini [:baku]]

Attachment: Part 3: correct #ifdef in npapi.h

Comment 46

[Andrea Marchesini [:baku]]

Attachment: Part 4: setMuted implemented

Comment 47

[Benoit Girard (:BenWa)]

Comment on attachment 8633490 [details] [diff] [review]
Part 3: correct #ifdef in npapi.h

Review of attachment 8633490 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, can you please upstream the fix to https://github.com/mozilla/npapi-sdk?

Comment 48

[Andrea Marchesini [:baku]]

Attachment: https://github.com/mozilla/npapi-sdk/pull/27

Comment 49

[Andrea Marchesini [:baku]]

I guess we are ready to land these patches. Benoit, shall I proceed? Do you want to do it?

Comment 50

[Benoit Girard (:BenWa)]

Can we get a try run first?

Comment 51

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=333361ec56e8

Sure. There are other patches in this push, but if you prefer I can send a new push just for these few patches.

Comment 52

[(no longer active)]

I applied the patches from the try push in comment 51 into my tree, and installed the latest Flash beta (version 18.0.0.210), and with those in OSX I don't get the audio playing notifications...  I am testing with Flash videos on bbc.com.

When I set a breakpoint in the Flash plugin-container process on PluginInstanceChild::NPN_SetValue, it seems like the NPPVpluginIsPlayingAudio variable is never set.

Benoit, did this work for you before?

Comment 53

[Benoit Girard (:BenWa)]

It did. Make sure you're setting the breakpoint in the child plugin process.

If there's a regression then I can take a look tomorrow.

Comment 54

[Benoit Girard (:BenWa)]

Looks like there might be a regression with Flash Beta 18.0.203. Last working version is Flash Beta 18.0.194 which is still available on the archives:
https://helpx.adobe.com/flash-player/kb/archived-flash-player-versions.html

Comment 55

[(no longer active)]

Can this land while we wait for the regression in Flash to be fixed?

Comment 57

[(no longer active)]

I attempted to write a test for this, and realized that NPNVmuteAudioBool is not fully hooked up yet, so I fixed that too.  Patches coming up.

Comment 58

[(no longer active)]

Attachment: Part 2.1: Make nsNPAPIPluginInstance's QueryInterface support nsIAudioChannelAgentCallback

Comment 59

[(no longer active)]

Attachment: Part 5: Hook up NPNVmuteAudioBool to the plugin process

Comment 60

[(no longer active)]

Attachment: Part 6: Add support for testing plugin audio channel integration to the test plugin

Comment 61

[(no longer active)]

Attachment: Part 7: Add tests for plugin audio channel integration

Comment 62

[Andrea Marchesini [:baku]]

Comment on attachment 8639127 [details] [diff] [review]
Part 7: Add tests for plugin audio channel integration

Review of attachment 8639127 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/test/test_pluginAudioNotification.html
@@ +25,5 @@
> +  }
> +
> +  ok(false, "Could not find plugin tag with plugin name '" + name + "'");
> +  return null;
> +}

// Copied from /dom/plugins/test/mochitest/utils.js

Comment 63

[(no longer active)]

(In reply to Benoit Girard (:BenWa) from comment #54)
> Looks like there might be a regression with Flash Beta 18.0.203. Last
> working version is Flash Beta 18.0.194 which is still available on the
> archives:
> https://helpx.adobe.com/flash-player/kb/archived-flash-player-versions.html

FWIW I verified the patches with Flash Beta 18.0.194 and everything seems to work well.

Comment 64

[Josh Aas]

Comment on attachment 8639126 [details] [diff] [review]
Part 6: Add support for testing plugin audio channel integration to the test plugin

Review of attachment 8639126 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/test/testplugin/nptest.cpp
@@ +3726,5 @@
> +static bool
> +toggleAudioPlayback(NPObject* npobj, uint32_t argCount, bool playingAudio, NPVariant* result)
> +{
> +  if (argCount != 0)
> +    return false;

Please use {} for all blocks.

@@ +3754,5 @@
> +static bool
> +audioMuted(NPObject* npobj, const NPVariant* args, uint32_t argCount, NPVariant* result)
> +{
> +  if (argCount != 0)
> +    return false;

Please use {} for all blocks.

Comment 65

[(no longer active)]

I'm trying to land these patches, and things have been slightly bitrotten by bug 1156472.  I've needed to do two small fixes.  Patch coming up...

Comment 66

[(no longer active)]

Attachment: Final fixes for bug 1167690

Comment 67

[(no longer active)]

Attachment: Fix a leak in the test by releasing the reference to the audio channel agent and breaking a cycle that would hold this object alive

Comment 68

[(no longer active)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd61fa5f3225

Comment 69

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70970017e3d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/6aad3344f9a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd2cbe32eff
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa68347a16cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba3739202368

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70970017e3d5
https://hg.mozilla.org/mozilla-central/rev/6aad3344f9a0
https://hg.mozilla.org/mozilla-central/rev/4dd2cbe32eff
https://hg.mozilla.org/mozilla-central/rev/aa68347a16cd
https://hg.mozilla.org/mozilla-central/rev/ba3739202368

Comment 71

[(no longer active)]

FWIW I double checked that this works well with Flash 19.0.0.124 which was released today.