Closed Bug 1107534 Opened 5 years ago Closed 5 years ago

[Settings]Tapping set or manage ringtones tone preview will not play

Categories

(Firefox OS Graveyard :: AudioChannel, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.2+, firefox38 wontfix, firefox39 wontfix, firefox40 fixed, b2g-v2.1 unaffected, b2g-v2.2 verified, b2g-master verified)

VERIFIED FIXED
2.2 S9 (3apr)
blocking-b2g 2.2+
Tracking Status
firefox38 --- wontfix
firefox39 --- wontfix
firefox40 --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: rmitchell, Assigned: alwu)

References

()

Details

(Keywords: regression, Whiteboard: [caf priority: p2][CR 808206][2.2-flame-reduced-run][2.2-nexus-5-l])

Attachments

(2 files, 8 obsolete files)

20.10 KB, patch
alwu
: review+
Details | Diff | Splinter Review
20.67 KB, patch
Details | Diff | Splinter Review
Attached file logcat_20141204_0923.txt (obsolete) —
Description:
When user is tapping ringtones in set or manage ringtones no ringtone plays  


Repro Steps:
1) Update a Flame to 20141203040207
2) Go to settings menu
3) Go to sound menu ringtones menu 
4) Tap veriouse ringtone 


Actual:
No ringtone plays when name is tapped 


Expected:
Ringtone plays when name is tapped 

Environmental Variables:
Device: Flame 2.2
Build ID: 20141203040207
Gaia: 725685831f5336cf007e36d9a812aad689604695
Gecko: 2c9781c3e9b5
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 37.0a1 (2.2)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


Repro frequency:100%
See attached:logcat,video clip:https://www.youtube.com/watch?v=rnfTJMUlooI&feature=youtu.be
Flags: needinfo?(dharris)
issue was not unaffected in  2.1 flame
ringtones play 
Flame 2.1 

Device: Flame 2.1 (319mb)(KitKat)(Full Flash)
Build ID: 20141203001205
Gaia: dbaf3e31c9ba9c3436e074381744f2971e15c7bf
Gecko: ebce587d2194
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
This is a regression from 2.1 so nominating 2.2?
blocking-b2g: --- → 2.2?
QA Whiteboard: [QAnalyst-Triage?]
QA Contact: ddixon
Flags: needinfo?(dharris) → needinfo?
Mozilla Inbound Regression Window

Last Working

Device: Flame 2.2
BuildID: 20141117080228
Gaia: ddf5b92f43ec27c93ad4fea4fd1207da8936b8e7
Gecko: c007f1136acd
Version: 36.0a1 (2.2)
Firmware: V188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

First Broken

Device: Flame 2.2
BuildID: 20141117080828
Gaia: ddf5b92f43ec27c93ad4fea4fd1207da8936b8e7
Gecko: acc3209d766a
Version: 36.0a1 (2.2) 
Firmware Version: v188-1
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0

Last Working Gaia and First Broken Gecko
Issue DOES occur here. 
Gaia: ddf5b92f43ec27c93ad4fea4fd1207da8936b8e7
Gecko: acc3209d766a

Last Working Gecko and First Broken Gaia
Issue DOES NOT occur here. 
Gaia: ddf5b92f43ec27c93ad4fea4fd1207da8936b8e7
Gecko: c007f1136acd

Gecko Pushlog: 
hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c007f1136acd&tochange=acc3209d766a

Possible Cause: 

Bug 1073615 - MediaStreamGraph::GetInstance() is broken
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(jmitchell)
Broken by the patch for Bug 1073615  - can you take a look Andrea
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(jmitchell)
Flags: needinfo?(amarchesini)
Flags: needinfo?
QA Contact: ddixon
Component: Gaia::Settings → Gaia::Ringtones
Regression from Bug 1073615  based on previous comment. 

Blocking Reason: Basic ringtone function is affected. 

Andrea, could you please help check what is going on. Feel free to NI Jim Porter for any questions on the ringtones side.
blocking-b2g: 2.2? → 2.2+
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][COM=Gaia::Ringtones]
Duplicate of this bug: 1114917
Andrea, Any input? Based on comment 4, it looks like this broke after patch: https://bugzilla.mozilla.org/show_bug.cgi?id=1073615

