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

RESOLVED FIXED in mozilla42

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Depends on 2 bugs)

unspecified
mozilla42
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(14 attachments, 6 obsolete attachments)

1.25 KB, text/plain
Details
1.78 KB, patch
Details | Diff | Splinter Review
5.63 KB, patch
Details | Diff | Splinter Review
44 bytes, text/x-github-pull-request
jaas
: review+
Details | Review
7.66 KB, patch
Details | Diff | Splinter Review
1.08 KB, patch
BenWa
: review+
Details | Diff | Splinter Review
1.24 KB, patch
jaas
: review+
Details | Diff | Splinter Review
44 bytes, text/x-github-pull-request
Details | Review
1.02 KB, patch
baku
: review+
Details | Diff | Splinter Review
4.73 KB, patch
jaas
: review+
Details | Diff | Splinter Review
7.56 KB, patch
jaas
: review+
Details | Diff | Splinter Review
5.82 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.48 KB, patch
baku
: review+
Details | Diff | Splinter Review
859 bytes, patch
baku
: review+
Details | Diff | Splinter Review
Assignee

Description

4 years ago
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.
Assignee

Comment 1

4 years ago
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.
Tentatively interested in doing this.
Assignee: nobody → bgirard
Blocks: 486262
I assume this would work on Windows and OSX only?
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.
Attachment #8609567 - Attachment is obsolete: true
(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.
: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?
Flags: needinfo?(amarchesini)
Assignee

Comment 10

4 years ago
(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...
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.
Assignee

Comment 12

4 years ago
(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 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.
Flags: needinfo?(amarchesini)
You're more familiar with that code so it should be easier for you to implement it. Go ahead.
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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
Can you please test this patch?
Attachment #8609571 - Attachment is obsolete: true
Attachment #8622480 - Flags: feedback?(bgirard)
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.
Flags: needinfo?(bgirard)
Attachment #8622480 - Flags: feedback?(bgirard)
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.
Attachment #8623024 - Flags: review?(ehsan)
Assignee

Comment 22

4 years ago
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
Attachment #8623024 - Flags: review?(joshmoz)
Attachment #8623024 - Flags: review?(ehsan)
Attachment #8623024 - Flags: review+
> ::: 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?
Flags: needinfo?(ehsan)
Assignee

Comment 24

4 years ago
(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.
Flags: needinfo?(ehsan)
Assignee: bgirard → amarchesini

Updated

4 years ago
Attachment #8620539 - Flags: review?(joshmoz) → review+
Attachment #8622480 - Attachment is obsolete: true

Comment 25

4 years ago
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"?
Attachment #8623024 - Flags: review?(joshmoz) → review-
> 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.
Attachment #8623024 - Attachment is obsolete: true
Attachment #8625930 - Flags: review?(joshmoz)

Comment 28

4 years ago
(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?
> 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

4 years ago
(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

4 years ago
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.
Attachment #8625930 - Flags: review?(joshmoz) → review+
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!
Flags: needinfo?(bgirard)
Just give me a bit more time, I'll get to it!
Flags: needinfo?(bgirard)
Keywords: leave-open
Here I wasn't sure how to test the audio channel stuff.
Assignee: amarchesini → bgirard
Attachment #8625930 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
> 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.
Flags: needinfo?(amarchesini)

Comment 38

4 years ago
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.)
Assignee

Updated

4 years ago
No longer blocks: 923247
Assignee

Comment 39

4 years ago
For a slightly easier way to test this, you can apply my patches in bug 1180421 and bug 486262, and watch the tab bar.  :-)
(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'.
(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.
OK. Thanks. When bug 1113086 lands, we can land this second patch. Otherwise we mute the flash plugin all the time we change tab.
Depends on: 1113086
Attachment #8629492 - Attachment is obsolete: true
Attachment #8609571 - Attachment is obsolete: false
rebasing the patch I preferred to split it in 3 independent parts as suggested by comment 36.
Attachment #8633489 - Flags: review?(bgirard)
Attachment #8633489 - Attachment is obsolete: true
Attachment #8633489 - Flags: review?(bgirard)
Attachment #8633490 - Flags: review?(bgirard)
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?
Attachment #8633490 - Flags: review?(bgirard) → review+
I guess we are ready to land these patches. Benoit, shall I proceed? Do you want to do it?
Flags: needinfo?(bgirard)
Can we get a try run first?
Flags: needinfo?(bgirard) → needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini)
Attachment #8633491 - Flags: review?(jaas)
Assignee

Comment 52

4 years ago
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?
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
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

Updated

4 years ago
Attachment #8633491 - Flags: review?(jaas) → review+
Assignee

Comment 55

4 years ago
Can this land while we wait for the regression in Flash to be fixed?
Assignee

Updated

4 years ago
Duplicate of this bug: 593897
Assignee

Comment 57

4 years ago
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.
Attachment #8639124 - Flags: review?(amarchesini) → review+
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
Attachment #8639127 - Flags: review?(amarchesini) → review+
Assignee

Comment 63

4 years ago
(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.

Updated

4 years ago
Attachment #8639125 - Flags: review?(jaas) → review+

Comment 64

4 years ago
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.
Attachment #8639126 - Flags: review?(jaas) → review+
Assignee

Comment 65

4 years ago
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...
Flags: needinfo?(amarchesini)
Assignee

Comment 66

4 years ago
Attachment #8640161 - Flags: review?(amarchesini)
Assignee

Updated

4 years ago
Assignee: bgirard → ehsan
Attachment #8640161 - Flags: review?(amarchesini) → review+
Attachment #8640257 - Flags: review?(amarchesini) → review+
Flags: needinfo?(amarchesini)
Assignee

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee

Comment 71

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

Updated

4 years ago
No longer blocks: 1189796

Updated

4 years ago
Depends on: 1205477

Updated

4 years ago
Depends on: 1199928

Updated

4 years ago
Depends on: 1218168

Updated

4 years ago
Depends on: 1231244

Updated

3 years ago
Depends on: 1238167

Updated

2 years ago
Depends on: 1327391

Updated

2 years ago
See Also: → 1350947
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.