Closed
Bug 1119936
Opened 9 years ago
Closed 9 years ago
[Audio Channel][Camera] Audio from FM Radio or Music app ceases to play when switching between front/back camera
Categories
(Firefox OS Graveyard :: AudioChannel, defect)
Tracking
(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.0 affected, b2g-v2.1 affected, b2g-v2.2 verified, b2g-master verified)
People
(Reporter: jmitchell, Assigned: alwu)
Details
(Keywords: verifyme, Whiteboard: [2.2-Daily-Testing])
Attachments
(3 files, 14 obsolete files)
1.48 KB,
patch
|
alwu
:
review+
|
Details | Diff | Splinter Review |
1.41 KB,
patch
|
bajaj
:
approval-mozilla-b2g37+
|
Details | Diff | Splinter Review |
4.57 MB,
video/mp4
|
Details |
Description: When playing music (from Music or Radio app) and going into the Camera app and then switching to front face camera will kill the audio. Repro Steps: 1) Update a Flame to 20150109010206 2) Play audio with Radio or Music app 3) Launch Camera app 4) Switch to front face camera Actual: The audio / music is killed Expected: The audio will continue to play Environmental Variables: Device: Flame Master Build ID: 20150109010206 Gaia: 5f0dd37917c4a6d8fa8724715d4d3797419f9013 Gecko: b3f84cf78dc2 Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76 Version: 37.0a1 (Master) Firmware Version: V18D User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Repro frequency: 5/5 See attached: logcat ------------------------------------------------------------------------- This issue also occurs on 2.2 (V188-1 Base) Device: Flame Master (KK - Nightly - full Flashed) Build ID: 20150109010206 Gaia: 5f0dd37917c4a6d8fa8724715d4d3797419f9013 Gecko: b3f84cf78dc2 Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76 Version: 37.0a1 (Master) Firmware Version: V188-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 -------------------------------------------------------------- In 2.1 audio is killed when entering the Camera app so can not be said to be affected or unaffected Device: Flame 2.1 (KK - Nightly - Full Flashed) Build ID: 20150108001214 Gaia: ed2e278753e8c9301ba322dcf2c3591f5928408d Gecko: 127a0ead5f83 Gonk: a814b2e2dfdda7140cb3a357617dc4fbb1435e76 Version: 34.0 (2.1) Firmware Version: V18D User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(pbylenga)
Reporter | ||
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?]
Comment 1•9 years ago
|
||
NI on Component owner for nomination decision and assignment.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga) → needinfo?(edchen)
Comment 2•9 years ago
|
||
[Log] ================================================================================================================================================================= 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.890] close with reduce 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.891] opened,closing,::,close 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.891] timer to ensure closed does occur. 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.892] publishing internal event: closing 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.893] handling _closing 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.893] publishing external event: closing 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.894] publishing internal event: willclose 01-09 12:22:43.511 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35547.894] publishing external event: willclose 01-09 12:22:43.941 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35548.322] closeApp has been ENDED! 01-09 12:22:43.941 215 215 I GeckoDump: [system] [AppWindow][Music][AppWindow_24][35548.322] closing,closed,::,complete . . . 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.600] openApp has been ENDED! 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.600] opening,opened,::,complete 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.602] publishing internal event: opened 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.603] publishing internal event: requestforeground 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.604] publishing external event: requestforeground 01-09 12:22:53.221 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.605] set visibility -> ,true 01-09 12:22:53.231 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.605] setActive on browser element:true 01-09 12:22:53.241 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.616] before showing frame 01-09 12:22:53.241 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.619] locking screen orientation to default 01-09 12:22:53.241 215 215 I GeckoDump: [system] [AppWindow][Camera][AppWindow_25][35557.620] focusing this app. ================================================================================================================================================================= It seems the Music app was killed before camera launching. I marked it to blocking 2.2? then trace status.
blocking-b2g: --- → 2.2?
Flags: needinfo?(edchen)
Comment 3•9 years ago
|
||
david, could you help to investigate this in camera app? not sure why camera app stop music play when switch front camera? thanks.
Flags: needinfo?(dflanagan)
Comment 4•9 years ago
|
||
I can reproduce this on a Flame running yesterday's nightly build. The music dies when I switch from rear camera to front camera or vice versa. The music app is not killed as comment #2 suggests. The music starts playing again when I switch back to the Music app. And it will keep playing while I use the camera to take still pictures. When I use the camera to record a video the music stops. That might actually be a feature rather than a bug. So perhaps switching camera bug is related to music stopping for video recording.
Flags: needinfo?(dflanagan)
Comment 5•9 years ago
|
||
Jim and Diego: do either of you know what might be going on here? Jim: does the music app ever get signals to stop playing from the system app because of media recording? Or is this more likely to be an audio competing policy issue? Diego: does the camera explicitly do anything to stop music playback when video recording starts? Mike: does this sound like it could be a gecko or hardware bug to you?
Flags: needinfo?(squibblyflabbetydoo)
Flags: needinfo?(mhabicher)
Flags: needinfo?(dmarcos)
Comment 6•9 years ago
|
||
This sounds like audio competing. I think the only other things that stop music are the hack for sharing a ringtone, and bluetooth stuff.
Flags: needinfo?(squibblyflabbetydoo)
Comment 7•9 years ago
|
||
I tried commenting out this line in the camera's js/lib/sounds.js file: audio.mozAudioChannelType = 'notification'; That did not affect the bug. Even commenting out the entire Sounds.play() method in that file does not affect the bug, so it is not the camera creating audio elements or setting mozAudioChannelType that is causing this issue.
Comment 8•9 years ago
|
||
Looking through attachment 8546836 [details], I don't see anything unexpected related to audio/media/sound/player. The camera stack in Gecko never pokes any of the audio APIs unless video recording is started (and stopped).
One question would be whether or not this happens on devices other than the Flame. If not, then it's likely a Gonk issue.
Jim, when this issue happens, does the music player stop? or does it keep playing, just without audio?
Flags: needinfo?(mhabicher) → needinfo?(squibblyflabbetydoo)
Comment 10•9 years ago
|
||
I've played around with a bunch of console.log statements in apps/camera/js/lib/camera.js and find that the music seems to stop when I can this.mozCamera.release(). If I comment that line out, the music does not stop. (Of course, I can't switch cameras either). If I leave that line in and comment out the onSuccess callback then the music does stop which makes me think (though it does not make me certain) that something is happening when the hardware is released.
Comment 11•9 years ago
|
||
This is the logcat I see between the console.log() calls I inserted into the release method of js/lib/camera.js. I don't know what most of it means, but here are some things I notice: 1) The release() method at the gaia level is being called reentrantly. The second call returns without doing much, but it does seem to be called a second time. 2) At the end of this logcat, there are errors coming from "AwesomePlayer" because it cannot load audio files for the camera. IIRC, the Flame camera driver tries to play its own sounds and we disabled that by removing the files. But it is causing errors at the audio level, so maybe that has something to do with this audio bug. 3) While the camera hardware is being released there are lots of low-level logging messages that seem to have to do with shutting down streams of video data. Could that somehow also be cutting off the audio data? I doubt it, but I wonder. Needinfo Mike in case he wants to take a closer look at this.
Flags: needinfo?(mhabicher)
Comment 12•9 years ago
|
||
Setting QA needed and setting needinfo for Joshua: per comment 8, we need to know if this bug is Flame-specific. The Flame camera driver has some weird stuff where it tries to play its own audio...
Flags: needinfo?(jmitchell)
Keywords: qawanted
Comment 13•9 years ago
|
||
On Flame 2.0 and 2.1, when launch Camera app (doesn't need to switch to front face camera), the FM radio/Music player will stop playing. See attachments: Flame2.1_logcat_1540.txt & Video.MP4 Repro time: 15:40 Rate: 5/5 Flame 2.0 build: Gaia-Rev 736933b25ded904f0cb935a0d48f1f3cf91d33ad Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6c9aefc84244 Build-ID 20150118000204 Version 32.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20150118.032138 FW-Date Sun Jan 18 03:21:46 EST 2015 Bootloader L1TC000118D0 Flame 2.1 build: Gaia-Rev 77c57eb8a985d5cbd34a597fb1b978ba6e205af6 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/452a023ae7b2 Build-ID 20150118001331 Version 34.0 Device-Name flame FW-Release 4.4.2 FW-Incremental eng.cltbld.20150118.034742 FW-Date Sun Jan 18 03:47:53 EST 2015 Bootloader L1TC000118D0 Due to Woodduck2.0 Camera doesn’t support front face function, this problem can’t be repro on it.
Comment 14•9 years ago
|
||
Updated•9 years ago
|
status-b2g-v2.0:
--- → affected
status-b2g-v2.1:
--- → affected
Comment 15•9 years ago
|
||
Some additional data: this isn't specific to switching to the front camera. If once you've switched to the front camera, you return to the Homescreen and (re)open the FM Radio app, the audio resumes. When you return to the Camera app, the audio continues to play, but is stopped when you switch to the back camera. Observed on: - gonk v18d - gecko b2g-inbound:f1e3caec8675 - gaia master:67b424672a69d73b4bc94b354ba088b50758d482
Flags: needinfo?(mhabicher)
Summary: [Audio Channel] [Camera] Audio from FM Radio or Music app cease to play when switching camera to front face camera. → [Audio Channel][Camera] Audio from FM Radio or Music app ceases to play when switching between front/back camera
Comment 16•9 years ago
|
||
Comment 17•9 years ago
|
||
(I've been trying to reproduce this on a Nexus 4, but the Nexus 4 seems to be horribly broken: the camera seems to work, but WiFi doesn't work, the Music player doesn't work, etc.)
Comment 18•9 years ago
|
||
Resetting the qawanted flag. We still need to know whether this reproduces on any device other than the Flame. Joshua, can you help with this?
Keywords: qawanted
Reporter | ||
Comment 19•9 years ago
|
||
Sorry No; I only have Flame devices here.
Flags: needinfo?(jmitchell)
Comment 20•9 years ago
|
||
The nexus-4-kk prebuilts all seem to be horribly broken, so I had to roll my own. With a b2g-inbound/master build flashed onto my Nexus 4, I see exactly the same behaviour as on the flame.
Comment 21•9 years ago
|
||
Confirmed that when we change cameras, we hit the following case in the Music app: https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp#4118
Comment 22•9 years ago
|
||
Comment 23•9 years ago
|
||
Updated•9 years ago
|
Attachment #8551956 -
Attachment description: bt full on mute → |bt full| on mute (child process)
Updated•9 years ago
|
Attachment #8551969 -
Attachment description: bt full before mute (parent process) → |bt full| before mute (parent process)
Comment 24•9 years ago
|
||
Further to comment 21, the muting happens somewhere else -- the linked-to code is simply where the HTMLMediaElement state is updated. (Confirmed by commenting out most of this function. :)
Comment 25•9 years ago
|
||
Andrea -- any idea what might be going on here?
Flags: needinfo?(amarchesini)
Updated•9 years ago
|
QA Contact: jmercado
Updated•9 years ago
|
QA Contact: jmercado
Comment 26•9 years ago
|
||
(In reply to David Flanagan [:djf] from comment #12) > Setting QA needed and setting needinfo for Joshua: per comment 8, we need to > know if this bug is Flame-specific. The Flame camera driver has some weird > stuff where it tries to play its own audio... See comment 20.
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Assignee | ||
Comment 27•9 years ago
|
||
Hi, All, I found that the audio is stopped by the media element of the camera app. For some strange reason, the attributes |mPlayingThroughTheAudioChannel| become to playable for a while, then suddenly recover to non-playable. It should always be non-playable in our test situation, so I am still looking for more details.
Assignee | ||
Comment 28•9 years ago
|
||
Hi, Wilson, Sorry to bother you, here are some questions need your help :( When I commented this line "videoElement.play()", the problem is solved. http://mxr.mozilla.org/gaia/source/apps/camera/js/lib/camera/camera.js#417 What is the purpose of loadStreamInto()? Why this function be called when we start the apps or change to the front camera? Thanks a lots :)
Flags: needinfo?(wilsonpage)
Comment 29•9 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #28) > Hi, Wilson, > Sorry to bother you, here are some questions need your help :( > > When I commented this line "videoElement.play()", the problem is solved. > http://mxr.mozilla.org/gaia/source/apps/camera/js/lib/camera/camera.js#417 > > What is the purpose of loadStreamInto()? > Why this function be called when we start the apps or change to the front > camera? > Thanks a lots :) .loadStreamInto() is used to load the mozCamera video stream into the <video> element in the Camera App DOM. I'm not sure if the .play() call is required or not, :mikeh may know. This bug is again a indication of the mess that Gaia audio-channels are in. We can keep hacking around them in app space, but IMO these bugs should be fixed once and for all at the source.
Flags: needinfo?(wilsonpage)
Updated•9 years ago
|
blocking-b2g: 2.2? → 2.2+
Assignee | ||
Comment 30•9 years ago
|
||
Thanks, Wilson! We are implementing the audio channel refactoring now, and I think it could solve the mess of audio channels :) If you want, you can see more details in Bug1113086 & Bug1100822. Hi, Mike, Do we need this? If I remove this code, the problem can be solved. http://mxr.mozilla.org/gaia/source/apps/camera/js/lib/camera/camera.js#417 Thanks a lots :)
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 31•9 years ago
|
||
[Root cause] In line#417 camera.js, the function |videoElement.play()| would enable a new audio agent, but this video element doesn't play any sound. It confused the AudioChannelService, because it doesn't know about the REAL behavior of audio agent. Therefore, the AudioChannelService stop the hidden content audio in order to playback the new foreground audio. [Solution] I think we should remove the line#417 in camera.js. We should only playback the elements which actually playback the sound.
Assignee | ||
Comment 32•9 years ago
|
||
Sorry for the wrong information, I found the real root cause of this bug. Please ignore the comment31. [Root cause] The order of changing the ready state and network state error. [Bug situation] The HTMLMediaElement would check whether the element can be playback by checking its ready state. In this case, we reference to the previous state, that results in the error playback state. When we try to play the new sound, the AudioChannelService would stop the hidden content audio. [Solution] Patch is WIP. We can use Gecko patch to fix it. But I think the line#417 in camera.js could also be removed (if it is meaningless).
Assignee | ||
Comment 33•9 years ago
|
||
Hi, JW, Could you give me some suggestions? |UpdateAudioChannelPlayingState()| will check the ready state to decide whether media element use the audio channel to play. However, we also use this function on the |AddRemoveSelfReference()|, so that |UpdateAudioChannelPlayingState()| makes wrong decision by the error ready state. I think we should only call this function after we update the ready state. Thanks a lots :)
Attachment #8563261 -
Flags: feedback?(jwwang)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → alwu
Comment 34•9 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #33) > so that > |UpdateAudioChannelPlayingState()| makes wrong decision by the error ready > state. Can you elaborate how it made wrong decision? UpdateAudioChannelPlayingState() is also called in ChangeReadyState(). Even though UpdateAudioChannelPlayingState() is called in AddRemoveSelfReference() with staled ready state, the one called in ChangeReadyState() should correct it, right?
Assignee | ||
Comment 35•9 years ago
|
||
In this case, when we change the back camera to the front one. It reset the MozSrcObject, then it triggered the |AbortExistingLoads()|. In this function, we updated the network state first, then the ready state. Therefore, the |UpdateAudioChannelPlayingState()| would be triggered twice time. In the first updating, the ready state was still "HAVE_CURRENT_DATA" (not be updated yet) and the new source stream wasn't loaded at that time. (we abort the old source, then reload the new source). It would let the variable |playingThroughTheAudioChannel| become true. See the following line. https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLMediaElement.cpp?from=HTMLMediaElement.cpp&case=true#4198 When the |playingThroughTheAudioChannel| becomes true, that means we playback the new normal audio at foreground, then the AudioChannelService would stop the background content audio (the music).
Comment 36•9 years ago
|
||
IIRC, the first UpdateAudioChannelPlayingState() leave |playingThroughTheAudioChannel| to be true. Why didn't the 2nd one correct it which should happen after ready state is updated.
Assignee | ||
Comment 37•9 years ago
|
||
Hi, JW, Thanks for your useful suggestion. It can be fixed when we updated the mPause first. I also recover the changing of |UpdateAudioChannelPlayingState()|. The reason of using that function in the |AddRemoveSelfReference()| is described here. https://bugzilla.mozilla.org/show_bug.cgi?id=805333#c79 Thanks a lots :)
Attachment #8563261 -
Attachment is obsolete: true
Attachment #8563261 -
Flags: feedback?(jwwang)
Attachment #8563283 -
Flags: feedback?(jwwang)
Comment 38•9 years ago
|
||
Comment on attachment 8563283 [details] [diff] [review] Update pause state first Review of attachment 8563283 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +693,5 @@ > > if (mNetworkState != nsIDOMHTMLMediaElement::NETWORK_EMPTY) { > NS_ASSERTION(!mDecoder && !mSrcStream, "How did someone setup a new stream/decoder already?"); > + // We should update the mPause first, avoiding to reference the wrong state. > + mPaused = true; The comment should be like "ChangeNetworkState() will call UpdateAudioChannelPlayingState() indirectly which depends on mPaused. So we need to update mPaused first." @@ +698,2 @@ > ChangeNetworkState(nsIDOMHTMLMediaElement::NETWORK_EMPTY); > ChangeReadyState(nsIDOMHTMLMediaElement::HAVE_NOTHING); ChangeReadyState() will also call UpdateAudioChannelPlayingState(). It is not good to change playing-through-the-AudioChannel state within a short time even we cache the value in mPlayingThroughTheAudioChannel to prevent duplicate changes. However, it doesn't prevent intermediate changes. You should consider make it an async function so that changes are aggregated and take effect in the next run.
Attachment #8563283 -
Flags: feedback?(jwwang) → feedback+
Comment 39•9 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #30) > Do we need this? If I remove this code, the problem can be solved. > http://mxr.mozilla.org/gaia/source/apps/camera/js/lib/camera/camera.js#417 > Thanks a lots :) Alastor, do you still need to remove this? If so, it's probably fine as long as it doesn't break anything, but it would be good to know why it needs removal.
Flags: needinfo?(mhabicher)
Assignee | ||
Comment 40•9 years ago
|
||
Hi, Mike, It seems that we don't remove that line, sorry! Now I am thinking about how to avoid the question which JW mentioned in comment38.
Assignee | ||
Comment 41•9 years ago
|
||
Hi, JW, According to your suggestion, I add the async function to update the audio channel state. Thanks a lots :)
Attachment #8546836 -
Attachment is obsolete: true
Attachment #8550647 -
Attachment is obsolete: true
Attachment #8551130 -
Attachment is obsolete: true
Attachment #8551131 -
Attachment is obsolete: true
Attachment #8551420 -
Attachment is obsolete: true
Attachment #8551956 -
Attachment is obsolete: true
Attachment #8551969 -
Attachment is obsolete: true
Attachment #8560379 -
Attachment is obsolete: true
Attachment #8564084 -
Flags: feedback?(jwwang)
Comment 42•9 years ago
|
||
Comment on attachment 8564084 [details] [diff] [review] Async runnable for updating audio channel state Review of attachment 8564084 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +211,5 @@ > + nsMediaEvent(aElement) > + { > + } > + > + NS_IMETHOD Run() This can be done in a member function so you don't need FinishUpdating(). @@ +3822,5 @@ > + if (!mIsDispatchAudioChannelUpdateRunner) { > + mIsDispatchAudioChannelUpdateRunner = true; > + nsCOMPtr<nsIRunnable> event = new asyncAudioChannelStateUpdateRunner(this); > + NS_DispatchToMainThread(event); > + } The code is duplicated. You should have a function like: void QueueUpdatingAudioChannelPlayingState() { if (!mIsDispatchAudioChannelUpdateRunner) { mIsDispatchAudioChannelUpdateRunner = true; // dispatch the runnable to the main thread. // It might be also better to run the task in stable state. } } ::: dom/html/HTMLMediaElement.h @@ +1345,5 @@ > > ElementInTreeState mElementInTreeState; > > + class asyncAudioChannelStateUpdateRunner; > + bool mIsDispatchAudioChannelUpdateRunner; The name "mUpdateAudioChannelDispatched" might be better.
Attachment #8564084 -
Flags: feedback?(jwwang) → feedback-
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 43•9 years ago
|
||
Hi, JW, Here is my revised patch, thanks :)
Attachment #8564084 -
Attachment is obsolete: true
Attachment #8569679 -
Flags: feedback?(jwwang)
Comment 44•9 years ago
|
||
Comment on attachment 8569679 [details] [diff] [review] Async runnable for updating audio channel state Review of attachment 8569679 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +214,5 @@ > + } > + > + NS_IMETHOD Run() > + { > + mElement->UpdateAudioChannelPlayingState(); You might need to check IsCancelled() as other sub-classes of nsMediaEvent. @@ +4554,5 @@ > +{ > + if (!mUpdateAudioChannelDispatched) { > + mUpdateAudioChannelDispatched = true; > + nsCOMPtr<nsIRunnable> event = new asyncAudioChannelStateUpdateRunner(this); > + NS_DispatchToMainThread(event); It might be better to call RunInStableState() here. ::: dom/html/HTMLMediaElement.h @@ +1348,5 @@ > }; > > ElementInTreeState mElementInTreeState; > > + class asyncAudioChannelStateUpdateRunner; Class name should start with upper case letter.
Attachment #8569679 -
Flags: feedback?(jwwang) → feedback-
Assignee | ||
Comment 45•9 years ago
|
||
Hi, JW, Here is my revised patch, Very appreciate!
Attachment #8569679 -
Attachment is obsolete: true
Attachment #8569751 -
Flags: feedback?(jwwang)
Comment 46•9 years ago
|
||
Comment on attachment 8569751 [details] [diff] [review] Async runnable for updating audio channel state Review of attachment 8569751 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/html/HTMLMediaElement.cpp @@ +4558,5 @@ > +{ > + if (!mUpdateAudioChannelDispatched) { > + mUpdateAudioChannelDispatched = true; > + nsCOMPtr<nsIRunnable> event = new AsyncAudioChannelStateUpdateRunner(this); > + RunInStableState(event); A runnable passed to RunInStableState() will be wrapped in nsSyncSection which will check IsCancelled(). Therefore, AsyncAudioChannelStateUpdateRunner doesn't have to inherit from nsMediaEvent.
Attachment #8569751 -
Flags: feedback?(jwwang) → feedback-
Assignee | ||
Comment 47•9 years ago
|
||
I found that my patch will cause some regression errors, I am trying to find the reason.
Assignee | ||
Comment 48•9 years ago
|
||
After I added the async method, I found that when I start playing the audio, the runnable wouldn't be executed, even I have dispatched it. That results in multiple audio can be playback at the same time, because we can't execute the UpdateAudioChannelPlayingState() means that we playback audio without an audio agent. I create another bug to handle the async issue (See Bug 1139274), because I think we should close this bug first.
Flags: needinfo?(dmarcos)
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 49•9 years ago
|
||
Hi, Robert, Sorry to bother you, could you help me to review this patch? Thanks a lots :) The root cause is that we reference the wrong state of mPause. ChangeNetworkState() will call UpdateAudioChannelPlayingState() indirectly which depends on mPaused. So we need to update mPaused first.
Attachment #8563283 -
Attachment is obsolete: true
Attachment #8569751 -
Attachment is obsolete: true
Attachment #8572454 -
Flags: review?(roc)
Attachment #8572454 -
Flags: review?(roc) → review+
Assignee | ||
Comment 50•9 years ago
|
||
Attachment #8572454 -
Attachment is obsolete: true
Attachment #8574546 -
Flags: review+
Assignee | ||
Comment 51•9 years ago
|
||
Try-server result. https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b60f995c2d
Keywords: checkin-needed
Comment 52•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab68008ae9de
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab68008ae9de
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
Comment 54•9 years ago
|
||
Please request b2g37 approval on this patch when you get a chance.
status-b2g-master:
--- → fixed
status-firefox37:
--- → wontfix
status-firefox38:
--- → wontfix
Flags: needinfo?(alwu)
Assignee | ||
Comment 55•9 years ago
|
||
Ask approval for b2g37 from comment54. Thanks!
Flags: needinfo?(alwu)
Attachment #8577859 -
Flags: approval-mozilla-b2g37?
Comment 56•9 years ago
|
||
(In reply to Alastor Wu [:alwu] from comment #55) > Created attachment 8577859 [details] [diff] [review] > [v2.2] Update pause state first. r=roc. > > Ask approval for b2g37 from comment54. > Thanks! can you please fill the form with the approval ? NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): User impact if declined: Testing completed: Risk to taking this patch (and alternatives if risky): String or UUID changes made by this patch:
Updated•9 years ago
|
Flags: needinfo?(alwu)
Assignee | ||
Comment 57•9 years ago
|
||
Sorry, Bhavana, Here is the approval form. [Approval Request Comment] Bug caused by (feature/regressing bug #): Reference to error pause state of the media element, making the audio stop User impact if declined: It is a common usage of users, we need to fix it. Testing completed: Yes Risk to taking this patch (and alternatives if risky): None String or UUID changes made by this patch: None
Flags: needinfo?(alwu)
Comment 58•9 years ago
|
||
This issue is verified fixed on Flame Master. Result: The music or fm radio continues to play when the front camera is launched. Device: Flame Master (KK, 319mb, full flash) Build ID: 20150317073344 Gaia: 738987bd80b0ddb4ccf853855388c2627e19dcc1 Gecko: 008b3f65a7e0 Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b Version: 39.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0 ================================ Leaving verifyme for 2.2 uplift/verification.
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: verifyme
Updated•9 years ago
|
Attachment #8577859 -
Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Updated•9 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Comment 60•9 years ago
|
||
This bug has been successfully verified on latest Flame v2.2. See attachment: verified_v2.2.mp4 Reproduce rate: 0/5 STR: 1.Play audio with Music or FM Radio app. 2.Launch Camera app. **Music or FM Radio will continue to play. 3.Switch to front face camera. **Music or FM Radio will continue to play. Device: Flame 2.2 (Pass) Build ID 20150323162503 Gaia Revision e54c4ed1cc188f70ddf1156534d364005dc45490 Gaia Date 2015-03-23 19:09:26 Gecko Revision https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/7ba1778d237b Gecko Version 37.0 Device Name flame Firmware(Release) 4.4.2 Firmware(Incremental) eng.cltbld.20150323.200543 Firmware Date Mon Mar 23 20:05:54 EDT 2015 Bootloader L1TC000118D0
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Comment 61•9 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•