Thanks
Hema
I solved a similar bug (bug 1043762) last time. Let's give it a try.
Assignee: nobody → salva
Duplicate of this bug: 1123285
I talked to Baku on IRC and he said me about changing the channel in the player. It seems the AudioContext was muting all the content in 'normal' channel including the player at that moment.
Flags: needinfo?(amarchesini)
(In reply to Salvador de la Puente González [:salva] from comment #10)
> I talked to Baku on IRC and he said me about changing the channel in the
> player. It seems the AudioContext was muting all the content in 'normal'
> channel including the player at that moment.

That's intentional: the reason we use an AudioContext is so that we can mute all background audio until we close (or hide) the ringtones app.
Yeah, I know. But the problem was the _player was created without assigning its channel in [1], so when creating the AudioContext with ringer as channel in [2], it muted all, including the _player, even if the _player is attached to an AudioContext with a higher priority channel. I confirmed with Baku that the only thing we need now is to get rid of the AudioContext and set the mozAudioChannelType property described in [3].

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L17
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L146
[3] https://developer.mozilla.org/en-US/docs/Web/API/HTMLMediaElement#Properties
Jim, do you see my former comment? I asked you for reviewing a working patch?

Thank you!
Flags: needinfo?(squibblyflabbetydoo)
Comment on attachment 8551418 [details] [review]
Reimplementing _setExclusiveMode to simply raise the _player channel priority when needed.

As mentioned above, this regresses the feature in the ringtones app that causes any background audio to pause once you start previewing a tone, *and* stay paused until the ringtone app loses focus. I added this feature after discussion with UX, so if you'd like to change this (I don't have a strong opinion, myself), you'll need ui-review first. However, I wouldn't be surprised if you don't get ui-review for this, it regresses a usability feature.
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8551418 - Flags: review?(squibblyflabbetydoo) → review-
Duplicate of this bug: 1119165
Comment on attachment 8551418 [details] [review]
Reimplementing _setExclusiveMode to simply raise the _player channel priority when needed.

But Jim, this is not regressing anything, this patch continues to stop any background sound until it stops playing. If your are noticing another behaviour, please provide a reproducible STR because I can not reproduce the scenarie in which the audio is not being muted.

Which problem / regression do you detect with this patch?
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8551418 - Flags: review- → review?(squibblyflabbetydoo)
1) Open Music.
2) Play some Music.
3) Open the Ringtones Manager.
4) Preview a ringtone.
   - Music from (2) pauses.
5) Wait for ringtone preview to end.
   - Music from (2) resumes - it shouldn't.
Flags: needinfo?(squibblyflabbetydoo)
Attachment #8551418 - Flags: review?(squibblyflabbetydoo) → review-
I get your point. I think this is a back-end bug since from the moment you creates the AudioContext with ringer, it is not possible to "free" the ringer channel even if the AudioContext object is no more referred. Asking :baku for clarifications here.
Flags: needinfo?(amarchesini)
Attached file Toy application to test audio contexts (obsolete) —
I think we have a couple of problems here.

  1. In the specification, AFAIK, it is not possible to keep muted a "normal" channel if there is not a "ringer" channel **reproducing**. If this was working before, it is probably due to some bug in the implementation.
  2. For some reason, connecting an Audio element as a source node of an AudioContext and playing it, is not working. I'm uploading a toy application for this.

See instructions in the app.
This also occurs on Flame 3.0

