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

RESOLVED FIXED in Firefox 55

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: Gijs, Assigned: alwu)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

3 months ago
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.
(Reporter)

Updated

3 months ago
Blocks: 1195386
Flags: needinfo?(alwu)
Component: Audio/Video → Audio/Video: Playback
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
Flags: needinfo?(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).
(Reporter)

Comment 3

3 months ago
(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.
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?
(Assignee)

Updated

3 months ago
Assignee: nobody → alwu
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8850291 - Flags: review?(s.kaspari)
Attachment #8850292 - Flags: review?(s.kaspari)
Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

https://reviewboard.mozilla.org/r/122912/#review125308
Attachment #8850291 - Flags: review?(s.kaspari) → review+
Comment on attachment 8850292 [details]
Bug 1348803 - part2 : modify event name to lower case letter.

https://reviewboard.mozilla.org/r/122914/#review125310
Attachment #8850292 - Flags: review?(s.kaspari) → review+

Comment 9

3 months ago
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)
(Reporter)

Comment 10

3 months ago
(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.
(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
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 12

3 months ago
(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
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(s.kaspari)
(Reporter)

Comment 13

3 months ago
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).
(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.
(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.
(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.
(Reporter)

Comment 17

3 months ago
(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?
Flags: needinfo?(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!
Flags: needinfo?(alwu) → needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 19

3 months ago
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8850291 - Flags: review+
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!
Attachment #8850291 - Flags: review?(s.kaspari)
Comment on attachment 8850291 [details]
Bug 1348803 - part1 : move fennec-only code to android/browser.js.

https://reviewboard.mozilla.org/r/122912/#review126262
Attachment #8850291 - Flags: review?(s.kaspari) → review+
(Assignee)

Updated

3 months ago
See Also: → bug 1351150

Comment 24

3 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

3 months ago
Pushed by alwu@mozilla.com:
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

3 months ago
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4ac559eea9ec
followup, take eslint's advice and remove an unused var
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
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.
Flags: needinfo?(s.kaspari)
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
Flags: needinfo?(s.kaspari)
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?
Flags: needinfo?(s.kaspari) → needinfo?(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?
Flags: needinfo?(gbrown)
(Reporter)

Comment 34

2 months ago
Alastor, are you planning to land this soon? If tests are prohibitively complicated, can we move those to a followup bug?
Flags: needinfo?(alwu)
Sorry I'm working on other bugs now, I'll back to this bug soon.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d2639a14ead76fde17b33a41c40e74646544213
Flags: needinfo?(alwu)

Comment 39

2 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

2 months ago
Pushed by alwu@mozilla.com:
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8be047fc5c9
https://hg.mozilla.org/mozilla-central/rev/af746f31572f
Status: NEW → RESOLVED
Last Resolved: 2 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.