*Environment: Gaia: 6724eb7733dd425b65027d0c2fd414bc9b74d624 B-D 2013-07-14 00:31:48 Gecko: http://hg.mozilla.org/mozilla-central/rev/18467a85acf6 BuildID 20130714030201 Version 25.0a1 pvt master build with manually pushed audio.conf adb remount adb push B2G/system/bluetooth/data/audio.conf /etc/bluetooth/audio.conf *How to reproduce: 1. Launch "Settings" app 2. Go to "bluetooth" subsection 3. Turn on bluetooth 4. Pair with a A2DP-supported bluetooth earphones 5. Launch "Music" app 6. Play any music 7. You can hear it from earphone 8. Launch "browser" app 9. Go to youtube site 10. Play any video with sound *Expected Result: 1.You should hear only the later played video/music OR 2.It should mix clearly without sound on and off *Actual Result: Music is mixed with Youtube Video sound, and both sound come out on and off.

FirefoxOS UX team, Can you help define the right behavior? Or, can you get someone who know more about this? I suggest that the later played video/music should has its sound, and the previous played video/music should pause&mute.

Music => Content channel Browser -> Web Activity -> View Youtube via Video app running as inline activity => Content channel They should conquer each other by last time page visibility switches. Does this only occurs under BT A2DP connecting?

This is a regression. Browser shouldn't play youtube video directly, otherwise it would always using normal channel instead of content channel, thus it couldn't interrupt background music. Jason, Walter told me you may know this change.

This is also happen when we played without BT A2DP. Also, reference bug: https://bugzilla.mozilla.org/show_bug.cgi?id=887454 I actually don't know this change before I found this. Kind of strange that there is no notification on this change.

