[Audio Channel][Camera] Audio from FM Radio or Music app ceases to play when switching between front/back camera

VERIFIED FIXED in Firefox 39, Firefox OS v2.2

Status

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: jmitchell, Assigned: alwu)

Tracking

({verifyme})

unspecified
2.2 S8 (20mar)
ARM
Gonk (Firefox OS)
verifyme

Firefox Tracking Flags

(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)

Details

(Whiteboard: [2.2-Daily-Testing])

Attachments

(3 attachments, 14 obsolete attachments)

1.48 KB, patch
alwu
: review+
Details | Diff | Splinter Review
1.41 KB, patch
Details | Diff | Splinter Review
4.57 MB, video/mp4
Details
(Reporter)

Description

4 years ago
Created attachment 8546836 [details]
logcat_20150109_1222.txt

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

4 years ago
Flags: needinfo?(pbylenga)
(Reporter)

Updated

4 years ago
QA Whiteboard: [QAnalyst-Triage?]
NI on Component owner for nomination decision and assignment.
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(pbylenga) → needinfo?(edchen)

Comment 2

4 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

4 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)
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)
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

4 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)
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.
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 9

4 years ago
It appears to just pause.
Flags: needinfo?(squibblyflabbetydoo)
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.
Created attachment 8550647 [details]
logcat

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)
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
Created attachment 8551130 [details]
Video

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.
Created attachment 8551131 [details]
Flame2.1 logcat
Keywords: qawanted
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
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
Created attachment 8551420 [details]
[the annotated] adb logcat -v threadtime | grep -i audio
(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.)
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

4 years ago
Sorry No; I only have Flame devices here.
Flags: needinfo?(jmitchell)
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.
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
Created attachment 8551956 [details]
|bt full| on mute (child process)
Created attachment 8551969 [details]
|bt full| before mute (parent process)
Attachment #8551956 - Attachment description: bt full on mute → |bt full| on mute (child process)
Attachment #8551969 - Attachment description: bt full before mute (parent process) → |bt full| before mute (parent process)
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. :)
Andrea -- any idea what might be going on here?
Flags: needinfo?(amarchesini)
QA Contact: jmercado
QA Contact: jmercado
(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.
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
(Assignee)

Comment 27

4 years ago
Created attachment 8560379 [details]
gdb bt

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

4 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)
(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

4 years ago
blocking-b2g: 2.2? → 2.2+
(Assignee)

Comment 30

4 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

4 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

4 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

4 years ago
Created attachment 8563261 [details] [diff] [review]
always update audio channel after chaning ready state

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

4 years ago
Assignee: nobody → alwu
(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

4 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).
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

4 years ago
Created attachment 8563283 [details] [diff] [review]
Update pause state first

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 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+
(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

4 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

4 years ago
Created attachment 8564084 [details] [diff] [review]
Async runnable for updating audio channel state

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 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

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 43

4 years ago
Created attachment 8569679 [details] [diff] [review]
Async runnable for updating audio channel state

Hi, JW,
Here is my revised patch, thanks :)
Attachment #8564084 - Attachment is obsolete: true
Attachment #8569679 - Flags: feedback?(jwwang)
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

4 years ago
Created attachment 8569751 [details] [diff] [review]
Async runnable for updating audio channel state

Hi, JW,
Here is my revised patch,
Very appreciate!
Attachment #8569679 - Attachment is obsolete: true
Attachment #8569751 - Flags: feedback?(jwwang)
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

4 years ago
I found that my patch will cause some regression errors, I am trying to find the reason.
(Assignee)

Comment 48

4 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

4 years ago
Created attachment 8572454 [details] [diff] [review]
Update pause state first

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)
(Assignee)

Comment 50

4 years ago
Created attachment 8574546 [details] [diff] [review]
Update pause state first. r=roc.
Attachment #8572454 - Attachment is obsolete: true
Attachment #8574546 - Flags: review+
(Assignee)

Comment 51

4 years ago
Try-server result.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33b60f995c2d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ab68008ae9de
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S8 (20mar)
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

4 years ago
Created attachment 8577859 [details] [diff] [review]
[v2.2] Update pause state first. r=roc.

Ask approval for b2g37 from comment54.
Thanks!
Flags: needinfo?(alwu)
Attachment #8577859 - Flags: approval-mozilla-b2g37?
(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

4 years ago
Flags: needinfo?(alwu)
(Assignee)

Comment 57

4 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)
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?]
status-b2g-master: fixed → verified
Flags: needinfo?(ktucker)
Keywords: verifyme

Updated

4 years ago
Attachment #8577859 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
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+]
status-b2g-v2.2: fixed → verified
You need to log in before you can comment on or make changes to this bug.