1348803 - Audio playback uses inefficient observers to update the UI for mute/playback state

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, enhancement)

Description

[:Gijs (he/him)]

In e10s, every tab registers 3 observer notifications here:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/content/browser-content.js#973-975

and then filters out audio-playback notifications that aren't supposed to be for its own window here:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/content/browser-content.js#1031-1041

I have to admit I don't understand how the other 2 observer notifications (AudioFocusChanged and MediaControl) are supposed to work at all correctly - they seem to indiscriminately set various aspects of the nsIDOMWindowUtils audio state no matter where the message came from. So it's not clear to me how it's distinguishing between 2 or more tabs with audio.

More generally, it feels like all of this should just use system events, so that only the right tab gets the relevant event.

Comment 1

[Alastor Wu [:alwu]]

Because these two events are sent from java side [1] and it's not just for specific tab, it might notify multiple tabs if we're playing audio in different tabs at the same time.

[1] https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/mobile/android/base/java/org/mozilla/gecko/media/AudioFocusAgent.java#59

Comment 2

[Alastor Wu [:alwu]]

Since these two events are Fennec-only, maybe we can only register them when running on Fennec (If we could know that in browser-content.js).

Comment 3

[:Gijs (he/him)]

(In reply to Alastor Wu [:alwu] from comment #2)
> Since these two events are Fennec-only, maybe we can only register them when
> running on Fennec (If we could know that in browser-content.js).

You could know that with AppConstants.platform and probably in various other ways, but really, ideally we should be using better methods of messaging than just piling things into the observer service.

Comment 4

[(no longer active)]

Ugh, we should definitely not waste time running Fennec code on desktop!  Alastor, can you please fix this?  Should be pretty easy.

On the question of the observer service, it's not a pretty API, but I'd like to understand the objection better.  Gijs, are you complaining about the aesthetics of the API, or about the performance of the code?

Comment 5

[Alastor Wu [:alwu]]

Attachment: Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Review commit: https://reviewboard.mozilla.org/r/122912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122912/

Comment 6

[Alastor Wu [:alwu]]

Attachment: Bug 1348803 - part2 : modify event name to lower case letter.

Other events in browser.js are all lower case letter, also change these two to make them consistent.

Review commit: https://reviewboard.mozilla.org/r/122914/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/122914/

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

https://reviewboard.mozilla.org/r/122912/#review125308

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

https://reviewboard.mozilla.org/r/122914/#review125310

Comment 9

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 48f4a99c4182 -d 36b934545efb: rebasing 383522:48f4a99c4182 "Bug 1348803 - part1 : move fennec-only code to android/browser.js. r=sebastian"
merging mobile/android/chrome/content/browser.js
rebasing 383523:976295920a4e "Bug 1348803 - part2 : modify event name to lower case letter. r=sebastian" (tip)
merging mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java
merging mobile/android/chrome/content/browser.js
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 10

[:Gijs (he/him)]

(In reply to :Ehsan Akhgari (busy) from comment #4)
> Ugh, we should definitely not waste time running Fennec code on desktop! 
> Alastor, can you please fix this?  Should be pretty easy.
> 
> On the question of the observer service, it's not a pretty API, but I'd like
> to understand the objection better.  Gijs, are you complaining about the
> aesthetics of the API, or about the performance of the code?

Both. The patches so far don't change the audio-playback observer notification usage, which is just very inefficient (if you have N tabs, N tabs will get the notification, check if the notification is for their tab, and only do something if the notification matches their tab, and so we end up crossing the XPCOM JS/C++ threshold M * N times, and taking up memory for N observers when we really only need some small constant number). A better API to use would be a custom (system) event. We could fire that at the toplevel content frame's window, and a content script could just have an event listener for that event, and then would only get events it actually cared about.

In fact, looking at the code, it looks like the code here: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/content/browser-content.js#1031-1041 is essentially just using JS to forward the observer notification to the relevant message manager. The C++ code dispatching the observer notification could find the relevant message manager itself and just dispatch the relevant message directly.

If that's somehow complicated (not sure how the C++ abstractions are on the sending side and if accessing the DOM or message manager is somehow non-trivial), it would still be better to use 1 process-wide observer listener that then simply sent the same async message the code at https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/content/browser-content.js#1031-1041 sends already, but with the frame message manager for that particular toplevel window.

Comment 11

[Alastor Wu [:alwu]]

(In reply to :Gijs from comment #10)
> Both. The patches so far don't change the audio-playback observer
> notification usage, which is just very inefficient (if you have N tabs, N
> tabs will get the notification, check if the notification is for their tab,
> and only do something if the notification matches their tab, and so we end
> up crossing the XPCOM JS/C++ threshold M * N times, and taking up memory for
> N observers when we really only need some small constant number). A better
> API to use would be a custom (system) event. We could fire that at the
> toplevel content frame's window, and a content script could just have an
> event listener for that event, and then would only get events it actually
> cared about.

Could you give me any example using custom event sending message from JAVA side to Gecko side? Because I only know this way [1] to send message from Java.

Another idea is we could only register observer when the tab really needs to, tabs can know whether they have alive media via "media-playback" event. Register observer when tab has alive media, and unregister it after media died.

Thanks!

[1] http://searchfox.org/mozilla-central/rev/9af9b1c7946ebef6056d2ee4c368d496395cf1c8/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#257

Comment 12

[:Gijs (he/him)]

(In reply to Alastor Wu [:alwu] from comment #11)
> (In reply to :Gijs from comment #10)
> > Both. The patches so far don't change the audio-playback observer
> > notification usage, which is just very inefficient (if you have N tabs, N
> > tabs will get the notification, check if the notification is for their tab,
> > and only do something if the notification matches their tab, and so we end
> > up crossing the XPCOM JS/C++ threshold M * N times, and taking up memory for
> > N observers when we really only need some small constant number). A better
> > API to use would be a custom (system) event. We could fire that at the
> > toplevel content frame's window, and a content script could just have an
> > event listener for that event, and then would only get events it actually
> > cared about.
> 
> Could you give me any example using custom event sending message from JAVA
> side to Gecko side? Because I only know this way [1] to send message from
> Java.

"audio-playback" isn't being sent from java, it's being sent from C++. See https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/AudioChannelService.cpp#136 . The other two notifications for which you've already created patches are being sent from Java.

I don't actually work on Java and so Sebastian or other Firefox-for-Android people are better people to ask, but based on poking DXR for a bit, it seems there's an EventDispatcher class that can send custom events which JS components listen to, e.g. https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1384 and https://dxr.mozilla.org/mozilla-central/rev/e03e0c60462c775c7558a1dc9d5cf2076c3cd1f9/mobile/android/chrome/content/FeedHandler.js#61

Comment 13

[:Gijs (he/him)]

To be quite clear: if the 2 notifications you're changing:

1) only apply to Firefox for Android
2) need to affect *every* loaded tab

then the observer service might still be the right API choice. But neither of those 2 things seems to be true for the "audio-playback" notification (as opposed to the "AudioFocusChanged" and "MediaControl" ones, which are the ones your patches are affecting).

Comment 14

[Alastor Wu [:alwu]]

(In reply to :Gijs from comment #12)
> "audio-playback" isn't being sent from java, it's being sent from C++. See
> https://dxr.mozilla.org/mozilla-central/source/dom/audiochannel/
> AudioChannelService.cpp#136 . The other two notifications for which you've
> already created patches are being sent from Java.

Yes, I know it's from C++, I ask this question is only because my patch is trying to move out fennec-only events.

Comment 15

[Alastor Wu [:alwu]]

(In reply to :Gijs from comment #13)
> To be quite clear: if the 2 notifications you're changing:
> 
> 1) only apply to Firefox for Android

Yes

> 2) need to affect *every* loaded tab

No, only affect the tab with playing media.

Comment 16

[Alastor Wu [:alwu]]

(In reply to :Gijs from comment #13)
> To be quite clear: if the 2 notifications you're changing:
> 
> 1) only apply to Firefox for Android
> 2) need to affect *every* loaded tab
> 
> then the observer service might still be the right API choice. But neither
> of those 2 things seems to be true for the "audio-playback" notification

For audio-playback, it would affect EVERY tab and it would be used on every platform.

Comment 17

[:Gijs (he/him)]

(In reply to Alastor Wu [:alwu] from comment #16)
> (In reply to :Gijs from comment #13)
> > To be quite clear: if the 2 notifications you're changing:
> > 
> > 1) only apply to Firefox for Android
> > 2) need to affect *every* loaded tab
> > 
> > then the observer service might still be the right API choice. But neither
> > of those 2 things seems to be true for the "audio-playback" notification
> 
> For audio-playback, it would affect EVERY tab

Uhh, can you elaborate? It seems to me audio-playback observer notifications pass a DOM window reference, and the consumer checks whether that window matches:

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/content/browser-content.js#1031-1041

      if (subject && subject.top == global.content) {
        let name = "AudioPlayback:";
        if (data === "blockStart") {
          name += "BlockStart";
        } else if (data === "blockStop") {
          name += "BlockStop";
        } else {
          name += (data === "active") ? "Start" : "Stop";
        }
        sendAsyncMessage(name);
      }

this code only does something if the process script's content reference matches the toplevel window on which the notification was dispatched, AIUI. Am I missing something?

Comment 18

[Alastor Wu [:alwu]]

Oh, I misunderstanded what you said.

My meaning is, every tab need to listen the "audio-playback" event because there might be any audible playing media at same time. But yes, one "audio-playback" event only affects one tab, and only affected tab need to do something special. (ex. show the sound indicator)

As you said, this way might be inefficient. It's waste to call the tabs which are not the target tab. 

---

But I would like to file another bug to handle the "audio-playback" event because now I don't have much time to modify it. If we choose send the message to specific window, we also need to modify the tests which listen to that event, but I don't very understand how to do. Except the tests, AndroidBridge would also need to listen this event (after bug1347648), and I'm also not sure how to send the message to AndroidBridge using another way.

---

I would like to add another patch for "mediacontrol" and "audioFocusChanged", to register observer only when the tab has need (has playing media). I think that might be helpful, because Fennec doesn't allow multiple playing tab. Therefore, everytime we sent the events, only one tab would receive them.

---

How do you think?
Thanks!

Comment 19

[:Gijs (he/him)]

(In reply to Alastor Wu [:alwu] from comment #18)
> Oh, I misunderstanded what you said.
> 
> My meaning is, every tab need to listen the "audio-playback" event because
> there might be any audible playing media at same time. But yes, one
> "audio-playback" event only affects one tab, and only affected tab need to
> do something special. (ex. show the sound indicator)

OK, this makes sense to me. Thanks for clarifying.

> But I would like to file another bug to handle the "audio-playback" event
> because now I don't have much time to modify it. If we choose send the
> message to specific window, we also need to modify the tests which listen to
> that event, but I don't very understand how to do. Except the tests,
> AndroidBridge would also need to listen this event (after bug1347648), and
> I'm also not sure how to send the message to AndroidBridge using another way.

OK. Let's file a separate bug for audio-playback, then.

> I would like to add another patch for "mediacontrol" and
> "audioFocusChanged", to register observer only when the tab has need (has
> playing media). I think that might be helpful, because Fennec doesn't allow
> multiple playing tab. Therefore, everytime we sent the events, only one tab
> would receive them.
> 
> ---
> 
> How do you think?

This combined with not listening for these on desktop sounds good to me. In any case, I expect any inefficiencies are likely smaller on mobile because there are much fewer (active, loaded) tabs due to device constraints.

Comment 20

[Alastor Wu [:alwu]]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122912/diff/1-2/

Comment 21

[Alastor Wu [:alwu]]

Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122914/diff/1-2/

Comment 22

[Alastor Wu [:alwu]]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Hi, Sebastian,
Could you help me review this revised patch again?
Thanks!

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

https://reviewboard.mozilla.org/r/122912/#review126262

Comment 24

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 013583e6fded -d 3be5ffca0b5a: rebasing 385488:013583e6fded "Bug 1348803 - part1 : move fennec-only code to android/browser.js. r=sebastian"
merging mobile/android/chrome/content/browser.js
rebasing 385489:83df5f5535ae "Bug 1348803 - part2 : modify event name to lower case letter. r=sebastian" (tip)
merging mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java
merging mobile/android/chrome/content/browser.js
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/media/MediaControlService.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 25

[Alastor Wu [:alwu]]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122912/diff/2-3/

Comment 26

[Alastor Wu [:alwu]]

Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122914/diff/2-3/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f966aef934b1
part1 : move fennec-only code to android/browser.js. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/2ab6e0b8aec6
part2 : modify event name to lower case letter. r=sebastian

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ac559eea9ec
followup, take eslint's advice and remove an unused var

Comment 29

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/85236733cf5f for failures in a bunch of browser-chrome tests, like https://treeherder.mozilla.org/logviewer.html#?job_id=86831240&repo=autoland

Comment 30

[Alastor Wu [:alwu]]

Ah, it's embarrassed... since Fennec UI tests doesn't support for testing notification, most of the media control testing is chrome browser test and they're running on the desktop. 

If I remove these events on browser-content.js, it means we need to drop all these tests. Because now we don't have Fennec-only test for media control, and the browser test is not workable on Fennec (I guess, because I don't see any browser tests in try).

---

I don't want to be the situation we have no test for this feature, especially this feature has already landed on the release channel. So we need to find the way to test media control on Fennec before landing these patches.

Comment 31

[Alastor Wu [:alwu]]

Hi, Sebastian,
Could we have any method to write and run the browser test on Fennec?
If we have, how should I implement this behavior [1] on Fennec? (IIRC, fennec doesn't have browser.xml)

[1] I would like to call browser.pause(), browser.resume()...
https://goo.gl/dXzOjM

Comment 32

[Sebastian Kaspari (:sebastian; :pocmo)]

I actually do not know much about our tests that cover the browser part and not Android code/frontend. Maybe gbrown has a good answer to that?

Comment 33

[Geoff Brown [:gbrown]]

We do not run browser-chrome ("mochitest-bc") tests on Android. I have heard that many of those tests, and perhaps some of the browser-chrome test framework, are not compatible with Firefox for Android.

We do run most regular, plain mochitests on Android.

We also run many mochitest-chrome tests on Android, including some that are Android-only. I'm not sure of what Alastor needs, but suggest having a look at some of the Android mochitest-chrome tests in mobile/android/tests/browser/chrome. Hopefully something like that will work?

Comment 34

[:Gijs (he/him)]

Alastor, are you planning to land this soon? If tests are prohibitively complicated, can we move those to a followup bug?

Comment 35

[Alastor Wu [:alwu]]

Sorry I'm working on other bugs now, I'll back to this bug soon.

Comment 36

[Alastor Wu [:alwu]]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122912/diff/3-4/

Comment 37

[Alastor Wu [:alwu]]

Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122914/diff/3-4/

Comment 38

[Alastor Wu [:alwu]]

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

Comment 39

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s ddcb2136263d -d 9d86f0c3ddd4: rebasing 390332:ddcb2136263d "Bug 1348803 - part1 : move fennec-only code to android/browser.js. r=sebastian"
merging mobile/android/chrome/content/browser.js
merging toolkit/content/browser-content.js
warning: conflicts while merging mobile/android/chrome/content/browser.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging toolkit/content/browser-content.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 40

[Alastor Wu [:alwu]]

Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122912/diff/4-5/

Comment 41

[Alastor Wu [:alwu]]

Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122914/diff/4-5/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e8be047fc5c9
part1 : move fennec-only code to android/browser.js. r=sebastian
https://hg.mozilla.org/integration/autoland/rev/af746f31572f
part2 : modify event name to lower case letter. r=sebastian

Comment 43

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8be047fc5c9
https://hg.mozilla.org/mozilla-central/rev/af746f31572f