(In reply to Alive Kuo [:alive] from comment #3) > This is a regression. > Browser shouldn't play youtube video directly, otherwise it would always > using normal channel instead of content channel, thus it couldn't interrupt > background music. > > Jason, Walter told me you may know this change. bug 887454 removed the UA override for YouTube such that we render h264 media content directly in the mobile app/site directly now in alignment with YouTube's needs. We're no longer going to render YouTube videos within the Gaia Video application as a result. (In reply to Walter Chen from comment #4) > This is also happen when we played without BT A2DP. > > Also, reference bug: > https://bugzilla.mozilla.org/show_bug.cgi?id=887454 > > I actually don't know this change before I found this. Kind of strange that > there is no notification on this change. There was notification of change - I emailed the QA team when this landed along the implications of what the change implied. There's also ongoing thread with a large cc list on the status of getting YouTube working in alignment with YouTube's certification process for 1.1.

Also - Is the problem in this bug possible to reproduce on b2g18 as well?

Now the problem is that anything in browser is normal channel by default, which means it couldn't interrupt playing content. If we could change the audio competing policy, and split: * Background playing * Interrupt other content into 2 different parts, and then make browser has permission to interrupt but no background playing... Andrea, thought? Briefly: The youtube before was running in a web activity in video app, which uses content channel. But now it's in browser natively so it couldn't interrupt any background content again.

Donovan - Can you find out if this is a blocker for YouTube certification or a severe issue from the content team's perspective for 1.1?

I will find out. Isn't the solution to this simple, though? Shouldn't the browser app just use the content channel? It seems like these audio problems would affect any website which uses the <audio> or <video> tags, not just YouTube.

(In reply to Donovan Preston from comment #9) > I will find out. > > Isn't the solution to this simple, though? Shouldn't the browser app just > use the content channel? It seems like these audio problems would affect any > website which uses the <audio> or <video> tags, not just YouTube. mozAudioChannel is implemented on media element, it's not an app attribute. Not-that-easy---Make default channel as content would cause us big trouble, I do think.

(In reply to Alive Kuo [:alive] from comment #10) > (In reply to Donovan Preston from comment #9) > > I will find out. > > > > Isn't the solution to this simple, though? Shouldn't the browser app just > > use the content channel? It seems like these audio problems would affect any > > website which uses the <audio> or <video> tags, not just YouTube. > > mozAudioChannel is implemented on media element, it's not an app attribute. > Not-that-easy---Make default channel as content would cause us big trouble, > I do think. Or could we add a new attribute into manifest for declaring that my app's default audio channel is XXX? Then browser can set it to "content". What we concern about this is 1. browser will be allowed to play in the background. (This seems to be what we want for content provider too.) 2. the other content channel on playing and in the background will be paused if there is any audio/video played by browser. 2.a If a page play a short sound or alert sound, do we really want to pause music in the backgound? (trade off?)

(In reply to Marco Chen [:mchen] from comment #11) > (In reply to Alive Kuo [:alive] from comment #10) > > (In reply to Donovan Preston from comment #9) > > > I will find out. > > > > > > Isn't the solution to this simple, though? Shouldn't the browser app just > > > use the content channel? It seems like these audio problems would affect any > > > website which uses the <audio> or <video> tags, not just YouTube. > > > > mozAudioChannel is implemented on media element, it's not an app attribute. > > Not-that-easy---Make default channel as content would cause us big trouble, > > I do think. > > Or could we add a new attribute into manifest for declaring that my app's > default audio channel is XXX? Then browser can set it to "content". Disagree if we don't resolve the following issues. My draft idea is to change the implementation detail of audio competing: Decouple competing and background play and finetune video element behavior. * Video element natively competes with others. * But only content channel has background play permission. (We may pause it by default from AudioChannelAgent when a page with video goes to background) * Audio element competes if it's has content channel (or higher) * Content audio element decides on its own what to do when go to background. * Normal audio would be paused. > > What we concern about this is > 1. browser will be allowed to play in the background. (This seems to be > what we want for content provider too.) > 2. the other content channel on playing and in the background will be > paused if there is any audio/video played by browser. > 2.a If a page play a short sound or alert sound, do we really want to > pause music in the backgound? (trade off?)

(In reply to Alive Kuo [:alive] from comment #12) > My draft idea is to change the implementation detail of audio competing: > Decouple competing and background play and finetune video element behavior. > * Video element natively competes with others. It sounds feasible to me. One possible side effect here: 1. User played a music in the background. 2. User surf the internet via browser. 3. User hits a page within a video tag and set as autoplay. (advertisement or other small effects). 4. Then background music is paused. If this is also acceptable then maybe we can try this idea.

(In reply to Marco Chen [:mchen] from comment #13) > (In reply to Alive Kuo [:alive] from comment #12) > > My draft idea is to change the implementation detail of audio competing: > > Decouple competing and background play and finetune video element behavior. > > * Video element natively competes with others. > > It sounds feasible to me. > One possible side effect here: > 1. User played a music in the background. > 2. User surf the internet via browser. > 3. User hits a page within a video tag and set as autoplay. (advertisement > or other small effects). > 4. Then background music is paused. > > If this is also acceptable then maybe we can try this idea. I personally think this is a small side effect enough to be ignored.

iOS has a behavior where if music is playing in the background and an alert sound is played (such as a mail arrived sound), the music is ducked (the volume of the background music is pulled down smoothly while the alert plays, and then volume is pulled back up).

(In reply to Donovan Preston from comment #15) > iOS has a behavior where if music is playing in the background and an alert > sound is played (such as a mail arrived sound), the music is ducked (the > volume of the background music is pulled down smoothly while the alert > plays, and then volume is pulled back up). That would be the thing on 823273 but not the case here.

> I personally think this is a small side effect enough to be ignored. I agree. Patch is coming. Then... as followup we can restart music if the played audio was shorter than 1 secs.. maybe.

Sorry, I cannot work on this. I have 2 weeks off.

needsinfo on mchen to help find an assignee here given :baku will be out.

Will take care this on next Monday.

As discuss with CJ, Randy or JW may help on this issue. Thanks.

+ JW in cc and discuss how to solve this problem also.

Let media tag with video can playback with content audio channel. Test and it can suspend when press power key. With music playback with background. it also can suspend the music while playing the video.

Comment on attachment 786771 [details] [diff] [review] patch v1 Review of attachment 786771 [details] [diff] [review]: ----------------------------------------------------------------- Originally I think this idea is a possible solution but this seems to break something 1. Security Check: Browser app didn't have permission of content channel but we do it inside the Gecko. 2. Inconsistent of audio channel definition: We expect the default channel is normal but when we do "var channel = video.mozAudioChannelType" it showed "content." After we really set content into mozAudioChannelType, we got error returned. So I would say that 1. normal channel can't pause other channels and can't play in the background. 2. normal channel on video element can pause other content channels but can't play in the background. Then if we want to keep all policy in AudioChannelService then we should put rules there.

JW is working on this issue right now, let him take it.

Just found this patch: https://bugzilla.mozilla.org/attachment.cgi?id=741770&action=diff 'TreatNormalAsContent' is another way for a normal channel to interrupt background content channel.

Comment on attachment 791171 [details] [diff] [review] Part 1 - Add a channel type for video to interrupt background content channel Review of attachment 791171 [details] [diff] [review]: ----------------------------------------------------------------- It looks great with some small comments only. Thanks. ::: dom/audiochannel/AudioChannelService.cpp @@ +364,5 @@ > > + else if (!mChannelCounters[AUDIO_CHANNEL_INT_NORMAL_EXCLUSIVE].IsEmpty()) { > + // Treat AUDIO_CHANNEL_INT_NORMAL_EXCLUSIVE as AUDIO_CHANNEL_INT_NORMAL > + // when it comes to notification. > + higher = AUDIO_CHANNEL_NORMAL; Since you already added the mapping between AUDIO_CHANNEL_NORMAL_EXCLUSIVE and "normal", it seems there is no necessary to do this here. ::: dom/audiochannel/nsIAudioChannelAgent.idl @@ +30,5 @@ > * The agent will invoke a callback to notify Gecko components of > * 1. Changes to the playable status of this channel. > */ > > [scriptable, uuid(f012a9b7-6431-4915-a4ac-4ba7d833e28e)] Please bumping the uuid. @@ +36,5 @@ > { > const long AUDIO_AGENT_CHANNEL_NORMAL = 0; > + /** > + * A normal channel which will interrupt background content channel. > + * Used by <video> to stop background music. nit: remove the space.

By the way, I remember that we discussed two ways to implement this: 1. Add new channel type and decide in media element. 2. Expose an additional info from media element to audio channel service about whether this element is audio or video. IMO, I would like to choose option 2 and the key point is that To keep all policies/logical in AudioChannelService and AudioChannelAgent just provide suitable information. I want to avoid that policies/logical spread every where then it is hard to maintain and trace the code. May I know your thought? Thanks.

Approach 2 suggested by :mchen.

Approach 2 suggested by :mchen.

Hi jwwang, Which branch does your attachment will use? I can't find HTMLMediaElement.cpp in masters.

Comment on attachment 795252 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 795252 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for taking the suggestion. The only concern now is that there will be leaks in mWithVideoChildIDs when a content process is crashed and killed by card view. In that case AudioChannelAgent didn't follow the regular steps to un-register it so you need to handle it in AudioChannelService::Observe with "ipc:content-shutdown" topic. ::: dom/audiochannel/AudioChannelService.cpp @@ +421,5 @@ > mCurrentHigherChannel = higher; > > nsString channelName; > if (mCurrentHigherChannel != AUDIO_CHANNEL_LAST) { > + channelName.AssignASCII(ChannelName(mCurrentHigherChannel, aChildID)); Since we didn't change the channelName according to video property yet, we can save the change until we need it.

Comment on attachment 795252 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Hi Matthew, Could you help to review part of HTMLMediaElement? I am not sure that is the safe way to check media element is audio or video. Thanks.

I developed the patch on Mozilla-central. B2G seems to contain a quite old version of gecko which uses the old name nsHTMLMediaElement.cpp and the like.

(In reply to Marco Chen [:mchen] from comment #36) > Comment on attachment 795252 [details] [diff] [review] > Ability to associate video with a channel so that it can interrupt > background content channel. > > Hi Matthew, > > Could you help to review part of HTMLMediaElement? > I am not sure that is the safe way to check media element is audio or video. > > Thanks. mVideoFrameContainer could be null for video elements in some cases. Note that it's only initialized when GetVideoFrameContainer is first called after the media has reached a ready state of least HAVE_METADATA. You could either place code on each of the HTMLAudioElement and HTMLVideoElement subclasses, or use a QI like HTMLMediaElement::GetVideoFrameContainer does: 3152 // Only video frames need an image container. 3153 nsCOMPtr<nsIDOMHTMLVideoElement> video = do_QueryObject(this); 3154 if (!video) 3155 return nullptr;

I test your patch attachment 795252 [details] [diff] [review], find it doesn't works well.

What's your repro steps? It works on my device.

(In reply to jwwang from comment #40) > What's your repro steps? It works on my device. I launch music and make it play background, then lunch browser to play inline music, the local music doesn't pause.

(In reply to Ray REN from comment #41) > (In reply to jwwang from comment #40) > > What's your repro steps? It works on my device. > > I launch music and make it play background, then lunch browser to play > inline music, the local music doesn't pause. What's inline music? We're only trying to solve video case here.

(In reply to Alive Kuo [:alive] from comment #42) > > What's inline music? We're only trying to solve video case here. Sorry my mistake. "inline music" means play music by browser. The patch can stop local music in background when play video, but when the video ended or paused, the music doesn't re-play itself, is it OK?

(In reply to Ray REN from comment #43) > (In reply to Alive Kuo [:alive] from comment #42) > video ended or paused, the music doesn't re-play itself, is it OK? Yes, it is as what we designed. You can test background music with video app and the behavior is the same.

(In reply to Ray REN from comment #43) > (In reply to Alive Kuo [:alive] from comment #42) > > > > What's inline music? We're only trying to solve video case here. > > Sorry my mistake. "inline music" means play music by browser. > > The patch can stop local music in background when play video, but when the > video ended or paused, the music doesn't re-play itself, is it OK? Please open another bug if you don't think so. It's out of scope of this bug. BTW, my android doesn't resume music playing after watching youtube video on browser.

Use QueryInterface to detect video element.

Comment on attachment 796476 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796476 [details] [diff] [review]: ----------------------------------------------------------------- Hi JW, I didn't see this patch addressed points on comment 35. Could you give a new patch for that? Thanks. ::: content/html/content/src/HTMLMediaElement.cpp @@ +3784,5 @@ > // Use a weak ref so the audio channel agent can't leak |this|. > mAudioChannelAgent->InitWithWeakCallback(mAudioChannelType, this); > + > + nsCOMPtr<nsIDOMHTMLVideoElement> video = do_QueryObject(this); > + if (mAudioChannelType == AUDIO_CHANNEL_NORMAL && video) { Please explicitly use video.get() instead of video only. You can refer to the comment on the link as below. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#809

https://hg.mozilla.org/mozilla-central/file/416075f77249/dom/audiochannel/AudioChannelService.cpp#l516 It looks like the if statement should be moved out of the for loop. I will take comment 35 and submit a patch again. Thanks.

Hi jwwang, If I launch an app from everything.me to play video, the background music also playing,could you help check why?

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796509 [details] [diff] [review]: ----------------------------------------------------------------- r+ = me after clearing that one comment. This r+ is for AudioChannel area only. We still need reviewer from HTMLMediaElement and IPDL. ::: dom/audiochannel/AudioChannelService.cpp @@ +531,5 @@ > NS_WARNING("ipc:content-shutdown message without property bag as subject"); > return NS_OK; > } > > + int32_t index; Please put declaration to newest position of the place using it. ex: before line 549. @@ +549,5 @@ > + while ((index = mActiveContentChildIDs.IndexOf(childID)) != -1) { > + mActiveContentChildIDs.RemoveElementAt(index); > + } > + while ((index = mWithVideoChildIDs.IndexOf(childID)) != -1) { > + mWithVideoChildIDs.RemoveElementAt(index); Good catch. Thanks.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Hi Matthew, Could you help to review the part of HTMLMediaElement again? I think this version already addressed your comment.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Hi Ben, Could you help to review IPC part? thanks. The patch added one function into PContent.ipdl.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796509 [details] [diff] [review]: ----------------------------------------------------------------- I just looked at HTMLMediaElement.cpp changes. Please let me know if you expected me to review any other hunk. ::: content/html/content/src/HTMLMediaElement.cpp @@ +3784,5 @@ > // Use a weak ref so the audio channel agent can't leak |this|. > mAudioChannelAgent->InitWithWeakCallback(mAudioChannelType, this); > + > + nsCOMPtr<nsIDOMHTMLVideoElement> video = do_QueryObject(this); > + if (mAudioChannelType == AUDIO_CHANNEL_NORMAL && video.get()) { It's not a huge deal, but you don't need to call .get() here. (In reply to Marco Chen [:mchen] from comment #47) > Please explicitly use video.get() instead of video only. You can refer to > the comment on the link as below. > http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#809 Doesn't that effectively say "only use get() where operator T*() would be ambiguous"?

(In reply to Matthew Gregan [:kinetik] from comment #54) > (In reply to Marco Chen [:mchen] from comment #47) > > Please explicitly use video.get() instead of video only. You can refer to > > the comment on the link as below. > > http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsCOMPtr.h#809 > > Doesn't that effectively say "only use get() where operator T*() would be > ambiguous"? Hi JW, I think kinetik is right and sorry to give wrong comment on it. Hi kinetik, Thanks for correcting this.

(In reply to Matthew Gregan [:kinetik] from comment #54) > Comment on attachment 796509 [details] [diff] [review] > Ability to associate video with a channel so that it can interrupt > background content channel. > > Review of attachment 796509 [details] [diff] [review]: > ----------------------------------------------------------------- > > I just looked at HTMLMediaElement.cpp changes. Please let me know if you > expected me to review any other hunk. > Hi kinetik, If you are willing to review other parts, it is welcome to check and listen anyone's suggestion. Thanks.

Ben Please review the patch at the earliest.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796509 [details] [diff] [review]: ----------------------------------------------------------------- I only looked at the IPC parts, but I need an answer to the last point before I can approve this. ::: dom/audiochannel/AudioChannelServiceChild.cpp @@ +133,5 @@ > +AudioChannelServiceChild::SetWithVideo(AudioChannelAgent* aAgent, bool aWithVideo) > +{ > + MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Default); > + > + ContentChild *cc = ContentChild::GetSingleton(); Nit: * on the left. @@ +134,5 @@ > +{ > + MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Default); > + > + ContentChild *cc = ContentChild::GetSingleton(); > + if (cc) { This can't be null so no need to check. ::: dom/ipc/ContentParent.h @@ +414,5 @@ > > virtual bool RecvAudioChannelRegisterType(const AudioChannelType& aType); > virtual bool RecvAudioChannelUnregisterType(const AudioChannelType& aType, > const bool& aElementHidden); > + virtual bool RecvAudioChannelSetWithVideo(const bool& aWithVideo); Please add MOZ_OVERRIDE here (even though the others don't have it...) ::: dom/ipc/PContent.ipdl @@ +407,5 @@ > > sync AudioChannelRegisterType(AudioChannelType aType); > sync AudioChannelUnregisterType(AudioChannelType aType, > bool aElementHidden); > + sync AudioChannelSetWithVideo(bool aWithVideo); I don't understand why you need to make this synchronous. This means that the child process will block until the parent has processed this message... Is that really important somehow?

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #58) > Comment on attachment 796509 [details] [diff] [review] > > sync AudioChannelRegisterType(AudioChannelType aType); > > sync AudioChannelUnregisterType(AudioChannelType aType, > > bool aElementHidden); > > + sync AudioChannelSetWithVideo(bool aWithVideo); > > I don't understand why you need to make this synchronous. This means that > the child process will block until the parent has processed this message... > Is that really important somehow? Hi Bent, Thanks for your review comment. And according to one purpose of audoichannel is to check whether this audio element can play the audio in this moment or not so we set the function here as sync or we will get the mute result but audio tag already is playing.

(In reply to Marco Chen [:mchen] from comment #59) > And according to one purpose of audoichannel is to check whether this audio > element can play the audio in this moment or not so we set the function here > as sync or we will get the mute result but audio tag already is playing. I'm sorry, I don't really understand this. Can you please describe the difference in behavior that would result from making this message asynchronous?

Yes, it's our duty to let reviewer know any detail. The one of main purpose for applying AudioChannel to HTMLMediaElement is "before HTMLMediaElement starts to play, the Audiochannel needs to report whether it can play or should go to pause state first." Sync version: ask ask via PIC HTMLMediaElement -----> AudioChannel ------------------------> AudioChannelService | return | <------------------------------------------------------------ | can play or not Async version: 1. HTMLMediaElement is running on main thread only so this function is called by main thread. If I used async version, the main thread is still blocked. I am not really understand IPC too and please give us the suggestion if my thought is wrong. Thanks.

> Async version: > > 1. HTMLMediaElement is running on main thread only so this function is > called by main thread. > If I used async version, the main thread is still blocked. > > I am not really understand IPC too and please give us the suggestion if my > thought is wrong. > > Thanks. You means the below error log by leo device? ---------------------------------------------------------------------------------- [Child 2451] WARNING: pipe error (3): Connection reset by peer: file /home001/seil.park/d300_V1_1/b2g/gecko/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 435 ---------------------------------------------------------------------------------- We need this patch for resolving video leak. Can you share the schedule?

We are trying to solve an unsolvable problem here. The problem that we keep trying to solve is "there's lots of content on the web that uses audio. We sometimes want to let it play in the background, sometimes want to play exclusively, sometimes mix the audio, sometimes treat it as a music player, sometimes treat it as sound effects, sometimes treat it as ambient background audio. However all of the content is just written for the normal web and not for FirefoxOS, and we don't have enough marketshare for anyone to change their content to use APIs that we provide to decide how the audio should be treated. But we still want FirefoxOS to magically figure out what to do". Magically figuring out what to do in all situations, and doing it correctly, is obviously impossible. I'm very worried about creating a very complicated UX which is hard for users to understand and complicated for authors to use. So before we make more changes to behavior here I'd really like to have a comprehensive UX spec that we all agree to. I'm sure we'll have to iterate on that spec, but that's better than the current game of whack-a-mole. The fix that I had had in mind in the past is to add UI which allows users to select if they want a particular website to mix with background music or not. This would allow us to have less magic and keep things simple, but it does require more work from the user. The policy change in this bug actually seems pretty reasonable (though I didn't look at the implementation details), but I'd really rather get a proper UX spec before we make many more changes.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796509 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for chatting with me about this today! I'll be happy to review the next version. ::: dom/ipc/PContent.ipdl @@ +407,5 @@ > > sync AudioChannelRegisterType(AudioChannelType aType); > sync AudioChannelUnregisterType(AudioChannelType aType, > bool aElementHidden); > + sync AudioChannelSetWithVideo(bool aWithVideo); As discussed offline I think we can just pass the 'aWithVideo' argument to AudioChannelRegisterType.

Comment on attachment 796509 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 796509 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +531,5 @@ > NS_WARNING("ipc:content-shutdown message without property bag as subject"); > return NS_OK; > } > > + int32_t index; index is also used in the while statement (line#544).

Modified according to comment #64.

(In reply to Ray REN from comment #50) > Hi jwwang, > > If I launch an app from everything.me to play video, the background music > also playing,could you help check why? I can't repro the issue. Can you detail the repro steps?

Comment on attachment 802841 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 802841 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this is really close. One thing: ::: dom/audiochannel/nsIAudioChannelAgent.idl @@ +109,5 @@ > + * Used by <video> to stop background music. > + * > + * Note : This function should be called immediately after initXXX() above. > + */ > + void initWithVideo(); Is there any benefit from requiring the consumer to call two methods called Init in quick succession? I think this needs to be an argument to the init calls.

The original idea was not to break any existing javascript code especially when the API is already published. Adding a function is backward-compatible while adding a parameter is not. Actually I would prefer just adding a parameter which is more concise and coherent if there is no concern about API incompatibility.

Comment on attachment 802882 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 802882 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the IPC parts, thanks!

Comment on attachment 802882 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 802882 [details] [diff] [review]: ----------------------------------------------------------------- Hi JW, Thanks for your effort on this bug. And please help to address these comments then this bug will be done. ::: dom/audiochannel/AudioChannelService.cpp @@ +186,5 @@ > !mChannelCounters[AUDIO_CHANNEL_INT_CONTENT].Contains(aChildID)) { > mActiveContentChildIDs.RemoveElement(aChildID); > } > + > + mWithVideoChildIDs.RemoveElement(aChildID); How about to add this before this line? MOZ_ASSERT(mWithVideoChildIDs.Contains(aChildID)); ::: dom/audiochannel/nsIAudioChannelAgent.idl @@ +82,5 @@ > + * @param withVideo > + * true if channel is associated with video. > + */ > + [noscript] void initWithVideo(in long channelType, in nsIAudioChannelAgentCallback callback, > + in boolean weak, in boolean withVideo); If "withVideo" is false then it can be covered by existing functions now. InitWithWeakCallback Init so I think there is no necceary to add "withVideo" and this functioin will be used when video is true only. And may I know why do you add a keywork "noscript" here?

Comment on attachment 802882 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 802882 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +186,5 @@ > !mChannelCounters[AUDIO_CHANNEL_INT_CONTENT].Contains(aChildID)) { > mActiveContentChildIDs.RemoveElement(aChildID); > } > + > + mWithVideoChildIDs.RemoveElement(aChildID); mWithVideoChildIDs.Contains(aChildID) doesn't hold true when it is registered with [withVideo=false]. ::: dom/audiochannel/nsIAudioChannelAgent.idl @@ +82,5 @@ > + * @param withVideo > + * true if channel is associated with video. > + */ > + [noscript] void initWithVideo(in long channelType, in nsIAudioChannelAgentCallback callback, > + in boolean weak, in boolean withVideo); yeah, you are right. The prototype can be revised to void initWithVideo(in long channelType, in nsIAudioChannelAgentCallback callback, in boolean weak); [noscript] is to prevent this API change from propagating to Javascript users. We can always publish it to the public later when needed. However, it is much harder to revoke a published API without impacting existing code.

Comment on attachment 802882 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 802882 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/audiochannel/AudioChannelService.cpp @@ +186,5 @@ > !mChannelCounters[AUDIO_CHANNEL_INT_CONTENT].Contains(aChildID)) { > mActiveContentChildIDs.RemoveElement(aChildID); > } > + > + mWithVideoChildIDs.RemoveElement(aChildID); Consider to the case as below. 1. There are three media elements on one process. (one is video and two are audio.) 2. All media elements are on playing. 3. Then there will be only one aChildID in mActiveContentChildIDs array. 4. Once one of audios are stopped, that only one aChildID will be removed from mActiveContentChildIDs. But actually it is not a video. ::: dom/audiochannel/nsIAudioChannelAgent.idl @@ +82,5 @@ > + * @param withVideo > + * true if channel is associated with video. > + */ > + [noscript] void initWithVideo(in long channelType, in nsIAudioChannelAgentCallback callback, > + in boolean weak, in boolean withVideo); This is not a DOM Web API and it just a XPCOM interface. And the purpose of this component is to expose the access from JS XPCOM object. So we should remove [noscript].

(In reply to jwwang from comment #67) > I can't repro the issue. Can you detail the repro steps? Sorry, our mistake. It is OK for video.

Comment on attachment 804212 [details] [diff] [review] Ability to associate video with a channel so that it can interrupt background content channel. Review of attachment 804212 [details] [diff] [review]: ----------------------------------------------------------------- r=me on AudioChannel part after fixing the nits. Thanks. ::: content/html/content/src/HTMLMediaElement.cpp @@ +3787,2 @@ > // Use a weak ref so the audio channel agent can't leak |this|. > + if (withVideo) { We can put "mAudioChannelType == AUDIO_CHANNEL_NORMAL && video" here so save a variable. ::: dom/audiochannel/AudioChannelService.cpp @@ +138,5 @@ > nsAutoPtr<AudioChannelAgentData> data; > mAgents.RemoveAndForget(aAgent, data); > > if (data) { > + UnregisterType(data->mType, data->mElementHidden, CONTENT_PROCESS_ID_MAIN, data->mWithVideo); nits: chars more then 80. ::: dom/audiochannel/AudioChannelServiceChild.cpp @@ +110,5 @@ > AudioChannelAgentData data(*pData); > > AudioChannelService::UnregisterAudioChannelAgent(aAgent); > > + ContentChild::GetSingleton()->SendAudioChannelUnregisterType(data.mType, data.mElementHidden, data.mWithVideo); nits: chars more then 80 ::: dom/ipc/PContent.ipdl @@ +405,5 @@ > bool aElementWasHidden) > returns (bool value); > > + sync AudioChannelRegisterType(AudioChannelType aType, bool aWithVideo); > + sync AudioChannelUnregisterType(AudioChannelType aType, bool aElementHidden, bool aWithVideo); nits: chars more then 80.

This doesn't apply cleanly to b-i at all. Please rebase and re-request checkin.

I'm also going to assume that this will need some fixing up to land on b2g18, so you might as well attach a branch-specific patch while you're at it for easier uplift.

Btw, I can't find the repo for b2g18. Is it http://hg.mozilla.org/mozilla-b2g18 or something?

(In reply to jwwang from comment #83) > Btw, I can't find the repo for b2g18. Is it > http://hg.mozilla.org/mozilla-b2g18 or something? http://hg.mozilla.org/releases/mozilla-b2g18/

Needs a branch-specific patch for the b2g18 uplift. https://hg.mozilla.org/releases/mozilla-aurora/rev/274c79cdb759

John, Can we please uplift this? If this, hasn't already.

Preeti, see comment 87. This is waiting on a branch-specific patch from JW Wang.

backport to b2g18.

Please check in part1_b2g18.patch, thx.

Thanks :) https://hg.mozilla.org/releases/mozilla-b2g18/rev/18175c7fdaa6