Device: Flame 3.0 (KK - nightly - full flashed)
Build ID: 20150129010239
Gaia: 9d2378a9ef092ab1fc15c3a9f7fc4171aab59d57
Gecko: 6bfc0e1c4b29
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0
We need feedback from platform guys. I'm leaving the bug but I added a toy application reproducing the problem. If you need my help, ni me.
Assignee: salva → nobody
(In reply to Salvador de la Puente González [:salva] from comment #22)
> We need feedback from platform guys. I'm leaving the bug but I added a toy
> application reproducing the problem. If you need my help, ni me.

Thanks Salva!

Andrea/Andrew: Who can help here?

Thanks
Hema
Flags: needinfo?(amarchesini) → needinfo?(overholt)
Looks like I accidentally cleared baku's NI, adding it back
Flags: needinfo?(amarchesini)
From the AudioChannel point of view I don't see any errors. I debugged this issue with gdb and the AudioContext and the HTMLMediaElement are both playing the correct ringer audiochannel.

It seems that the problem is with the volume, or with something in the AudioManager.
Let's ask rlin a feedback.
Flags: needinfo?(amarchesini) → needinfo?(globelinmoz)
Flags: needinfo?(overholt)
I will take a look of this, because Randy isn't in Mozilla now.
Flags: needinfo?(globelinmoz)
Component: Gaia::Ringtones → AudioChannel
Assignee: nobody → alwu
See Also: → 1139157
The following code results this issue [1]. Previous gecko version is all audio type would share the same MediaStreamGraph, but now the different types have difference MediaStreamGraph.

In the code here [2], we would create a MediaStreamGraph for ringer type, in here [3], we would create another MediaStreamGraph for normal type. It seems that we add the ring-tone stream to the wrong MediaStreamGraph. (If I return the correct MediaStreamGraph in [1], the preview would be back to normal)

Now I am still trying to find a good way to fix it.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/MediaStreamGraph.cpp?from=MediaStreamGraph.cpp#2834
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L146
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L147
Hi, Jim,
Sorry to bother you,
I found that even I revert the patch to recover the ringtone preview, but it doesn't have a feature you mentioned in comment18.

I see the code here [1], I found that you use the AudioContext to stop the background audio, but the AudioContext would be stopped when its source stream (_player) stopped. (See Bug 1027172) Therefore, the context here can't stop the background music resume.  

Is my understanding right? If so, after I fix this, you may need to modify the gaia part to make sure your feature is workable. If not, maybe there still exists some problem we need to find out.

Thanks :)
Flags: needinfo?(squibblyflabbetydoo)
In tone_player.js, we create a ringer type audio context, and this context would create a destination node in the "ringer" type MSG. When we create a MediaElementSource from the |_player|, it would create another stream to the "normal" type MSG. (because the _player is a normal type audio element) It seems that the normal type MSG doesn't have the destination node, so the stream from the |_player| would not be output.
(In reply to Alastor Wu [:alwu] from comment #28)
> Is my understanding right? If so, after I fix this, you may need to modify
> the gaia part to make sure your feature is workable. If not, maybe there
> still exists some problem we need to find out.

I think you understand things correctly.

I'm happy with any solution that fixes this; if we need to change Gaia code, that's fine. As long as we keep the same behavior we used to have, things should be good.
Flags: needinfo?(squibblyflabbetydoo)
Hi, Baku,
Could you give me some suggestions for this issue?
Very appreciate :)

After the Bug 1073615 landed, the MSG is per audio channel now. That causes the issue here. The problem is that we try to connect the stream from different MSG.

In the tone_player.js [1],
Step1. We created a "ringer" type AudioContext and we also created a "ringer" MSG. 

Step2. We created MediaElementSource to get the stream from |_player|. When we captured the stream from the MediaElement, the stream would be added to the "normal" MSG.

Step3. We connect the MediaElementSource to the destination node of AudioContext. This step causes the problem! The stream of MediaElementSource is in the "normal" MSG, and the stream of AudioContext is in the "ringer" MSG. We can't connect the streams in the different MSG. (If my understanding is correct) 

---

My rough idea is like that..
"Let streams have its audio channel type, and check streams type to adjust them to correct MSG"

ex. When we did the step2, we would find that the input stream is in "normal" MSG which is different with our one. Therefore, we would add this stream to the "ringer" MSG. That means one stream can be connect to multiple MSG.

BUT, I discussed it with JW, he said that is conflict with the concept of the DOMMediaStream. If one stream can be connect to multiple MSG, that means a DOMMediaStream have multiple streams in Gecko. It should be a one-by-one relationship.

Do you have any ideas about that?
Thanks you :)

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/ringtones/js/tone_player.js#L146
Add flag for comment32.
Flags: needinfo?(amarchesini)
Hi, Robert,
Sorry to bother you.

In the bug1073615 [1], you mentioned that "you can't connect a MediaStream to two different AudioChannel outputs". But you also said that "It's fixable later". 

In this issue, we want to connect the MediaStreams from "normal" to the "ringer". You can see more details on the comment32. 

