Closed Bug 1242874 Opened 9 years ago Closed 9 years ago

Enable pause/play control ability for the AudioChannel API

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Platform:
ARM
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: alwu, Assigned: alwu)

References

Details

User Story

When the user is playing the music from arbitrary web contents, then

(1) Audio competing happens (other higher-level interruption)
- The music should be paused, and its control icon should be changed from "pause" to "play" that indicates the music is paused now.
- If the remote media-control exists, its control icon should also be changed from "pause" to "play".

(2) Audio competing ended
- The music should be resumed, and its control icon should be changed from "play" to "pause" that indicates the music is playing now.
- If the remote media-control exists, its control icon should also be changed from "play" to "pause".

(3) Use the remote media-control to pause
- The music should be paused, and its control icon should be changed from "pause" to "play" that indicates the music is paused now.

(4) Use the remote media-control to play
- The music should be paused, and its control icon should be changed from "play" to "pause" that indicates the music is playing now.

Attachments

(5 files)

MozReview Request: Bug 1242874 - part1 : create suspened types
58 bytes, text/x-review-board-request
baku
: review+
Details
MozReview Request: Bug 1242874 - part2 : window's suspend attribute.
58 bytes, text/x-review-board-request
baku
: review+
ehsan.akhgari
: review+
Details
MozReview Request: Bug 1242874 - part3 : implement different suspended methods.
58 bytes, text/x-review-board-request
baku
: review+
jwwang
: review+
Details
MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying.
58 bytes, text/x-review-board-request
baku
: review+
Details
MozReview Request: bug 1242874 - part5 : add test.
58 bytes, text/x-review-board-request
baku
: review+
ehsan.akhgari
: review+
Details
*** Two main reasons *** Correct the state of the control UI & Support remote media-control *** More detailed introduction *** In present design, when we use the AudioChannel API to suspend the MediaElement, it doesn't use the pause() function so that the state of the control UI is always wrong during the suspended process. For example, if you're listening the music, and then get the incoming call. When the call is connected, the music would be paused. At that time, drag the utility tray and check the music-control-widget, you would find that the UI still keeps "pause" icon instead of "play" icon. [1][2] Another reason I filed this issue is we need this ability if we want to implement the remote media-control for all web content (bug1240423). The users can use remote media-control to operate the music, this behavior is as the same as user press pause within the web page. Therefore, the AudioChannel API needs this ability to directly control the MediaElement. BTW, Here are two chrome example [3][4]. [1] Wrong state in FxOS https://drive.google.com/file/d/0B8z53fPg4O7NbW00aXhLSGhpMFE/view?usp=sharing [2] Wrong state in browser (* note : opened the pref=media.useAudioChannelAPI) https://drive.google.com/file/d/0B8z53fPg4O7NVXFxRXVxT1BMVHc/view?usp=sharing [3] Correct state in Android Chrome https://drive.google.com/file/d/0B8z53fPg4O7NOGh5YldnYy1qYjg/view?usp=sharing [4] Remote media-control for web content in Android Chrome https://drive.google.com/file/d/0B8z53fPg4O7NRlRpNS1zRGdLOUk/view?usp=sharing
Ni baku and ehsan, because you might have interesting about remote media-control. BTW, these changesets won't change the sound indicator behavior. Please feel free to remove the flag :)
Flags: needinfo?(ehsan)
Flags: needinfo?(amarchesini)
Thanks! (Not sure if you just wanted to draw my attention or were asking for something specific. If the latter, can you please clarify?)
Flags: needinfo?(ehsan)
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/1-2/
Attachment #8712047 - Flags: review?(amarchesini)
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/1-2/
Attachment #8712048 - Flags: review?(amarchesini)
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/1-2/
Attachment #8712049 - Flags: review?(amarchesini)
Hi, Baku, Except the reasons mentioned in comment0, this patch also have another extra usefulness, which can correct the "moz-interrupt" event dispatching. Although we have bug1217711 which intends to remove these events, this is still working because the system app doesn't implement the logic yet and still need modify other gaia apps. In this patch, since I change the behavior of muted-by-default from "muted=true" to "volume=0", and it's equivalent behavior and can reduce the risk for accidentally suspending the whole process of MediaElement/MediaResource/Decoder. Very appreciate!
Flags: needinfo?(amarchesini)
Hi, Ehsan, Is former one, thanks :)
Sorry Alastor for the delay, I'll review these patches early next week.
Attachment #8712047 - Flags: review?(amarchesini)
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types https://reviewboard.mozilla.org/r/32421/#review31541 ::: dom/html/HTMLMediaElement.h:1543 (Diff revision 2) > + bool mSuspendedBySystemOrRemoteControl; There is some confusion here between mSuspendedBySystemOrRemoteControl and MUTED_BY_CHANNEL. Why we have 2 different (but similar) concepts? ::: dom/html/HTMLMediaElement.cpp:4674 (Diff revision 2) > + SetMutedInternal(mMuted | MUTED_BY_AUDIO_CHANNEL); Hold on, we can set the volume to 0, without muting the channel. Muting the channel means that we stop the loading. We don't want that, right? ::: dom/html/HTMLMediaElement.cpp:4685 (Diff revision 2) > + Pause(); Read below. ::: dom/html/HTMLMediaElement.cpp:4691 (Diff revision 2) > + rv = PlayInternal(nsContentUtils::IsCallerChrome()); Maybe this element is muted for other reasons. We should not call PlayInternal but use SuspendOrResultElement. ::: dom/html/HTMLMediaElement.cpp:4692 (Diff revision 2) > if (UseAudioChannelAPI()) { we should dispatch the event only if playInternal succeeds. ::: dom/html/HTMLMediaElement.cpp:4803 (Diff revision 2) > + bool aSuspended) This must be aMuted. For the audio tab icon we are muting the elements, not suspending them.
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. https://reviewboard.mozilla.org/r/32423/#review31545 What about b2g here? ::: dom/audiochannel/AudioChannelService.h:178 (Diff revision 2) > - , mMuted(IsAudioChannelMutedByDefault()) > + , mSuspended(false) what about b2g?
Attachment #8712048 - Flags: review?(amarchesini)
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. https://reviewboard.mozilla.org/r/32425/#review31547 ::: dom/browser-element/mochitest/browserElement_AudioChannelMutedByDefault.js:52 (Diff revision 2) > - ac.getMuted().onsuccess = function(e) { > + ac.getVolume().onsuccess = function(e) { Why this change? I mean, what does getMuted() return?
Attachment #8712049 - Flags: review?(amarchesini)
(In reply to Andrea Marchesini (:baku) from comment #12) > ::: dom/html/HTMLMediaElement.h:1543 > (Diff revision 2) > > + bool mSuspendedBySystemOrRemoteControl; > > There is some confusion here between mSuspendedBySystemOrRemoteControl and > MUTED_BY_CHANNEL. Why we have 2 different (but similar) concepts? The suspending would stop loading but muting won't. When mSuspendedBySystemOrRemoteControl is true, the media element is equal to call pause(). But the MUTED_BY_CHANNEL is called by tab indicator, it only affects the volume of the media element. > ::: dom/html/HTMLMediaElement.cpp:4674 > (Diff revision 2) > > + SetMutedInternal(mMuted | MUTED_BY_AUDIO_CHANNEL); > > Hold on, we can set the volume to 0, without muting the channel. Muting the > channel means that we stop the loading. We don't want that, right? No, muting shouldn't affect loading, it should only affect the volume. I change the muting behavior from "muted=true" to "volume=0". See below. > ::: dom/html/HTMLMediaElement.cpp:4803 > (Diff revision 2) > > + bool aSuspended) > > This must be aMuted. For the audio tab icon we are muting the elements, not > suspending them. I remove |aMuted| because changing volume and muting is similar, they're all modify the volume but not affect the loading. We can only use one parameter to do these two behaviors, so the second input parameter can be used to "totally suspend". Or you think I should use three parameters (volume, mute, suspend)? In AudioChannelService::GetState(), the code should be like that, so the functionality of the tab indicator is the same, it still mute the tab instead of suspend it. (but I forgot to add it in this patch, it would be in the next patch) > *aVolume = (window->GetAudioMuted())? 0.0 : window->GetAudioVolume(); > *aMuted = *aMuted > ::: dom/html/HTMLMediaElement.cpp:4691 > (Diff revision 2) > > + rv = PlayInternal(nsContentUtils::IsCallerChrome()); > > Maybe this element is muted for other reasons. We should not call > PlayInternal but use SuspendOrResultElement. > Here is only called when we want to suspend the element, using SuspendOrResultElement can't trigger the element.onpaused() and the UI of video would be incorrect. The purpose of the suspend is used by bug1240423.
(In reply to Andrea Marchesini (:baku) from comment #13) > ::: dom/audiochannel/AudioChannelService.h:178 > (Diff revision 2) > > - , mMuted(IsAudioChannelMutedByDefault()) > > + , mSuspended(false) > > what about b2g? * in present b2g, we don't have "muted" behavior, only have "suspend". In b2g, the reason we want to suspend the MediaElement by default is to avoid two sound happens at the same time. In other words, the final goal is user don't want to sound mixing together, so the incoming sound should be heard after the previous sound stops. Therefore, what we need is "mute" instead of "suspend", we can set volume to zero, don't need to stop loading (we have some bugs is caused by stopping loading from audio channel API)
(In reply to Andrea Marchesini (:baku) from comment #14) > dom/browser-element/mochitest/browserElement_AudioChannelMutedByDefault.js:52 > (Diff revision 2) > > - ac.getMuted().onsuccess = function(e) { > > + ac.getVolume().onsuccess = function(e) { > > Why this change? I mean, what does getMuted() return? In comment16, what we need in b2g is muted (zero volume, not suspend) by default, so I check the volume in the beginning. In patch2, I change the |aMuted| to |aSuspend|, so that |getMuted()| is to get the suspend state and |setMuted()| is to suspend the MediaElement.
Hi, baku, Do you feel well for above comments? If you think this idea can continue going on, I'll update the new patches and ask for review again. Thanks :)
Flags: needinfo?(amarchesini)
Blocks: 1249579
[Blocking reason for bug1249579] For the audio competing in Fennec, we need the method to pause the MediaElement instead suspend its internal state. It's a serious problem if we use the present AudioChannel API to manage audio competing. Thinking about the following case, (1) Use Fennec to open a website which is producing audible sound (ex. bandcamp) - Fennec gets Android audio focus (2) Open arbitrary android app, and then start to play music (ex. spotify) - Another app gets Android audio focus, Fennec loses the audio focuse, so it should be paused. (by API) (3) Open Fennec again, and notice the its media contorl UI state - The expected situation is the UI state shows "play button", and users can resume it by themselves In step3, we have two problems, (a) wrong UI state the audio UI state is wrong, it's already described in comment0. (b) can't playback audio In this case we don't need to resume the audio automatically, the audio should be resumed by user's intention, that means to press the play button again. However, we already suspend the audio in step(2), so any audio in this *window* can't be playback, and we don't have any trigger point to resume it at that time. Even if we use the present API to resume it, but we can't only resume the window but keep the audio as pausing state. Therefore, the result is the audio can't be playback again when the user press play button. * Note : b2g would automatically resume the audio when the webpage returns to the foreground, but it's not the perfect result. The audio should be done by users themselves instead of depending on the system.
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/2-3/
Attachment #8712047 - Attachment description: MozReview Request: Bug 1242874 - part1 : use AudioChannelAPI to directly control the MediaElement. → MozReview Request: Bug 1242874 - part1 : pause or play MediaElement from remote.
Attachment #8712047 - Flags: review?(amarchesini)
Attachment #8712048 - Attachment description: MozReview Request: Bug 1242874 - part2 : implement tab-muting by changing window's volume. → MozReview Request: Bug 1242874 - part2 : rename muted to suspended.
Attachment #8712048 - Flags: review?(amarchesini)
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/2-3/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/2-3/
Attachment #8712049 - Flags: review?(amarchesini)
Let me explain the purpose of these patches more detailedly, --- (1) The confused name and usage In WindowVolumeChanged(), the second parameter is named as mute, but we have totally different behaviors depend on what platform we are on. In b2g, we use this parameter to "suspend", and use to "mute" on other platforms. It's very confused! Same for the method setMuted(), the actual usage is to suspend audio. In addition, the mute functionality is not done by BrowserElementAPI, it's controlled from the nsPIDOMWindowOuter. It's no reason to use "mute" name in BrowserElementAPI, because functionality is fulfilled by nsPIDOMWindowOuter. The mute implement can be done in ACS::GetState() and no need to show in the name. Therefore, we should make name and usage consistent, these name should be rename to "suspend-related". --- (2) Wrong UI state In general, the UI state is depend on the "play/pause" events from MediaElement. (ex. Music app) In present design, the UI state would keep showing as playing when we suspend the media, because we don't touch the actual play()/pause() methods in MediaElement. In bug1249579, I would implement the audio focus in Fennec, it's very important to keep the UI state correctly. You can see some other details in comment19. -- (3) Remote media control Remote control should be same as to press the control UI directly. Therefore, we need to use play()/pause() to control every channels. -- (4) About "dom.audiochannel.mutedByDefault" As its name, the audio would be muted (volume=0) at beginning if you turn this pref on. This is used to prevent the sound mixing together with other incoming sound in b2g. After applying these patches, the situation in b2g would be like that, > (a) start audio > (b) no sound in the beginning [volume=0] > (c) request audio focus to system app > (d) unmuted by system app [using setVolume(1.0)] We can also open it on other platforms if there are bothering by the same problem.
No longer blocks: 1249579
See Also: → 1249579
Flags: needinfo?(amarchesini)
Attachment #8712047 - Flags: review?(amarchesini)
Attachment #8712048 - Flags: review?(amarchesini)
Attachment #8712049 - Flags: review?(amarchesini)
Now I'm working on the new patches which was mentioned from bug1187778 comment47. I would ask for a review after finishing them.
Component: AudioChannel → Audio/Video: Playback
Product: Firefox OS → Core
OS: Gonk (Firefox OS) → All
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/3-4/
Attachment #8712047 - Attachment description: MozReview Request: Bug 1242874 - part1 : pause or play MediaElement from remote. → MozReview Request: Bug 1242874 - part1 : create suspened types
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/3-4/
Attachment #8712048 - Attachment description: MozReview Request: Bug 1242874 - part2 : rename muted to suspended. → MozReview Request: Bug 1242874 - part2 : window's suspend attribute.
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/3-4/
Attachment #8712049 - Attachment description: MozReview Request: Bug 1242874 - part3 : modify tests. → MozReview Request: Bug 1242874 - part3 : implement different suspended methods.
Attachment #8712047 - Flags: review?(amarchesini)
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/4-5/
Attachment #8712048 - Flags: review?(amarchesini)
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/4-5/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/4-5/
Attachment #8712049 - Flags: review?(amarchesini)
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/1-2/
Attachment #8732791 - Flags: review?(amarchesini)
Attachment #8732792 - Flags: review?(amarchesini)
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/1-2/
https://reviewboard.mozilla.org/r/32425/#review39707 ::: dom/audiochannel/AudioChannelService.cpp:361 (Diff revision 5) > -void > -AudioChannelService::GetState(nsPIDOMWindowOuter* aWindow, uint32_t aAudioChannel, > - float* aVolume, bool* aMuted) > +WindowMediaConfig > +AudioChannelService::GetMediaConfig(nsPIDOMWindowOuter* aWindow, > + uint32_t aAudioChannel) const > { > MOZ_ASSERT(!aWindow || aWindow->IsOuterWindow()); > MOZ_ASSERT(aVolume && aMuted); This assertion isn't need now that results are returned via WindowMediaConfig and not aVolume, aMuted pointers.
Blocks: 1262053
Blocks: 1265981
Blocks: 1257738
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/5-6/
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/5-6/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/5-6/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/2-3/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/2-3/
Rebase all patches after landing bug 1249579.
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types https://reviewboard.mozilla.org/r/32421/#review45401 ::: dom/audiochannel/nsIAudioChannelAgent.idl:22 (Diff revision 6) > + * - remote media control (Fennec) > + * - block auto-play video in non-active page > + * > + * Note : the "remote side" means that controls nsIAudioChannelAgentCallback > + * via its callback function instead of using the play/pause button on the > + * control interface from webpage itself. Note: the "remote side" must control the AudioChannelAgent using nsIAudioChannelAgentCallback.windowSuspendChanged() callback instead using play/pause methods or any button in the webpage. ::: dom/audiochannel/nsIAudioChannelAgent.idl:26 (Diff revision 6) > + * via its callback function instead of using the play/pause button on the > + * control interface from webpage itself. > + * > + * - SUSPENDED_PAUSE : > + * It's used when transiently losing audio focus, the media can't be resumed > + * until we gain the audio focus again. It would Dispatch JS event when being "It dispatches a JS event". but which JS event? What's the name? ::: dom/audiochannel/nsIAudioChannelAgent.idl:32 (Diff revision 6) > + * suspended/resumed. > + * > + * - SUSPENDED_BLOCK > + * It's used to prevent auto-playing media in inactive page in order to > + * reduce the power consumption, and the media can't be resumed until the > + * page becomes active. Doesn't dispatch JS event. ... becomes active again. It doesn't dispatch any JS event. ::: dom/audiochannel/nsIAudioChannelAgent.idl:37 (Diff revision 6) > + * page becomes active. Doesn't dispatch JS event. > + * > + * - SUSPENDED_PAUSE_DISPOSABLE > + * It's used for remote media-control to pause the playing media and when we > + * lose audio focus permanently. It's disposable suspended, so the media can > + * be resumed arbitrary after that. Dispatch JS event. It dispatches a JS event. Again write the name of this event. ::: dom/audiochannel/nsIAudioChannelAgent.idl:41 (Diff revision 6) > + * lose audio focus permanently. It's disposable suspended, so the media can > + * be resumed arbitrary after that. Dispatch JS event. > + * > + * - SUSPENDED_STOP_DISPOSABLE > + * It's used for remote media-control to stop the playing media. The remote > + * control would be disappear after stop the media, so we would disconnect would disappear after stopping the media. ::: dom/audiochannel/nsIAudioChannelAgent.idl:43 (Diff revision 6) > + * > + * - SUSPENDED_STOP_DISPOSABLE > + * It's used for remote media-control to stop the playing media. The remote > + * control would be disappear after stop the media, so we would disconnect > + * the audio channel agent. It's disposable suspended, so the media can be > + * resumed arbitrary after that. Dispatch JS event. It dispatches the JS event... ::: dom/audiochannel/nsIAudioChannelAgent.idl:64 (Diff revision 6) > void windowVolumeChanged(in float aVolume, in bool aMuted); > > - /** > + /** > + * Notified when the window needs to be suspended or resumed. > + */ > + void windowSuspendedChanged(in uint32_t aSuspend); windowSuspendChanged ? ::: dom/fmradio/FMRadio.cpp:472 (Diff revision 6) > // TODO: what about the volume? > return NS_OK; > } > > NS_IMETHODIMP > +FMRadio::WindowSuspendedChanged(SuspendedTypes aSuspend) nsSuspendedTypes
Attachment #8712047 - Flags: review?(amarchesini) → review+
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. https://reviewboard.mozilla.org/r/32423/#review45403 1. rename nsSuspendTypes to SuspendTypes 2. you must have a firefox peer to review the toolkit/content/* ::: dom/interfaces/base/nsIDOMWindowUtils.idl:52 (Diff revision 6) > interface nsITranslationNodeList; > interface nsIJSRAIIHelper; > interface nsIContentPermissionRequest; > interface nsIObserver; > > -[scriptable, uuid(46b44e33-13c2-4eb3-bf80-76a4e0857ccc)] > +[scriptable, uuid(81f63a84-93cf-4510-b346-32926024ecf8)] no needs to change this anymore. ::: toolkit/content/browser-content.js:742 (Diff revision 6) > + handleMediaControlMessage(msg) { > + let utils = global.content.QueryInterface(Ci.nsIInterfaceRequestor) > + .getInterface(Ci.nsIDOMWindowUtils); > + let suspendTypes = Ci.nsISuspendedTypes; > + > + dump("AudioPlaybackListener, handleMediaControlMessage, msg = " + msg + "\n"); remove this. ::: toolkit/content/widgets/browser.xml:737 (Diff revision 6) > > + <method name="pauseMedia"> > + <parameter name="disposable"/> > + <body> > + <![CDATA[ > + var suspendedReason; let
Attachment #8712048 - Flags: review?(amarchesini) → review+
Attachment #8712049 - Flags: review?(amarchesini) → review+
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. https://reviewboard.mozilla.org/r/32425/#review45405 To me it's ok but you must have a media peer to review the HTMLMediaElement part. ::: dom/audiochannel/AudioChannelService.h:39 (Diff revision 6) > #define NUMBER_OF_AUDIO_CHANNELS (uint32_t)AudioChannel::EndGuard_ > > +class WindowMediaConfig > +{ > +public: > + explicit WindowMediaConfig(float aVolume, bool aMuted, no explicit ::: dom/audiochannel/AudioChannelService.h:100 (Diff revision 6) > > /** > * Return the state to indicate this audioChannel for his window should keep > - * playing/muted. > + * playing/muted/suspended. > */ > - void GetState(nsPIDOMWindowOuter* aWindow, uint32_t aChannel, > + WindowMediaConfig GetMediaConfig(nsPIDOMWindowOuter* aWindow, ditto ::: dom/html/HTMLMediaElement.cpp:5146 (Diff revision 6) > + } > +} > + > +void > +HTMLMediaElement::ResumeFromAudioChannelPaused(nsSuspendedTypes aSuspend) > +{ MOZ_ASSERT(mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE || mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_PAUSE_DISPOSABLE); ::: dom/html/HTMLMediaElement.cpp:5148 (Diff revision 6) > + > +void > +HTMLMediaElement::ResumeFromAudioChannelPaused(nsSuspendedTypes aSuspend) > +{ > + SetAudioChannelSuspended(nsISuspendedTypes::NONE_SUSPENDED); > + nsresult rv = PlayInternal(nsContentUtils::IsCallerChrome()); Should you check if we have been paused/stopped by something else? ::: dom/html/HTMLMediaElement.cpp:5157 (Diff revision 6) > + DispatchAsyncEvent(NS_LITERAL_STRING("mozinterruptend")); > +} > + > +void > +HTMLMediaElement::ResumeFromAudioChannelBlocked() > +{ MOZ_ASSERT(mAudioChannelSuspended == nsISuspendedTypes::SUSPENDED_BLOCK); ::: dom/webidl/HTMLMediaElement.webidl:178 (Diff revision 6) > [Pref="media.useAudioChannelService.testing"] > readonly attribute double computedVolume; > [Pref="media.useAudioChannelService.testing"] > readonly attribute boolean computedMuted; > + [Pref="media.useAudioChannelService.testing"] > + readonly attribute unsigned long computedSuspended; This is not needed for this patch. Move it in the next one.
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. https://reviewboard.mozilla.org/r/41361/#review45407 ::: dom/audiochannel/AudioChannelAgent.cpp:223 (Diff revision 3) > MOZ_LOG(AudioChannelService::GetAudioChannelLog(), LogLevel::Debug, > ("AudioChannelAgent, NotifyStartedPlaying, this = %p, " > "mute = %d, volume = %f, suspended = %d\n", this, > config.mMuted, config.mVolume, config.mSuspended)); > > - *aVolume = config.mVolume; > + nsCOMPtr<nsIAudioChannelAgentCallback> callback = GetCallback(); I think it's time to make the callback mandatory. Any reason why we can have callback == nullptr here? ::: dom/audiochannel/nsIAudioChannelAgent.idl:86 (Diff revision 3) > * > * The agent will invoke a callback to notify Gecko components of > * 1. Changes to the playable status of this channel. > */ > > -[uuid(ab7e21c0-970c-11e5-a837-0800200c9a66)] > +[uuid(f494b338-7a73-4f5c-a1aa-c8dd007c9679)] no needs to change this. ::: dom/audiochannel/nsIAudioChannelAgent.idl:147 (Diff revision 3) > void initWithWeakCallback(in mozIDOMWindow window, in long channelType, > in nsIAudioChannelAgentCallback callback); > > /** > * Notify the agent that we want to start playing. > - * Note: Gecko component SHOULD call this function first then start to > + * Note: Gecko component MUST call this function after they started playing. Gecko components ... or 'it starts' ::: dom/audiochannel/nsIAudioChannelAgent.idl:149 (Diff revision 3) > - * muted state: the agent has registered with audio channel service but > - * the component should not start playback. > - * faded state: the agent has registered with audio channel service the > - * component should start playback as well as reducing the volume. > */ > - void notifyStartedPlaying(out float volume, out bool muted); > + void notifyStartedPlaying(); I don't like this. Doing this you are making so that the callback is called from this method synchronously but it's also called asynchronously when the volume changed for other reasons. This can create errors on other codes and it will be hard to follow for other developers. I prefer to keep the old approach.
Attachment #8732791 - Flags: review?(amarchesini)
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. https://reviewboard.mozilla.org/r/41363/#review45409
Attachment #8732792 - Flags: review?(amarchesini) → review+
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/6-7/
Attachment #8732791 - Attachment description: MozReview Request: Bug 1242874 - part4 : trigger callbacks in NotifyStartedPlaying → MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying.
Attachment #8732792 - Attachment description: MozReview Request: Bug 1242874 - part5 : add test. → MozReview Request: ug 1242874 - part5 : add test.
Attachment #8712048 - Flags: review?(ehsan)
Attachment #8712049 - Flags: review?(jwwang)
Attachment #8732791 - Flags: review?(amarchesini)
Attachment #8732792 - Flags: review?(ehsan)
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/6-7/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/6-7/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/3-4/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/3-4/
These patches provide a way to achieve different suspended behaviors. Eg. It would be used on remote-media control (bug 1240423) and blocking auto-play media (bug 1262053). --- Hi, Ehsan, Could you help me review the codes under "toolkit/content/*"? Very appreciate your help! -- Hi, JW, Could you help me review the changing part of MediaElement? Very appreciate your help!
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. https://reviewboard.mozilla.org/r/32423/#review45759
Attachment #8712048 - Flags: review?(ehsan) → review+
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. https://reviewboard.mozilla.org/r/41363/#review45761
Attachment #8732792 - Flags: review?(ehsan) → review+
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. https://reviewboard.mozilla.org/r/32425/#review45779 ::: dom/html/HTMLMediaElement.cpp:5207 (Diff revision 7) > +} > + > +bool > +HTMLMediaElement::IsSuspendedByAudioChannel() const > +{ > + return (mAudioChannelSuspended & nsISuspendedTypes::SUSPENDED_PAUSE || The usage of nsISuspendedTypes::SUSPENDED* is inconsistent. Are they bit flags or distinct enum values? ::: dom/html/HTMLMediaElement.cpp:5231 (Diff revision 7) > + false); > +#endif > + return false; > + } > + > + // The MediaElement can start playback until it's resumed by audio channel. s/can/can't/
Attachment #8712049 - Flags: review?(jwwang) → review+
(In reply to JW Wang [:jwwang] from comment #55) > ::: dom/html/HTMLMediaElement.cpp:5207 > (Diff revision 7) > > +} > > + > > +bool > > +HTMLMediaElement::IsSuspendedByAudioChannel() const > > +{ > > + return (mAudioChannelSuspended & nsISuspendedTypes::SUSPENDED_PAUSE || > > The usage of nsISuspendedTypes::SUSPENDED* is inconsistent. Are they bit > flags or distinct enum values? > It should be used for distinct enum value, I would modify it.
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/7-8/
Attachment #8732792 - Attachment description: MozReview Request: ug 1242874 - part5 : add test. → MozReview Request: bug 1242874 - part5 : add test.
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/7-8/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/7-8/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/4-5/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/4-5/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/5-6/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/8-9/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/5-6/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/6-7/
Blocks: 1235612
Attachment #8732791 - Flags: review?(amarchesini) → review+
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. https://reviewboard.mozilla.org/r/41361/#review45983 ::: dom/audiochannel/AudioChannelAgent.cpp:207 (Diff revision 6) > } > > -NS_IMETHODIMP AudioChannelAgent::NotifyStartedPlaying(float *aVolume, > - bool* aMuted) > +NS_IMETHODIMP > +AudioChannelAgent::NotifyStartedPlaying(AudioPlaybackConfig* aConfig) > { > - MOZ_ASSERT(aVolume); > + MOZ_ASSERT(aConfig); if (NS_WARN_IF(!aConfig)) { return NS_ERROR_FAILURE; } ::: dom/audiochannel/nsIAudioChannelAgent.idl:60 (Diff revision 6) > const uint32_t SUSPENDED_STOP_DISPOSABLE = 4; > }; > > +%{C++ > +namespace mozilla { > +namespace dom { Maybe write a comment saying where this is defined. ::: dom/fmradio/FMRadio.cpp:456 (Diff revision 6) > FMRadio::EnableAudioChannelAgent() > { > NS_ENSURE_TRUE_VOID(mAudioChannelAgent); > > - float volume = 0.0; > - bool muted = true; > + AudioPlaybackConfig config; > + mAudioChannelAgent->NotifyStartedPlaying(&config); just for fun: rv = mAudioChannelAgent->NotifyStartedPlaying(&config); MOZ_ASSERT(NS_SUCCEEDED(rv)); ::: dom/html/HTMLMediaElement.cpp:5063 (Diff revision 6) > // this method has some content JS in its stack. > AutoNoJSAPI nojsapi; > > if (aPlaying) { > - float volume = 0.0; > - bool muted = true; > + AudioPlaybackConfig config; > + mAudioChannelAgent->NotifyStartedPlaying(&config); same assertion here. ::: dom/media/webspeech/synth/nsSpeechTask.cpp:717 (Diff revision 6) > mAudioChannelAgent->InitWithWeakCallback(mUtterance->GetOwner(), > static_cast<int32_t>(AudioChannelService::GetDefaultAudioChannel()), > this); > - float volume = 0.0f; > - bool muted = true; > - mAudioChannelAgent->NotifyStartedPlaying(&volume, &muted); > + > + AudioPlaybackConfig config; > + mAudioChannelAgent->NotifyStartedPlaying(&config); same assertion here. ::: dom/plugins/base/nsNPAPIPlugin.cpp:2295 (Diff revision 6) > return NPERR_NO_ERROR; > } > } else { > - float volume = 0.0; > - bool muted = true; > - rv = agent->NotifyStartedPlaying(&volume, &muted); > + > + dom::AudioPlaybackConfig config; > + agent->NotifyStartedPlaying(&config); same assertion ::: dom/telephony/Telephony.cpp:564 (Diff revision 6) > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; > } > } else if (!activeCall.IsNull() && !mIsAudioStartPlaying) { > mIsAudioStartPlaying = true; > - float volume; > + AudioPlaybackConfig config; same assertion
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/8-9/
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/8-9/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/9-10/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/6-7/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/7-8/
has conflicts Hunk #1 FAILED at 347 1 out of 3 hunks FAILED -- saving rejects to file dom/audiochannel/AudioChannelService.cpp.rej patch failed to apply abort: fix up the working directory and run hg transplant --continue
Flags: needinfo?(alwu)
Keywords: checkin-needed
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/9-10/
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/9-10/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/10-11/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/7-8/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/8-9/
Flags: needinfo?(alwu)
Keywords: checkin-needed
Comment on attachment 8712047 [details] MozReview Request: Bug 1242874 - part1 : create suspened types Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32421/diff/10-11/
Comment on attachment 8712048 [details] MozReview Request: Bug 1242874 - part2 : window's suspend attribute. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32423/diff/10-11/
Comment on attachment 8712049 [details] MozReview Request: Bug 1242874 - part3 : implement different suspended methods. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32425/diff/11-12/
Comment on attachment 8732791 [details] MozReview Request: Bug 1242874 - part4 : wrap the volume/mute/suspend for notifyStartedPlaying. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41361/diff/8-9/
Comment on attachment 8732792 [details] MozReview Request: bug 1242874 - part5 : add test. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/41363/diff/9-10/
Keywords: checkin-needed
Depends on: 1271697
Depends on: 1276119
Sai: this needs update in the B2G OS documentation on MDN
Flags: needinfo?(kskarthik91)
Keywords: dev-doc-needed
Flags: needinfo?(kskarthik91) → needinfo?(alwu)
Hi, what do you want to know for this NI?
Flags: needinfo?(alwu) → needinfo?(kskarthik91)
Flags: needinfo?(kskarthik91)
(In reply to Jean-Yves Perrier [:teoli] from comment #91) > Sai: this needs update in the B2G OS documentation on MDN Removing ddn. Not going to document the deprecated B2G AudioChannel API anymore.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: