Closed Bug 989921 Opened 6 years ago Closed 5 years ago

Add an API to the MediaStreamGraph to register a mixer callback function

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: padenot, Assigned: padenot)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

This is necessary if we want to be able to provide the audio output for a tab (for testing purposes), generally to be able to have multiple consumer of the mixed output (rather than hard-wiring it to the AEC).
This would very nice for doing functional testing of audio applications that do any sort of audio transmission, or even more mixing.
I figured I needed this for the msg refactor, so here it is.
Attachment #8418657 - Flags: review?(rjesup)
Comment on attachment 8418657 [details] [diff] [review]
Add an API to the MediaStreamGraph to register a mixer callback function. r=

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

::: content/media/AudioMixer.h
@@ +51,5 @@
>     * tracks have been mixed in. The caller should not hold onto the data. */
>    void FinishMixing() {
> +    for (MixerCallback* cb = mCallbacks.getFirst();
> +         cb != nullptr; cb = cb->getNext()) {
> +      cb->mReceiver->MixerCallback(mMixedAudio.Elements(),

will this ever call me back with frames == 0 or channels == 0?

::: content/media/MediaStreamGraph.cpp
@@ +617,2 @@
>    } else if (mMixer && !shouldMix) {
> +    mMixer->RemoveCallback(gFarendObserver);

I should note that gFarendObserver is basically a hack to avoid having to register... 

There should be an API available on MSG, and then MediaEngineWebRTCAudio should register/unregister with it.

This could be done in a follow-on bug, however.  File a followup, assign it to me, and add a comment here?
Attachment #8418657 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8418657 [details] [diff] [review]
> Add an API to the MediaStreamGraph to register a mixer callback function. r=
> 
> Review of attachment 8418657 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/AudioMixer.h
> @@ +51,5 @@
> >     * tracks have been mixed in. The caller should not hold onto the data. */
> >    void FinishMixing() {
> > +    for (MixerCallback* cb = mCallbacks.getFirst();
> > +         cb != nullptr; cb = cb->getNext()) {
> > +      cb->mReceiver->MixerCallback(mMixedAudio.Elements(),
> 
> will this ever call me back with frames == 0 or channels == 0?

In theory, no, but I'll MOZ_ASSERT it before landing so this stays true.

> ::: content/media/MediaStreamGraph.cpp
> @@ +617,2 @@
> >    } else if (mMixer && !shouldMix) {
> > +    mMixer->RemoveCallback(gFarendObserver);
> 
> I should note that gFarendObserver is basically a hack to avoid having to
> register... 
> 
> There should be an API available on MSG, and then MediaEngineWebRTCAudio
> should register/unregister with it.
> 
> This could be done in a follow-on bug, however.  File a followup, assign it
> to me, and add a comment here?

Sure.
Hrm, I think I pushed the wrong version or something. Backed out for now, until I investigate https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b03ddc3161
Attached patch mixer-api (obsolete) — Splinter Review
This is the right version. The only change is the added StartMixing member and call.
Attachment #8430127 - Flags: review?(rjesup)
Attachment #8430127 - Attachment is patch: true
Comment on attachment 8430127 [details] [diff] [review]
mixer-api

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

::: content/media/AudioMixer.h
@@ +89,5 @@
>      }
>    }
> +
> +  void AddCallback(MixerCallbackReceiver* aReceiver) {
> +    mCallbacks.insertBack(new MixerCallback(aReceiver));

Is there a presumption as to which thread these occur on (Add and Remove, and Access for mixing)?  If so (MSG I'm guessing) please add a comment here so we know why a lock isn't needed.
Attachment #8430127 - Flags: review?(rjesup) → review+
Blocks: 1029554
Not sure if this changed, but I'm uploading it again along all the patches for
bug 848954 to make sure.
Attachment #8418657 - Attachment is obsolete: true
Attachment #8430127 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.