I don't very understand how to fix this problem, could you give me suggestions?
Thanks a lot :)

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1073615#c1
Flags: needinfo?(roc)
(In reply to Alastor Wu [:alwu] from comment #32)
> Step2. We created MediaElementSource to get the stream from |_player|. When
> we captured the stream from the MediaElement, the stream would be added to
> the "normal" MSG.
> 
> Step3. We connect the MediaElementSource to the destination node of
> AudioContext. This step causes the problem! The stream of MediaElementSource
> is in the "normal" MSG, and the stream of AudioContext is in the "ringer"
> MSG. We can't connect the streams in the different MSG. (If my understanding
> is correct) 

Seems to me we should be able to make this work.

We need to extend HTMLMediaElement::MozCaptureStream to take an optional MediaStreamGraph parameter, so that it constructs a MediaStream belonging to that MediaStreamGraph. Then AudioContext::CreateMediaElementSource can pass in the AudioContext's MSG when it calls MozCaptureStream.
Flags: needinfo?(roc)
Hi, Robert,
It sounds great! 
Very thanks for your suggestion!
This patch is working on process, and it can fix this issue successfully \o/
Main idea is to send an extra parameter to indicate which MSG is the destination of the streams.
I will update more information later.
Attachment #8532071 - Attachment is obsolete: true
Attachment #8551418 - Attachment is obsolete: true
Attachment #8555881 - Attachment is obsolete: true
This issue also can be repro on Nexus5_2.2&Nexus5_3.0.

Nexus5_2.2:
Build ID               20150316002502
Gaia Revision          a6b2d3f8478ec250beb49950fecbb8a16465ff6f
Gaia Date              2015-03-15 14:33:22
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/18619f8f6c5c
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150316.042754
Firmware Date          Mon Mar 16 04:28:10 EDT 2015
Bootloader             HHZ12d

Nexus5_3.0:
Build ID               20150316160204
Gaia Revision          4868c56c0a3b7a1e51d55b24457e44a7709ea1ae
Gaia Date              2015-03-14 18:50:17
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/436686833af0
Gecko Version          39.0a1
Device Name            hammerhead
Firmware(Release)      5.0
Firmware(Incremental)  eng.cltbld.20150316.192850
Firmware Date          Mon Mar 16 19:29:05 EDT 2015
Bootloader             HHZ12d
Whiteboard: [2.2-flame-reduced-run] → [2.2-flame-reduced-run][2.2-nexus-5-l]
Whiteboard: [2.2-flame-reduced-run][2.2-nexus-5-l] → [CR 808206][2.2-flame-reduced-run][2.2-nexus-5-l]
Whiteboard: [CR 808206][2.2-flame-reduced-run][2.2-nexus-5-l] → [caf priority: p2][CR 808206][2.2-flame-reduced-run][2.2-nexus-5-l]
Waiting for the try-server result. 
If the result is well, I would ask for the review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c9f9a5d8a76b
Hi, Robert,
Sorry to bother you,
Could you help me review this patch?
The description of this issue is on the following, very appreciate :)

---

[Root cause]
We try to connect the difference audio channel type streams across the different MSGs, it doesn't work. (You can see the actually happened situation on comment32)

[Solution]
Add extra parameter for the MozCaptureStream() and add the audio channel type member in the AudioContext, so that we can use the correct audio channel type to get the correct MSG.
Attachment #8578014 - Attachment is obsolete: true
Attachment #8579335 - Flags: review?(roc)
Comment on attachment 8579335 [details] [diff] [review]
Can choose difference audio channel types for MozCaptureStream()

Review of attachment 8579335 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLMediaElement.h
@@ +581,3 @@
>  
> +  already_AddRefed<DOMMediaStream> MozCaptureStreamUntilEnded(ErrorResult& aRv,
> +                                                              AudioChannel aChannel = AudioChannel::Normal);

I think it's better to pass the actual MediaStreamGraph into these methods. That might be more useful later. Let it default to null, and treat null as the normal graph.

@@ +1041,5 @@
>    // writing to.
>    struct OutputMediaStream {
>      nsRefPtr<DOMMediaStream> mStream;
>      bool mFinishWhenEnded;
> +    AudioChannel mAudioChannel;

I don't think you'll need this. You can get the MediaStreamGraph from mStream's MediaStream.

::: dom/media/DOMMediaStream.h
@@ +181,5 @@
>    /**
>     * Create an nsDOMMediaStream whose underlying stream is a SourceMediaStream.
>     */
> +  static already_AddRefed<DOMMediaStream> CreateSourceStream(nsIDOMWindow* aWindow,
> +                                                             dom::AudioChannel aChannel = dom::AudioChannel::Normal);

Same here

@@ +252,5 @@
>    void Destroy();
> +  void InitSourceStream(nsIDOMWindow* aWindow,
> +                        dom::AudioChannel aChannel = dom::AudioChannel::Normal);
> +  void InitTrackUnionStream(nsIDOMWindow* aWindow,
> +                            dom::AudioChannel aChannel = dom::AudioChannel::Normal);

Same here

@@ +341,5 @@
>     * Create an nsDOMLocalMediaStream whose underlying stream is a TrackUnionStream.
>     */
>    static already_AddRefed<DOMLocalMediaStream>
> +  CreateTrackUnionStream(nsIDOMWindow* aWindow,
> +                         dom::AudioChannel aChannel = dom::AudioChannel::Normal);

And here

::: dom/media/MediaDecoder.h
@@ +541,5 @@
>    // it while it is playing back.
> +  virtual void AddOutputStream(ProcessedMediaStream* aStream,
> +                               bool aFinishWhenEnded);
> +
> +  void SetOutputStreamAudioType(dom::AudioChannel aChannel = dom::AudioChannel::Normal);

I think we won't need this. The ProcessedMediaStream in AddOutputStream will know what its MediaStreamGraph is.
Attachment #8579335 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> I think it's better to pass the actual MediaStreamGraph into these methods.
> That might be more useful later. Let it default to null, and treat null as
> the normal graph.

Ok.

> I don't think you'll need this. You can get the MediaStreamGraph from
> mStream's MediaStream.

Ok.

> ::: dom/media/MediaDecoder.h
> @@ +541,5 @@
> >    // it while it is playing back.
> > +  virtual void AddOutputStream(ProcessedMediaStream* aStream,
> > +                               bool aFinishWhenEnded);
> > +
> > +  void SetOutputStreamAudioType(dom::AudioChannel aChannel = dom::AudioChannel::Normal);
> 
> I think we won't need this. The ProcessedMediaStream in AddOutputStream will
> know what its MediaStreamGraph is.

I think we still need it, because we need the audio channel type to create a corresponding MSG in RecreateDecodedStream(). 

Except the AddOutputStream(), there is still another place we will call the RecreateDecodedStream(). That is in the MediaDecoderStateMachine::InitiateSeek(), and the MDSM doesn't know about the stream we captured. Therefore, we should save the audio channel of output stream in the MediaDecoder.

How do you think :) ?
Thanks!
Hi, Robert,
Here is my revised patch.
Very appreciate for your help :)
Attachment #8579335 - Attachment is obsolete: true
Attachment #8580558 - Flags: review?(roc)
(In reply to Alastor Wu [:alwu] from comment #42)
> I think we still need it, because we need the audio channel type to create a
> corresponding MSG in RecreateDecodedStream().
> 
> Except the AddOutputStream(), there is still another place we will call the
> RecreateDecodedStream(). That is in the
> MediaDecoderStateMachine::InitiateSeek(), and the MDSM doesn't know about
> the stream we captured. Therefore, we should save the audio channel of
> output stream in the MediaDecoder.

I think we can pass the MSG to MediaDecoder::RecreateDecodedStream. In MediaDecoder::AddOutputStream we can get it from aStream. When it's called via MediaDecoderStateMachine::InitiateSeek, we can get it from the old mDecodedStream before we destroy that stream.
Comment on attachment 8580558 [details] [diff] [review]
Can choose difference audio channel types for MozCaptureStream()

Need to modify.
Attachment #8580558 - Flags: review?(roc)
Hi, Robert,
Her is my revised patch,
Very appreciate :)
Attachment #8580558 - Attachment is obsolete: true
Attachment #8581418 - Flags: review?(roc)
Comment on attachment 8581418 [details] [diff] [review]
Can choose difference audio channel types for MozCaptureStream()

Review of attachment 8581418 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaDecoderStateMachine.cpp
@@ +1894,5 @@
>      nsCOMPtr<nsIRunnable> event =
> +      NS_NewRunnableMethodWithArgs<int64_t, MediaStreamGraph*>(mDecoder,
> +                                                               &MediaDecoder::RecreateDecodedStream,
> +                                                               seekTime - mStartTime,
> +                                                               mDecoder->GetDecodedStream()->mStream->Graph());

It all looks fine except this part here. There's nothing to guarantee we keep the MediaStreamGraph alive while we wait for the event to run.

You need to move the getting of the graph into RecreateDecodedStream.
Attachment #8581418 - Flags: review?(roc) → review-
Hi, Robert,
I change getting the graph into RecreateDecodedStream now :)
Here is my revised patch.
Very thanks for your help!
Attachment #8581418 - Attachment is obsolete: true
Flags: needinfo?(amarchesini)
Attachment #8583704 - Flags: review?(roc)
Comment on attachment 8583704 [details] [diff] [review]
Can choose difference audio channel types for MozCaptureStream()

Review of attachment 8583704 [details] [diff] [review]:
-----------------------------------------------------------------

No, you're still passing a MediaStreamGraph* through a runnable to RecreateDecodedStream, and there's still nothing to guarantee we keep the MediaStreamGraph alive while we wait for the event to run.

RecreateDecodedStream should not take a MediaStreamGraph* parameter.
Attachment #8583704 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #49)
> No, you're still passing a MediaStreamGraph* through a runnable to
> RecreateDecodedStream, and there's still nothing to guarantee we keep the
> MediaStreamGraph alive while we wait for the event to run.
> 
> RecreateDecodedStream should not take a MediaStreamGraph* parameter.

Hi, Robert,
I have confused about "RecreateDecodedStream should not take a MediaStreamGraph* parameter".

When the first time we create the mDecodedStream, we need the MediaStreamGraph, and it should be passed from the caller, right?
If we don't use the MediaStreamGraph to create the mDecodedStream, how do we get its graph?

--

In previous patch, I passed the MediaStreamGraph*(nullptr) through a runnable. Even the graph doesn't alive when we wait for the event to run, the actually graph will be got from the RecreateDecodedStream() instead of the input parameter.

Is this way not proper or...? Because I don't know why that way involves the alive problem of MediaStreamGraph.

Thanks!!
Flags: needinfo?(roc)
Comment on attachment 8583704 [details] [diff] [review]
Can choose difference audio channel types for MozCaptureStream()

Review of attachment 8583704 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I totally misread the patch!!!
Attachment #8583704 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/2229f27ee4ba
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S9 (3apr)
Please uplift this solution to v2.2, thanks.
See Also: → 1116732
Attachment #8585840 - Flags: approval-mozilla-b2g37?
Flags: needinfo?(bbajaj)
Flags: needinfo?(bbajaj)
Attachment #8585840 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Needs rebasing for b2g37 uplift.
Flags: needinfo?(alwu)
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Select wrong MSG for preview streams
User impact if declined: No preview in setting app
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)
Attachment #8588897 - Flags: approval-mozilla-b2g37?
Comment on attachment 8588897 [details] [diff] [review]
[v2.2] Can choose difference audio channel types for MozCaptureStream(). r=roc.

The previous approval can carry over. Thanks for rebasing!
Attachment #8588897 - Flags: approval-mozilla-b2g37?
Verified on:
[2.2]
Gaia-Rev        5e09637414269728f6f1bc0152d0160f3b6b380e
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/245f37f44017
Build-ID        20150407002501
Version         37.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150407.040845
FW-Date         Tue Apr  7 04:08:57 EDT 2015
Bootloader      L1TC000118D0


[3.0]
Gaia-Rev        84cbd4391fb7175d5380fa72c04d68873ce77e6d
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/078128c2600a
Build-ID        20150407160201
Version         40.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150407.193600
FW-Date         Tue Apr  7 19:36:12 EDT 2015
Bootloader      L1TC100118D0
Status: RESOLVED → VERIFIED
Duplicate of this bug: 1122538
Blocks: 1073615
Blocks: CVE-2015-4477
No longer blocks: 1122538
Depends on: 1228484
You need to log in before you can comment on or make changes to this bug.